Transfer rework (IItemHandler, IFluidHandler, IEnergyStorage)

[hollow maple]

Err, no longs

My bad

[ivory mauve]

item stack sizes are ints, so are fluid stack sizes

you can't create a fluid stack containing more than int max, so why should the transfer handler support it>?

[stone dragon]

Yes but item stack sizes are capped at 64 anyways so that's irrelevant... it's not like you can turn any old int into an itemstack and have it be a sensible itemstack

[hollow maple]

I'm microwaving food so please don't mind the phone induced typos

[ivory mauve]

now I have to in every one of my fluid handlers check if its bigger than long max before converting to a fluid stack?

[stone dragon]

I mean, you have to do that in any old item handler anyways in case it's more than the max stack size, right?

[ivory mauve]

its added "flexibility" that no one needs outside of modpacks who don't care about their players time

I've not heard you justify why its useful

and I am making a case for why its annoying and not useful

if its annoying and not useful then its not worth having

[stone dragon]

Eh? If the focus of the modpack is massive automation I'm not sure it's necessarily awful design or the like, depending on how the modpack is structured. Besides which, the point is that as a modder, if I query a storage, I should get an accurate response for how much stuff is there

[ivory mauve]

are we seriously designing Neo APIs around modpacks that use massively compressed cobblestone?

really?

[stone dragon]

Stuff like ME systems definitely stores more than 2 billion items in practice, even if generally only for a particular item or two.

[stone dragon]

No, we're designing the API over wanting to accurately query a storage

[ivory mauve]

just because you can doesn't mean it happens

the average player will never have 2 billion of anything

you might get particular streamers or players of "expert packs" getting to 2 billion

thats the minority, not the majority

[stone dragon]

🤷‍♀️ I still don't feel like you've justified the downside of using a long here. Why shouldn't you be able to query for one?

