#Transfer rework (IItemHandler, IFluidHandler, IEnergyStorage)

1 messages · Page 8 of 1

loud stirrup
#

but it's literally defined 😄

flat root
#

Surprising to you? Probably. Undefined? Definitely not

#

It's a bit surprising but ultimately the most sane alternative

loud stirrup
#

If it wasn't defined, we probably wouldn't even be talking about it

#

Since that's excactly what the old IItemHandler wrapper did

#

didn't talk about it, but ultimately do the same thing.

#

For years, I might add.

hidden geode
#

Just because something was bad in the past, does not mean we don't need to improve it.

loud stirrup
#

Your improvement is crashing in peoples faces

#

Or what do you suggest the constructor does when the flag you propose is true?

flat root
#

I definitely don't want a crash if I am trying to use this helper on a handler I just received from a capability query

hidden geode
flat root
#

that is a different feature request, and is currently out of scope as discussed earlier

loud stirrup
hidden geode
loud stirrup
#

We did? where?

flat root
#

not at all? just not make arbitrary changes to previous slices

#

but if a reasonable patch needs to be made (e.g. like I already did to allow for nullable snapshots), it can ideally be sent as a separate mini-slice

loud stirrup
#

that makes no sense given this code doesn't become immutable once it is merged into main

#

given it will hit in a BC window

flat root
#

you mean merged in feature/transfer-rework?

loud stirrup
#

no, even after all slices are done

#

you can obviously still change it even then

flat root
#

we can mutate it both in feature/transfer-rework before it hits main, and we can also mutate it again in main

#

yeah ofc

#

and we'll likely be adding helpers for a while

hidden geode
#

Fine, if we can at least have a discussion this before this gets merged into "main", I am willing to lift the reject of this slice.

But I don't want to have this in main without having someway for people that pull an arbitrary resource handler from a capability to check for this.

I don't need any of the other characteristics really, except this one, as it alters the behaviour of my code and how I interact with them.

loud stirrup
#

anyhow, back to the topic of undefined.... I was trying to make this point:

when you use utils out of spec, you get an exception for your own code
but if you combine external handlers, what are you going to do, realistically?
if you combine your own handlers, you already know anyway

flat root
loud stirrup
#

eh, we can discuss it, i think there are reasons it could only ever be a hint too

#

given there are proxying handlers that can change while they exist

hidden geode
loud stirrup
#

or rather, their target can change

hidden geode
#

Nothing more

#

Something that indicates to me to be more carefull

loud stirrup
#

sure but then you cant use it to reject construction of a combined wrapper, realistically

flat root
#

if it's a hint you'll not be able to trust it, and should always be more careful

loud stirrup
#

you can use it in your own code to not construct the wrapper to begin with

#

as a hint

hidden geode
loud stirrup
#

but: i think you are underestimating the robustness of how the combined wrapper is defined, IMHO

hidden geode
loud stirrup
#

or rather, in combination with the definition of indexing behavior for dynamically sized wrappers handlers

flat root
hidden geode
loud stirrup
#

Well it is robust in the sense that:
it will not corrupt the state
it will not dupe or void items
and it'll not crash

#

it will not offer a full view of added slots

#

and show non-interactive read-only slots if it shrinks

hidden geode
loud stirrup
#

but i find that the acceptable middle of the road approach

hidden geode
#

But that is simply my opinion

loud stirrup
#

keep in mind that having a fully dynamically sized wrapper is prohibitively expensive to do

#

you need to essentially rebuild your index map on every operation

hidden geode
#

Because it would turn into:

protected int getHandlerIndex(int index) {
        if (index < 0)
            throw new IndexOutOfBoundsException("Index " + index + " is out-of-bounds for combined handler with size " + sizeCache);

        for (int handlerIndex = 0; handlerIndex < baseIndex.length - 1; handlerIndex++) {
            var size = handlers[i].size()
            if (index < size)
               return handlerIndex;
            index -= size;
        }
        
        //Index bigger then combined inventory
        throw new IndexOutOfBoundsException("Index " + index + " is out-of-bounds for combined handler with size " + sizeCache);
    }
flat root
#

you'd kind of want to do a "stable hashmap"

hidden geode
#

Which in my opinion is not that much more expensive

flat root
#

actually you'd literally want a hashmap

hidden geode
#

The only extra cost is the call to size()

flat root
#

in the sense that you can leave out slots that become empty to avoid "moving" existing resources in later slots, and then compact if the "load factor" becomes too low 😛

hidden geode
#

Which is mostly constant as either a return of a constant in memory or a lookup in a variable in my opinion

hidden geode
#

Sure it is probably a bit slower

#

But not prohibitively

flat root
#

I mean if you have a truly dynamic handler like an AE2 disk but still want to expose int indices

hidden geode
#

Sure, I see no reason why you could not do that

#

Even with the solution above

#

Where the combined handler querries your size() to determine the relevant handler index

#

Instead of looking it up in the array

flat root
#

well, different story IMO

hidden geode
#

From the combined handlers perspective?

#

How?

#

Your AE2 Disk would be its own implementation with its own size, no?

#

I assume so at least

#

Or are we talking about something else?

flat root
#

I was describing the internals of the disk with the map

hidden geode
#

Sure

#

There a hashmap like behaviour would be amazing performance wise

flat root
#

the bigger issue with allowing resizes is that we need to be lenient in our handling, which means providing an empty resource to no-op getResource

hidden geode
#

That would IMHO be implementation dependent

#

But yeah

#

If you do a sparse to condensed hashmap approach

#

Then yeah

#

You can implement it as such

#

And condense and change size later on

flat root
#

I mean if we made the combined RH itself re-query the sizes, it would need to be given an empty resource in the ctor as well, which is quite annoying

loud stirrup
#

I'd find it very surprising that the indices would no longer be contiguous though

hidden geode
#

The combined RH would just store an array of handlers

#

Done

loud stirrup
#

No that doesn't work

#

if you want to dynamically size

hidden geode
#

There is no reason why it would need an empty resource anywhere

loud stirrup
#

you also need to adhere to the leniency of getResource/getAmount

#

which means if your overall handler srhinks

flat root
#

to no-op getResource in case you go out of bounds you need to know an empty resource

loud stirrup
#

you need to return empty for the indices that are no longer valid

#

but where do you get the empty from..

hidden geode
#

I am saying: Don't allow the lenient clause

#

If the handler changes size

loud stirrup
#

It literally will lead to abundant crashes in prod if you are not lenient as a dynamically sized handler

hidden geode
#

Then that should be considered a possible side effect of commiting a transaction

flat root
#

that just makes the helper less useful; I'd have to copy-paste the current version in a bunch of mods

hidden geode
#

I mean, we can accept an empty resource

#

I get the point

#

If the total shrinks

loud stirrup
#

the point isn't that we can't, it's that it's inconvenient to use for the sake of the absolute niche use case 😄

hidden geode
#

You need someway to allow for people to ask at least up to the max of all sizes ever

hidden geode
#

For one we only support one type of resource for all the handlers

flat root
#

the point is that it's annoying to pass an extra parameter, when there is an alternative that doesn't require it

hidden geode
#

And I don't see a situation where a modder combines a bunch of IResourceHandler<?>

hidden geode
# flat root the point is that it's annoying to pass an extra parameter, when there is an alt...

I can see that, but then we should document that properly.

The single <strong> on line 17 does not reflect the required impact on dynamically sized handlers this has. It should properly describe what happens in those cases, especially when inserting (aka that the dynamically sized inventory is then not fully accessible anymore, and the combined inventory handler works more like a RangedResourceHandler)

#

I will sleep on this overnight. But right now I feel like we should add some documentation on this, to clarify to users what the concequences are.

loud stirrup
#

Let me see

#

Current text is:

 * The range of indices handled by each wrapped handler is assigned when the combined handler is created.
 * <strong>As a result, later changes to a wrapped handler's size will not be reflected.</strong>
#

That could probably be reworded to:

<strong>The size of the wrapper will not react to changes of wrapped handlers that have a dynamic size after its creation.</strong>
This means indices added to such handlers will not be visible, and indices that are removed will appear empty and read-only.
To adjust to size changes of the wrapped handlers, you need to recreate the wrapper.
hidden geode
loud stirrup
#

I'll run it through DeepL to maybe clean it up some more

hidden geode
#

Be my guest yeah

#

I really want it to be clear that this is not a supported scenario, what the concequences are when somebody does it anyway and how to deal with it. Which that documentation line above takes care of

loud stirrup
#

Revised:

<strong>The size of the wrapper will not change in response to changes in the size of wrapped handlers.</strong>
This means that indices added to such handlers will not be visible and indices removed will appear empty and read-only.
To adjust to size changes of the wrapped handlers, the wrapper must be recreated.
hidden geode
#

Good enough for me

flat root
#

ok let me quickly add this in

#

OK pushed

hidden geode
#

Can you at the same to RangedResourceHandler

#

Which has the same behaviour in my opinion

flat root
#

well, in that case you are already giving an explicit start and end so it's logical that the size won't change, no?

hidden geode
flat root
#

what is not clear about the range continuing to check indices between start and end?

#

I don't really see what else you'd expect to happen

hidden geode
#

You maybe right

#

I had a case earlier

#

Where I thought it did not work

#

I will think about it again over night

#

But it is likely not needed there

cunning mulch
#

I still feel meh about the stored amount being able to be greater than the capacity. Making it clear it might be an overestimate is fine for sure though

stone dragon
#