Worst case scenario -- just have a query method and a queryLong method (or whatever it's called), the latter with a default implementation that returns the int one. We've already basically gotten that it doesn't make sense for transfers, more so for queries

[ivory mauve]

why should you be able to ?

let me give you a strawman that is what your argument sounds like

we should make the fluid transfer API use doubles!

anyone who doesn't care about doubles can just work with ints with no loss of precision

you can't tell me its not useful, some of us want fractions of a millibucket

some modpacks probably want fractional millibuckets to display their 8/3s of a bucket

[silent root]

Let's not digress into what ifs on either side (for or against), the downsides I have listed are more mechanical in nature and easy to mixup at times.

[ivory mauve]

worst case scenario, we just have query and queryDOuble methods

[silent root]

As an example that happened yesterday: I had getAmount(handler) return a long, and used the helper.assertValueEqual

since amount was a long and the passed in b param was an int it instantly failed

[stone dragon]

That is not at all comparable. First of all, double arithmetic would have innate precision issues. Second, and more relevantly, if you're uding doubles -- or indeed, longs -- everywhere, then yes, I agree, it's onerous. But the proposal here is using longs -- or providing an overload that does -- specifically for querying how many of an item there is. To be clear -- if you don't use longs, this should not affect you at all, right? You will just only provide an int value, and call the int-value-providing querying method

[vernal dust]

Knight, you want to read up more. Multiple modders and library authors are already working around ints being the max. It's much nicer for everyone if Neo removed that friction.

[silent root]

these sorts of things are very easy to accidentally do in the longevity

[ivory mauve]

which modders? which libraries?

what friction?

[silent root]

For the moment inquiries having a getXAsLong() seems to be a viable approach atm (which I just added)

[ivory mauve]

all increasing it to long will do is have them have to work around long max instead

Every argument I have heard for long amounts to "what if I want to display as long?"

so here is a pitch: make every single method use ints except the method to query contents of storage, read only

[hollow maple]

Yes. That is roughly what we've done.

Thank you for getting to the conclusion 🤣

[silent root]

We are on the same page then

[vale birch]

-# BigInt is the only solution

[hollow maple]

But yeah, exposing a long query API is purely about, well, just querying

[ivory mauve]

I'd also argue such query method should be a default method that just calls the int query method by default

[hollow maple]

Everything else is int-based

[silent root]

-# Strings

[ivory mauve]

but that might be pointless boilerplate

[hollow maple]

Yup, that's exactly what it'll do

[stone dragon]

...that is... exactly what was proposed, yes

[ivory mauve]

honestly, I want to store everything is rationals

[silent root]

    @Range(from = 0, to = Long.MAX_VALUE)
    default long getAmountAsLong(@Range(from = 0, to = Integer.MAX_VALUE) int index){
        return getAmount(index);
    }

[hollow maple]

Now, I won't say I agree, but yeah that would certainly be nice 🤣

[ivory mauve]

I forget @Range is a thing, is that Intellij?

[hollow maple]

Yeah, JB annotations

[silent root]

org.jetbrains.annotations

[vernal dust]

-sends u Math.PI buckets of molten iron-

[ivory mauve]

I mostly use google errorprone

[silent root]

It is to hopefully alleviate compile time misuse

[hollow maple]

Ah, 22/7 buckets, my favourite!

[silent root]

It actually helped clean up some code here and there

[stone dragon]

jetbrains annotations are what is used throughout neo for a variety of stuff

[ivory mauve]

you telling me I cannot have negative contents in a tank?

[hollow maple]

Anticontents?

[silent root]

No negatives correct

[hollow maple]

Is having -1 bucket of water the same as having a bucket of antiwater?

[ivory mauve]

I want my tank to store anti-fluid, which when added to your tank of fluid explodes

[vernal dust]

No, you have a bucket of Dry

[silent root]

You can visualize a negative and store internally, but when someone inquires how it is used, it is transactionally non-negative

[hollow maple]

A bucket of "oh no a black hole"

[ivory mauve]

I think it probably breaks a contract somewhere if when someone gives me a water bucket, my tank contents go down

[stone dragon]

It shouldn't

[vernal dust]

A bucket of dry can optionally be traded for its equal value, referred to as "a pocket full of sunshine"

[ivory mauve]

I have a block of code somewhere that makes the assumption "if I insert N fluid, the tanks contents will be unchanged or increase, never decrease"

[stone dragon]

When you insert something into an inventory all sorts of stuff could happen

[silent root]

It is an amount that is mutated, not a negative amount passed in

[stone dragon]

I have an inventory that when you extract from a slot, shuffles around the ordering of a bunch of its slots

[vernal dust]

Oh that's easy, just go use early 1.12/1.14 mods

Your GUI will break within an hour, almost guaranteed

[stone dragon]

So, yeah. I would say that you should never assume that a block won't do extra processing once you insert/extract. You might insert, but then its internal logic goes "ah, this tank is over this threshold, so we shove it all into this tank instead and empty this tank" and that should be fine

[hollow maple]

Or it's a trashcan, in which case it's always empty even if you put something into it

[stone dragon]

Exactly

So, no, definitely not a contract there, I would say

[vernal dust]

I just want to make sure this all also works with the proxy system, in that I won't have to make a buffer inventory just so pipes can connect cross-dimensionally

[hollow maple]

Really, all this transfer API is, is an API for recipes

we should just make this a recipe API instead

(I'm bringing back that meme)

[vernal dust]

Okay yeah, Monica time for bed you're still ill

[hollow maple]

Aww, you don't want to see my block breaking recipe?

hckd-- I forgot this curry had cardamom pods in it and I just ate one

that was an experience

I guess that's what I get for having recipes execute loot tables though

[vernal dust]

get rekt

I still foresee Mojang eventually merging blocks and block entities together at some point

With components

[silent root]

@flat root, for move when dealing with handler -> handler util, is the return valuable if you don't know the resources it moved necessarily? Should we make that void?

ResourceHandlerUtil.move seems like it would make sense if it just returns "boolean" or void for successfully did something but maybe the int is useful to see a total of arbitrary changes

[wind steppe]

i guess its helpful to say X items moved, and is easy to say "if moved > 0" -> do moved success action or something.

[silent root]

Likely, just want to be sure we don't give the false pretext that it is only moving one item

as all other returns are returning an amount you use to mutate your internal on a given resource

this one... we have no idea which resources were those numbers were mapped to

I'll document it for now

[silent root]

Curiously, is @Nonnegative usable? javax.annotation.Nonnegative

there is also org.checkerframework.checker.index.qual.NonNegative

[clear tusk]

would it be worth copying over isEnabled and getCapability into resources from stack counter parts?
just looking at utils on ItemResource compared to ItemStack, these are only 2 i would say are missing to allow the same level of state/data look ups that stacks allow without needing to invoke toStack on said resources

could see some throwback for isEnabled but imo if we are now allowing things to be feature flagged our apis should support validating the enabled state

for resources that dont directly implement FeatureElement (fluids) the isEnabled can be genericlly implemented like so (should match logic in ItemStack)

public boolean isEnabled(FeatureFlagSet enabledFeatures) {
    return isEmpty() || !(value() instanceof FeatureElement element) || element.isEnabled(enabledFeatures);
}

[silent root]

    public boolean isEnabled(FeatureFlagSet enabledFeatures) {
        return innerStack.isItemEnabled(enabledFeatures);
    }
``` likely

[clear tusk]

yeah that would work for resources that do implement FeatureElement (Item)

but say someone wanted their fluid to be flagged, vanilla does not implement feature element so would need the generic implementation above

[silent root]

   public boolean isEnabled(FeatureFlagSet enabledFeatures) {
        return isEmpty() || !(getInstanceValue() instanceof FeatureElement element) || element.isEnabled(enabledFeatures);
    }

Fluid one?

[clear tusk]

looks correct to me, if getInstancedValue returns Fluid

[silent root]

    public Fluid getInstanceValue() {
        return innerStack.getFluid();
    }

ORiginally this was in the helper middleman inteface for IRegisteredResource

but that was since removed, so we COULD call it getFluid() not really leaning one way or the other on that atm

[clear tusk]

should match the same logic in ItemStack#isItemEnabled enabled for

• empty stacks
• none feature elements (items skip this cause they directly implement the iface)
• the feature element deems it to be enabled

[clear tusk]

not the right PR to make this change rly but would anyone be opposed to us moving this upstream into FluidStack and the resource just quries the stack for the state

feel it would make more sense to be defined in the stack and the resource simply quries its innerStack for if it should be enabled or not

[past lodge]

you generally don't wanna use annotations from checkerframework, it's a transitive dep of another lib iirc

[silent root]

Noted but the javax one looks good perchance?

[past lodge]

yeah, that's fine to use

[silent root]

They at least look better in code than the range one

[past lodge]

it'll have the best ide support considering jetbrains doesn't have its own version of that

[silent root]

(Wished it had a little better casing)

so far this is coming along. currently adding builtin resources supporting isEnabled checks

[silent root]

As a heads up, I will be traveling for the next day or so to go visit my grandmother, so while I will have my tablet, lengthier feedback requests will likely be resume on Monday evening at the earliest. (Depending on what the feedback is at least as 1monitor programming on a surface pro is not necessarily the most ideal ) I'll be here for a few more hours but just wanted to let ya know

[loud stirrup]

I'd stick to the annotations you already find in use in the Neo codebase. Some libraries are only visible because of some weird transitive dependency trees.

I.e. we have recently gotten a second and third JSON parser

Thanks to the MSAL authentication library dependency in MC

[silent root]

javax is included in MC at least

As they use their Nullable which is in the same annotation package iirc

[cursive geyser]

I would strongly avoid javax.annotations stuff, in general

is there no equivalent jetbrains annotation?

[silent root]

Have another alternative to Nonnegative then?

They have Range(from = 0, to = Integer.MAX_VALUE)

which... is lengthy when you have 2 in the same method

I wish they did

[cursive geyser]

how about the one in checker-qual, which we apparently also have? 🤔

how many annotation libraries are there in minecraft lol

[silent root]

Lol

[cursive geyser]

apparently wehave TWO versions of checker-qual

[silent root]

I get conflicting info #1183818213134446742 message

[cursive geyser]

oh

if it's transitive then ignore

[silent root]

got it

[cursive geyser]

ah neoform depends on it

[silent root]

So for now, I'll use the javax one, but if an clean alternative presents itself we can switch

[cursive geyser]

on 3.49

[silent root]

@hidden geode you about?Was curious if you had your mock up code forthe transaction operation thing, I MAY have an possible solution but unsure

[hidden geode]

Probably somewhere but not at my desk

[silent root]

No worries!

[hidden geode]

Stripping my dog currently

[silent root]

When ever your dog is dapper and you have time, could you gist it over perhchance?

[hidden geode]

If i can find it

But i wanted to try something myself as well

[silent root]

Oh sure! no worries, if nothing else I just want to understand the pattern we had started even if we fully adopt a solution you make

[loud stirrup]

Yeah re: annotation libs, if it's used already in NeoForge, you can also use it, I'd avoid introducing first use of libs that we currently don't use. I think we mostly rely on the JetBrains annotation lib for most things

[silent root]

Seems javax.annotation. is used by neo so @Nonnegative should be safe

[copper fjord]

do IDEs recognise it

[loud stirrup]

Hm, looked into it. I'd probably not bother with Nonnegative. We use that three times in Neo as far as I can tell, not that it hurts. But it's unlikely to ever trigger for a user (who passes -1 statically to transfer methods 🤔 )

[silent root]

It isnt just statically

If it can resolve it through method calls pretty well

So in own code transactions it can minimize, but it is also to help prevent people from doing negative, which does happen from time to time

For now, it is pretty effectivr at its intent, and is pretty minimal 🙂

[silent root]

IDEA does, I dont have eclipse so hard to say

[loud stirrup]

IntelliJ is pretty good about it, I'd guess

I'd prefer the JetBrains annotation version of this

But since we already use Nonnegative somewhere in Neo

I can't say it's wrong to use it

[silent root]

I wish they had one personally, or a little nicer use of Range

[loud stirrup]

I think I'd personally try to get rid of javax.annotation since it's unmaintained, but while it's still in there, it's fair game

[untold eagle]

issue is tho that Jetbrains' equivalent is very verbose and you can't annotate another @interface to shorten it like you can with others

[silent root]

I even went down a rabbit hole of seeing what it takes to make our own. And I discovered two things
1: Im sleep deprived
2: I am woefully ill equipped with experience in the matter for doing so, as ghey seemed like custom annotation processors which seem... tricky in this environment

[loud stirrup]

yeah I'd not spend time on that

too little return on that investment

and we're not going to do APs in Neo

[silent root]

I sort of assumed. I looked first to see if we had any before deciding to stop

[past lodge]

yeah but it's also the only thing that adds stuff jetbrains doesn't and is still widely used

[loud stirrup]

jetbrains seemingly added some new stuff recently, do you know off-hand what's still missing?
I think previously the default-non-null package-level stuff was not in jetbrains, but I think they added that

[past lodge]

they have added not null by default

but it's experimental

https://javadoc.io/doc/org.jetbrains/annotations

i don't think minecraft bundles this version of it though so neoforge would need to do that

oh wait wrong link

https://github.com/JetBrains/java-annotations/blob/master/CHANGELOG.md

i wonder if we can just PR the annotations we want to use

it should be as simple as PRing it to the lib and then PRing ide support to intellij

[loud stirrup]

minecraft doesn't bundle it at all, I thought

[flat root]

Yes, it's very useful to know if something happened or not. I actually use the exact return value in my pipes to know "how much transfer" was used by the move

So please keep the int return

[noble latch]

IIRC, last time I tried that annotation, it also affected local variables which is completely stupid IMO

[silent root]

Can do, the "has moved" makes sense and to a degree the amount totally moved makes sense but it is of note it is of difference than the others. I documented it differently in theory for now at least.

[silent root]

Looool its like edna "no capes!"

[past lodge]

well it is still marked as experimental

i don't think they actually mean for it to be like that

[loud stirrup]

oO what the....

[dapper dust]

i use a NotNullDefault that does that, and i actually use it package wide

i like that it does, to each their own i guess

[silent root]

Annotation Processors are always fun lol

[silent root]

Never used one so would assume it gets dicey even in a mod project.

For now, javax' nonnegative solves the problem for hinting without causing too much visual noise.

[queen wagon]

APs are bad at their main job since it is only easy to generate new source files and not easy at all to modify the annotated source

I once tried to make an AP that generates a codec for the annotated record but putting the constant in the annotated record was way too difficult

[flat root]

good enough for a first batch

my condelences

[silent root]

Looool

WIll look at it tonight

I'll peruse now a bit, but the tablets are not ideal for actually doing changes

[silent root]

@MakeCodecForMe
public record thing(...1400params){}

Result:
No you...

[silent root]

@flat root with a lot of the ones I can peruse right now on tablet I've replied 🙂 a few I couldn't confidently do without looking at the code

Time to get ready to have Mexican with my grandmother

[flat root]

sounds good. I will have a look another day 😛

[silent root]

Is there a normal pattern of doing junit tests rather than test - gametestserver? (I think I have a world needed for most of my tests, but wasn't really sure there is a normal pattern for moving to junit)

[flat root]

From IntelliJ you should be able to run them

If you right click on the junit folder it should be possible to debug the tests

You can also run individual ones by clicking on the green arrow next to a test method or class

[short mirage]

hello, i havent been keeping up with this. Will this stuff be released for 1.21.1 or another version?

[loud stirrup]

It's a breaking change, so no, not for 1.21.1 (or 1.21.5 for that matter)

[wind steppe]

the PR is targetting 1.21.6 when that releases

as is noted in the PR tags

ugh that reminds me i need to update the PR description

[silent root]

Loool yep I can help with that tonight if you are still awake in a few hours

I would have done it already... but uh..... access issues

[short mirage]

so if i wanna use this when its out i guess i gotta port my mod

[silent root]

Yep

My personal plan is 21.6 but that is purely up to review and some changes Adrian and I can deliver at this point

Fortunately, my eyes are focused heavily on this, and just waiting to get back from my grandmother's to continue

[silent root]

I'm not totally convinced we should have some of these methods persisting ... and instead just point to a new class and say use that, as while it'd make some uses require changing... it may be for the best in the long term to cut the fat now. (Migration deprecations are easy enough, but some of these methods are fundamentally needing to change as it is.

[silent root]

Probably best for maintainers to make a decision on this one:
https://github.com/neoforged/NeoForge/pull/1115#discussion_r2138373797 In regards to patches

[flat root]

So @cunning mulch what do you think wrt that? I don't really mind either way, just wanted to point out what I would personally write

[cunning mulch]

I am fine yielding to you and the other maintainers who are more active when it comes to porting

Some of my review is by nature going to be based on the old forge stance of minimizing patches, so until I hear otherwise for specific things (like wrapping a single line statement) I will tell people how they can minimize them

[noble latch]

I personally would also go for the variant that only adds a line. To my knowledge we're already doing the porting with pretty low fuzziness so this shouldn't be an issue

[cunning mulch]

Now if it turns out that case is easier for porting to not minimize, then I don’t need to recommend people further shrink it

Yeah if the fuzziness is low enough that it shouldn’t be an issue, then it would help prevent conflicts if mojang say adds a param to the setChanged

[flat root]

Out of curiosity, why? I tend to avoid braceless if in my code, so there's that

[cunning mulch]

I avoid braceless in general as well

But for patches I from habit prioritize avoiding changing vanilla code that isn’t necessary

[noble latch]

Yup, same for me

[clear tusk]

yeah ive always been told to keep patch sizes small and in that scenario that would mean dont indent the if branch or even 1 line it

[flat root]

It was the Forge policy for sure but I never got the point vs keeping the code readable. 😛

In any case it doesn't matter here, just do whatever imo

There are more important topics that will need a maintainer decision (e.g. slotted energy)

[noble latch]

Simple: no

[silent root]

There are benefits that provide energy to be indexed, so don't pull it out of context

[cunning mulch]

I haven’t gotten there yet, only received maybe 1/4 to 1/3 of the PR so far

[silent root]

If someone wants to interact with it as they once did it is COMPLETELY built to be done so

ISingleEnergyHandler provides literally a near 1:1 what you had

It then changes 1 or 2 methods to be in a util otherwise, but it is still pretty much allowing more advanced features with pretty much no impact to implementers as they are now

[flat root]

Tbh I skipped all of the resource code for now as I wanted to focus on the other aspects

It's really a huge pr

[loud stirrup]

Same, I avoid braceless

XFact is the porting monster 😅

Personal experience so far has been that the patch being understandable is key to making it quick to port if it rejects

So I personally do prefer readable code in patches too when possible

[noble latch]

Looks at the + , doSomething() patches in codecs and similar things, almost all of which are my doing
Yeah, you're probably right

[cunning mulch]

I mean I have committed "worse" than that in some ways looks at https://github.com/neoforged/NeoForge/blob/1.21.x/patches/net/minecraft/world/level/TicketStorage.java.patch#L8-L9

[noble latch]

That looks pretty sane to me

[dapper dust]

don't worry, ill probably be adding to that

ive found errors with a handful (well, more accurately almost all, but only a handful have an issue with vanilla's shaders) of the pipeline definitions, and the fix is going to be shoving a single line somewhere in the builder calls

[cunning mulch]

yes, but I so much didn't want to modify the line above to add a comma in it... I used one of the methods basically no one uses/knows above for combining the things

which might make the data flow harder for some people to read what is going on than just seeing post patch the codec being complete but with a comma on the next line instead of the previous

[silent root]

What?

public net.minecraft.world.entity.EquipmentSlot countLimit #Used for resource handler wrapper

Like... I have no idea what is happening here

we good now goodness that was a blackbox automagic

[hollow maple]

It was the AT and a change you made locally conflicting; after you add an AT you either need to apply it manually or run setup

[silent root]

Ok @flat root Weird possible problem introduced with hoppers (unless I misunderstand) @cunning mulch has pointed out that before the hopper was doing a custom cooldown but only when inserted into like the following.

NOW we have it when we commit... which then implies insert and extract correct? That extract may not necessarily be correct behaviour Unless I am missing somewhere we are skipping that part

I SUPPOSE we could check to make sure the amount is more than the original to "assume" insert

OR we could do away with the cooldown

[cunning mulch]

And on top of that we should evaluate if the cooldown is even necessary anymore or if https://github.com/neoforged/NeoForge/pull/1787 made it obsolete. Especially the difference with the comment for hopper to hopper being 7 ticks instead of 8 is likely now obsolete as hopper to hopper should be using the vanilla code path after the linked PR

[flat root]

Ah you're right. This I never tested so it could be wrong. We need a different way to do it (maybe add more parameters to onTransfer and add a journal to the hopper for the cooldown?)

Or yeah maybe we don't need it at all, idk

[cunning mulch]

so apparently the custom cooldown concept is a vanilla one(?) even though it doesn't seem like a cooldown higher than 8 is ever set in vanilla. But it does mean we likely need to handle it, and from what I can tell from browsing code paths recently, when things eject to hoppers in general it sets the cooldown for it as well. But yeah, I believe it is still needed for when things manually insert into the hopper via its itemhandler

Would adding a param for if it is inserting or extracting for IContainerExtension#onTransfer be a solution that doesn't break logic relating to transactions somehow?

so that then we could override it for hoppers similar to how we override for chiseled books but only end up keeping track of that there was an insertion if the onTransfer was for an insertion

[flat root]

Yeah that would be correct

[silent root]

So concensus is new params in onTransfer to indicate direction (instert/extract)?

[silent root]

Do we also want to set the cooldown in onTransfer or just keep track of if the "io" ever had an insert?

Technically doing either of those, that would mean the simulate would control a cooldown which could lock it down just by inquiring

(Err no I suppose only one of those would do that)

Well ok. Actually...

transaction {
  subTransaction {
    doHopperInsert
    //Don't commit
  }
  doHopperExtract
  commit
}

This would stall it if we JUST did the onTransfer keep track of if it was inserted at anypoint.

BUT if we add a journal that keeps track of that, that would I think work, but the patch becomes a little silly

+    private boolean insertingFlag = false;
+    private final net.neoforged.neoforge.transfer.handlers.wrappers.items.HopperJournal journal = new net.neoforged.neoforge.transfer.handlers.wrappers.items.HopperJournal() {
+        @Override
+        public void set(boolean value) {
+            insertingFlag = value;
+        }
+        @Override
+        public boolean get() {
+            return insertingFlag;
+        }
+    };
+
+    @Override
+    public void onCommit(int slot, ItemStack originalStack) {
+        if (insertingFlag && originalStack.isEmpty() && !getItem(slot).isEmpty()) {
+            if (!isOnCustomCooldown()) {
+                // This cooldown is always set to 8 in vanilla with one exception:
+                // Hopper -> Hopper transfer sets this cooldown to 7 when this hopper
+                // has not been updated as recently as the one pushing items into it.
+                // Hopper <-> hopper interactions don't use transactions so we don't need to worry about that.
+                setCooldown(8);
+            }
+        }
+        insertingFlag = false;
+    }
+
+    @Override
+    public void onTransfer(int slot, IODirection ioDirection, net.neoforged.neoforge.transfer.transaction.TransactionContext context) {
+        super.onTransfer(slot, ioDirection, context);
+        if(ioDirection == IODirection.EXTRACT) return;
+        journal.updateSnapshots(context);
+        insertingFlag = true;
+    }
+

The solutions and their problems

• Current : All commits insert/extract would cause cooldown
• onTransfer + param : This is run on simulates too
• onTransfer + param + onCommit : This would work, until we don't commit the insert, then we stall

New final possible solution: onTransfer + param + onCommit + journal: This will allow us to save keep a revertable entry for scenarios like the above where we don't commit the insert. No idea if this is the correct solution, but gut reaction is that it should work

[cunning mulch]

Similar to the bookshelves right? Aka what I was suggesting of having the override then use a journal to keep track of it

[silent root]

Yeah, the journal would be needed, but the rest of what you were saying I think is on point. I just missed the journal at first

[silent root]

Ok... so someone explain this to me, as I am lost on the function:

  if (!isOnCustomCooldown()) {
      // This cooldown is always set to 8 in vanilla with one exception:
      // Hopper -> Hopper transfer sets this cooldown to 7 when this hopper
      // has not been updated as recently as the one pushing items into it.
      // Hopper <-> hopper interactions don't use transactions so we don't need to worry about that.
      setCooldown(8);
  }

    public boolean isOnCustomCooldown() {
        return this.cooldownTime > 8;
    }

so.... isOnCustom is NEVER changed?

[flat root]

the point is to avoid lowering the cooldown

[silent root]

My point, is that !isOnCustomCooldown is ALWAYS true?

Unless vanilla goes passed 8

Or is this just to ignore if someone else added some cooldown?

[flat root]

this

[silent root]

Making sure. We never had a test for it, so we need one (Unless someone hid it somewhere)

[flat root]

there's none that I'm aware of

[noble latch]

Mentioning this here for completeness sake: I would be in favor of ditching the Range and Nonnegative annotations in favor of specifying the supported range in the javadoc. It's a lot of noise for something that IDEA has crappy support for (and Eclipse probably has none for), IDEA can barely see through compile-time constants passed directly (i.e. a magic number or primitive static final field passed in directly or a loop counter initialized with such a value [it fails to detect an out-of-range value for the counter limit]) and fails on anything barely "indirect" such as the case shown in the screenshot

[silent root]

I'll double check, but last time I did something like this, it handled the first and last method

The middle one... looks correct?

Otherwise it has been pretty solid when writing the tests and such, which is the most basic uses.

[void thicket]

The middle one is wrong since the annotation is not a guarantee

[noble latch]

You need to consider these three methods together, specifically at the call-site in main(), on their own each of these methods is irrelevant and doesn't prove anything.

[silent root]

Interestingly, this... did highlight before

hopper.insert(ItemResource.of(Items.APPLE), getThing(), transaction);

Which was why I was such a proponent for it. HOWEVER, now I want to know why it isn't now...

Even this used to highlight before...
So not sure why it isn't now

So for params probably worth looking into either a solution + removing the annotations, but the returns definitely have value still

[copper fjord]

I'm not sure what value returns provide if intellij is unable to propagate this

[void thicket]

Yeah, it seems worthless across the board

Write the contracts in the javadocs

[silent root]

Hoping people read those though... is one of the reasons this PR exists

" This FluidStack MUST NOT be modified."

And yet

People do/did

So while java doc is nice, I'd also like a more "Hey don't do that" at dev time

[flat root]

I would go for a runtime "don't do that", i.e. an assert for >= 0

[silent root]

assert doesn't play

[flat root]

the problem with the stack mutations is that you don't see it at runtime either

[silent root]

You have to have a special flag for asserts to work

[flat root]

yes, I would never use a Java assert. More a precondition if you want

[copper fjord]

annotations that don't work are far from that

checks are fine

but annotating the return is quite pointless given how pointless annotating the parameter is

[void thicket]

Really the only issue with "the fluidstack must not be modified" is there's no enforcement mechanism

But with an integer, well, if(x < 0) throw works perfectly

[flat root]

maintainers, what are your thoughts on int index vs int slot to select a specific slot/tank/whatever of the handler?

[silent root]

Only inventory have slots really

and they are only called that because of UI

FluidHandlers are tanks, but those are no longer the specific implementations

[hollow maple]

index is generally more agnostic. I don't really mind either way, but if it's using index currently I'd rather keep it the way it is (and vice-versa if it were slot)

[silent root]

Considering the backing structure is likely an array or a list, index seems more understandable long term

[noble latch]

I'm fine with either

[hollow maple]

I think that's likely gonna be the outcome; either is fine, but don't bother changing it

GENERALLY the name of parameters isn't that important anyway, since implementers can just rename it

[flat root]

I am asking because my PR originally used int index, but I kept changing it to int slot every time I was writing an implementation and eventually just decided to rename the param in the base method so I wouldn't have to rename the parameter in every implementation 😄

[silent root]

I have personally no problem doing index

[hollow maple]

And like, what's the difference between // @param index the slot index to extract from vs // @param slot the slot index to extract from?

(which is realistically the only place the difference in name is going to matter)

[cunning mulch]

My guess is so mapmakers can set a hopper that doesn’t eject or whatever the cooldown prevents by placing the block with nbt using commands

[silent root]

Possibly

In which case, interesting

[cunning mulch]

Index. Sub impls can have better names. In mek we have slot, tank, capacitor, and container depending on the handler type for what we name params in it

[loud stirrup]

no particular opinion on slot vs index

i find slot easier to address in javadoc

I guess I could go with the argument of keeping whatevr IIH had

(okay that was slot)

[silent root]

Unless I am given a practical compelling reason, I think it will stay as index given it is more agnostic and likely to go into what is the backing structure List.get(index) ; array[index]

But again, that is purely something that can change per implementation to better match what you are seeking

As different resources may not even have a concept of a "slot" at all

Items have slots because of the ui slot

Fluids have tanks

Energy, for some, have buffers or capacitors

etc

[hollow maple]

Do drawers have slots?

Personally, I tend to just chuck all of my stuff in the drawer and it all heaps up in there

[cunning mulch]

One

[silent root]

Typically one. I saw an implmentation that has a few for data storage of sorts for its stacks, but usually just the one

[hollow maple]

😭 you're taking my joke too seriously

[clear tusk]

"chest of drawers" == IResourceHandler<DrawerResource>
so depends on your wording
a single drawer could be a ISingleResourceHandler<DrawerResource>

[silent root]

You can joke??

[cunning mulch]

No, linking this is taking it too seriously https://github.com/mekanism/Mekanism/blob/1.21.x/src/main/java/mekanism/common/inventory/slot/BinInventorySlot.java

[hollow maple]

But on a more serious note, I do think that index makes more sense for drawer-style blocks, since you don't have individual slots like in a UI but instead you're just referring to which drawer you're accessing

Like, the 2x2 drawer block as an example; I wouldn't say each drawer is a slot, I'd say it's a drawer, and that drawer has an index

(I've now achieved semantic satiation with regards to the word drawer)

[stone dragon]

Index would be more sensible for me because I have a block where the order in which GUI slots are exposed via capability changes but the order in the GUI stays the same

So "slot" would just be... innacurate, in that case, because the thing exposed in index 0 could be, at varying points in time, any of 9 different GUI slots

(This particular thing I have left off updating for... quite a while mostly because it was a pain to model sensibly, but I'm gonna give it a shot again now that transactions are on the table since that should make it much nicer, in theory)

[void thicket]

I would go for slot

[silent root]

fluids don't really make sense with "slot"

Nor does any arbitrary resource just innately

[void thicket]

Why? It's a magic index, slot is not explicitly an item thing

[stone dragon]

I don't really care that much, to be fair -- "index" just seems more directly accurate for what it is, and there's enough cases I can think of where it isn't really the same as a slot (at least in terms of a Slot, that is, the vanilla class)

[silent root]

An "index"? So why not just call it index?

[stone dragon]

I think it's that slot sorta makes it sound like it matches with a Slot

[void thicket]

Well, from a GUI perspective, if you had any object it would still be in a Slot-like thing

[silent root]

Tanks would like to have a word

[stone dragon]

Yes but as mentioned, it need not line up with the index of a GUI Slot, and in fact I have an inventory with 9 slots where the (gui) slot index should be completely different from the exposed index in the capability

Calling it "slot" implies to me that those should line up

[flat root]

I was wondering what I did for Fabric... turns out it's all slot https://github.com/FabricMC/fabric/blob/8ea25362f47bbd552ef4e5db68f3cf58d7bf7c74/fabric-transfer-api-v1/src/main/java/net/fabricmc/fabric/api/transfer/v1/storage/SlottedStorage.java#L47

[stone dragon]

But that makes no sense in my case, because "which spot should be prioritized to extract/insert" doesn't line up with the ordering in the GUI, and could shift even while the GUI is open

End of the day though -- I don't really care, this definitely isn't something I'd fight it out on because it doesn't really matter

I'm just giving my potential justification for calling it "index" instead. I'm going to treat it as an index whether you call it that or not, so so long as people don't expect it to line up with Slot slots its all good to me

(That's not an expectation, right?)

[void thicket]

I don't think anyone expects it line up with the Slot object

[stone dragon]

👍 then I don't care what it's called. My only issue with slot was that it made it seem like it was somehow related to the Slot, where in my case I have Slots and their ordering is completely uncoupled from that of the spots you can interact with via capability of the inventory

[silent root]

I had a thought, we should probably make this toStack limit to the itemstack limit of the given resource shouldn't we?

public ItemStack toStack(int count) {
    if (count == 0 || this.isEmpty()) return ItemStack.EMPTY;
//        count = Math.min(count, this.innerStack.getMaxStackSize()); Currently missing line
    return this.innerStack.copyWithCount(count);
}

This is in ItemResource

[stone dragon]

Maybe. I would almost say it should throw if it's too high a number?

[silent root]

Mm not sure it should throw really. But clamping it to the max size or the absolute max size may be best

Since we have the data point, max stack size seems like the more reasonable result

[stone dragon]

I feel like if we're doing the check at all, it should throw -- after all, having the count decrease on a conversion seems like unexpected behaviour, as you'd expect the conversion to be reversible. So folks should be checking the stack size before calling -- but if they're not, then silently decreasing how many of the thing they have could lead to unexpected behaviour, while throwing is clearly "something is messed up"

[silent root]

We COULD log like I have in the ResourceHandler Util

so less silent but less "Hey I pushed this to prod, suffer players"

Silently doing it, I agree wouldn't be ideal

[stone dragon]

I guess my question is, is there a reason not to throw on what is almost certainly an error state? Either way I agree it should definitely be logged

[silent root]

So for these, typically a dev testing this behaviour, will place down a few blocks and do test. Then when it crashes, it will close. So far that is the expected. When they reopen, due to it crashing all setup for the test is not saved so they have to redo the entire test setup to see if they got it right, and HOPE nothing else is crashing doing the same logic

where as logging, with the throwable will at least help them isolate it, while leaving the game running

Crashing really should be done as a last resort, or "if we continue unrecoverable events will happen"

[stone dragon]

Hmm? The setup for the test will still save when crashing, though -- the game saves the level when something crashes in a tick, worst you should lose is the last tick's worth of behaviour iirc?

[silent root]

Not to my experience

[stone dragon]

It's got a whole mildly-cursed system to half-recover even during OOMs enough to save stuff

[silent root]

Typically when it crashes, I have to go and redo my test setup

[stone dragon]

🤷‍♀️ I've never had that be a situation I've ran into, really -- probably depends on where the crash is. Server thread should be safe I'd think? This is testable though

Lemme poke it quickly

See if there's any obvious try-catch-es sitting around

[silent root]

if (count > innerStack.getMaxStackSize()) {
    Util.logAndPauseIfInIde("Stack size is too large", new IllegalArgumentException("Amount mustn't exceed Max stack size of resource " + this + " " + innerStack.getMaxStackSize()));
    count = innerStack.getMaxStackSize();
}

Honestly will likely be enough

Fluids don't have this, so crashing would be a little... extreme for items

[stone dragon]

MinecraftServer#runServer is a big try-catch-finally block; in the finally there's a call to stopServer(), which invokes the various saving logic. Unless the crash actually occurs while saving any exception on the server thread should not prevent the game from saving the current state

[silent root]

(Oop need the amount that was passed in too)

[stone dragon]

I would really prefer a crash because it's obviously-wrong behaviour, and shouldn't be considered "recoverable" since it involves deleting items from existence

[silent root]

Makes debugging these a pain though, as crashing means restarting the full game to fix one thing

Where as: blob of red text is very identifiably wrong

[stone dragon]

Yes but that's fine, obviously-wrong state should crash the game

[silent root]

That is the thing though... it isn't wrong state per se for vanilla, as copyWithCount accepts any value

[stone dragon]

And yes that means you have to restart the game to fix it. But it also means it actually gets fixed, whereas log messages are very ignorable

I feel like preferring log messages over throwing is only fine if the state in question is in some sense expected to arise in normal gameplay

[silent root]

Considering you can do this with stack.copyWithCount already, yes?

    public void setCount(int p_41765_) {
        this.count = p_41765_;
    }

they don't have a limiter iirc until they serialize

[stone dragon]

Well, no, because this is a neo class, so we get to define the contract on it. Just because it is backed by an ItemStack doesn't mean it can't have a stricter contract

And I think making the contract stricter (as in, "you shouldn't be calling this with a stack size larger than allowed") is sensible

But if we're saying that using too big of a stack size is allowed / expected, it probably shouldn't error either?

Basically I think having it log rather than throwing requires more justification than "a few more seconds to fix during debugging" -- especially as it means, the way you've proposed, that if it's going wrong in prod, it's completely silent, with no crash or log.

[silent root]

It isn't silent in prod

the method name is.... odd

it logs then checks ide for pause

public static void logAndPauseIfInIde(String p_200891_, Throwable p_200892_) {
    LOGGER.error(p_200891_, p_200892_);
    if (SharedConstants.IS_RUNNING_WITH_JDWP) {
        doPause(p_200891_);
    }
}

Any dev ignorign the log is.... impressively ignorant, or "will fix later"

As it isn't a one line error. It is the stacktrace

[stone dragon]

Ahh, I see. I still think a crash is more sensible here -- mostly because not crashing means returning a non-sensical value

[stone dragon]

Eh, still easily burried in the IJ console if only ~15 other lines are logged afterwards before they check the console

And my point is more that such log messages have been ignored in the past with ease. Log messages are fine for providing information about what's happening but shouldn't be the way to point out that there's an issue unless said issue is somewhat recoverable and somewhat expected (and thus doesn't need to be pointed out except to help track down the cause of other issues) -- and this one isn't, really, because you have to return a nonsensical value to recover

[silent root]

It DOES mean that the log is at least latest to their test (runtime) vs having to scroll past the "Cowardly yada yada" nonsense

[stone dragon]

🤷‍♀️ yes, as I said. A couple more seconds when debugging. That's not enough justification for no throwing, I would argue

Because really, this is a situation where throwing makes perfect sense -- you can't recover sensibly

If the contract of the method is that it returns a "sensible" ItemStack (aka, one where the stack size is valid), with the given count, then you cannot recover without breaking that contract

And the contract should be the place to start imo -- so either the contract is weakened, at which point why even bother checking and logging, the vanilla one, which is weaker, doesn't -- or you keep the strong contract, and throw to maintain it

At the end of the day, this is also not something I'm going to argue into the ground because, you know, not worth it, we're talking about the behaviour in a case that should never happen -- I would just question the point of doing the check or clamp or logging in that case, if you're not throwing, especially if the vanilla copyWithCount doesn't.

[ripe crest]

as someone who does user support, error cases are very important too

I also believe that a crash would be better here, as transfer is something that usually happens automatically and very often, a logged error message can blow the size of the log file quite quickly

[silent root]

This isn't exclusive to transfering, this is anytime you go from an item resource to an itemstack

Crashing feels like a quick way to get a user upset

As you assume a modder would run into this in dev

Before prod

Having been in many a scenario where a dev has never tested their stuff before pushing, I'd rather it be something more akin to "Hey... stop that" lol (We can remove the clamp, but it doesn't seem as something that would be unexpected)

Throws really make the most sense in scenarios where the next couple steps would crash anyway. (Array out of bounds, or similar) But less so forced into a crash because it could have continued with what ever data it had technically

[vernal dust]

Solution: production check, if in dev crash, if in prod log at debug

[silent root]

Possibly, but then we go back to "a pain to debug"

[vernal dust]

What? You 'd have a stack trace in dev

[silent root]

Once up and running I don't see it much of an issue, but migration will feel it

[vernal dust]

Surely as the dev you'd find it pretty quick

[silent root]

You have a stack trace for that issue:
Crash, Fix, load up, run, crash to something else, when in this case we could have just given you an error to work with and still let you keep testing

Loading the game is not a "few seconds" for many mods

[vernal dust]

Maybe a flag then

The point is to be real loud in dev by default

[silent root]

Oh, it is loud

Again, if someone doesn't have their log open at all, then taht is kinda on them

This is the stacktrace printed as if it did crash+throw

But allows you to address the problem without restarting

[vernal dust]

Tell that to the dozens of mods that hit prod with thousands of lines of debug and recipe error logs

[silent root]

I'm not sure those would fix this even if we crashed

[vernal dust]

Log = "please fix, maybe"
Crash = "FIX this. Now."

[silent root]

There is a definite difference from startup log and in world log

the startup, even if you didn't want to ignore it, it is at times hard to fetch the errors given how many things are logged.

Once in world, the log is usually the timer saved state

[stone dragon]

Better it makes a user upset and the issue is found, than mysteriously deletes items in prod and goes unreported or unnoticed, imo. For anything like this, I'd say it should start with a contract of how the method is expected to behave, and then make sure it always behaves that way, without exception

In my mind, a sensible contract is either "produces a matching itemstack with the given count", which is what the vanilla method does, in which case any logging or erroring doesn't have a place, or "produces a matching sensible itemstack with the given count", in which an invalid count is an error state that cannot be sensibly recovered without breaking the contract, and thus should throw with the relevant exception.

I just don't see "produces a matching itemstack if possible, otherwise deletes items" as a contract you'd want when the others are options

[flat root]

Vanilla allows having large stacks and this method should allow it too. There is no need to guard against that here

[silent root]

Serialization wouldn't be supported though?

ExtraCodecs.intRange(1, 99).fieldOf("count") from ItemStack

This includes network packets in theory

Everywhere they call copyWithCount, it is either a hardcoded number, or they do a max check before it

[untold eagle]

ehh no need to babysit. If they're calling it with a larger stack size than possible or not checking thereafter its on the modder

[flat root]

(There are many mods that manipulate large stacks internally)

[silent root]

I can remove the check, but those mods can also just use ResourceStacks

[flat root]

I'm still undecided on those stacks

[silent root]

They are incredibly useful, so very against removing them at this point

[flat root]

Part of the problem is really the extra weight in a PR that is already incredibly large

[silent root]

@cunning mulch for the "sub" transactions, were you thinking of something like this?

default Transaction open(){
    return  TransactionManagerImpl.MANAGERS.get().open(this);
}

It is a shame we can't have a static method and an instanced method be the same signature

Then we could do

 static Transaction open(){
        return TransactionManagerImpl.MANAGERS.get().open(null);
    }

    default Transaction open(){
        return TransactionManagerImpl.MANAGERS.get().open(this);
    }

in one class

Though, we could move the static into the Manager really

[loud stirrup]

I am necro-replying, can you check what Fabric does for this?

(Unless this was already resolved)

[silent root]

You'll have to do that for me as I have no idea how Fabric is laid out but for now I am just removing the check and hoping no one breaks it (I expect people to break it )

[loud stirrup]

Just google itemvariant 😛

https://maven.fabricmc.net/docs/fabric-api-0.97.9+1.21/net/fabricmc/fabric/api/transfer/v1/item/ItemVariant.html#toStack(int)

[clear tusk]

https://github.com/FabricMC/fabric/blob/8ea25362f47bbd552ef4e5db68f3cf58d7bf7c74/fabric-transfer-api-v1/src/main/java/net/fabricmc/fabric/api/transfer/v1/item/ItemVariant.java#L105-L108
looks like fabric does not limit their to stack method

[loud stirrup]

Yes, Javadoc says as much "It may lead to counts higher than maximum stack size." 😄

[noble latch]

I personally would be in favor of oversized stacks going bang immediately, at the very least ones that are over the absolute max stack size of 99 because those will blow up if they manage to reach serialization, at which point debugging becomes a pain in the ass

[copper fjord]

there are mods that have their own serialization to bypass vanilla limits and store bigger stacks

[loud stirrup]

The bigger issue is imho that you can easily void stuff if you silently clamp

[stone dragon]

Silently (even "silent" in the sense of "logs but does not throw") clamping is imo the worst possible option. Either don't do any check, or do a check and throw if it's outside; heck, those could totally be two different methods, toStack and toStackUnchecked or the like.

[silent root]

I do have an interesting one, in regards to memory. In say the itemstack, there is a @Nullable entityRepresentation. Should we clear that when turning it into a itemresource? My fear is that once it is an itemresource, it can no longer be cleared for that resource

In THEORY, it should only ever be null when going in, but I have seen mods do ... silly things in the past

I'm fine either way, but it would be trivial to do something like

private ItemResource(ItemStack innerStack) {
    this.innerStack = innerStack;
    this.innerStack.setEntityRepresentation(null);
}

[noble latch]

The safer solution would almost certainly be to either copy the stack or have ItemResource only store the stack's underlying data it actually cares about (and copies thereof to be safe) instead of the entire stack

[silent root]

it does a copy if the default doesn't exist

[loud stirrup]

Shouldn't that usually come from a copy?

i.e. it's private, the question is where it's being called from

[silent root]

So in theory it isn't set in copy is it

[loud stirrup]

It shouldn't be

[noble latch]

Correct, copying a stack ignores the entity representation

[silent root]

cool cool

The ItemResource file if you wanted to peruse it, the entity rep was just something that came to mind https://github.com/CodexAdrian/NeoForge/blob/7417a9adaf341d43a153daddddbdf5f33f34fb79/src/main/java/net/neoforged/neoforge/transfer/resources/ItemResource.java#L118

During normal gameplay, we only create new resources should there be patches on the stack

otherwise it takes the default

[crude zephyr]

the size of IResourceHandler is dynamic or fixed? some container is weird, its size will be bigger after insert

[stone dragon]

Yes, please, it better not be fixed. I have a container that is going to shift in size between inserts

[silent root]

The templates are fixed, but dynamic is fine, my only qualms is if done before commit as that will lead to problems when extracting

[loud stirrup]

one clarification we attempted in the closed PR: if a container has dynamic size, it should try to emulate the slots between current_size and its maximum size to be empty, instead of throwing an index out of bounds exception

[crude zephyr]

I'm wondering if it's possible to add a method to differentiate between dynamically sized containers, they should be handled differently than fixed sized containers and fixed sized containers seem to be easier to optimize and handle

[silent root]

You just need to handle cases where if someone is inserting, expect them to be iterating until a transaction is completed really. So really, if you are dynamic, you can't really throw out of bounds.

(unless negative)

[crude zephyr]

but if i want to insert two or more stacks to a resource handler, how i can know whether i can insert them all, normally i don't want to insert a part of the stack, i want to insert them all.
due to the size is fixed, i don't know its internal "max size"

[silent root]

You just do ... insert(resource, amount, transaction) and determine what the value is. That is irrelevant of dynamic sizing. BUT
If say a consumer is doing insert(index, resource,amount-current, transaction) My point is that while they iterate through the size you gave it is expected to be static from their start

so

  int size = handler.size();
  for (int index = 0; index < size; ++index) {
    ...      
  }

THAT shouldn't break whether it is dynamic or not

You shouldn't expect someone to be reasking for the size every index

Since it is transaction based, you can add as much as you want until you finish, see the result, then decide if you commit or not

https://github.com/neoforged/NeoForge/blob/72a5b30756561cf5c28b4a8745dcc68ee7ecfafb/src/main/java/net/neoforged/neoforge/transfer/ResourceHandlerUtil.java#L365-L419

That for instance is the util for moving between 2 handlers

[hollow maple]

I might be late to the discussion, but is there any reason why Transaction is an interface as opposed to a (potentially final) class? As far as I can see, we only ever use one implementation and as far as I can imagine, there'll only ever be a need for a single implementation - If we expect users to do something like Transaction.open() IMHO there should only ever be the one implementation

I mentioned this on the GH PR under the TransactionManager stuff, but it feels like designing for a scenario that will never happen by design

[flat root]

It can be turned into a final class, yeah

Arguably hiding the implementation details in a separate class has its advantages, but putting the API and the impl in the same class is more aligned with what we do in other places in NeoForge

[cunning mulch]

Yeah that makes more sense to me given how we do things in Neo.

[void thicket]

I never really liked the concept of having a single-implementation-only interface

I feel like it's a remnant of API design imposed by C and how headers work

[flat root]

There are cases where it makes a lot of sense. For example AE2's grid services

But it also depends on how you organize your API. It's quite common for separate API packages (e.g. AE2, Fabric)

[cursive geyser]

single-implementation interfaces are useful for providing upgradeable API. version 2 of the API can then implement a new set of classes while providing backward compat with v1

[cunning mulch]

yeah there are cases where it makes sense and is beneficial. But in this context, I don't think it provides much benefit. Especially as we tend to encourage people look at vanilla's impls of things, so having it in a single place may help improve understandability of people (for if they have any confusions about specific parts of the javadoc)

that being said, a different reason we might want to just have them separated for this case, is that our impl of Transaction is a non-static inner class of TransactionManagerImpl, so it might be a little more convoluted to move out. But then again, we could just explicitly pass the manager to the constructor, instead of having java do it automagically

[flat root]

It's not a trivial move but it's not particularly difficult either, indeed

Did you finish your first pass through the PR?

[loud stirrup]

What was this discussion actually about?

[stone dragon]

I would argue they should re-check the size after each operation of a transaction, but I may be in the minority there

[vernal dust]

Far better to mark the storage as dirty and do a single validation run after X ticks instead

That way you don't do 10 checks when a pipe is actively pulling out

[stone dragon]

What?

If that was in response to what I said, I think you're misunderstanding what my inventory looks like, really; it's a special queued buffer, where the ordering and number of slots has to change after each operation to make any sense. But I now realize that the "iterate and try extracting from multiple slots" approach people take is... going to cause me some pain no matter what, I fear, because it means even if I try to shove it into a somewhat-statically-sized inventory things interacting with me may fail to pull out the right number of items if they do that loop. Ah well.

[vernal dust]

Imagine the scenario:

• Pipe: Extracting!
• Storage, tick 1: Here's a stack, checking slots...
• 2: Here's another stack, slot check again,...
• 3, 4, 5, 6, 7, 8, 9, 10 -- Repeat 2

Versus:

• Pipe: extracting!
• Storage: here's a stack, will revalidate sizes once things calm down
• 2 - 10 stacks: given
• 10 ticks after final stack extracted: Okay, we're gonna fix our sizes now... okay cool we only have 1 slot

[stone dragon]

Okay but you're missing the point entirely. Scenario 2 is not valid here as which slots exist is a function of what items the inventory contains. It has to re-figure-out its slots after each transaction

It's not just "resize to fit contents"

It's "ah, slot X has 3 items instead of 5, that means that slot Y is available to be extracted from when it wasn't before"

[vernal dust]

Sounds like you're combining screen and storage logic way too tightly

Recipe logic, too, probably

[stone dragon]

Wut. No this is entirely unrelated to screen logic

[vernal dust]

The crafting table doesn't change the number of slots for slotless/2-ingredient recipes

[stone dragon]

Screen logic just has a static 9 slots no matter what

But the way those slots are interacted with by... whatever interacts with them other than the screen -- the ordering they should be in in terms of priority to pull from, which are even available to pull from -- is a function of the slots' contents

That's, in fact, the whole point of this particular inventory -- it's a programmable buffer of sorts

Regardless -- I'll squish it around until I find some way to mostly-sensibly represent it that most inventories work with. I did it with the old system too (though TBH it sucked; transactions will help). I do really dislike the assumption that anything about an inventory stays unchanged after an insertion or extraction though

Like, if you're going to extract 10 items from an inventory -- it may not be correct to iterate the inventory and extract until you get 10 total! Because the earlier slots that you skipped because they're empty, may become full after extracting from the later slots. I know people will make this assumption anyways, and I'll engineer my handlers around this, like always.

[cunning mulch]

Nah, just started today doing more of the api side (such as the javadoc’s description of intended impl) rather than random impl stuff that I had looked at previously. Hope to finish up this week/by the end of next weekend as I will have more time to just set aside.

[vernal dust]

Then I'd have two handlers, one that's a static 9 slots that matches a ghost inventory (insert only, no automated extraction), and once you match a given/amount it switches to a static item handler that only allows for extraction

But trying to add and remove slots and change inventory size is asking for pain

Have the ability to lock the buffer down and secure your storage size

[stone dragon]

they're on the same block though. So... which do I expose to someone? Besides which, the insertion properties are also dependent on the contents. The only part that is "static 9 slots" in any way, shape, or form is the GUI, which is unrelated to the handler...

[stone dragon]

...right but that defeats the point. Of a programmable buffer

The point is that you program how it lets stuff through it, as a function of its contents...

[vernal dust]

Right. You wouldn't have the capability until the buffer is actually programmed and locked in

[silent root]

That would be expensive in some cases; remember many inventories (various chests) have over 100 indices.

And then you need to take into account a player can have several hundred of these

Dynamically sized containers, are fine, but you will need to be careful with the implementation as the consumption will be (and should) assuming that the size won't change during their inserts/extracts in a form of crashing. They may not get anything when they extract and you move, but do remember, even if we were to check size, if you shift all your indices down, we'd skip and we'd have no idea we are doing so. Hence why I'd recommend holding off size changes until after a commit

Adding is fairly safe, you just shouldn't expect the iteration to follow those increases until the next time they inquire

[stone dragon]

Yes, but checking the size after each insertion/extraction should not be an issue -- i.e., you make a change and then check if the size has increased/decreased. If people did that I could support much of the other weirdness with ease -- if I need to make new slots people can interact with after a transaction, I just shove them after the existing ones and grow the representation!

Since the "slot number" doesn't really matter more than "this is the order these get checked in"

The issue is that if, say, someone wants to extract 100 items from this thing in one transaction, and it's programmed to supply one A, then one B, at a time, alternating -- the only sensible way to model that is with 100 slots. Which is fine, but obviously I can't have that statically -- I need to keep exposing more slots as necessary until the transaction is committed.

[silent root]

You ask most people here, and I'm fairly confident you'd have them recommend caching a size before iterating it. I know @cunning mulch definitely would. You should not expect them to though.

You don't need to have you indices be 1:1 exact for what you have, you could have it so when nothing is in, you have a size of say 10

then when you hit adding the 11th you add 10 more

Removing, wait until you commit

[stone dragon]

Yeah I just really question that. If the container implementation is simple enough, then fetching the size isn't going to be expensive at all. If it's complex enough that that operation involves more, then... there's an issue

[silent root]

We have no idea though.

I have seen containers bring servers to their knees, because they were doing silly things in a for loop that could have been cached

You also have no idea what structure size() will look into

[stone dragon]

Right, but, if someone pre-fetches and caches the size before iterating it, then that's an issue -- because they see 10 things, they extract from 10 things, and then they stop because they think there's only 10, when in fact there's 200

[silent root]

That is, in fact, what will happen

[stone dragon]

Which is an issue

Because that's not the expected behaviour here, if the extractor is something that extracts up to 100 items at a time -- there's 100 available, it should extract 100!

[silent root]

Why are you extracting and increasing the size?

I think... you may have your thought process mixed up here XD

If there are 100 items, and the prefetch they will see 100, iterate 100 and call it a day

IF you add new slots as they extract... that sounds ... like you are creating things randomly

[stone dragon]

I can't know how many things there are to extract at the beginning of the transaction, though?

Because they might insert more

Like, example, my thing has 64 items. It dispenses 2 at a time, alternating between different kinds of items or whatever. That means I need to expose 32 slots to have stuff in the right ordering. But they insert 10 more items during the transaction -- from that point forwards it needs to expose 37 slots, assuming nothing was extracted before then

Even ignoring that, this thing can store 9 stacks of items. If it's set up to dispense round-robin 1 at a time, you would need 576 slots to represent it if it's full! That seems... unnecessary, and slow for someone who's not interacting with it and is instead just trying to get a count of what it contains

I can make something that works in most cases, I suspect. But I'm not really sure how to cleanly model such a thing, if people are pre-caching the size

If they weren't, I'd model the full thing with just 9 slots, and each time something is pulled from the "unlocked" slot, empty that slot and expose the remainder of the stack in a new slot at the end. And then it's fast to look at the contents (you just look at it) and reasonably sensible to insert/extract from both from the handler

[silent root]

you'll need to model it so it can work with something like the move in resource handler, most util methods and similar precache

If their goal is to just insert a specific resource they can do so indexless

[cunning mulch]

if you are looping the size in the first place, you are in general going to use indexed insert/extracts

[stone dragon]

Yeah my point is that that's impossible for me to model without the possibility of a ludicrous number of slots

[cunning mulch]

so the case of it potentially adding more slots to your thing shouldn't be particularly likely

[stone dragon]

I think you're missing some context here; doing an indexed insert/extract does add more slots to the thing in this case

[silent root]

Realistically, dynamic indices will be suboptimal in some scenarios. Which isn't uncommon when comparing resizable structures to fixed structures

[stone dragon]

The issue is, effectively, this extreme case -- I really don't know how to model this, in a way that works with people pre-caching a size

[silent root]

because again, take extraction, do you remove indices when they are "dead"?

[stone dragon]

The indices would stay for the transaction so that iteration is sensible

To be clear -- if I use just 9 slots and add more as interactions happen and they're needed, it's easy and efficient to get a count of what the inventory contains, and it works fine if you iterate it without caching the size

[cunning mulch]

wait if someone tells you to insert into index 1, why do you add an index 9

[stone dragon]

Extraction is the real issue -- there's only one insertable index anyways for reasons. But if someone extracts from slot 1, they might only be able to extract 2 items even though the slot contains 20. They might be able to extract 2 more though after they extract from slot 2 -- so, I would model that by, after the extraction from slot 1, instead exposing that slot in a new slot 10 so that when iterating, it'll be re-visited after slot 2

Which works, and works well!

Except not if someone pre-caches the size

Unless I make the size "big enough", guaranteed, to begin with

Which requires a size of, in the worst case, 576 slots

[silent root]

When I say precache, just to confirm I mean in that instance of the call...

So size()
then iterate
then when they get back they will call size again the next time they go to iterate fully

Not store it indefinitely

[stone dragon]

What I mean by precache is java int size = blah.size(); for (i = 0; i < size; i++) { // maybe extract stuff } Which will not work in the case I describe unless I expose hundreds of slots. However, if they instead do ```java
for (i = 0; i < blah.size(); i++) {
// maybe extract stuff
}

[cunning mulch]

people already precache in that way

how do you handle it now?

[stone dragon]

Currently? I don't, and things that do that just... pull out less stuff than they're able to in a given tick

[silent root]

The simplest form of the index is to access say an array

[stone dragon]

So either I (a) force people to iterate 60 times more stuff than they need to or (b) I have something that's just incompatible-by-default, in effect, with anything that works that way

[silent root]

You CAN do it so that you have a list of size 9 and have 1000 indices (please don't)

[stone dragon]

Yes. That's the alternative I'm trying to avoid

[silent root]

So you just modulo through it when people go to do things

But dynamic sized thing is going to be worse

1000 indices is too many

[stone dragon]

I would need 576 indices if I wanted to make it work with people who do this

[ripe crest]

is there a reason indices need to be available, or could an iterator work?

[silent root]

which if you no-op for most of them, it would be fine as you aren't allocating

Iterator would be slow in a lot of cases

For where we are at

And allocate unnecessarily

[cunning mulch]

the issue is callers might do their own logic before the call

[stone dragon]

Would be fine except that (a) now someone who wants to look at what the inventory contains is looking at 567 empty slots and (b) now anyone who assumes every slot is a "normal" slot in terms of displaying inventory contents is... very confused

[stone dragon]

And this too

[silent root]

true

[stone dragon]

I really do not care how my inventory is implemented, I just need to be able to model this in a way that plays nice with others

[silent root]

But ultimately, having the handler have this round robin concept I'm not sure it is on the handler side's control

[stone dragon]

Which I currently cannot do, and which I will not be able to do if people assume static sizing while iterating.

[silent root]

At least in terms of inserting by index

[stone dragon]

It very much is on the handler side control. Every slot that isn't "next" is locked and prevents extraction

[silent root]

If I say extract(cobble) that is MUCH easier to allow the handler to choose

[cunning mulch]

is there really that big a downside to it taking two ticks instead of a single to have the extraction happen?

[stone dragon]

Well, short version: other people's fast pipes are no longer fast

And the whole point of this block is as a sequencing tool in automation setups

So yes, it would be nice if it worked with transporting ludicrous numbers of items in one tick

[silent root]

Like... sequencing tool to divy up which handler gets something?

[stone dragon]

Sequencing tool to enforce that items are output or input in a particular exact order

[silent root]

ah

Good luck, but I will say, sadly for you,

  int size = from.size();

is unlikely to be going away

[stone dragon]

At its most basic, it enforces FIFO ordering for insertions/extractions

[silent root]

As there would be no way to enforce one way or another

[stone dragon]

Well then I really don't know what to do other than exposing 500-odd slots, I guess. Ugh

[cunning mulch]

yeah, I think the best you can hope for is one tick delay between items going into it and items going out

[stone dragon]

How slow would an iterator approach be, in practice?

[stone dragon]

I know that; all I'd like is (a) built-in stuff not doing that an (b) some javadoc warning that the size staying static within a transaction is not guaranteed

(Basically, I want to be able to say "sorry, that's someone else's problem" if someone implements their pipes or whatever in a way that causes issues with my setup)

[cunning mulch]

more annoying thing imo would be not being able to use enhanced for loops as you would have to have a custom iterator in order to be able to have an insert/extract method on the things to target specific slots

[silent root]

The built instuff needs to be performant for 90%use cases

[stone dragon]

Couldn't you use enhanced for loops exposing a single-slot thing or whatever? But yeah, I suppose I see the point

[vernal dust]

You should not be changing state that much during an open transaction. Especially not changing the size of the inventory.

Only on commit.

[stone dragon]

I am... a bit doubtful that pre-caching the size like that actually saves you anything in 90% of use cases. It's just going to be a final field fetch most places I'd assume?

[vernal dust]

Otherwise you ABSOLUTELY WILL screw other mods up

[stone dragon]

I wholeheartedly disagree with this. Why the heck not?

[silent root]

You havent seen most mod inv sizes then

[vernal dust]

Because that's what transactions and commits are FOR

It guarantees you only COMMIT state when you commit it, otherwise you can roll back

[silent root]

Commit only plays after all are done nano

Which means sub transactions as an example would flare a problem here

[stone dragon]

Yes, and you can roll back here. I don't change the actual block's state, I just change what the representation exposed in the transaction looks like after each extract/insert...?

Which is totally allowed and should be expected

[stone dragon]

Hmm. I'd still say worth a benchmark. It's just a final field fetch in most cases. Or a constant even. I guess the dynamic dispatch is the slow bit? And the JIT probably can't do too much here

[ripe crest]

a bundle is a good example - for a static size it would need to declare 99 slots because thats the max capacity, even though typically it needs much less than that.

[stone dragon]

Hmm, actually, good point -- how are bundles implemented at present with this?

Do they just always expose however many slots their max is?

[silent root]

There currently is no builtin bundle support (waiting on 21.6 for nbt changes) for me at least

[vernal dust]

Bundles are an inventory only thing, I don't think they even have item handlers

[stone dragon]

Well how do you plan to implement it? I figure as long as I match whatever it does, I should be fine

[stone dragon]

why wouldn't they...?

[silent root]

Bundles iirc have a fixed size

[stone dragon]

A bucket has a fluid handler, after all

[stone dragon]

They have a fixed max size

But their actual size depends on how full they are

[vernal dust]

Because the only way to add/remove items in vanilla bundles is to interact with inventory slots

[stone dragon]

They're not really anything like a chest or whatever

[stone dragon]

So?

[vernal dust]

So there's no item handler backing it

It's just the vanilla inventory management

[stone dragon]

...well no, item handlers are a modded concept, aren't they

There's no fluid handler backing a bucket normally either, until neo adds one

Despite the only way to interact with it being, well, placing or removing the water, normally

[silent root]

Keep this civil. I personally havent looked at the bundle for this. When we were first writing this it was still experimental

I can look again, but it will likely use the max size

[cunning mulch]

doesn't mean a modded block can't input into them

[stone dragon]

Hmm. Okay. I can do that for my inventory too then despite the max size being potentially far larger

[silent root]

Just becareful with how large as this is assuming 64 for the bundle

[stone dragon]

Or, well, I'll just match whatever the bundle ends up doing and call it good, because at that point it's Someone Else's Problem™️

[silent root]

Mmmm would say it is still a you problem :p

[ripe crest]

it needs to be 99 because thats the stack size limit of items, 64 is only the default stack size

[silent root]

Bundle isnt sequencing at all really

[silent root]

Fair, it was going to pull what ever const they have

Or if data component driven

[vernal dust]

If you recalc stack and storage limits every time something calls your handler, you're gonna light up on spark profiles REAL fast

[stone dragon]

It won't always be 576; that's just the worst case scenario. I'll start at a max size assuming extractions only and then dynamically grow it if there's insertions, which should keep the max size... lower, except if the buffer is full and it does stuff one at a time, and it'll still work in the majority of cases

[ripe crest]

they dont have a const, thet have a list of items and a calculated weight as a fraction

[silent root]

So as an example, if we did that, THAT would be an example of why we should cache

[stone dragon]

I think you're misunderstanding what this case looks like; I've explained the exact issues above. The actual work done on a given insert/extract is minimal; it's just that I may expose far more slots than are in the actual backing storage mechanism for the inventory.

Well, 576 in the worst case is kinda sucky but still eh, whatever, the worst case is not the common one (hopefully). My bigger worry is that something like one of those overlay mods that shows you the contents of inventories may assume that all the slots present are, like, "normal" slots and all when they're not, but there's really nothing I can do about those sort of cases anyways

[silent root]

Really the weight of that isnt high given there are only 9 stacks yes?

[stone dragon]

Hmm? Only 9 stacks, but if all are full and it's set to dispense one at a time (and it needs a fixed size for extraction) then that's 576 slots needed. Unless I'm misunderstanding?

[silent root]

I mean it is a list(or collection) of 9 elements.

So you are already better off than having a list of 576

[stone dragon]

I mean, I was never planning on having a list of 576 . The bigger worry is that anyone looking at the inventory has to iterate all 576 even if there's no reason for them to

[silent root]

You just are doing different accesses to a low element vount list which means it should be fine

[stone dragon]

Yeah, but don't mods like WAILA or whatever the modern things are use these capabilities to figure out what things are in a container?

[silent root]

The heaviest thing will be the resource matching filter and is empty at index

Those are handled on your side

[stone dragon]

Ah, are they? Well, that's fine then

[silent root]

You need to huild custom integrations for custom things

If you dont they will default to what they do

Jade I have a random block tell me it is a tank because xy tanks dont have a backing block

[stone dragon]

I assumed the default was, just display all the slots exposed by the capability

[silent root]

They are made of what ever the player chose

Possibly is, but most cap it

As 576 is not the largest inv Ive seen

[stone dragon]

Sure, okay. And anyone trying to figure out the contents of the inventory still has to iterate it which seems... silly, but eh, what do I do

Plus, someone trying to insert who, for whatever reason, can't, but is trying by index, is going to iterate a lot for no result

[cunning mulch]

But hey, at least you can improve the performance of some for the people who are inserting with the new non indexed method

[ripe crest]

what about a helper method that gets all contents that defaults to iterating but can be overridden by invs to be more efficient?

[stone dragon]

Yeah fair enough

(Heck, for folks using the new non-indexed stuff it's all trivial to implement)

[cunning mulch]

Given I suspect (I won’t actually know until I play with it some and am porting to it), that in general people will use the non indexed methods now that simulating is just done by not committing the transaction

[stone dragon]

Hmm, fair point

[cunning mulch]

And the main cases they will used indexed is for implementing their own handler and iterating its indices

[stone dragon]

So it's fine for all those cases and the other cases can just suffer looking at an inventory that looks 64 times bigger than it actually is in the worst case, alright. That should be fine

I can work with that

Would it be possible to stick a note in the javadoc noting that there is no guarantee that the size, or slot contents, or whatever else remain constant when you do some operation in a transaction? Just so to make it clear that "this slot had 50 items, but I extracted 5, and now it has 0!" is totally something that could happen within a transaction

(Also I know I've been going on a bit about this so I just wanted to make sure to say thanks to all the folks working on getting transactions working! This all is amazing and I am fully aware that I have a hecking weird use case here)

You all have given me the motivation to resurrect the mod in question, which I gave up on in part due to the pain of sensibly exposing it to other stuff without transactions, so thank you!

[cunning mulch]

As in that querying stored amount, extracting some, then querying stored amount and it isn’t n-x? If so I am not opposed to mentioning that, though it almost feels like stating the obvious (though I can see why someone who hasn’t made or interacted with complex handlers could come to the assumption it will be equal)

[stone dragon]

Yeah, basically that. If you extract/insert some, the actual amount the view of the slot changes by may not be what the handler gave you from the extraction, that's up to the handler. Some obvious cases being an infinite source of something or a trash can, but with more non-trivial stuff possible too

[cunning mulch]

Yeah as I said I at least am not opposed (especially if you figure out what methods to put the comment on), to stating the “obvious” to help people starting out from making “easy” mistakes

[ripe crest]

simplest example i can think of is a block that exposes the inventory of the item inside it

(and for some reason exposes them to the same side)

[stone dragon]

Left a comment on the PR with this; thank you all again!

[flat root]

Ok maintainers, what are your thoughts on this?

• https://github.com/CodexAdrian/NeoForge/blob/feat/handler-rework/src/main/java/net/neoforged/neoforge/transfer/resources/IResourceStack.java
• https://github.com/CodexAdrian/NeoForge/blob/feat/handler-rework/src/main/java/net/neoforged/neoforge/transfer/resources/MutableResourceStack.java
• https://github.com/CodexAdrian/NeoForge/blob/feat/handler-rework/src/main/java/net/neoforged/neoforge/transfer/resources/ResourceStack.java

(note that there is no link to ItemStack or FluidStack here except for conversion methods)

My personal opinion: this feels a bit "too much"

Maybe only the immutable stacks would be enough?

[wind steppe]

I agree, i only initially had immutable stacks

I think the mutable one was mostly a performance optimization?

[hidden geode]

I like the idea that we start to generify this properly

[hollow maple]

I'm okay with keeping them both because it seems like a common use case and this is a reasonably hot path so avoiding allocations would be nice

[hidden geode]

And yeah if you think about this a bit, it is clear that allthough this is a lot, it is kind of what is needed

To properly create mutable and immutable infrastructure of this level

As well as a common super type declaration

For people that don't care

However

You can get a way with an Immutable

That just creates a new instance as well

[wind steppe]

i guess the question is

To properly create mutable and immutable infrastructure of this level
is that needed

[flat root]

I don't see what the mutable version achieves. This is not an ItemStack/FluidStack, it's something else. Only the count can change not the resource

[hidden geode]

Not sure about the performence consequences though

[hidden geode]

It kind of depends on the resource IMHO

There are probably situations where the performance overhead of creating new instances will be too much

And then having a mutable variant is nice

But IMHO it is not needed

[flat root]

The resource is always immutable

[hidden geode]

At least not initially

[wind steppe]

i believe it was created to make a more performant way of handling generic resource storage data

the mutable stacks allow you to keep a list of them for incrementing and decrementing without having to create a new stack everytime you need to change the value of an item in the list

[flat root]

But you still need a new stack if you change the resource

[wind steppe]

yes, but that only happens if a new resource is being added. If you want to add more of an existing one or take away some of an existing one you could opt to modify a stack instead of making a new one there

This is only really helpful in cases of non component data, since components have to be immutable and changing a stack there would be very bad. But in the case of data attachments or something this is more performant

[opaque vector]

Can immutable be added first and then later mutable if there are actual use cases/demand for it.

[wind steppe]

There are already use cases in the PR

[silent root]

Both have use cases, no reason to not add both really.

To pitch the same logic argument: All templates are unnecessary should we remove those?

Not saying we should, just pointing out the awkward part of the argument for "unnecessary"

[wind steppe]

I think we could also simplify it by just removing the interface, making the mutable one extend the immutable one, doing it that way

[flat root]

We should at least have ones that existed in the previous system

[silent root]

Resources didnt exist in the previous system...

[flat root]

Additional ones would be fine to postpone

Sure but the concept of Combined, Ranged, etc handlers existed already. Modders should be able to migrate to the new equivalents

[silent root]

The reason mutablr is helpful, is that it reduces object churn when just changing a value, while keeping the immutable nature of resource

While resource stack is for something like data components that have to be full immutable

[flat root]

I'm not saying we should go and remove them. But it is worth considering how much can be left for subsequent PRs vs what needs to be implemented here

[silent root]

If they are already implemented....? Not sure I folloe that argument

It just sounds like you want to delay reviewing

[wind steppe]

reducing the scope of a PR this large isnt necessarily a bad thing

[opaque vector]

It means breaking up a monolithic PR into smaller parts which in some cases can be more manageable

[flat root]

The PR is huge (+15k lines currently). If we can postpone non essential parts we will be able to do a better job at reviewing it.

There's a reason why nobody has gone through the full pr in one go (that I know of) 😄

[silent root]

If they were dealing with different feature sets, Id be inclined to agree, but given this is pretty much used in various parts not sure it is worth breaking apart now

[wind steppe]

Tho im not really sure what non essential templates we'd remove. The base templates for the item and fluid resources both use the generic one as a base, which uses both the mutable and immutable stacks for the item and non-item versions of the storage and serialization

[silent root]

Pup works on it when she can, I dont expect a full review to be done in a week

I cut out mostly ehat I think we can if you want the templates to thrive

I removed my templates, even though I think theyd be good for new users, as they are also coincidentally hard for new users to understand the back end

[flat root]

I haven't had a detailed look through them yet so could very well be

[silent root]

At this point in the PR, breaking it up, would be more work for both reviewer and pr a like for not a lot of benefit if the goal is to put them in anyway

Given, a lot of these had been around for a year or so. Breaking it up for later isnt really a compelling argument for me

We'l even say half a year to be generous

[wind steppe]

we shouldnt be doing a cost benefit analysis for something that could be in Neo forever. remember that the old handlers were here for like, 10 years

ideally we do this once and leave it at that

That being said, i dont really know what benefit there would be to removing the extremely helpful generic storage classes

[past lodge]

yeah, i started reviewing it myself but got extremely bored after looking at the insanely large diff lol

[silent root]

Im of the opinion templates should be looked at last for any review as they are also last for any actual beneficial element

[cunning mulch]

Tbh the most annoying part of reviewing the templates is that because of the package move the diff for them is the entire file instead of just whatever logic changed from the old templates

[cunning mulch]

I probably could, but procrastination

[cunning mulch]

Good, you can always make a PR after the fact adding more templates.

[flat root]

Well honestly there's very little overlap since the templates were rewritten

[cunning mulch]

Ah okay

[flat root]

I'll do a pass through the open comments later and see what can be marked as resolved

[loud stirrup]

Why is IResourceStack an interface?

Oh. I see

Hm, I don't think that's really needed

[cunning mulch]

well I have half the files marked as viewed now... but if I had to guess already like 150 comments deep given I think I posted around 100 today (unsure how much of the ones I posted the past few days have been resolved though, so just guessing it is around 50 unresolved between those)

[silent root]

At least 30 of those or so are omitted since they are in the toremove package. So you are over half way by quite a bit 🙂

[cunning mulch]

Why does the to remove package exist?

[silent root]

To ensure changes I make maintains the use case be working.

Other part is that there are notes in their for review and documentation makers

[cunning mulch]

Just remove them in a singular commit and put a link to said commit in the PR description. Then if documentation makers want to view them they can, but then the review as a whole is a bit smaller and not wasting reviewers time with files that are not meant to be part of it

[silent root]

I would like someone to look over them though (not all reviewers need to) as these were removed initially out of "fear the pr wouldn't be accepted" Which is something I put in the notes

As there may be usable templates/code in there

[cunning mulch]

I mean, that feels like a prime example of #1183818213134446742 message and something that could probably be in a follow up pr if it is just adding more templates

[silent root]

Removal of IResourceHandlerModifiable breaks ResourceHandlerSlot just as a heads up. With no real way to migrate it

[void thicket]

Is that a renamed version of SlotItemHandler?

[silent root]

Yes

It extends Slot

And the sibling ResourceHandlerCopySlot also breaks

[void thicket]

Well something equivalent will need to be spun back up

Having no equivalent for that fundamental piece is unacceptable

[silent root]

Right... but we need IResourceHandlerModifiable for that

    @Override
    public void set(ItemStack stack) {
        ((IResourceHandlerModifiable<ItemResource>) handler).set(getSlotIndex(), ItemResource.of(stack), stack.getCount());
        //this used to setChanged() are we handling that now a little more sensibly?
    }

(not sure why it is casting (old pr) but the point still stands, we can't just get rid of the thing

I think we should leave IResourceHandlerModifiable in for now; if/when it seems viable to remove, have at it. Until then, it provides some pretty core behaviour for some of the more "required" aspects of the api to function and has been a thing before that is trusting developers don't just call haphazardly.

[cunning mulch]

At least restore the part of it being documented that it can throw if called in a way it is not expecting

[untold eagle]

Personally I'd do it either as a lambda/functional interface that's passed into the Slot constructors or specifically name the interface like SlotSettable (documented as used in menus/containers/whatever)

[cunning mulch]

I like the lambda/functional interface idea

[silent root]

I can explore that one

[silent root]

The problem is currently: when you don't necessarily have owner ship of the handler the slot is for (at least from what I can tell)

So doing a lambda field doesn't quite work given you may not have enough info for what should happen

Personally, I don't use these classes so not 100% certain what is expected of them

[untold eagle]

I don't think these slots should be used in those cases (what would one do normally if they weren't ItemHandlerModifiable?)

[silent root]

Wouldn't instantiate them in the first place. But again, I don't use these and I do use ItemHandlerModifiable

so no idea what people are normally doing

@void thicket is one of the few people I know who has a definite requirement on it (doesn't necessarily imply he uses them though)

[silent root]

In cases before it would hard crash due to a required cast unless you happened to go look at the comment (not the java doc) of the method saying to override it if it wasn't a Modifiable

[untold eagle]

so, sounds like a contrived scenario where it's not actually a problem - forcing the dev to consider how the update happens is much better imo

[silent root]

like this looks like it will work

    @Override
    public void set(ItemStack stack) {
        slotModifier.set(getSlotIndex(), ItemResource.of(stack), stack.getCount());
        setChanged();
    }

But I need to know what considerations there are given this is a completely unknown class to me

(Personally I prefer everyone handle their slots as they need rather than we provide this )

BUT providing a thing that was there in some form is at least doable

I am curious how many people actually do use IItemModifiable in something of (what was it waifu?)

[untold eagle]

yeah that's a badly implemented class lol

[silent root]

Lol yeeeeep

This method... for casting has been there for 9 years

oh

lol

[untold eagle]

But I need to know what considerations there are given this is a completely unknown class to me
AFAIK - someone using one of the templates / base impls to tack a storage on the TE (i.e. letting it handle all the stack list stuff) and being able to have a GUI without needing to remake the (item)handler

[silent root]

So having those keep their sets public likely would be handy

So they can just do Template::set

[untold eagle]

sure, but it doesnt need to be an IResourceHandler interface method - its impl specific

[silent root]

Right

That is what I did earlier, but this class full stopped it

so need to do those changes

[untold eagle]

[silent root]

If anyone is upset at IResourceHandlerModifiable being removed please form an orderly line to access the Thiakil

[untold eagle]

yay, FWENDS!

[silent root]

Fwends!! Armed with fire!

[vernal dust]

Don't forget the pitchforks. Armed with those, too

[silent root]

I took those out a couple commits ago. They weren't valid resources

[ripe crest]

We use hoes for hay bales round here

[silent root]

Thiakil fortunately is not a hay bale

[vernal dust]

[silent root]

Nor is a IResourceHandler<BaleResource> so we are safe

[vernal dust]

Hm. Guess I need to finish Gander, so I can make mirrors. Then we can start using reflection hackery

[silent root]

To what, turn Thiakil into a bale of hay? or unrelated?

[vernal dust]

Yes?

[silent root]

Run @untold eagle

[vernal dust]

So, Soaryn. You're gonna port AE2 to the PR to prove everything works right

[silent root]

Nope

That isn't my mod

[vernal dust]

It's your favoritest mod

[silent root]

Doesn't mean I want anything to do with the porting

I have my own suite of mods to handle

[flat root]

right

[silent root]

I wouldnt want to given I am unfamiliar with all of the internals

[untold eagle]

ed...warrdd

[loud stirrup]

internals? according to our addons, everything is API 🙂

[untold eagle]

sometimes things not in the api are needed 😛

[queen wagon]

well you can use internals, there are just no guarantees it's not going to break

[untold eagle]

indubitibly

[copper fjord]

I've started a review and left only 20-something comments right now but I have to completely agree with this

please remove just-in-case, for-the-sake-of-it, why-not, it's-just-a-small-utility additions or utilities otherwise the PR ***will ***end up holding much more than it should in review limbo because it is absolutely painful to review it

[silent root]

I do wonder if there is a way we can do a "sandbox" where a package is designated in neo as git ignored, but some examples can be made locally to handle refactors and such. That way I can keep iterating them without having to go make an entire separate branch/project for them to ensure compatibility. I was going to remove them this week though given the biggest hold was what we were going to do with IResourceHandlerModifiable

[cunning mulch]

(You hope)

[untold eagle]

You can? Just don't add them to git?

[silent root]

True,

[vernal dust]

Unstaged .gitignore go brrr

[silent root]

Ill be able to get to those tomorrow then. XyCraft was rather easy to mostly (ui not handled and lasers not rendered) port to 1.21.6. So that freed up a lot more time than expected. Going to bed early to commence swapping out some things in the tests where I was using the wrong template handler. Ive also fixed my accidental forgetting of not inverting the IContainerExtension if checks, just havent committed it yet.

[vernal dust]

oh look, found the solution to not having the energy to review this PR /s

[silent root]

@wind steppe I have been instructed I must be benevolently mean to you. That is all, have a nice day

[wind steppe]

huh

[past lodge]

soaryns been allowed to be mean to you, as a treat

[silent root]

Note, you are lovely and do not deserve meanness; it was more to a comment of #neoforge-github message

Nothing to fear 🙂 Just documentation gods are looking down upon us and thinking... "oh no".

[silent root]

@cunning mulch I think I got all of the ones from today regarding ResourceHandlerUtil

[silent root]

@cunning mulch could you check on this one some point today? 🙂 https://github.com/neoforged/NeoForge/pull/1115/files#r2148737629

It is the extractHook in the hopper disc

[cunning mulch]

pass

I will try to take a look at it tomorrow

I was about to turn my pc off for the night

[silent root]

[silent root]

OK! In theory that should be most of the reviews in question. I THINK. Look them over I'll do more when I can (when I have reviews to work with at least) But we may be good to turn on the PR builds in theory

[flat root]

Ah thanks for adding back the old stuff, that looks a lot closer to what I had in mind

[silent root]

As long as we heavily encourage moving OFF the old system sooner than later then it can be fine, but
A: I have no idea if it fully works as those tests are pretty bare bones that were there
B: Wrapping the thing into a legacy handler is non ideal given it loses transactional control besides Do or don't commit, which is in some scenarios wasteful.

Ideally, we remove the legacy utils next minor version (not hot fix which I expect 1.21.7 will be), then legacy handlers the version after that at the latest

[flat root]

Should be kept until the next "big version" included

In any case I have just enabled PR publishing, there's no reason to delay it

[silent root]

[flat root]

People that will try it out: There's still plenty to review (looking at IEnergyHandler which is still slotted 🙄 ) so keep in mind that this is mostly for testing the PR but there might still be many changes

[silent root]

Yes, it still has the indices, and is COMPLETELY usable without needing to worry about indices, @see ISingleEnergyHandler and the utils

They literally do a 1:1 to what you had before

[copper fjord]

whether we will have slotted energy handlers is a discussion still to be had that I will not be surprised if it ends up with a vote

[flat root]

Well I would rather not vote for random features like this

It's pretty clear from the discussions I've seen here that slotted energy will have to go, IMO

[silent root]

That is because everytime you bring it up in discussion: you leave out all of the context as well as the fact it can be used without worry of indices

Try it out

A consumer or implementer don't need to worry about indices if they don't want to, it is an opt-in system

[flat root]

There is no need for context, you're adding a feature that is not essential and that was not really requested before. It feels a lot like self-service which doesn't sit well with me

[silent root]

Any and all requests of IEnergyStorage were always met with "don't touch it"

[flat root]

Don't fix what isn't broken 😉

[vernal dust]

Consistency > Legacy imo

But I do kinda agree that most people probably won't have any use for slotted energy

[flat root]

Oh I think the other proposed changes for the energy API are fine:

• Change naming for consistency with the resource handlers
• Use transactions
• Add optional long query methods

[vernal dust]

That said, just because WE can't see use cases does not mean there aren't modders silently coping with the lack of it

[flat root]

In fact they are good changes

[vernal dust]

So, both consistency and the additional options trump "but why tho" argument here

[flat root]

The big difference here is that there is a single "resource type", which is why slots are superfluous

[silent root]

Not necessarily

As you could have multiple "tanks" in fluid handlers that held differing amounts of one fluid like water

Part of the use case I have was to expose my buffers separately to allow someone else to choose how to distribute energy as they see fit

Rather than they give me an amount and say... "Here... I can't do anything but give you this raw value"

They can still do that of course

All features that were doable before are doable now without a dev needing to worry about it, but it does provide a new avenue for creativity

[flat root]

If all your buffers are equivalent this is completely pointless

[flat root]

The key point is that multiple tanks might hold different fluids and this is why having multiple slots is very important in that case

[silent root]

That is A use case And honestly, I've seen many systems have multiple tanks, but only allow 1 fluid type across all of them before

[flat root]

Examples?

[silent root]

They have dwindled over time, but that is also modded minecraft

I honestly can't come up with one at the moment, as I have played this game for over 14 years (the api has been out for 10), it has occurred.

[silent root]

but the buffers for energy in my particular scenario are in item form

so I can proxy to those per index

[flat root]

So the choice is between filling all batteries at once vs filling the first one first then the second etc?

[silent root]

Allowing the "inserter" to decide

Imagine something like the Redpower manager

where it could do per slot round robin and threshold limits

[flat root]

Cause that sounds like a reasonable player-facing feature, but it would make a lot more sense to have a toggle in the charging block for this than letting the cables handle this

[silent root]

Cables are a single point, but I'm imagining anything that wants to be a "smart" buffer of sorts

"I get power, I want to force evenly split this energy across the items in this chest" (assuming they expose it)

I was very careful in adding this to try to ensure that those who wanted the possibilities could, and those who were "meh" could remain so and not have to worry. The player may ask for the compat layer, but that is still up to the implementer. It wasn't arbitrarily added.

• More possibilities that are not possible atm
• Consistency with the other handlers in function + name
• No change to those who didn't need it

Now... how do I get PR

[untold eagle]

your dismissive attitude is precisely why it should go to a vote tbh

[silent root]

Ah well I see the problem despite pasting the worong thing

I put the PR blob without the maven{} block

I knew I missed something

[flat root]

There is no need for personal attacks. If we hold a vote for each subfeature of the PR we might end up with a completely incoherent final result

[untold eagle]

no one but you said each subfeature

[flat root]

It is a subfeature though. There are many small things like this, and holding a vote for each doesn't make sense

In this case it's pretty clear that other maintainers such as @void thicket and @noble latch are also against slotted energy, so it's not "just me"

[untold eagle]

and holding a vote for each doesn't make sense
which is why no one suggested that...

so it's not "just me"
also not something that was said (not by me at least)

[opaque vector]

If slotted energy is added, how would modders know of its existence and how many will actually make use of it? Trying to see if it ends up being too niche of a feature or something worth the effort to add

[silent root]

It is the parent interface, so when you go to insert you can call handler.insert(amount, transaction) as you normally would, but the auto complete would also show the handler.insert(index, amount, transaction)

So by default all energy handlers have an index, the child interface just simplifies it all the way down to what you had before

IEnergyHandler - index
ISingleEnergyHandler - size of 1 extends IEnergyHandler

This is also inline with the Resource handler
IResourceHandler<T> - index
ISingleResourceHandler<T> - size 1 extends IResourceHandler<T>

[loud stirrup]

I don't see the point for "slotted energy", energy is a singleton resource and countable, "stacking" doesn't apply. splitting "energy stacks" doesn't make sense in my opinion

As such: we're not using the other base transfer API for energy to begin with (since it's a singleton resource), and as such I see no reason to introduce slots for energy

[opaque vector]

Honestly, I think it might be better overall not to slot energy. Energy slots could be introduce in a future PR if there is more demand and use cases. Rn I am thinking of a block that takes in energy and has 6 batteries to fill. The logic for deciding which battery the energy goes to and be pulled from can be handled by the block's own logic. Not the cables connecting to the block. Also might be more clear to users too if they can set the priority and behavior of the drain/charging on the block itself than the cables

[untold eagle]

Having indexes doesn't preclude that block handling it itself either (it can just expose a single index)

[clear tusk]

well im prepped and rdy for transfers, yay for pr publishing

[untold eagle]

What is the harm in having indexes/slots for energy?

[loud stirrup]

Pointless complication of the user-facing API

[opaque vector]

I was thinking the complication is on the internal implementation side. User side seems simple but under hood, do we need the extra complication for maintainability? Especially if only like 1 or 2 people ever uses it

We did remove the minecart events due to lack of use

(And maintenance pain)

[onyx basalt]

Minecarts being painful is one of the most consistent parts of Minecraft code ngl

[loud stirrup]

Well yeah the semantics are very unclear due to no one using it in the first place. Should you try to split inserts across present slots evently? Should you fill the first slot fully, then the second slot? All that becomes responsibility of the caller, and while there's a common pattern for how to do this for items, there isn't one for energy. I don't want users of the energy storage to have to worry about that, really.

[distant merlin]

I wonder if the minecart experiments will prove better or worse in that respect but alas, a topic for another thread/time

[untold eagle]

Have you looked at ISingleEnergyHandler? Iirc it literally provides defaults for the indexed methods and one implements ones that are indexless

[untold eagle]

Erm, how do you handle that for tanks that have multiple empty sub tanks? Is that documented? If it's a concern here, why not document it?

[cunning mulch]

Technically mek has had slotted energy since at least 1.15

Whether I actually make use of it anywhere, that I don’t remember offhand

[cunning mulch]

All of the handlers including items now have methods for insert/extract ignoring the index though. So just use those methods?

[flat root]

I don't see a getAmount() method on IEnergyHandler. I don't want to have to use a util for such basic functionality

The provided implementations are also way too long

[flat root]

Might be good to check. If not then you could just remove it

What was your motivation for adding it in the first place?

[clear tusk]

IEnergyHandler has getAmount(index)

https://github.com/CodexAdrian/NeoForge/blob/6b666e8dc226d6d7db58fb4ca956c6d348c76236/src/main/java/net/neoforged/neoforge/transfer/handlers/energy/IEnergyHandler.java#L33-L56

but ISingleEnergyHandler is missing a none indexed getAmount feel it should maybe have one, using a util for that would be a pain yeah

[flat root]

For a general energy handler it should be easy to get the amount of energy

[clear tusk]

yeah sum up the amounts for all indices, any more complicated handler can just override the method and do what it needs to

[cunning mulch]

Consistency with my other handlers, and I thought it would make things much easier for cases like the induction matrix (but that needed its own impl anyway for other reasons due to multiblock stuff)

[cunning mulch]

Yeah, tbh the util should be a default method instead of in a util class

[clear tusk]

my inventory blocks are not saving to disk when using ItemStackListHandler

my best guess is the onChangedListener (which i have set to this::setChanged for my BE) is not being fired when mutated via ui
since that listener is only invoked for transaction commits and uis/ResourceHandlerSlot directly invoke set (which does not fire the change listener)
so my BE is never notified it changed causing its contents to not be saved

[clear tusk]

Confirmed this as if i use a hopper to insert items (which would go through transactions, which in turn fires the change listener) my block entity is now notified and serializes correctly but now ive found a new issue

when it comes to deserialize the current implementation in StackListHandler throws silently UnsupportedOperationException causing nothing to be loaded

which i think is due to me using the int size contructor which called NonNullList.withSize which uses Arrays.asList disallowing the use of add since the backing lists size is immutable

this implementation should really be iterating the size of the list and calling set(index, stack) insead of addAll(list)

[clear tusk]

tldr;

• when using ItemStackListHandler#set the change listener is not being notified causing my block entity to not save its contents
• ItemStackHandler#deserialize silently throws UnsupportedOperationException due to constructor using NonNullList.withSize which creates a fixed size List internally (use set(int, T) instead of addAll)