I have a handler that will have to deal with this; what tech is proposing is sensible enough imo within a transaction (this was discussed earlier for my use case at least).

hidden geode
#

I think it is just not really a great solution, cause the concept of being lenient is not really well defined

#

As long as it is within a transaction it is probably fine-ish

stone dragon
#

Within a transaction is absolutely fine

#

And you should have no guarantees outside of a transaction anyways

stone dragon
cunning mulch
loud stirrup
#

Just from experience, you'll encounter this scenario in the wild a lot

stone dragon
stone dragon
#

Like, they'll keep outputting items if the primary slot has space and if the secondary slot is full, they just... keep filling it past its cap or whatever? I can't quite remember

cunning mulch
loud stirrup
#

Wouldn't it only die if the underlying handler dies?

cunning mulch
#

meh, depends on the "how lenient" the dynamic handler is

loud stirrup
#

Which leads back to what luke alluded to. A dynamically sized handler that isn't somewhat lenient w.r.t. access to now invalid indices will run into crashes in prod anyway

cunning mulch
#

true

stone dragon
loud stirrup
#

I think it's actually good we specify what a dynamically sized handler should do to avoid those crashes

stone dragon
#

Basically -- the highest slot a handler can handle should not decrease during a transaction, that's the leniency in question

loud stirrup
#

In the past, people had to figure that out the hard way (or just crash modpacks a lot and dont care, lol)

hidden geode
stone dragon
#

(Obviously it might become a no-op or something, that's the sensible way of handling this dynamically)

stone dragon
cunning mulch
#

and it isn't really "a lot"

#

just certain edge cases

hidden geode
#

As you can trivially recompute the public size when the last transaction shrinks

stone dragon
#

Yes as I have been saying this level of leniency is fine to expect from dynamically sized providers, and it avoids footguns for people who don't consider that case. Such providers growing may still act weird in some cases, but shouldn't be too bad in general.

#

Wait wait -- Is the leniency that the provider has to handle such slots outside its max size within the transaction, or that it has to report it's max size as being higher than it is within the transaction?

#

Oh yeah it's just the former, isn't it

loud stirrup
stone dragon
#

Yeah @hidden geode you don't even need that, this is trivial to support -- you just update the size whenever you normally would, within a transaction or whatever, and just no-op on slots higher than your size in general. That's likely what I'll do at least

cunning mulch
#

or it is definitely abusable by players to get the upgrade's effects for free

flat root
#

actually, MI also has a case where a slot might store an amount larger than its capacity

cunning mulch
#

yes, I am writing a response to one of your comments on github that I think will semi clarify my stance

#

as I understand the case of amount larger than capacity

flat root
#

this happens if the player reduced the capacity of a slot, but an ongoing recipe still needed to put its output somewhere, so it just ignores the player-limited capacity

loud stirrup
#

this is just one of those

#

Hm, or rather, this is stored amount>capacity, I don't even know what effect that really has on callers

cunning mulch
#

that's not what I am arguing about

#

especially now

#

if (amount >= capacity) then insert -> return false

#

at least if capacity is not say max long

#

that can be special cased

#

as I do see the reasons why the amount might be higher than the capacity. I might not like it, but I understand there are use cases or situations

#

I really don't like the idea that getCapacity is 100% useless as a method

#

because at that point, like why have it

loud stirrup
#

Well I wouldn't say useless

#

For heuristics, mostly

flat root
#

simple example: the comparator output

#

or checking if a handler looks full (e.g. for a BC gate)

loud stirrup
#

i.e. prioritize based on free capacity remaining, which a hint is fine for

cunning mulch
#

yes, which is why I want the stronger heuristic that people can assume that it won't accept more if it is at or above the capacity of the index

#

so that the heuristic is much more useful and consistently useful

flat root
#

I think even for your pipes it wouldn't be less efficient to just call insert directly, since the insert will usually redo the same capacity check internally

cunning mulch
#

for the new system, potentially. And I am definitely going to rewrite it to make use of transactions and stuff. But my argument still stands, why not make the heuristic much more useful

#

what is the problem? Just overshoot or return max long if you really don't want to calculate it or if it is super expensive, how is that different for the implementer than potentially undershooting it

flat root
#

my general opinion is that heuristics to avoid calling insert are suspicious, and we shouldn't encourage people to use those

stone dragon
#

Half the point of transactions is that you don't have the whole "check if I can do this; try to do this; do this" cycle; instead you just do the interaction you want to and see from the result whether it was a sensible thing to do. And if you just want to check if you can -- well, transactions let you simulate that

cunning mulch
#

I am not saying we should encourage people to use it vs just calling insert. I am saying why it would be useful to have a more strictly specified heuristic bound

flat root
#

It's a bit weird to link amount >= capacity to insert precisely because you should never do such a check before calling insert, imo

stone dragon
#

And it's a bit weird to have heuristics for when to call insert because the result of insert is that heuristic

#

How does a handler report that you should/shouldn't insert something? By letting you insert/not insert something

flat root
cunning mulch
#

I mean yes, this likely is less of a problem with the indexless inserts, as then you really aren't using the capacity or not. But if the capacity doesn't represent anything except that "hey this might be the capacity, but I don't know so I just pulled a random number out of a hat that may be lower or higher than the actual capacity", then it doesn't really let you do any sort of logic with said result. Heuristic or otherwise.

Like with what you are saying and arguing for, I could have a handler that just always returns 10 for the capacity, as that is what it will be in most cases, and who cares about representing the case where it may be 100, I don't feel like it so I will just say it is 10 as a single return statement is easier for me. Then that gives you no information towards being able to calculate said comparator signal, because the number is meaningless. You need to call insert regardless to see how much it could accept to see what the "full" state is and then compare current against that

stone dragon
cunning mulch
#

that's not my point

#

that's just an example based on when I currently do (where transactions likely would mostly if not entirely solve it)

stone dragon
#

The capacity is meant for stuff to report "how full is this compared to how full it could be". Like a comparator or, or some tool for telling how full your storage is

#

It's basically just a metric of "this is what 100% full should be"

#

That doesn't mean it can't be more full than that; it just means that generally you wouldn't expect it to be

cunning mulch
flat root
flat root
#

it is fine to use it when you ask yourself what the capacity of a handler is; but there is no relationship to whether you can insert or not

stone dragon
cunning mulch
#

you still have yet to give me an example against it, as to why you would want to return a lower capacity than can fit

#

yes I agree that you probably shouldn't especially with the new system be checking if it will be able to insert based on the amount and the capacity

stone dragon
cunning mulch
#

which at no point says you can keep adding more after the fact

#

or that it would support increasing the amount further

stone dragon
flat root
#

we can add the contract that amount >= capacity means that insert will return false, but what will this buy you, except doing a pre-check for insert which you should not?

#

if we add this to the javadoc, some users might be tempted to add an amount < capacity check before the insert which they should not

cunning mulch
#

we can recommend against doing it and that implementers of insert should do the check themselves to fail fast

stone dragon
#

Might be that it's above the capacity but you can't insert stuff. Might be you can. Why should it report anything related to that, when nobody should be checking it to see if you can insert stuff?

Basically -- it's an extra restriction on this method that imo isn't worth adding, when nobody should be relying on such a guarantee to begin with.

cunning mulch
#

the point is in the past there have been cases where implementations definitely don't do so and are performance bottlenecks, and if that is an issue then I want to be able to have a heuristic that is valid by contract rather than being valid in general like we have now

stone dragon
#

In generaly I think the requirements on a method like this should be left broad unless there's a reason to add the restriction -- and in this case I've yet to see a reason someone would want that guarantee

cunning mulch
#

back to tech's comparator example then. amount >= capacity means that it is as full as possible, if it still accepts things then it wasn't really full and shouldn't be indicitive of such

stone dragon
#

We can't predict every use case, so we don't know if someone has a sensible case where breaking that guarantee would make sense. But we can know that there is no reason to rely on that guarantee in general

loud stirrup
#

oh yes I kinda lost the thread

#

I think an implNote that the capacity should not be less than the amount that can theoretically fit is fine

#

but keep in mind that insert can arbitrarily fail even if the capacity indicates there is capacity

#

just the inverse can be part of the implnote I guess

cunning mulch
cunning mulch
loud stirrup
#

In that if the amount and capacity indicate that an index is full, an insert into that index should fail.

cunning mulch
#

with the exception that you can have max long if it literally has no way of knowing

flat root
#

I wouldn't link the capacity to whether insertions will succeed or not

stone dragon
loud stirrup
#

True, see, already forgot about that 😄

loud stirrup
#

But I fail to see why a mod would implement it any other way anyway?

flat root
flat root
cunning mulch
#

that was based on gimp's suggestion earlier

stone dragon
# loud stirrup hence an impl note

My point about it still remains. You should not ever check capacity if you're trying to insert. Why restrict what mods can do to provide a guarantee nobody should be using?

cunning mulch
#

I don't really care if that exception is in there or not

loud stirrup
cunning mulch
# loud stirrup But I fail to see why a mod would implement it any other way anyway?

yes I agree people likely won't implement it any other way, which is why I want it to be more of part of the contract, so that logically people can better understand what it means. (And is also why I don't see why you all seem to be so against having it be part of the contract if you don't know why any impl would actually be different). As truth be told from all the arguments I am hearing is that one should literally never use getCapacity and just attempt to insert max int and see how much it can accept to get a better sense of what the actual capacity is. Which given some things have fake backing slots is not ideal as then it is having to insert until it gives out.

Like that's the whole reason I had proposed that getCapacity takes a resource like my current impls in mek do. It is so that it can give you a better more narrow capacity if it knows it, and otherwise it can fallback to the broader general capacity of what it might be able to accept.

stone dragon
flat root
#

getCapacity is independent of the current state of the handler unlike insert; this is why it exists, similarly to isValid

cunning mulch
#

sure

flat root
#

if you care about the current state of the handler, or mutating it -> use insert
if you don't -> use getCapacity

#

these are super different contexts IMO; for a mod like mek there is a good chance you will never use the capacity of a handler you got from a capability

#

I don't think MI or even AE2 ever checks the capacity of an arbitrary handler, for example

#

Create probably does since it has a state observation block I think? idk how much functionality it has for the capacity though

cunning mulch
# flat root if you care about the current state of the handler, or mutating it -> use `inser...

what about a slot with a zero capacity. Yes there is the argument the modder shouldn't be exposing it in general, but that is similar to the type of thing of isValid and cases where people may want to optimize out further checks. Though speaking of which do we currently have it in the contract that the capacity shouldn't be changing like is valid shouldn't be? Because if that is the only real case for if you don't care about the current state... shouldn't it make sure that it isn't actually caring?

stone dragon
#

I mean, I'm going to have slots with 0 capacity exposed. The extra slots in a shrunk dynamic handler are that, for instance

stone dragon
#

None

#

You could just do the insert, see that it fails, and it would be just as cheap

flat root
#

the formulation is quite similar to isValid

#

it is true though that we currently specify If this method returns {@code false}, the insertion of the resource is never valid. for isValid

#

I lifted this from IItemHandler but I am not even sure it is useful

#

you don't know for how long this guarantee holds - maybe it changes the moment you interact with the handler for some other reason

stone dragon
#

isValid has a "never" there so I would assume it can't ever change

#

Or at least not till a capability invalidation

flat root
#

which is not even true tbh

stone dragon
#

If getCapacity acted that way that would be... obnoxious

flat root
#

I don't want to tie any of these methods to cap invalidation

stone dragon
flat root
#

imagine I change the filter of a slot for some reason - am I allowed to change the result of isValid or not?

stone dragon
flat root
#

forget about guaranteeing stuff inside a transaction IMO

stone dragon
flat root
#

why not? there is no point in wrapping a billion things inside one big transaction

cunning mulch
# flat root I lifted this from IItemHandler but I am not even sure it is useful

the whole point is it helps things for future checks against a cached handler. GRANTED with the indexless methods I don't know how "useful" that concept becomes (given I don't think we have an indexless isValid). But that's my entire point with the getCapacity. That while the use may not really turn up anymore... we might as well have the methods make more logical sense in terms of their contracts as it makes it easier to reason about how things function

flat root
#

isValid was added so that railcraft wouldn't wait forever trying to fill minecarts that won't accept the item

stone dragon
flat root
#

that is literally it

stone dragon
#

Whatever guarantees you ask a handler to provide, you need to leave times when handlers are allowed to change those underlying things. Outside of a transaction was the one that popped to mind for me

flat root
#

it's a tempting answer but it seems very artificial to me

#

especially since these methods are otherwise not related to transactions at all, and don't mention the concept of transactions in the javadocs

stone dragon
#

🤷‍♀️ I mean, as long as you make it clear to me when I should be allowed to change this stuff

#

Cause I have an inventory with varying size, and because of that varying slot capacity, that also is going to have changes quite often in what stuff is allowed in what slots.

flat root
#

the problem with invalidations is that we don't want to be performing them too often either

#

(a secondary problem is that invalidations only exist for block capabilities)

stone dragon
#

Because anything longer than that is inately problematic (you're not going to be able to change them at all, really)

flat root
#

technically a furnace would have to invalidate on a resource reload if we tied isValid to cap invalidations kek

stone dragon
#

Yeah, I agree that invalidation is not a good time. But I would say let's be explicit on any of these sort of method with when they're allowed to change, if it's not just "whenever the handler wants"

flat root
#

I don't see an issue with the capacity changing at arbitrary times. Most reasonable use cases for the capacity are not burdened by this?

#

isValid is a bit trickier

#

believe it or not Fabric does not have an equivalent for it

#

most base implementation will have such a method, but do we even need it in the core interface? debatable

#

if we remove it, people migrating from IItemHandler might not be happy though 😄

#

just had a look at Create for example and there is not a single isItemValid use that would require it being in the core interface - but it is generally nice to access the override in the base implementations

flat root
#

OK, I pushed a change from the insertion of the resource is never valid to the insertion of the resource is not currently valid. I think that makes the most sense. This method won't be super useful for an arbitrary handler, but I think it's ok to still leave it in

cunning mulch
#

… how is not currently valid any different at all to attempting to insert and it returning zero

flat root
#

I would say

  1. the fact that it does not depend on the handler's state; and
  2. potentially the two might be decoupled, e.g. technically something might be valid but still not insertable for reasons™
#

the only change here was never -> not currently since I don't want to tie this to cap invalidation (we could but then we have to make furnaces invalidate when tags are updated, could get quite messy imo; additionally this would not provide clear semantics for entities or stacks which don't have a concept of invalidation) nor transactions (potentially too short to be useful in that case)

#

and technically the resource might be invalid but still contained in the handler - in that case you probably can't insert more of it, but it is still there and can be extracted

#

and with this we are really done with this slice as far as I am concerned

flat root
#

@mental raven we shouldn't get the sizes on demand in CombinedRH because it will force us to pass an empty resource in the ctor which is annoying. (We'd have to noop getResource for OOB accesses which requires an empty resource)

mental raven
#

Hmm, I'm not sure how useful it'd be without it being dynamic (endpoints of a tesseract, or drawer controller, etc)

#

We can't throw on OOB?

flat root
#

Well if it happens that one of the sub handlers is dynamic, we become dynamic and thus need to be lenient with OOB slot access

flat root
mental raven
#

Hmm, I'm not sure the best path forward here then

flat root
#

I am quite sure that we should just keep what's in tve PR

mental raven
#

If we keep it, we need a way for dynamic handlers to be excluded from it

flat root
#

Just read the code of CombinedInvWrapper

mental raven
#

Oh, this is literally just CombinedInvWrapper for resources

#

thats fine then, send it

flat root
#

The behavior is reasonable for combined handlers as well, for example if you want to temporarily concatenate multiple handlers it's fine that you won't see updates to the size of the handlers

flat root
cunning mulch
#

(I am arguing either we shouldn’t change it and just have people return true if it might be in the future valid for the handler, or potentially just remove the method from the API)

#

I don’t actually make use of it for caching, I think @silent root might? So I don’t fully have a stake in us keeping it, but the current change to remove the “ever” restriction makes the method seem totally pointless

flat root
#

For 90% of handlers that might return false, the accepted items are dependent on some state that might change (player reconfiguration, resource reload, config option change)

#

To make the method useful for caching you need to define a time bound for isValid, which either a) makes it apply to almost no handler, b) ties it to cap invalidations which is not great, or c) ties it to transactions which are likely too short to provide meaningful caching benefits

#

So my personal opinion @cunning mulch is to just keep the current semantics and move on with the rest of the rework

cunning mulch
flat root
#

I don't have one except for convenient access to the impl-specific implementation of the method, which you can't otherwise get in a general way

#

My main reason to keep it would be for people migrating from the old API to the new one

cunning mulch
#

convenient access sure, but what I am saying is I don't think there is a need to access it with the change vs just not providing it given it is more likely to confuse consumers at that point

#

I mean

#

given the change from never valid to not currently valid

#

consumers mainly would be using it for caching if something is worth sending

#

so we want that to be a compile break so they don't accidentally prevent things

#

and implementers could just delete it (or if we have it in our base handler impls as a protected method, just keep overriding it in those cases)

flat root
cunning mulch
#

why

#

thats the main use case I have heard from people for that method

flat root
#

Because it literally does not work for furnaces

#

Unless someone started invalidating the caps of all furnaces on resource reloads, but I am not aware of that

cunning mulch
#

tbh sounds like a bug for furnaces more so. But, I say just move it to a protected method in our templates so people can override easier, but not have it be part of the API. Unless someone can give me a use case of why you would want to be calling the method (especially if it is just currently valid and not ever valid)

flat root
#

Supposedly for some railcraft use case the method makes sense

#

But personally I am not buying it

#

I am concerned about not providing a simple migration path from IItemHandler though

cunning mulch
#

which part

#

callers or implementers?

flat root
#

Callers

cunning mulch
#

and what I am saying is that I am fairly sure that 90%+ of callers likely use it for caching if things are going to be valid, at which point the change being made to it being "currently valid" and not "ever valid" would introduce bugs that wouldn't be obvious in porting

flat root
#

The bug was already there though

cunning mulch
#

I would rather have it so they realize that rather than just have a method to call that "seems right" but doesn't work for their use case

#

for furnaces sure, but how many times do people actually do a /reload in modded mc

#

it isn't necessarily one that is noticable to players

flat root
#

I think you're vastly overstating the extent of the problem. If modders made a bad assumption they'll eventually get a bug report and fix their stuff

flat root
cunning mulch
#

if the mod that has a filter in their UI doesn't implement the item handler spec properly

flat root
#

Imagine, "I changed the MI Interface filter from A to B and now pipes still won't insert A in it"

flat root
cunning mulch
#

I just want an example of what case the method has a use if it isn't for the purpose of caching stuff

#

like why not just call insert

#

what good is it to know that it could accept a melon if there are only apples in it now

#

and by the time there aren't it may no longer accept melons

flat root
#

I don't know. Arguably a pipe might skip scheduling melons to arrive there later. But that's a bit of a stretch

cunning mulch
#

which is the caching that I am talking about

flat root
#

But it seems to me that you're making the same argument as for the capacity getter, which is that "if it's not useful when inserting then it's not useful", which I think is a bit reductive

cunning mulch
#

for isValid, it is more strongly tied to inserting given the docs literally say:

  • If this method returns {@code false}, the insertion of the resource is not currently valid.
    Personally I dislike the idea that it has anything to do with insertion, and I argued with soaryn about that, that I think it should be whether it is valid in the index, regardless of if you can insert it or extract it
flat root
#

Tbh I don't like this bit either

#

So I agree with you that we can reword this

cunning mulch
#

providing for if it is valid in general and fully disconnecting it from insertion (other than maybe being used in our templates as an implementation detail of how they implement insert to make overriding be easier) I am fine with having it, I still don't know if it has a use, but with it being so strongly tied to insertion, as it is currently it is the same antipattern you guys claim exist for what I was wanting with getCapacity

flat root
#

Ok I just found a stretch use case: item pipes with very lenient oversending enabled. You will always send the items towards the destination, unless isValid is false, and only try to insert when the items actually arrive.

cunning mulch
#

personally I still think it has more use for "ever valid" and then just return true, if there is user based filtering that it can't know ahead of time. But that's another matter and one I don't feel as strongly on (in terms of trying to enforce/get changed if other people don't think that is worth it)

flat root
cunning mulch
#

I still don't like the fact that it seems you guys are against getAmount() >= getCapacity() not meaning that the slot can be considered full though

#

like sure I understand my wording with insertion was confusion

#

but it was more how I was trying to describe the main "side effect" of it being full

flat root
#

IMO: it can be considered full (for example by ResourceHandlerUtil.isFull), just not making the connection to insert

cunning mulch
#

I don't care if the connection is stated explicitly, it is an obvious connection that still covers 99% of cases if not 100%. I just don't think the current wording is explicit about the fact of being able to consider it full

#

also better example that I just remembered I do use in places (at least internally)

#

needed = getCapacity() - getAmount()
So only try to transfer needed, as then you aren't say having to try to extract from yourself as much as possible, you can just extract up to that to then insert what you could extract into the other destination. Granted idk how much that will be something used without the simulation step of things, but still

flat root
#

IMO that's fine for your own internal handlers, I would just be careful when doing that for an "unknown" one

flat root
cunning mulch
#

yes, basically if you can ignore it. Which is why I previously was describing it as being "tied" to insert, given if you are trying to extract, usually it being full means it may be more likely you can actually get something from it

flat root
#

in practice it's probably ok to make the assumption and then run with it

#

I pushed the change for isValid

#

I think we are starting to get diminishing returns at this stage, and I am eager to instead move on to the next slices

#

I think it is fine for mods to make some extra assumptions than what is strictly guaranteed in the docs, as long as they tell us if/when the assumption is broken -> then we can re-evaluate the spec

cunning mulch
#

meh, with gimp's suggested changes to get capacity I suppose it is close enough, I still would prefer if it mentioned that it can be considered full if the amount is >= than the capacity (without necessarily saying what being full means). Especially given we already have ResourceHandlerUtil#isFull so it isn't like it is a new concept, it is just mentioning that same concept in the handler docs itself instead of being hidden in a util method

#

you willing to add some sort of reference of that sort?

flat root
#

something like this?

     * This function serves as a hint on the maximum {@linkplain #getAmountAsLong(int) amount} the resource handler might contain,
     * for example the handler can be considered full if {@code amount >= capacity}.
     * Note that the returned capacity may overestimate the actual allowed amount, and it might be smaller than the current amount.
     * The only way to know if a handler will accept a resource, is to try to {@link #insert insert} it.
cunning mulch
#

exactly

flat root
#

pushed

cunning mulch
#

for the int variant it is slightly less true for purposes of if it is max int, but I don't think it matters enough and would make it potentially too wordy to try and explain

flat root
#

yeah I also had that thought

#

but it seems a bit too technical, and it's fairly intuitive that if the amount is >= max int you might report fullness too early

#

amazing, thanks for the approval!

#

@hidden geode are you around to give your approval as well?

#

so, the next two slices are the templates and the vanilla wrappers

#

currently at 800 and 1800 lines respectively

#

nonetheless, I am tempted to merge them back into a single slice as this might enable simplifications (e.g. using some of the templates' logic for the vanilla wrappers)

cunning mulch
#

I don't really have any preference

silent root
#

Will need to do IItemContext first really

flat root
#

I don't think so? the slice as it is should not include the itemstack-based vanilla wrappers

silent root
#

Bucket?

flat root
silent root
#

Ah from that list then no, but we probably should go ahead and do IItemContext sooner than later before doing the "implementations" as that is still back end structuring

flat root
#

the contexts are dependent on the player inventory wrapper

silent root
#

Sure, if including the implementations

#

The templates/examples though I'd recommend holding off on doing those until the "end" as those will be incredibly subjective of what they should and should not provide. I am more or less indifferent to the example implementations (just as long as we have a generic handler) as I have my own solutions I can use.

loud stirrup
#

I wanna see the next slice 😄

hidden geode
#

Will do it later today

flat root
#

@hidden geode don't forget

#

Thanks

flat root
#

there, you can "see" it

#

still a mess though 😄

clear tusk
#

i like how latest commit is Make it compile again and CI fails to build blobxd
-# i mean its cause of missing license headers but still thats a little funny

cunning mulch
#

(they technically aren't even missing, there is just a TODO above them)

loud stirrup
flat root
#

They're all in the last slice, I need to bring some of them over

loud stirrup
#

I am getting better at PR-to-PR 😛

#

just point me to the actual last slice and I can help

loud stirrup
#

Found it

loud stirrup
#

Uh have we even registered the capabilities yet? Which slice does that? Otherwise doing tests doesnt work

flat root
#

That's for the last slice harold

#

You can create the wrappers manually

flat root
flat root
#

getting the hopper wrapper right is very tricky

silent root
#

It was working fine in the original. As well as test validated

flat root
#

I don't think so. It should check if the full hopper was empty, not a specific slot, and at no other time than right before the insertion

silent root
#

It was working. I did say it could be improved, but that was slated for later. It very much validated

#

Checking if the hopper is empty is doing a per index search anyway

flat root
#

I found these two bugs in the slice I took over, so idk. I'll look at the test though

silent root
#

Well do remember you also nuked or mutated the utils

#

I can load up my client later tonight. Busy now

flat root
#

It's fine to make mistakes. It's possible that the check wasn't testing all edge cases and it's possible that I misread some code

loud stirrup
loud stirrup
#

i think reviewing these wrappers is pointless without the gametests

cunning mulch
#

Tests should be per area being tested imo. If there are more generic tests they can be in a final slice that is more code coverage wise

#

Especially when it comes to wrappers as gimp is saying

#

And yeah I forget, I think the hopper stuff might be fine, but definitely was missing a bunch of test cases that I annoyed Soaryn about at some point

flat root
flat root
loud stirrup
#

Were there any tests for the combined / ranged wrappers ? didnt see any unit tests

flat root
#

no

#

they're simple enough IMO but if you want to write a few tests I suppose it doesn't hurt

flat root
# 
Tests: [22Aug2025 23:05:27] [TestFramework neotests:tests]: Test 'testHopperCooldown' has been enabled.
[23:05:27] [Server thread/ERROR] [minecraft/LogTestReporter]: neotests:tests/testhoppercooldown failed at -10597065, -59, -2976792! Encountered exception running method-based gametest test: public static void net.neoforged.neoforge.debug.transfer.VanillaHandlersTests.testHopperCooldown(net.neoforged.testframework.gametest.ExtendedGameTestHelper)
[23:05:27] [Server thread/INFO] [minecraft/GameTestRunner]: Running test environment 'neotests:tests/level.entity.player.event' batch 0 (10 tests)...
Tests: [22Aug2025 23:05:27] [TestFramework neotests:tests]: Status of test 'testHopperCooldown' has had status changed to [result=FAILED,message=GameTest fail: Encountered exception running method-based gametest test: public static void net.neoforged.neoforge.debug.transfer.VanillaHandlersTests.testHopperCooldown(net.neoforged.testframework.gametest.ExtendedGameTestHelper)].
Tests: [22Aug2025 23:05:27] [TestFramework neotests:tests]: Test 'testHopperCooldown' has been disabled.

@copper fjord why is the key information (exception + stacktrace) not here? harold

copper fjord
#

how would you like "yes" for an answer harold it should be though, those are the messages in the middle, no?

#

weird hmm

flat root
#

thanks mojang

image.png
copper fjord
#

did they break it in 1.21.6?

flat root
#

maybe?

copper fjord
#

I could've sworn I've seen it log something at some point

flat root
#

could well be

#

but right now it's not logging anything anymore, unfortunately

mental raven
#

Seems reasonable to patch or replace

copper fjord
#

nope it's been like that for 2 years

#

I guess I was just hallucinating

#

CI will however tell you the line it fails

#

and by that I mean there will be an annotation in the changed files view

#

so there's that

flat root
#

OK, I did a first pass on the Container wrapper classes but I am still not statisfied. The next thing is likely to look into the LivingEntity equipment wrapper -- that will also likely interact with the equipment slots of the player wrapper

#

it looks like at some point between 1.21.1 and 1.21.8 the entity equipment storage was refactored, with a clean split between storage and side-effects -- that's exactly what we need for transaction support

flat root
#

yeah the LivingEntity wrappers are a bit involved

#

I think I will trust the existing one (EntityEquipmentInvWrapper) to do the right thing

flat root
#

OK, I did something that looks quite reasonable to me in the end

#

still need to add a check for stack.canEquip, which was not there in EntityEquipmentInvWrapper but I think makes more sense overall (you don't want to be inserting random items into the entity's slots)

loud stirrup
#

Alright, at least I found a bug with the redstone calc 😅

#

Or maybe a point that should be discussed

#

If you have a slot filled two twice its capacity

#

That should count as 100% filled for the purposes of redstone level, not 200%, right?

flat root
#

I don't know

#

Maybe

loud stirrup
#

Well right now the return value exceeds the max redstone level

#

that definitely cant be correct

flat root
#

Ah yes that is wrong

loud stirrup
#

Since it averages the fill-level (percentage-wise) of all slots

#

I'd say an overfilled slot should count as 1

flat root
#

I would argue that if the slot is considered full (amount >= capacity as we discussed) it should give the max contribution for that slot, not more

flat root
loud stirrup
#

Yeah same

#

This is also odd

#

So if you have a container with 10 slots, 9 are full, 1 has capacity 0, the entire container is considered to have signal 0?

#

that seems odd

#

it should just skip the slot

flat root
#

Yeah I did it because something had to be done but I'm not convinced

flat root
loud stirrup
#

and if all slots were skipped, then return 0 at the end

flat root
#

@fickle forge @neat matrix do you allow me to use your Fabric Transfer API tests here? the testShulkerNoInsert and testJukeboxState tests respectively

neat matrix
#

Yep, sure.

flat root
#

thanks!

flat root
#

hmm, the patch to make AbstractFurnaceBlockEntity transactional was forgotten

#

the trick here is that the furnace will reset its recipe when the input is emptied or swapped out

#

so we have two possible implementation strategies:

  1. at the end of the transaction, check if the item changed or got emptied; if yes, reset the delay
  2. make recipe timer transactional. immediately after the item is emptied, change the recipe timer; in case of a rollback it will get reset to its previous state
#

what's the difference? Well, with 1, if the input is removed from the furnace and then added back to the furnace in the same transaction the timer does not get reset. With 2 the timer will get reset.

#

I think that 2 is preferable. Fabric and the experiments PR implement 1, but I think that it's slightly incorrect

loud stirrup
#

ya

flat root
#

hmmm, did you know that chests don't have a comparator output if they are obstructed? 😅

cunning mulch
#

Wait what?

loud stirrup
#

nope, didn't know that!

silent root
#

That's the first time ever hearing about that

#

I wonder if it does that with shulker boxes

flat root
#

yeah it's weird; one of my tests was failing because there was a barrier block above the (full) chest, which caused the comparator output to be 0 instead of 15 😅

#

the furnace patch is kinda shit - if (insideTransaction && net.neoforged.neoforge.transfer.transaction.Transaction.getLifecycle().isOpen()) cookingTimesJournal.updateSnapshots(net.neoforged.neoforge.transfer.transaction.Transaction.getCurrentOpenedTransaction()); but at least it works 😅

#

ah well, the slice is now at +3100 lines 😅

#

not a huge fan of this

image.png
#

I have to try an alternative, I don't like this one all that much 🤔

loud stirrup
#

hm

#

if you know you're inside transaction by passing the boolean would it help to instead pass a nullable TX context?

#

can't see the rest of the patch though, so uh no idea

flat root
#

I considered it, but there is a case where insideTransaction is true but we are actually in a close callback so transaction operations are invalid

#

so I went for this instead 😄

image.png
loud stirrup
#

Hm wait the entire thing (reset cooking time) is a neo change?

flat root
#

no but we have to make it transactional

#

these vanilla wrappers are quite complex and there's few people who understand them... thankfully they're all kinda hidden 😅

loud stirrup
flat root
#

ah yes

fickle forge
flat root
#

interesting change with the agreed-upon isValid semantics: the bottom handler of a composter cannot accept bone meal from insertion, but isValid should return true

#

hmmm it seems that vanilla will let you extract the bone meal from a composter from the null side

#

will leave it as a todo for now

flat root
#

but this time I will go for the simpler solution of only checking at the end of the transaction, and leave a TODO in the code in case we want to revisit it later

flat root
#

@copper fjord another thing with the current testframework. The summary it prints at the end just hides the much more focused game test summary. This is what I want to see not 200 lines of "Test whatever passed" 😄

[13:08:16] [Server thread/INFO] [minecraft/GameTestServer]: ========= 149 GAME TESTS COMPLETE IN 1.585 min ======================
[13:08:16] [Server thread/INFO] [minecraft/GameTestServer]: 1 required tests failed :(
[13:08:16] [Server thread/INFO] [minecraft/GameTestServer]:    - neotests:tests/testhorsearmorwrapper: Expected equip event count to be 1, was 0 on tick 0
[13:08:16] [Server thread/INFO] [minecraft/GameTestServer]: ====================================================
mental raven
#

(the real crime is not using snake case in that test name)/s

loud stirrup
flat root
#

the function name is testHorseArmorWrapper

cunning mulch
flat root
#

must be hidden in those 1650 comments 😄

#

I didn't find one on ComposterWrapper at least

cunning mulch
#

Lots are probably hidden from “outdated” given GitHub is so so at recognizing if the line or area the comment is about can still apply

flat root
#

ah right

#

well in any case this slice is slowly approaching a stage where it can be reviewed 😅

#

+3200 already

cunning mulch
#

Slices definitely make it nicer that we were able to decide on isValid instead of me having to leave a ton of comments saying I don’t like something, and can just shorten them to pointing out when impl doesn’t match

flat root
#

yup agreed

flat root
#

we should probably combine isValid and getCapacity in the default implementations, no?

#

given that isValid will in most cases be equivalent to getCapacity == 0

loud stirrup
#

well capacity can change, isvalid is not supposed to?

flat root
#

it can change as well, that was changed relatively late in the spec

#

precisely because if you say that it can never change it becomes relatively useless

mental raven
#

I'm not even sure why you'd want it, just trying the insert anyway is not good enough? What's its intended purpose?

loud stirrup
#

I suppose it only tests the filter and might be faster if it doesnt have to record a snapshot? Idk

flat root
#

for insert purposes it doesn't matter where you put your check since both get checked (currently)

mental raven
#

then where would i use it?

flat root
#

Well, it depends on what you are talking about. In the case of the general ResourceHandler interface, capacity and isValid are more metadata. You should just try to insert if that's what you're going for. But here I am talking about the provided base implementations

#

Here's an example for the vanilla Container wrapper:

        @Override
        protected boolean isValid(ItemResource resource) {
            return container.canPlaceItem(index, resource.toStack());
        }

        /**
         * Special cases because vanilla checks the current stack in the following functions (which it shouldn't):
         * <ul>
         * <li>{@link AbstractFurnaceBlockEntity#canPlaceItem(int, ItemStack)}.</li>
         * <li>{@link BrewingStandBlockEntity#canPlaceItem(int, ItemStack)}.</li>
         * </ul>
         */
        @Override
        protected int getCapacity(ItemResource resource) {
            // Special case to limit buckets to 1 in furnace fuel inputs.
            if (index == 1 && resource.is(Items.BUCKET) && container instanceof AbstractFurnaceBlockEntity) {
                return 1;
            }

            // Special case to limit brewing stand "bottle inputs" to 1.
            if (index < 3 && container instanceof BrewingStandBlockEntity) {
                return 1;
            }

            return container.getMaxStackSize(resource.toStack());
        }
#

we could totally change this to

        /**
         * Special cases because vanilla checks the current stack in the following functions (which it shouldn't):
         * <ul>
         * <li>{@link AbstractFurnaceBlockEntity#canPlaceItem(int, ItemStack)}.</li>
         * <li>{@link BrewingStandBlockEntity#canPlaceItem(int, ItemStack)}.</li>
         * </ul>
         */
        @Override
        protected int getCapacity(ItemResource resource) {
            if (!container.canPlaceItem(index, resource.toStack())) {
                return 0;
            }
            // Special case to limit buckets to 1 in furnace fuel inputs.
            if (index == 1 && resource.is(Items.BUCKET) && container instanceof AbstractFurnaceBlockEntity) {
                return 1;
            }

            // Special case to limit brewing stand "bottle inputs" to 1.
            if (index < 3 && container instanceof BrewingStandBlockEntity) {
                return 1;
            }

            return container.getMaxStackSize(resource.toStack());
        }
mental raven
#

Right, they are analogues to canPlaceItem and getMaxStackSize, carry on

flat root
#

I think the key "problem" for me is that canPlaceItem == false implies that getMaxStackSize == 0 for simple handlers

#

yet I don't want to write the same check in two places

mental raven
#

doesn't canPlaceItem check if the item could be put there? not if it currently can exist there?

flat root
#

yeah, it's independent of the current state of the inventory

#

so is the max stack size

#

another way to see it is that if the capacity is 0, it means that the item cannot fit and thus cannot be placed there

mental raven
#

I didnt think maxStackSize was, i thought that was always 64

flat root
#

if the Container wants it can override this to return 0 for specific items

mental raven
#

mm, yes, just looking at that

#

they totally could, and i see what you mean

#

i think you could implement maxStackSize as canPlaceItem(item) ? min(item.maxStackSize(), maxStackSize()) : 0, but not the other way around

#

however, slotted

flat root
#

yeah that's what I did in one of the base implementations:

    @Override
    public long getCapacityAsLong(int index, ItemResource resource) {
        Objects.checkIndex(index, size());
        return resource.isEmpty() || isValid(resource) ? getCapacity(resource) : 0;
    }
#

I suppose that if you're gonna return a capacity of 0 then isValid is the better place to put it

#

whereas if you're gonna limit the capacity to something > 0 you should put it in getCapacity

#

that makes sense to me at least

mental raven
#

That does

flat root
#

OK, finally got around to reactivating the provided implementations based on StackListHandler

#

still missing some javadocs but that means the PR is ready for review!

mental raven
#

I will try and go over the PR again in the coming days

flat root
#

not a fan of the ItemStackListHandler name

#

it doesn't make it clear that it's a resource handler, no?

#

an alternative name would be ItemStacksResourceHandler but that's not amazing either

#

(I am happy to keep ItemStackListHandler but maybe someone has a better suggestion)

wind steppe
#

i prefer ItemStacksResourceHandler as well

#

i like how the abstract resource handler ended up being implemented. i was worried for a second youd just eliminate the idea of a generic handler for resources altogether, this is good tho

loud stirrup
#

ItemStacksResourceHandler sounds good imho

flat root
#

thanks; I also like how it is now, feels quite simple and intuitive to me

#

currently writing some javadoc for it

#

ok will also rename

StackListHandler -> StackListResourceHandler
{Fluid,Item,Resource}StackListHandler -> {Fluid,Item,Resource}StacksResourceHandler
#

maybe ResourceStacksResourceHandler is a bit verbose, but at least it is quite clear

wind steppe
#

Generic maybe?

loud stirrup
#

StackListHandler -> StackListResourceHandler
StacksResourceHandler? hua hua hua

flat root
#

I mean that also works but StackList is a bit clearer (arguably most people probably won't be using it directly anyway)

loud stirrup
#

I was just commenting on the symmetry with ItemStacksResourceHandler

flat root
flat root
#

oops forgot to rename it lol

wind steppe
#

SuperStacksResourceHandler :>

loud stirrup
#

can live with that

#

not the super one 😄

#

the screenshot

wind steppe
#

ship it tech

flat root
queen wagon
#

I think we should do a list of class and package names and their description to better see which need renaming and then do all of those at once

waxen dawn
#

will this be the last slice?

flat root
#

wouldn't you rather review 20 class names rather than 100 at once? 😛

stone dragon
#

I just want to give a thank you to everyone that's put so much time into getting this rework done! It looks amazing and I'm excited to use it when it all comes together (and you all've given me the motivation to work on a mod I haven't touched in versions, with this now available...)

cursive geyser
#

meanwhile I'm dreading the day it's merged because the changes provide very little to no benefit to any of my mods (maybe except the inventory thingy in ender-rift which I'm not even sure I want to keep because it's a mess to maintain)

#

(I'm not saying the changes are negative or anything just that no benefit + changes = work that feels like a chore)

meager nexus
#

pretend its mc version update

waxen dawn
stone dragon
#

Adrian, soaryn, tech, and all the other folks who've put time into writing this and getting it through the review process and all ♥️ thank you!

flat root
waxen dawn
#

how many left approx.?

stone dragon
flat root
cunning mulch
#

(At least)

distant merlin
#

we probably ought to pin the current PR/branch/slice/etc. in here

#

also, @loud stirrup thoughts on adding the current slice-in-progress to your transfer rework preview PR? (so those who look at it know what's coming)

#

I have half a mind to edit the descs of all the slices so far to link to the prev. and next slices in series

loud stirrup
#

the PR itself has publishing already

distant merlin
#

just so there's convenient links between each of the slices

loud stirrup
distant merlin
#

ah, I should've clarified: you know how your transfer rework preview PR lists all the slices that've been merged so far into the branch?
I was thinking of adding the current slice-in-process to the list as well, so people who look at the preview PR also see the one being worked on and up for inclusion next

flat root
#

Why not

distant merlin
#

yea

#

wanted to clear it before I did it myself thinkies

#

also,

distant merlin
#

do we have a list of remaining slices that can go in the preview PR's list as well?

#

or is it more of a "look at what's remaining" kind of deal

loud stirrup
#

which is kinda moot since it is not merged into the source branch yet

flat root
#

item contexts, energy, and migration from the previous API are the 3 next big slices

loud stirrup
#

but linking it is totally fine

distant merlin
#

it'd be neat to at least an overview of what's remaining

flat root
#

Which also means that the current one should not be held up for too long, since we have 3 more to go and we don't know when the first pre will drop

distant merlin
#

ideally, this'd be in an issue regarding the Transfer Rework, but I'm settling for the PR at least since people are likely to look at it more

distant merlin
flat root
#

I need people to review the PRs not the overview of what work I will do in the future 😛

#

Sure

cunning mulch
clear tusk
distant merlin
#

sounds good to me

#

it is done

#

am I right that the intention is to rebase-merge all the separate slice-commits to the main branch?

flat root
flat root
cunning mulch
loud stirrup
loud stirrup
#

Hm Tech, is AbstractFurnaceBlockEntity.this.cookingTotalTime observable?

#

If it is, it'd be incorrect to defer resetting it until the end of the transaction, I think

void thicket
#

Rebase merge sounds reasonable

distant merlin
#

I asked because I assumed the not-to-be-merged preview PR implies that the transfer rework won't be merged in as a single PR squashed commit

loud stirrup
#

There is surprisingly little in terms of patches in this slice

loud stirrup
#

Alright, submitted my review. I think the most important bit here is the testing of the vanilla wrappers and some of the API design.

flat root
flat root
#

There's already a bit of testing code, I don't think it's a blocker. I would probably add regression tests if issues arise in the future

flat root
#

Pssst @mental raven you still have a request for changes as well

flat root
#

what is this abomination that vanilla is doing here 😅

image.png
#

it's so unnecessary, this could just have been return p_255922_.is(ItemTags.BOOKSHELF_BOOKS)

#

is this proof that even mojang devs don't understand how to implement Container correctly? 😄

#

@loud stirrup I addressed your comments except for 2 (for which I gave a comment on GitHub), can you check them?

loud stirrup
#

Approved

flat root
#

in the meanwhile I'm already going to start the item context slice

cunning mulch
flat root
#

I like no comments 😄

cunning mulch
#

To be fair a lot of my comments on the stuff in this slice got addressed pre split

flat root
#

makes sense yeah

flat root
#

I don't think that ItemContext is a good interface name

flat root
#

vanilla has SlotAccess which is relatively similar

#

what about ItemLocation? 🤔

flat root
#

ItemHandle?

#

Item(Resource)Access?

mossy chasm
#

ItemHandle reads very Win32

loud stirrup
#

ITEMHANDLE

mossy chasm
#

LPITEMHANDLE

flat root
# 
/**
 * Provides access to the location where an item is stored,
 * such that the current item resource and amount can be read,
 * and the stored item can be changed.
 *
 * <p>This interface is primarily used as the context type {@code C} for {@linkplain ItemCapability item capabilities}.
 * This allows the returned capability instance to modify the current item or even swap out the item entirely,
 * for example to replace an empty bucket by a filled bucket.
 * Use the {@link #getCapability(ItemCapability)} method to query a capability for this location.
 */
public interface ItemLocation {

I think this works

wind steppe
#

hmmmmm

flat root
#

I actually refined it a little:

/**
 * References an item storage location, like a slot in an inventory or a player's hand,
 * such that the current item resource and amount can be read,
 * and the stored item can be changed.
 *
 * <p>This interface is primarily used as the context type {@code C} for {@linkplain ItemCapability item capabilities}.
 * This allows the returned capability instance to modify the current item or even swap out the item entirely,
 * for example to replace an empty bucket by a filled bucket.
 * Use the {@link #getCapability(ItemCapability)} method to query a capability for this location.
 */
public interface ItemLocation {
wind steppe
#

But its not a reference to the location, it provides access to the location. you cant know where the item is by having an instance of this class

#

ItemAccess?

flat root
#

a reference provides access 😛

wind steppe
#

right, but a ResourceLocation doesn't provide access to the resource, it just gives you an address for where to find it. ItemLocation has nothing in it that someone could use to navigate to where the item is, just a way to access the item

mossy chasm
#

ItemHandle was not such a bad idea

wind steppe
#

I don't hate it

#

i think it makes more sense than ItemLocation

flat root
#

I don't think that RL is necessarily representative of what a location can be, but the confusion potential between ItemLocation and RL is not great

loud stirrup
#

Told ya ItemHandle was not suhc a bad idea 😄

flat root
#

I actually considered ItemHandle but the confusion potential with ItemHandler is worrisome

loud stirrup
#

I revive my idea: InItemStorageContext 😄

flat root
#

what's a Storage mr?

loud stirrup
#

God damnit

#

ItemEncapsulatedHandlerContext!

mossy chasm
#

ItemManipulator

#

ItemSetterGetterSwapper

onyx basalt
flat root
#

💎🖐️

flat root
mossy chasm
#

I actually like that

wind steppe
#

you like StorageMr?

onyx basalt
#

an ItemStorageLocation, so to speak?

mossy chasm
#

ItemSetterGetterSwapper has the same vibe of Outgredient

wind steppe
#

waaaaay too long

loud stirrup
flat root
wind steppe
flat root
#

pick your fighter

#

ChatGPT likes ItemHandle 😄

mossy chasm
#

ItemHandle, ItemGateway

loud stirrup
#

The fucking checkmark behind it. I am dying

#

😄

mossy chasm
#

Hey, speaking of gateway

#

ItemStargate

#

No, but seriously, I feel like ItemHandle is in fact the better choice

loud stirrup
#

I am kinda whatever about it, ItemHandle works for me

#

It's kinda in the ballpark for a handle at least

flat root
#

the fun thing is that Fluidity uses handles instead of slots or indices... full circle 😄

#

(nobody here knows what Fluidity even is)

#

ItemAccess or ItemHandle works for me

loud stirrup
#

Both are fine for me

wind steppe
#

I like ItemAccess more

loud stirrup
#

If we wanted to align with vanilla ig ItemAccess aligns better with SlotAccess

#

Or what did they call theirs that was specific to menu slots?

flat root
#

it's called SlotAccess yes

wind steppe
#

ItemHandle is also too close to what the old item handler was called, and people will be confused and try to use it for their new stuff

onyx basalt
#

ItemAccess sounds better imo as well

hidden geode
#

I would do ItemAccess or ItemHandle

onyx basalt
#

but ultimately both work

hidden geode
#

It is technically more an ItemHandle then an ItemAccess based on its usage, but the confusion potential on the XYZHandle in particular related to ItemHandle and ItemHandler is too large for me personally

wind steppe
#

Then we're in agreement! We scrap the whole thing and keep the existing system

hidden geode
#

Sure lets just throw the entire change in the bin

#

Lock this thread

#

And all go grab a nice beer in a beergarden

wind steppe
#

Ill drink to that!

hidden geode
#

IMHO Sounds better then deciding the name anyway

flat root
#

ItemAccess it is

hidden geode
#

Very good

#

Now I go grab a radler

flat root
#

I go eat pasta

hidden geode
#

Oeh

#

I had grilled chicken today

hidden geode
cunning mulch
#

@flat root left some comments for you, and that is all I have patience for right now, I might try to do a bit more later today, and if not I will finish it tomorrow, down to 9 files left

flat root
#

Thanks

mental raven
#

Those last comments are the only things i really have left. Approval for me after :)

flat root
#

Ok thanks I'll have a look tonight

#

One thing: there is no MapMaker with identity semantics 😅

#

But we could maybe detect equals overrides via reflection in VanillaContainerWrapper.of and throw?

#

(We have similar machinery for data component types so it shouldn't be too hard to do)

#

We could use a wrapper key object but that would allocate on every lookup

mental raven
#

hmmm

#

i hate rolling collections, but might be required here

#

not sure how MapMaker would be doing weak keys without allocating

flat root
flat root
#

The todo is inherited from Fabric and has existed for 4y so I am inclined to "do nothing" (or at least add a check) if we really can't find a better map implementation

#

We need a concurrent weak key weak value identity hashmap 😅

mental raven
#

Well, we can probably ignore it then, or do a warning if hashCode/equals is implemented

#

Annoying that MapMaker doesn't support it

#

Seems like a common thing

flat root
#

Even the JDK doesn't have a weak identity hashmap

#

I faked one for capabilities but it's quite messy

mental raven
#

yikes

ripe crest
#

i can never remember the proper class name tho

flat root
#

MapMaker?

#

Ohhh

#

If you specify weakKeys it seems to use identity comparisons for keys!!

#

@mental raven that means I can just remove the todo lol

mental raven
#

aha!

#

good shit

cunning mulch
cunning mulch
#

did a bit more, and will try to finish off the remaining few files later today, but posted what I did in case I don't get to it until after you no longer have time to look at the reviews for the day

flat root
cunning mulch
#

posted my remaining pending comments. I still need to review VanillaContainerWrapper and the two test classes. But for now I am going to go and get something to eat because I have a headache. I will also try to respond to your replies, though I am not sure if I will do that today or not

flat root
#

OK, thanks a lot! I will push what I have already and then work through the new set of comments

#

Don't look at the tests too closely 😄

cunning mulch
flat root
#

well, you squint, look for obvious mistakes, and ignore the rest 😄

#

at least that's what I usually do when reviewing tests, as "fixing" them is usually not very relevant compared to other tasks one could be doing

flat root
#

should we report 64 or 99 as the capacity for "empty" stacks?

cunning mulch
#

Imo 99, unless the handler has an explicit max size set and it happens to be 64

#

Having upper bound is more useful than having a potentially lower value

noble latch
#

For what it's worth, ItemStack#getMaxStackSize() returns 1 if the MAX_STACK_SIZE data component is absent which it is for the empty stack

flat root
#

notably, this means that any implementation of getCapacity needs to special case it since air should return 64?

#

hmm

flat root
#

thanks @noble latch 😅

#

does this warrant a disclaimer on ItemResource#getMaxStackSize?

cunning mulch
#

So yes we need to special case the empty resource, and imo we should do it at max theoretical, as it is more useful imo than a limited version

flat root
#

sure, if we're gonna be special casing we might as well return 99

noble latch
#

Agreed

flat root
#

related question for cauldrons queried with an empty resource: return 1000 or return the largest amount ever registered?

cunning mulch
#

I feel like largest makes more sense, but with the ability to query per type, I am not sure it matters. As long as a non discrete cauldron that only supports two buckets can actually be inserted into, and nothing caps it at one bucket from empty and then have insertion fail

flat root
#

stupid return resource.isEmpty() ? Item.ABSOLUTE_MAX_STACK_SIZE : Math.min(resource.getMaxStackSize(), Item.ABSOLUTE_MAX_STACK_SIZE); 😅

past lodge
cunning mulch
#

Why no oversized stacks?

past lodge
#

it doesn't use the default components items should

#

i'm 99% sure it's a oversight but it's hard to explain a issue like this on the mojira without it getting closed because "there's no observable issue in vanilla"

cunning mulch
past lodge
#

no

#

previously before components it returned 64

cunning mulch
#

Rather than re-evaluating all vanilla code paths

#

Ah

flat root
#

yeah it used to return 64; either 99 or 1 would make more sense

past lodge
#

the item stack constructor that takes void just doesn't set default components for empty

flat root
#

well yeah, there is no item

past lodge
#

had a annoying time fixing the issues caused in create by the change

flat root
#

it's also that if the stack is empty the component map is DataComponentMap.EMPTY

#

the alternative would be using another default for the component map, which would be quite unexpected tbh

#

(or defaulting to e.g. 99 instead of 1 in stack.getOrDefault(DataComponents.MAX_STACK_SIZE, 1))

#

OK, another weird thing in recent MC versions is the number of slots in a player Inventory

#

there are these two weird slots

image.png
#

what should we do about them? just expose them, and let the canEquip check hopefully disallow insertion in them?

noble latch
flat root
#

not in this case, but that is what I am doing for the vanilla Container wrapper:

return resource.isEmpty() ? container.getMaxStackSize() : container.getMaxStackSize(resource.toStack());
cunning mulch
#

I will finish up what I have been needing to do with the open slice today, been somewhat sick so haven’t gotten to it

flat root
#

Okay, thanks. Can you also check my replies to the previous comments?

#

Especially on the GH threads that are still open

cunning mulch
#

currently doing that

#

okay added more comments and responded to your replies. I still need to review VanillaHandlersTests, but I am going to go do some stuff irl first

cunning mulch
#

also one comment on your:

// TODO: I would like to bring this test over, but we need snbt test structure support first
I say either document that somewhere else, or uncomment the method, but just have the annotation commented out with that TODO rather than having a large block of code there that isn't using Neo's terms or stuff and that uses yarn names in it

#

I will make sure to review the rest of that file sometime today though, but given it is tests odds are there isn't that much to say on it, so it shouldn't be that hard

flat root
#

The actual test is very nice

cunning mulch
#

Yeah that’s the solution I would go with

#

That way we actually make sure the test at least compiles

cunning mulch
#

Potential idea for a future PR or something, maybe similar to how our extended game test helper has the startSequence that then makes them parameterized, we might want to provide a startTransaction so that when doing more functional styled tests that is chaining calls, then it can handle the try/close of a transaction automatically and nicely

cunning mulch
#

okay, in theory I have reviewed all the files for the template and vanilla wrapper slice. Granted I suspect you may have some more replies to some of my comments

flat root
#

@mental raven can you approve the PR? or can I dismiss your request for changes?

#

(still working through pup's comments but still)

mental raven
#

Feel free to dismiss

flat root
#

thanks

mental raven
#

Sorry, am currently slightly drunk, otherwise i'd have re-reviewed. But i think all my comments were addressed/discussed

loud stirrup
#

I'll raise a glass of sake. Kanpai!

silent root
cunning mulch
#

okay tech responded to your replies/marked things as resolved. Only added one comment in regard to the changes you made so far. But we are getting there

image.png
waxen dawn
mental raven
mental raven
loud stirrup
#

I was at a Japanese restaurant. It seemed fitting.

mental raven
#

Heh, I have heard good reviews

flat root
#

ok I'm back, let's try to get through these last 14 comments

flat root
#

@cunning mulch your mayPlace logic seems quite complicated to me

#

(in InventoryContainerSlot)

cunning mulch
#

mayhaps

#

mainly given mayPlace I guess needs to handle when it has things in it

#

so

#

I guess is valid makes sense

flat root
#

it's really weird that you check if you can extract thonk

flat root
cunning mulch
#

just add a comment that is like "We just use a rough estimate of if it can be inserted by checking if it is valid as we don't care about the current contents and want to allow swapping them"

#

which is why my impl has a impl specific handling of bypassing the current contents but still checking the insert predicate itself xlurk

#

so yeah just add a comment for why we are just using is valid

#

and that should be good enough and we can let people override if they want it more accurate

flat root
#

we could play some tricks with transactions (e.g. extract all current contents and then try to insert) but it does get quite complicated

cunning mulch
#

yeah, which isn't worth it for now imo

#

(your teswt double chest comparator thing has the awssertions mention "item variant"

#

instead of item resource

flat root
# 
        // Use isValid as a reasonable estimate.
        // We can't try to insert as we don't want to check to current contents to allow swapping.
        // This method is left for mods to override if this is not sufficient.
cunning mulch
#

sounds good

flat root
#

regarding the cauldron capacity maybe I just add a todo

cunning mulch
#

then at least if someone has a case like that they can know that we are unsure if it comes up and just report it or PR the fix

#

instead of worrying or just hacking around it

flat root
#

regarding the game events for the cauldron, I have no clue

#

maybe shulkers are the only thing that would care about the game event being emitted or not

cunning mulch
#

you mean sculk shriekers and sensors?

flat root
#

ermm yes

cunning mulch
#

and might lead to some interesting builds with sensors

#

for redstone but comparators also would work

flat root
#

ok then I don't change anything 😛

#

I'll add a quick note actually

#

this one I feel can be resolved?

image.png
cunning mulch
#

I thought I resolved that

#

like 10 minutes ago

flat root
#

ah I didn't refresh

#

what about this one?

image.png
cunning mulch
#

up to you, I still think for the 1 value it might be worth it, because the canPlace doesn't mean that it will only accept one bottle if you try to insert two at once

#

for the if statement part I agree it isn't worth not just doing < 3

flat root
#

ok then I will resolve it as I don't like referencing the menu (typically menu slots have their own custom I/O restriction logic)

#

for the player wrapper I'm just gonna limit it to main + armor + offhand

#

for this comment, this needs to be handled at the PlayerInventoryWrapper level

image.png
cunning mulch
#

guessing so, yes

#

on root commit or whatever

flat root
#

looks like we just need to call player.containerMenu.broadcastChanges() on the server side from the player wrapper onRootCommit

#

on the client side, we might also want to call setPopTime(POP_TIME_DURATION) on the created item stacks, but maybe I'll just leave that as a TODO for now

#

for players we can bounce for armor and hands and throw for the rest

image.png
cunning mulch
#

sounds good for me

#

just make sure to update the javadocs for which ones don't support players

flat root
#

ok, pushed

cunning mulch
#

any comments left unresolved?

#

looks like no

#

have your approval

flat root
#

amazing, thanks

cunning mulch
#

now you can make the item context PR actually reviewable after you rebase it rather than having it expand upon a slice that isn't merged yet!

flat root
#

ok I force-pushed the item access PR but it's still quite a mess

cunning mulch
#

If you get it at least semi reviewable/cleaned up I can try to give it at least a quick pass this evening/tomorrow

cunning mulch
#

Definitely not something necessary for initial release

#

Just wondering if it might make sense

cunning mulch
#

also given you are repackaging that, I would rather access.item to itemaccess as the package. That way if we need to add other access types than item, it is as simple as just adding a different subpackage

image.png
flat root
#

Hmmm in that case we'd just do fluidaccess, no? I really doubt it would be needed. We can also just generalize ItemAccess to arbitrary resources T if we need to in the future

#

I don't like deep nesting, the current handlers.wrappers.resources or whatever is crazy

cunning mulch
#

how about just access

#

then

wind steppe
#

I thought about fluid access but yeah, I don’t see how useful that’d be

flat root
#

Right now only the actual ItemAccess interface and the other classes in the same package are reviewable

#

(except for the Stack one iirc)

flat root
cunning mulch
#

fair enough

flat root
#

(I haven't tested this)

#

And these extra equipment slots are actual player Inventory slots as well (which is weird)

flat root
#

Okay, the access implementations look reasonable now

#

next up I suppose I'll have a look at the bucket wrapper

#

ok the goal is to provide a very general base implementation similar to StacksResourceHandler and then implement various specializations

wind steppe
#

so wait, we're actually having like, completely generic Access stuff? and the item based capabilities will be like, Capability<ResourceHandler<ItemResource>, Access<ItemResource>>

cunning mulch
#

More so that I suggested a broader package name so that it is easier to expand in the future if it comes up, without requiring people to do as much refactoring

#

TLDR I try and make sure to future proof package names and stuff

flat root
wind steppe
#

ohhhhhhhhh

#

okay yeah that makes sense

cunning mulch
#

Yeah we were talking about the package it is in

#

Not the class itself

wind steppe
#

its happening

#

1.21.9

clear tusk
#

#1378294669053792337 message
-# easy link to snapshot pin but yes its happening blobconcerned

echo saddleBOT
#
apexmodder

[Reference to](#1378294669053792337 message) #1378294669053792337 [➤ ](#1378294669053792337 message)# 1.21.9 - Pre Release 1

SlicedLimes Videos:

  • Main: N/A
  • Pack: N/A
flat root
cunning mulch
#

This weekend I will be back and be able to start reviewing the item access PR if it is ready

flat root
#

Hmm I need to get back into it

#

Writing the item access equivalent of StacksResourceHandler isn't fun though and I am running out of motivation

#

So maybe pup I'll ask you to directly send a PR-to-PR for your more mechanical comments and maybe also for some discussion items, if you have the time for it

#

Because I won't have a lot of time and we really need this out

loud stirrup
#

I think you mean cosmetic comments 😛

cunning mulch
flat root
#

OK, I just pushed a first set of base RH implementations backed by ItemAccess

#

I think they're a bit lacking in functionality, but won't be too difficult to extend

flat root
#

porting over a test from Fabric and I already found a bug 😄

cunning mulch
#

I will try to review the item access pr sometime this week. My original plans of this weekend are likely not going to happen given it seems I picked up a cold during my return trip so just taking it easy xlurk

flat root
#

Ah shit

#

We're getting tight on time

#

Maybe someone else can already review it and we fix your later comments in a follow-up PR?

loud stirrup
#

I went over it and left a few minor comments. I think it'll do the job. If there's any convenience ones we've missed we can add those on demand.

#

I think the complexity is not super high overall of this part of the system

mental raven
#

Nothing poped out at me with a quick glance over

flat root
#

Ok thank you both

cunning mulch
#

I will try to give it a quick skim today

cunning mulch
#

yeah I definitely have some comments, all relatively simple (at least so far). I am for the most part not going to bother evaluating the javadocs on things, and I likely won't bother reviewing the test class. Should hopefully be done in like 20-30 min tops

cunning mulch
#

Got distracted and semi reviewed other stuff that I said I wasn't (I didn't necessarily review javadocs of individual misc impls, but decided to at least give them a brief skim for ItemAccess and ItemAccessResourceHandler. Down to just having to review the ItemAccessResourceHandler class and then you can have your change request xlurk

cunning mulch
#

There you go @flat root have your ~25 review comments

#

I think all of mine should be pretty simple and likely not need a back and forth, but if there are any clarifications or stuff you want feel free to just ping me so that we can try and get this slice merged today

flat root
#

Okay thanks a lot!! I'll have a look a bit later

flat root
#

ok @cunning mulch @loud stirrup please re-check the PR

#

pup I think the only outstanding comment is the mismatch between a supplier and no supplier between the fluid and item base implementations

#

do you have a preference either way? (2 suppliers or 0 suppliers?)

cunning mulch
#

I would lean to no suppliers I think

#

okay, other than potentially making them not be suppliers @flat root it all looks fine. Once they are consistent (whether it is supplier or not) I will approve the PR

flat root
#

ok done

flat root
#

thank you both!

#

because NNN.transfer.handlers.templates.fluids is crazy 😅

#

I'll follow-up with energy tomorrow

#

and legacy migration should come towards the end of the week 🙂

flat root
cunning mulch
#

I will take a peak after I eat (so maybe in like 45-60 min)

#

Actually from PR description looks fine

#

Have an approval @flat root on the assumption the code changes are accurately reflected by the description

waxen dawn
#

How many slices left?

silent root
#

@flat root I did push some changes a while back to the energy slice that matched some of the changes in the Resource Handler changes so should help speed that one up a bit

loud stirrup
#

Didn't you do slotted energy or was that something else?

silent root
#

That was removed in pre slices

flat root
flat root
#

the energy slice is coming along well

#

I do kinda want to rename EnergyBuffer though as I don't think it's a great name (it's the "simple" base implementation)

#

maybe SimpleEnergyHandler works

cunning mulch
silent root
#

Autocomplete becomes a menace when making methods and such

queen wagon
#

I'm just gonna say this before anyone rushes the full PR of this. I am strictly against doing this quickly just to have it in right after the update, I prefer to have a breaking change a week later rather than rushing this very important and big refactor.

silent root
#

I wouldnt personally want a week (that will be rather long), but having a stepping stone version before it would be helpful

#

I say this also transparently as I am on the 21.6 version with the old PR build of this system, so Im in limbo until we can get it in

queen wagon
#

a week is just a ballpark number, I just don't want this rushed in just for the purpose of being early (same reason why I don't like rushing for same day releases)

flat root
#

FYI it's not particularly rushed

queen wagon
#

isn't there a whole slice of moving things around left?