#Transfer rework (IItemHandler, IFluidHandler, IEnergyStorage)

1 messages · Page 7 of 1

median cedar
#

😭

silent root
#

Oh. I have no idea what they will do, but when it comes to multiloader things this may make the logic similar, but require this implementation to use

#

It will at least make the concepts similar I believe, but I honestly dont know given I never really use fabric

cunning mulch
#

Given I have never looked at architectury nor its API… your guess is probably better than mine

median cedar
#

I'm sorry 😭

mental raven
#

I have used architectury in the past, but mostly only for loader infra, not content stuff. It's possible that since the Fabric and Neo api's align a lot more closely now, a common abstraction in architectury becomes easier, but that's something the architectury devs will have to figure out

wind steppe
#

i was unaware that architectury api even handled cross platform transfer

#

last i checked it explicitly didnt. one of the devs literally told people to use the xplat library I wrote designed for that purpose kek

#

okay i just checked, it doesnt

silent root
#

I'm sure a solution can be made with it, but it very much not a goal of mine soarynSip

wind steppe
#

I'll probably continue to support common storage lib in the future, so thatd probably be an option 😄

silent root
#

Like, <T> insert(int index, SomeResource resource, int amount, T transaction) may be a starting point but bleh

#

since the transaction system here is fully locked down

flat root
#

Ok I will merge the transactions slice in a bit. For the resources slice I'm still waiting for ResourceStack to be removed before having another look

void thicket
#

milestone 1™

median cedar
flat root
crude wraith
#

Sorry for butting in, but I was wondering about the annotated type parameter @Nullable T in the SnapshotJournal.
How can a type parameter be null, what's the meaning of this annotation?

And a follow-up question, is it sound to cast the sentinel object to a T and put it into the internal list?
Couldn't this lead to problems when retrieving values from that list and assuming they are of type T?

flat root
#

Ideally it should mean T extends @Nullable Object and allow the bound to take nullable types. In practice intellij seems to not care either way

#

The list is internal. It needs a bit of care with the sentinel value indeed, but that won't leak to the outside

crude wraith
flat root
#

Ideally it's only nullable if the type bound is (e.g. SnapshotJournal<@Nullable Something>)

crude zephyr
#

The new api can resolve the problem about proxy cap ？
proxy a proxy cap and we got a loopthonk

flat root
#

This doesn't change the cap API, however this problem already has a solution anyway

crude zephyr
crude zephyr
flat root
#

You have to check

flat root
silent root
#

So:

  1. What is "non-trivial" about the current implementation of the ResourceStack
  2. Have you even looked at it since the changes?
  3. What do you mean remove it? You were wanting it "trivialized" to a record, which is partially a problem with creating arbitrary empty instances which the current system prevents and provides the singular empty instance for each resource. The user side is a single method/field.
hidden geode
#

IMHO I don't think making it the EMPTY be a singleton is necessary

silent root
#

What?

hidden geode
#

I would just allow arbitrary empty instances

silent root
#

Not ideal when we take into account that people are likely to use this as each index. So say a chest with 108 slots, would you want them to have 108 of the exact same instance repeated when it could have been known? (Then expand that on chests scattered throug the world)

#

It DEFINITELY makes things easier when we can do ResourceStack.of() and shrink to get a singular instance when they would have been otherwise empty. Especially with how trivial it is to do

hidden geode
#

And the code you have for it now

#

Is for sure really difficult to track and maintain

silent root
#

Right now?

#

How?

hidden geode
#

I take that back

#

With the EmptyInfo now it looks quite good actually

silent root
#

It is REALLY trivial.

#

And solves all of the problems that were listed to me at the start of this

hidden geode
#

Yeah i missed that

#

Sorry

silent root
#

No worries

hidden geode
#

Your old code was actually quite horrible

silent root
#

Yeah it wasn't grand

#

The self referencing generic is really useful, but adds too much clutter and becomes too unwieldy

#

This was a hybrid approach of what @mental raven originally proposed without having a user need to implement their own

#

This also means that IStackFactory for resource stacks are no longer needed for the codec since now we can guarantee empty is returned correctly, which simplifies its use (along with the other slices have been helped). ResourceStack.codec(ItemResource.CODEC) is rather elegant

hidden geode
#

Yeah

silent root
#

@flat root review it again, though do your best to not be in the mind set of "We need to axe this" and actually see what problems remain with it.

mental raven
#

I still really dislike ResourceStack overall. I feel the few very limited cases its used in the full PR could be trivial records of IResource & amount. Anyone implementing their own resources is sure to want their own mutable variant to compliment vanilla. I don't think it will be overly useful, I think it should be removed, and re-added in the future when there is a lot more real-world load on the api, and we can properly evaluate if it will be useful for modders.

silent root
#

Us having the immutable ResourceStack, which is incredibly useful for generic resource handlers to be able to simplify their logic for cross mod support, doesn't prevent someone from having a mutable (or similar) stack type in their implementations should they desire it. I was alright removing the MutableResourceStack I had in the past, but I am not so keen on removing this.

This phrase "trivial record" is really you want a tuple, and unfortunately, I still would prefer we reduce allocations where we so clearly and simply can.

mental raven
#

No it does not prevent anyone from making their own mutable variant. However, the resource helpers can simply return a trivial record (yes a pair), it does not make these helpers any more complicated or difficult to implement/use.

silent root
#

Have you looked at the newer code?

mental raven
#

Yes, I have. Yes we cleaned up how empty instances are plumbed around. But I still feel that we don't need it, and would rather see it go and added back in the future once we have prod data and feedback on the api's usage.

silent root
#

I dont understand why we are in favor of having 1000s of copies of empty instqnces though

mental raven
#

what?

#

I want ResourceStack to go completely, thus no empty instances?

silent root
#

Between mods of different resource types? Now every mod needs to handle something we could provide them?

mental raven
#

How many mods do you estimate that have custom resource types?

#

I would say less than 20 types

#

excluding energy

#

How many of these resource types would already have a mutable data structure for their resource?

#

I would estimate basically all of them.

#

Given the immutable structure comes with increased object throughput, we should not be encouraging these mods downgrade from a mutable structure to an immutable structure, especially with something as frequent as resource transfers.

#

As i have said, I want to see real-world data on where it makse sense to add it as an api alltogether, opposed to it being a trivial pair for ResourceUtils.

silent root
#

Again... I had a mutable one and an immutable one. I was told to remove the mutable

opaque vector
#

Who asked for removal

silent root
#

It was on the original pr so would have to check

#

The resource stack though is ised in the handler for Contents and the ResourceStackListHandler (though my loval copy as the newer changes)

#

Having it trivialized storing values for arbitrary resources

mental raven
#

I don't think storing entries in handlers via immutable data structures is a good idea. These should be purpose built mutable structures for performance.

silent root
#

I had said this too, which is why I had the MutableResourceStack but in the cases of a component ResourceStack was effective and necessary

mental raven
#

I don't think there is a good solution here. Mutable ResourceStack would have too much overlap with vanilla, and I don't like that. The immutable ResourceStack promotes incredibly bad object usage, and memory awareness. I, still, would rather it go away and be replaced with purpose-built wrappers.

silent root
#

So, if there "is no good solution" why not have this as a entry level point for users to have a simple time for integration, but if they need something more advanced or purposeful they can still use that? As I'm not sure the proposed "everyone do it themselves" approach will help performance at all. Im all for improving what we can, but axing it doesnt help anyone and makes the non built in resources more tedious to support per modder.

#

The complaint was the plumbing. We pretty much solved that. With the newer code and streamlined modder use

mental raven
#

This 'entry level' solution, is the worst possible solution. We don't need more immutable data structures in hot paths.

#

I think i have provided more than enough technical arguments for my viewpoint

silent root
#

"Worst" ok even it it was, but really disagree with that, your proposed is to not have any. Which Id prefer alternatives or ideas rather than "I have spoken"

mental raven
#

I have proposed alternatives

#

you just don't like them

silent root
#

What were they?

#

Removing for a trivial record isnt a solution it is the same thing with now new allocations for empty??

mental raven
#

Purpose built wrappers required for the rework. I don't think a generic ResourceStack system is in-scope for the rework

#

I'm not impressed with how difficult this rework is to get through. This has been an uphill battle from day one to get moving. If 5 maintainers asking for changes is not good enough, why should we continue?

mental raven
silent root
#

Im going to be honest, I dont like being a burden, but it is incredibly scary to me that when code is submitted for review, that the only course of actions we can take are to get petted or roll over on demand, but we arent allowed to try to come up with solutions or have discussions. They are all just demands and expected to be followed.

Changing code is one thing, wiping it is not as useful.

mental raven
#

I think we have had sufficient discussion on this topic. You have presented arguments, and I'm personally still not convinced. Thats just how reviews go.

#

I don't expect submitters to rollover on demand, I expect to have a technical convosation, and we have, but personally for me, the time for discussion is over, i am unconvinced.

silent root
#

Just to point out, without an immutable struct, youd need to make a new allocation anyway for transfers due to the snapshot journal.

flat root
boreal plover
#

Just a slight ask what version will this be out for ?

onyx basalt
#

Whenever it's done

#

there is no ETA yet, and there probably won't be one until it's done, or close to done

hollow maple
#

Frankly, I'm a fan of ResourceStack. Almost every scenario where you're dealing with some amount of something is in some way related to transfer, and the whole point of this rework is partly to make a general interface, which IMHO rules out any "purpose-built structure". Furthermore, I see an incredible synergy with the fluid rework I'm doing which would completely eliminate the need for FluidStack, leaving only ItemStack for compatibility with vanilla.

#

This rework is particularly important to get right

Speaking from personal experience, this will never happen. We shouldn't be chasing impossible perfection. I believe it has been brought up a few times now already, where we should wait for real-world data before making a decision. So why don't we do just that, and allow these, frankly, trivial differences slide until we have the data which actually suggests whether people are using or not using something?

#

It seems that we are already mostly in agreement with the general shape of the API, but currently are grinding gears on comparatively trivial implementation details, which will almost certainly change in the future anyway

#

Why not embrace that we are also human and make mistakes? We can simply see whether people in the real world, in a production environment with multiple mods, make mistakes or have feedback, and then make further changes in response. This has happened many times, because it's a simple fact that the ways we think the API is going to be used are not, in fact, how people actually end up using it.

#

The maintenance burden is irrelevant - ultimately we are going to need to maintain what people actually want to use, or people will simply move to platforms where the maintainers do end up maintaining what people want to use

cunning mulch
silent root
hollow maple
#

Update: I'm redacting most of what I said earlier, after a (rather lengthy) internal discussion. I'm on board with simplifying ResourceStack to a resource/count tuple/record (potentially with a sentinel value for empty, pending future review)

silent root
# mental raven I don't think storing entries in handlers via immutable data structures is a goo...

I was looking over it all again. One reason I finally agreed to remove MutableResourceStack: when ever the value is changed in a transaction, we need to take a snapshot (journal.updateSnapshot(transaction), that allocates if it is mutable copying for the snapshot, and if immutable create an instance on setting the new value. Meaning the immutable versions will be the exact same costs as mutable, gaining you nothing unlike before where we could just do simulations. Mutable structures WOULD have been fine, but transactions providing us a great deal of improvements did have a cost of requiring rollbacks.

Can we lay out all of the current problems we have with it? What I have right now:

  • Immutable structures require allocation per transfer We have to regardless of structure chosen so that isn't really valid anymore.
  • Plumbing It was simplified for the user to be a field and method

Then following that, what example of a "purposeful built" structure can you give? As so far, it seems that what we have right now allows TRIVIAL generic containers to be made, but without it, it becomes very much a burden on the user to reinvent the wheel for each resource.

untold eagle
mossy chasm
untold eagle
#

but why?

mossy chasm
#

Maintainers decided so

#

Isn't that enough of a reason?

#

See: the previous discussions when maintainers were not convinced of the points in favor of the current design

hollow maple
#

To be clear: we decided democratically, with an internal maintainer vote, like we do with mostly everything

opaque vector
# untold eagle but why?

It is easy to add back helpers and such later. It is hard to remove bloat without causing breaking change. The PR should be minimal viable product basically. Simplest and cleanest form. Then see if later, if people need more or not from it.

Multiple maintainers looked at the current ReaourceStack class and basically all saw it as bloat. Therefore they wanted it to be simplified to a record of just the resource and int and nothing more.

silent root
#

It does shift a lot of the burden to the user though as now there are 4 different "valid" infinite instances of an empty resource stack vs the 1 we allowed and provided if it were empty in any case. I'm testing around with making solutions for just a "trivial record", but it definitely isn't great. Regardless, I'm waiting until Pup has at least had time, and we can discuss it more then.

hidden geode
#

We have discussed this to death

#

Over the last 2 weeks

#

You do not need to wait for pup to make this change

opaque vector
#

Basically, if this transfer work is to get moving, Soaryn, please perform the changes that maintainers ask for. Pushing back on every requested change is going to deadlock this PR. Right now, that pushback is becoming a massive pain point for us maintainers and we need you to stop and please listen to us

mossy chasm
cunning mulch
silent root
#

That was what I just pushed not what we had to be clear

#

the as was either something to keep or moved to utility, but having to type out what it all was before each time I wanted to use was not good UX which is why I added the as

cunning mulch
silent root
#

May have been just as soon as I pushed then soarynLOL

#

(just now)

#

But that link should be all changes up until the record

cunning mulch
#

Bunch of helped that are overkill for now. Indifferent on the single instance thing

#

Though if it is a performance bottleneck we could always just intern it down the line similar to how resource keys or whatever are. But only bother interning the empty variant

silent root
#

@mental raven since you want to get rid of the immutable and all other utilities related to ResourceStack, may I at least add a way to "deconstruct" the "built in" stacks (item/fluid) to their relevant resource types with an interface?

public interface IStack<T extends IResource> {
    T resource();

    int amount();
}

On ItemStack it would just have the extension extend the interface with IStack<ItemResource>, with the FluidStack, it would just implement it directly IStack<FluidResource>.

mental raven
#

That would be a question that other maintainers have to weigh in on. Personally, no

flat root
#

no, out of scope

silent root
#

How is it out of scope?

flat root
#

it is not needed in the first version of the transfer rework; the transfer primitives operate on the resources, stacks are really a secondary concern

mental raven
#

I think it's feature creep, we should focuss on finishing what we have and getting it in. We can add more stuff later

opaque vector
#

The commit names going into the PR is quite passive aggressive. Not very friendly…

flat root
#

Hey @silent root, we have decided with a near-unanimous vote that the NeoForge maintainers will be taking over the rest of the transfer refactor. We all want this rework to happen sooner rather than later, however given its scope and impact we also want to have it properly reviewed. The way you have reacted to requests for changes has been inappropriate, delaying the rework and creating increasing amounts of friction. This cannot go on for the remaining slices, so us maintainers have decided that we will be making the changes we want to see directly.
Concretely, this means that maintainers will be making the final changes to the currently open resources slice PR, and take care of the subsequent slices via maintainer-opened PRs, starting from the slice commits in Adrian's repo. Each slice will be properly reviewed by the maintainer team as usual. You're, as always, still free to discuss the PRs both on github and in this thread, and so is anyone else!
With this, we also hope to be able to deliver the transfer rework sooner and smoother overall, ideally in the upcoming Minecraft release.

flat root
#

so, I am finishing up the resource slice. I want to rename the ItemResource#is(ItemStack) method since it's confusing: it checks both the item and the components while the other is methods only check the item

#

we could do isSameItemSameComponents but that's a bit long

cunning mulch
#

I wonder if matches would make sense? Yes ItemStack#matches also checks the size (and is static), but item resources are basically countless stacks

#

so it might be semi evident that there isn't an amount for it to care about?

flat root
#

matches is what I used in Fabric so I would definitely be in favor 😛

#

I was hesitant because there is a matches method for stacks already, but it's static and it's quite obvious that we don't check the count, as you said

#

so let's go for matches

#

regarding this, I suppose we could check for the class and do a cast?

image.png
loud stirrup
flat root
# 
    public boolean equals(Object obj) {
        if (this == obj) return true;
        if (this.getClass() != obj.getClass()) return false;
        ItemResource other = (ItemResource) obj;
        return ItemStack.isSameItemSameComponents(this.innerStack, other.innerStack);
    }

like this?

#

looks about right to me at least

silent root
#

Need to check null then too

serene igloo
#

this == obj will fail if obj is null

#

(and if this is null then you can't call equals on it anyway)

flat root
#

ah right, forgot the null check

serene igloo
#

nvm I'm an idiot

#

read return true as return false

silent root
#

I was solidly confused for a moment and was quickly writing one to check myself soarynLOL

flat root
#

interestingly intellij doesn't complain about the missing @nullable

silent root
#

@noble latch and I had a brief discussion at one point between the speeds of the instanceof vs the class based check, and in theory for this case instance of was technically faster since it was doing more in one go iirc. It is negligable, but in readability, the class check one is more obnoxious typically

flat root
#

generally getClass is recommended to avoid symmetry problems with subclasses, but for a final class it doesn't matter

#

I already changed it to getClass here

#

@cunning mulch your comments have been addressed I believe, so we're just waiting for @hidden geode

mossy chasm
#

For final classes I usually do instanceof because it lets me inline

# 
return this == obj || (obj instanceof final ItemResource that && ItemStack.isSameItemSameComponents(this.innerStack, that.innerStack));
#

Though this is something I wouldn't recommend lol

#

Long lines

mental raven
#

instanceof on final classes is implemented as == getClass with null guards in the jit

flat root
#

ah ok so no perf difference either way then I suppose

#

(usually instanceof is somewhat slow)

mental raven
#

Atleast when i looked into it in j8, i highly doubt that has changed

flat root
#

it's a pretty obvious optimization, and it seems like it could make quite a difference

mental raven
#

Generally i prefer manual getClass comparisons for equals though, as its what intellij uses when you tell it no hierarchy

mental raven
#

Or maybe i'm thinking of invikeinterface vs invokevirtual

swift summit
opaque vector
#

Also autocomplete is a thing so long names are fine imo if they are excessively clear for everyone

serene igloo
#

my desktop is extended across two monitors so I can just make my IDE really wide if I need to

cursive geyser
#

my monitors are different size, different resolution and different pixel density, so if I did that it would look weird as fuck

opaque vector
#

One monitor is a curved 4K monitor and the other is a CRT

hollow maple
#

I would definitely prefer instanceof X to x.getClass() == ThisClass.class

wind steppe
#

transfer rework tomorrow

void thicket
silent root
#

It'd likely be better to have them be separate methods to ensure intent when checking.

untold eagle
cunning mulch
flat root
#

@hidden geode reminder: the resources slice is waiting for your review

hidden geode
hidden geode
mental raven
#

I agree, they don't really seem worth it.

untold eagle
#

what don't you like about it?

hidden geode
#

The entire system

#

It seems over-engineered for near to no gain in performance

#

And it causes a maintenance headache

untold eagle
#

iirc it's to reduce gc churn with all the allocations

hidden geode
#

IMHO we can address that later as it is an implementation detail

#

I would like to rip out everything that is not actually needed, and then have reality test it

#

If it is a problem with the GC Churn, which I doubt, since ItemStack does not have that logic, and its GC Churn is under control as well

flat root
#

Actually, the claim here is that most resource transfer happens without data components. And so it makes sense to cache the resources with an empty patch

#

Thus reducing the amount of allocations by quite a lot (e.g. getResource on a vanilla container would otherwise allocate for every single call)

#

It might be a little bit cleaner with a mixin to hide the method though

loud stirrup
#

The method being visible isn't great

#

Primarily because of auto completion

#

TBH one could do a separate class that accesses package-visible method on ItemResource to construct the Item instances

silent root
hidden geode
#

But right now from bare testing I don't see a need

silent root
#

The equals is also performantly better if we leave it in

flat root
#

I think the optimization is very valuable, otherwise scanning a double chest will potentially allocate 54 objects when that could have been avoided

#

But we can indeed remove it for now and figure out the best way to add it later if needed

hidden geode
hidden geode
#

These are details which we can handle later when we have more data on the usage and its costs

flat root
#

Most common resources that a player might obtain usually don't have a patch. But of course a chest full of mob drops will have patches

silent root
#

And even thrn most mobdrops dont have patches

#

Only weaps and armors from that pool typically.

cunning mulch
cunning mulch
#

And for items meh, deviates some, but there are still lots of blocks that get moved around that players broke in the world

loud stirrup
#

Yeah I think the "with default components" is just meant to be a generalization to mean "empty patch"

cunning mulch
#

It isn’t just machines or stuff that are being moved

hollow maple
#

Most items and fluids are their "default instance", so caching the allocation is certainly worthwhile

flat root
#

The way it's currently wired is certainly not great though

#

It adds a public method to Item and a public method to ItemResource - both internal

cunning mulch
#

That part is fair, but we still should have it, even if we just change how it is wired slightly.

#

I wonder if we could somehow do away with the one on the resource by instead just making it so that there is some way that while currently initializing it then defaults to a new instance and otherwise using the existing default

#

Unsure, and also need to go for a bit so can’t bounce ideas back and forth for a bit. I will probably be back in an hour or two

flat root
cunning mulch
#

That could work, but isn’t exactly what I meant. I sort of meant some custom lazy impl that stores if it has been initialized (and if it has started initializing) and then if it is initializing it is the one case it is allowed to return null. But then it calls Resource of, which if it gets null for the default resource instead returns a new instance which then the default instance that called it is able to cache

#

It would be a larger patch (and potentially something that should be in a helper class instead), but I think it would work and also allow skipping adding a map lookup

#

And then there is only a single method marked as internal instead of two

flat root
#

Ah yeah that can work. A bit meh though. Cause you'd probably need a setter then on the Item class

hidden geode
#

Imho it is still a premature optimization. I think it should be removed from this PR and reconsidered later with more data and a cleaner implementation

loud stirrup
#

The vast majority of transfers involve default item instances, so this has big impact. However, it can be done in a separate PR to remove the methods we dont want to be so visible now

#

I don't think however that new transfer will go live without this

hidden geode
#

That is all I am asking

hidden geode
flat root
#

Are you gonna do this work?

#

A guaranteed allocation for every single slot read for vanilla containers doesn't look good at all, so if nobody is gonna do the performance benchmarking (and GC benchmarking is really not fun), we should implement the optimization

#

We can discuss the best strategy to implement it (e.g. map lookup vs field in Item) but I think that removing it is not the right call

loud stirrup
#

Well three options:

  1. merge it as-is, hide the methods in a follow up PR
  2. remove it from this PR, re-add in a followup PR with better hidden impl details
  3. remove it altogether

I have no problem with either 1) or 2)

#

But I think we should hide the impl better

flat root
#

Either 1 or 2 is fine by me as well

mental raven
#

1 or 2 is fine by me

#

i'm not sold on the map, but we may be able to do some recursive init magic

cunning mulch
#

I think it is important to have, and we can just hide it better in the future. It isn't worth the risk of it somehow being missed/forgotten to be added when we aren't looking specifically at the resources and are instead looking at the transfer stuff

#

so I say 1 makes most sense

#

given it realistically is directly resource related which is what this slice is

cunning mulch
# flat root Ah yeah that can work. A bit meh though. Cause you'd probably need a setter then...

nah, it definitely would be a bit meh, but I more was thinking the following which doesn't need a setter:

private boolean cacheInitializationStarted;
@Nullable
private ItemResource defaultResource;

@Nullable
@Internal
public ItemResource getDefaultResource() {
    if (!cacheInitializationStarted&& defaultResource == null) {
        cacheInitializationStarted = true;
        //ItemResource#of will end up calling getDefaultResource, but we won't enter this branch because of the boolean set
        // and instead will just return null
        defaultResource = ItemResource.of(this);
    }
    return defaultResource;
}

//In ItemResource:
public static ItemResource of(Item item, DataComponentPatch patch) {
    if (patch.isEmpty()) {
        ItemResource resource = item.getDefaultResource();
        //Handle if the resource is null because we are being called by the item when it is instantiating the default resource cache
        return resource == null ? new ItemResource(new ItemStack(item)) : resource;
    }
    //existing code for making the stack with the proper patch
}
hidden geode
#

I am for 2)

silent root
#

I would avoid a map in comparison to immediate access. That lookup is much more expensive than just "get default of me" even if "immediate" goes through a lazy init

loud stirrup
silent root
#

There is an event that is called rather late

#

that modifies the default components for items

#

Can't remember if that event is expected to be called on world load or fine on main menu

loud stirrup
#

Does that matter if the ItemResource is just holding an ItemStack with an empty patch?

silent root
#

Yes, as empty patch isn't known until AFTER that event

cunning mulch
#

because it might make the default components not be empty

#

so then the item stack will be a patched component map with defaults or with empty

#

I guess

loud stirrup
#

Hm? Shouldn't the patch always be against the item default components?

#

so, empty patch always means == item default stack

cunning mulch
#

ItemStack creates patched component map

silent root
#

That event can modify the default to be a new default set

cunning mulch
#

and effectively "copies" the default components

#

iirc

loud stirrup
#

Okay it does store a reference to the prototype

#

that is the actual problem

#

So yeah, ok. In that case we should document that at the lazy initialization site.
As for the function to create the instance, we can do that in a package-private Internal class. I have no problem with an internal method being accessible, just with it being in the very visible auto completion list for ItemVariant/FluidVariant

flat root
#

one thing we might be able to do is cache the ItemResource directly inside the ItemStack

#

we can probably keep it quite cheap by playing some tricks with the copyOnWrite field of PatchedDataComponentMap

#

so yeah there's a number of ways to do it; for now I'll postpone (i.e. go for 2)

cunning mulch
#

Add a todo in item resource though (ideally with some standardized tag for us to search for) of things to address before final merge into master

flat root
#

I will just open an issue instead for more high-level things

#

we might also choose to release without this, but we should keep it in a list of "follow-up things"

cunning mulch
flat root
#

Ok removed. @hidden geode quick merge?

hidden geode
#

Github Is fucked

#

One second

hidden geode
cunning mulch
#

Which slice(s) is next?

flat root
#

first step: yeeting transfer characteristics

#

hmm, I actually wanted to have the branch on the NeoForge repo, not mine; oops

hollow maple
#

I actually really like the concept of transfer characteristics honestly

#

It lets us optimise common scenarios (e.g. "infinite" or "voiding" containers) without actually knowing the exact implementation

#

the supportsInsertion/Extraction is honestly kinda ugly and hard to extend by comparison

#

and also has the problem of people treating it as a ground truth rather than a hint

#

(the naming implies that if you return false from e.g. supportsInsertion nobody will try to insert to you, which isn't true)

cunning mulch
#

Though I haven’t looked at the system yet so still unsure if it actually makes sense/is implemented decently

hollow maple
#

What about, for example, if I have some sort of "condenser" which converts items into some energy/currency, but doesn't have a way to get the item back out?

#

That would be a "voiding" container, but it wouldn't use any of the existing templates

#

And subclassing the "voiding" handler also doesn't sound right to me

cunning mulch
hollow maple
#

Nope, I was thinking of something like the AWESOME Sink from Satisfactory 😅

cunning mulch
hollow maple
#

But also mods like AE2 and, yeah, equivalent exchange, have similar concepts

flat root
#

We already had a lengthy internal discussion on the characteristics, that was the tipping point for the slicing

cunning mulch
#

Ehh, more so from the commit being seemingly out of nowhere

hollow maple
#

Did we? My understanding that the "discussion" we had was that, yes, the commit was seemingly random

cunning mulch
#

I am currently undecided

flat root
hollow maple
#

I meant for users

#

Not us

cunning mulch
#

Consumers*

hollow maple
#

Right

#

If a consumer is making e.g. a item pipe network, they could automatically lower the priority of handlers which are "voiding"

#

I do agree that we should slim down the number of "templates"/"containers" we have, but the concept of characteristics presents a really clean way to categorise containers and handlers without relying on the exact implementation, and is WAY more expandible in the future than extra methods on the interface

flat root
#

This is all quite theoretical talk that doesn't hold up in practice I'm afraid

hollow maple
#

And, I also think it is likely to prevent misuse from people misunderstanding "supportsInsertion" and "supportsExtraction"

flat root
#

Another thing to discuss is whether to throw in case of negative amounts. Personally I would support that

hollow maple
#

(Within the context of Minecraft, that is; spliterators of course are where the concept originated)

cunning mulch
#

Meh tbh I still feel like the supports insertion/extraction in general is weird/a preoptimization. Like sure I know the existing system supports it, but I would honestly be more in favor of holding off on having them (or characteristics) in the new system (at least for this slice)

#

Then again I never have really used them xlurk

flat root
#

They exist in part as a parallel to canReceive/canExtract for energy

cunning mulch
#

I know

flat root
#

The usefulness is arguably very limited

hollow maple
#

I think the only people who would be interested in characteristics/supportsX/canX are people implementing resource routing/handling networks (e.g. Mek pipes, AE2 buses, Xycraft pipes)

cunning mulch
#

I mean I think they are a better solution. But I am not sure about for the initial PR. And I think one way to leave it as more open to them might also just be to not have the supports thing off the bat even if the existing system has it. Given how different people implement the methods currently, it isn’t really a loss. I don’t consume those results at all currently (and my returned results for mek’s stuff is meh as the place that wants them is way too many wrappers deep for me to want to pass through some sort of hack to be able to provide a “perfect” return

hollow maple
#

Right

#

I agree that I don't think we need them initially and should defer them

cunning mulch
hollow maple
#

Yeah; for a built network it's certainly convenient being able to skip any handlers that you know ahead of time are likely to never accept an item for insert

#

It also means that you can also e.g. provide a handler that can neither insert nor extract for "visual" connection, which is necessary for some mods/pipe networks/etc

noble latch
#

Apart from questioning the usefulness of the characteristics, I have exactly one thing to say about them (which I had also mentioned in the previous discussion on this topic): bit masks are amazing for implementation details but as part of the API they are garbage.
Case-in-point: I was playing with the Class File API earlier and building a method with it requires immediately specifying at the very least whether the method is static or not. Since the last parameter of ClassBuilder#withMethod() is a builder consumer, the flags cannot be passed as a vararg. Instead they opted to use an int for a mask with several ways to populate it and none of them referenced/documented properly.

hollow maple
#

Yeah; the design was to my understanding lifted mostly entirely from Spliterators

stone dragon
#

I'm going to be honest, transfer characteristics seem kinda obnoxious to me, and even the supportsExtraction/supportsInsertion I... don't really understand the point of?

#

Like, I guess so that someone can avoid interactions that they know will fail (aka, avoiding trying to extract if it's known to be unsupported)? With that in mind, the supportsExtraction/insertion seems fine to me even if a bit early (and I mean, is it even cacheable? Is it something a resourcehandler is allowed to change?), but the characterestics were... something that seemed like it'd be obnoxious to deal with, to me. Especially the "voiding"/"infinite" ones or the like -- they're just too blurry of a definition (I mean, consider vanilla, even -- is the vanilla composter "voiding"? Must it be destroyed without side effect to be voiding? And at that point, what counts as "destroyed" anyhow? Same with "infinite" or the other categories -- does "infinite" mean I can always extract? What if I have a thing that conditionally provides as much as you want to take from it? Etc. it's all just kind of ugly and I feel like categories, should they exist, need much more explicit requirements than the characteristics had), so I think tossing that is probably a good idea for now

#

Ahh, I see my question on caching is answered in the javadoc, cool. Well, supportsInsertion/supportsExtraction seems fine to me then

vernal dust
#

I'd actually like to see characteristics as an interface with some sane "basic defaults" supplied, calling it metadata

Something like

interface ITransferCharacteristics {
  boolean canInsert(): true;
  boolean canExtract(): true;
  default boolean transformsInputs(): false;
  default boolean isVoiding(): false;
}

class CommonTransferCharacteristics {
  ITransferCharacteristics STORAGE = (true, true, false, false);
  ITC SUPPLIER = (false, true, false, true);
  ITC TRASH_CAN = (true, false, true, true);

  // etc
}
#

Where our APIs offer one of these up as part of its contract, and we can control characteristics/metadata separately from implementation

#

Usage being something like:

// find everywhere on the network we can insert stuff safely
var storages = getAttachedHandlers()
  .filter(handler -> handler.characteristics().canInsert())
  .filter(handler -> !handler.characteristics().isVoiding())
  .toList();
silent root
# flat root Another thing to discuss is whether to throw in case of negative amounts. Person...

Negative amounts should throw, but empty resource should no-op in most every case we have. This is what I had on my local version

public final class TransferPreconditions {
    private TransferPreconditions() {}

    /**
     * Ensures the value passed in is non-negative, throws otherwise.
     *
     * @return The value passed.
     * @throws IllegalArgumentException when value is negative.
     */
    public static void checkNonNegative(int value) {
        if (value < 0) {
            throw new IllegalArgumentException("Non-negative check failed: " + value);
        }
    }

    public static void checkNonNegativeAndIndex(int value, int index, int size) {
        checkNonNegative(value);
        Objects.checkIndex(index, size);
    }

    /**
     * A utility method to validate if the resource and amount are valid values for {@link IResourceHandler} usage.
     * <p>
     * Typical use-case is in handler insert or extract implementations to determine if the operation should short circuit.
     * 
     * @param resource The resource to check.
     * @param amount   The amount.
     * @return {@code true} if the resource and amount were not empty (resource's method {@link IResource#isEmpty()} and the amount is greater than 0); {@code false} otherwise.
     * @throws IllegalArgumentException When the amount is negative
     * @see ResourceHandlerUtil#isEmpty(IResource, int)
     * @see ResourceContainerContentsHandler#insert(int, IResource, int, TransactionContext)
     */
    public static boolean validateHandlerInput(IResource resource, int amount) {
        checkNonNegative(amount);
        return !ResourceHandlerUtil.isEmpty(resource, amount);
    }

    /**
     * A utility method to validate if the resource and amount are valid values for {@link IResourceHandler} usage.
     * <p>
     * Typical use-case is in handler insert or extract implementations to determine if the operation should short circuit.
     * 
     * @param resource The resource to check.
     * @param amount   The amount.
     * @param index    The index the handler is currently operating on.
     * @param size     The size of the handler.
     * @return {@code true} if the resource and amount were not empty (resource's method {@link IResource#isEmpty()} and the amount is greater than 0); {@code false} otherwise.
     * @throws IllegalArgumentException  When the amount is negative
     * @throws IndexOutOfBoundsException When the index is negative or surpasses the size of the handler.
     * @see ResourceHandlerUtil#isEmpty(IResource, int)
     * @see ResourceContainerContentsHandler#insert(int, IResource, int, TransactionContext)
     */
    public static boolean validateHandlerInput(IResource resource, int amount, int index, int size) {
        checkNonNegativeAndIndex(amount, index, size);
        return !ResourceHandlerUtil.isEmpty(resource, amount);
    }
}
#

Anywhere there was a ResourceHandlerUtil.isEmpty() call in that version, the empty was handling the negative check. Since it is not anymore, we need to do that separately. I changed mine from

if (ResourceHandlerUtil.isEmpty(resource, amount)) return 0;

to

if (!TransferPreconditions.validateHandlerInput(resource, amount)) return 0;
#

( or the index one where relevant)

flat root
mental raven
#

I hate the characteristics, half of them are not useful at all and are micro ops, others are incompatible with eachother, the only ones that make sense are insertable && extractable

#

which are trivial getters

copper fjord
#

I'm opposed to anything that's a bit mask

#

and strongly so, they're a pain to debug and opaque when exposed as api

flat root
#

so a number of further questions:

  1. Do we want to throw or noop when inserting an empty resource? Personally I would go for a throw, as it is usually a logic error to attempt to insert an empty resource. And it will save us countless if (resource.isEmpty()) return 0; statements.
  2. We agreed to have both int and long getters for the amount and capacity. I would personally make the long one abstract, and the int one default. This simply makes more sense since returning a long is easy even if you work with integers. Happy do discuss this if you disagree.
mental raven
#

1, Current transfer api would just nop right? I think that should be preserved, its not really saving anything preventing that and just pushes all that isEmpty checks onto the user, who shouldn't really care.

#

2, long as default, with int as abstract makes the most sense to me, although, i guess it doesn't really matter, anyone can upcast their int length to long just fine

flat root
#

yes the current api would noop; personally I dislike it, if you try to insert 10x empty you really fucked up your logic and we should catch it

mental raven
#

Oh, inserting an empty resource, definately should throw

#

inserting zero of a non-empty resource should not

flat root
#

ah yeah that is what I meant

#

empty resource -> bad
0 amount -> fine

mental raven
#

I hope to go through the PR later this weekend, bug me if i forgor

copper fjord
#

well, not directly downcasting, rather clamping but you get what I mean

#

I mean wrt I'm fine with either but I think it's easier to implement abstract methods as longs than abstract int methods when you internally use longs

#

in the latter case you need to override both the int and long methods and manually clamp the int one, whereas up casting from int to long is implicit

mental raven
#

i think long being abstract is the least painful for implementors, as the downcast/clamp doesn't have to be reimplemented each time

#

upcasting/extending from int to long is free, so w/e

flat root
#

thoughts on int maxAmount (currently we have int amount) as the param name for insert etc?

flat root
#

ok I made the changes we discussed

#

and did a bunch of cleanup/simplification

#

I removed ScopedResourceHandler because I couldn't find a good name, and it wasn't there in the past. SingleIndexResourceHandler is gone in favor of static methods on RangedResourceHandler

#

I am tempted to flatten this all into the handlers package

image.png
flat root
#

OK the PR is now ready for review

silent root
#

Let me double check the "resources" package later

loud stirrup
silent root
#

I honestly prefer how it was for ux in regards to getAmount being the implemented method and getAsLong the added defaulted. It did make the implementation of the long slightly more involved, but that was sort of the point.... not to trick a user into thinking somehow this long is transferrable and that we always have the right typing when working with it

#

We had a discussion on this a WHILE back #1183818213134446742 message

flat root
mental raven
#

I think we wait till a few more slices before solidifying any flattening, I personally would like to see more of the rework first before agreeing to any particular package structure

loud stirrup
#

First pass didn't get that far will continue later 😅

silent root
#

@flat root your transaction follow up would likely be better without that extends @Nullable Object binding: It just causes IDEA to complain on EVERYTHING otherwise whenever someone has a package info remotely similar to neo's soarynHmm

image.png
loud stirrup
#

I created a draft PR we'll later delete that provides a PR publishing artifact of the current consolidated transfer staging branch: https://github.com/neoforged/NeoForge/pull/2574
(which hopefully builds successfully)

GitHub
Transfer Preview [PR will not be merged] by shartte · Pull Request...

This PR only exists to have a PR published artifact of the current transfer rework staging branch.
Series of PRs:

Transfer: Transactions Review #2414
Transfer: Resources Review #2415
Transfer Rewo...

cunning mulch
loud stirrup
#

Doesn't really matter, no

#

it's 64-bit registers anyway

silent root
#

The basic intent should always be an int. The advanced usage, while not necessarily java standard which shouldn't apply to this as java standards aren't really relevant, should be longs. But confusing a user with the very forward facing "here is a long, by the way you can't use this really anywhere" is not a good approach

loud stirrup
#

No, it's the opposite

#

By making the other method NonExtendable it's less confusing what to implement

#

Since you always implement the long method

silent root
#

What I'm saying is, that they are presented with a method that is always a long. When EVERYWHERE in the api is ints

#

so it creates a disconnect

loud stirrup
#

Well that part of the API isn't int

silent root
#

It only benefits a handful of mods

loud stirrup
#

¯_(ツ)_/¯

#

if the implementation side is identical I really dont think it matters for the implementor

#

And it is

#

If you want another example of this, see RandomAccessFile, etc.

silent root
#

If you are talking about "read" that isn't quite the same

#

as they encode 257 values for it

#

0-255 for the byte value : 256
-1 for a cancellation

loud stirrup
#

what?

#

I am talking about long length() and read taking 32-bit lengths

#

since arrays can only be 32-bit

silent root
#

You are "talking" about a file

#

so it was not very clear

loud stirrup
#

Since we know there can be more than Integer.MAX_VALUE of a resource, but we only allow transfering up to Integer.MAX_VALUE per operation, it's not that dissimilar

silent root
#

These two paradigms though are built for completely different set of users as well as extension.

#

Ours should be built with the intent to be used by arbitrary dev of varying skill levels

#

RandomAccessFile isn't really made to be extended, (it can be) but built for users who are familiar with file IO or similar systems

#

The audience is vastly different

flat root
#

I don't see anything confusing with the way I implemented it. The amount can be long for arbitrary users, but we provide convenience methods to clamp to an int. Anything else is more confusing

flat root
silent root
#

Switch the type... it will complain still

#

That was the one I had on hand

flat root
#

Show the full class? I wrote a test and I didn't see a warning. Maybe look at the test

silent root
#

Did your test have package info in it?

loud stirrup
#

Yeah I see that too

silent root
#

We likely don't need to specify that T is nullable

#

It only is relevant in Void

flat root
#

It's the right thing to do. Whether it works in intellij or not is a different question. If you don't show the warning I can't tell you what to do

silent root
#

... I did? #1183818213134446742 message

loud stirrup
#

I'll do a PR-to-the-PR with some doc improvements. Generally I'll try to move anything that is aimed at implementors into impl specs

#

And adjust the summary to be more from the perspective of a consumer

loud stirrup
#

CombinedResourceHandler is intended to be subclassed, yes? Given the protected methods

silent root
#

It can be yea, but should mostly work out of the box from what I remember

hollow maple
#

We should prefer ints in the API where possible. This is consistent with most if not all other APIs we expose, and vanilla itself.

#

long-returning APIs are inherently a more advanced use case and most handlers will not ever exceed the 32-bit limit

#

(Consider that most inventories are chest-sized or smaller, as they are e.g. machines with a single input/output slot)

#

I'll admit that it is convenient having the ""default"" be long, but we should be designing for the majority use case, and huge containers like Storage Drawers or AE systems are very often an exception to the norm

loud stirrup
#

AE doesn't have IItemHandlers with amounts > Integer.MAX_VALUE, I don't understand where this myth comes from.
This is purely for having full visibility to the actual amount in external storages, and not designing an API that is gimped from the get-go.

#

Can we stop re-litigating the points that were already litigated and led to having long getters in the first place?

hollow maple
#

I wasn't implying that it did; I was just talking about huge containers in general

loud stirrup
#

Since it's just a drawn out re-hashing of things that were already said.

hollow maple
#

What we agreed on, was there being int getAmount() and long getAmountAsLong()

#

I agree that there is indeed a need for such an API, but the shape that is being proposed was not what we agreed on

loud stirrup
#

That was under the assumption that an implementor would have to pick which to implement. Which is inferior to the updated version where it's crystal clear what an implementor has to do

#

without extra effort

silent root
#

But now the caller is going to inevitably wind up with issues

#

insert(something, getAmount()... oh it complains it is a long? I uh? cast int!

#

And that will result in an immediate crash if it was just slightly over an int

loud stirrup
#

IntelliJ will present you the getAmountasInt method first

#

in auto completion

silent root
#

That is REALLY depending heavily on autocomplete

#

Like a crutch

hollow maple
#

Not everyone uses IntelliJ too

#

(I don't know if Eclipse/VSCode/whatever presents in the same order)

silent root
#

The point of asLong was to be crystal clear your intent was to be outside normal scopes

#

in both implementation and consumption

loud stirrup
#

What does that even mean. "in normal scope"? It's obviously in scope to have amounts larger than Integer.MAX_VALUE, otherwise the method would not exist

silent root
#

Normal scope = normal use

hollow maple
#

Presenting long getAmount() as the "default" getAmount implies that we're encouraging longs everywhere, which we're not

silent root
#

MOST mods are going to have a capacity, or amount that is 64 or so. Or a several buckets. Not ... 9 quintillion

loud stirrup
#

what does this have to do with encouraging?

#

There ARE handlers that store more than Integer.MAX_VALUE

#

there's nothing to encourage, it's just a fact

#

We made the choice to limit individual operations to int, which is fine

#

But acting like it's somehow magic that a handler could contain more is mind boggling

#

ultimately, providing an amount clamped to int is purely a convenience due to the operation limit

hollow maple
#

Consider that you're a beginner modder, and you look at IResourceHandler because "the guys in NeoForge are smarter than me", but if you see long getAmount() you'll LIKELY assume that is the method you're INTENDED to use

cunning mulch
loud stirrup
hollow maple
#

Like I said, I'm not arguing against a long-sized amount/capacity accessor

loud stirrup
#

But just from what the methods do, it is more transparent this way, given the getAmount doesn't convert anything, while getAmountAsInt does.
But ultimately I'd be fine with swapping it around

hollow maple
#

I'm arguing specifically against the naming seeming to imply that the long-sized accessor is "preferred"

rough tinsel
#

getBackingAmount / getTrueAmount? (Not too happy with those however)

loud stirrup
#

It is not if you want to conveniently have a clamped amount for transfer

#

But whatever, we can suggest swapping the naming back

hollow maple
#

I already have; I left a review comment on the interface saying to do that

loud stirrup
#

But implementation wise it's definitely better to only have one of the two as preferred-to-implement

#

No, you asked for reverting it

#

Which isn't the same

hollow maple
#

Is it not?

loud stirrup
#

no

hollow maple
#

I was assuming that "revert" in this case was "revert to the agreed-upon int getAmount() / long getAmountAsLong()"

#

I'll clarify that

#

There - I've updated my comments to reflect my intent better

loud stirrup
#

The more important part is default-implementing the int-getter and marking it as NonExtendable so implementors dont get as confused on what to do

#

Since IJ will not default-select that when you do "implement methods" in your subclass

#

It probably does it purely because of the default implementation alone, heh. The annotation is just a marker

silent root
#

That annotation really does nothing for autocompletes

hollow maple
#

Yeah; I'm fine with getAmount being defaulted to getAmountAsLong as a convenience

#

Since I think MOST people won't be implementing the IResourceHandler themselves

loud stirrup
#

even if they do, they can blindly hack away at it and pay no attention

#

and will get it right

#

whether they use int or long internally

vernal dust
#

Clearly this discussion is an eternal argument, so we should make it be byte[] getAmount() where everyone has to length-check the array and figure out how big the storage is /s /s /s

trail fable
#

So BigInteger?

hollow maple
trail fable
loud stirrup
flat root
cunning mulch
#

it feels rather meh to have 95%+ of call sites having to have getAmountAsInt (and I don't mean about typing it, I more mean about reading it as part of control flow logic)

flat root
#

Well, if it's for an unknown handler you might be losing some info

#

The naming is there to remind you to check if that's ok or not

#

Even a simple "is full" check should really be using the long amounts, for example

serene igloo
#

what if we just made everything ints

trail fable
#

so the JVM stack

wind steppe
#

It is worth considering that the characteristics might be more useful in the item context than it is in the block one. For example, I use that feature to determine if an item is allowed to be in the drain slot of a battery block, or to see if I can skip it during an inventory wide scan of items for energy distribution

#

It’s also a fairly trivial thing to implement that makes it useful for this use case ^, a use case I don’t think is uncommon

stone dragon
#

The issue with the characteristics is that a lot of the ones that were there are just... highly ambiguous. The "voiding" and "infinite" ones in particular, jump to mine. The supportsInsertion/supportsExtraction, sure, that's useful for performance reasons -- but seeing as both should default to true that's also a trivial thing to add later if it turns out it's desired

hollow maple
#

I'm not sure how "voiding" was ambiguous; it means that any insert will succeed but the handler will never contain anything

#

Nonetheless, we should prefer the characteristics approach because I guarantee that we will in the future want to disambiguate certain handlers beyond a simple "can i insert? can i extract?"

loud stirrup
vernal dust
#

I can very easily picture something like a factory manager showing an option to skip trash cans on a network

#

"If you pull this item out, it MUST go somewhere and it MUST NOT be deleted"

#

Wouldn't it be nice to know that info without having to do simulations all the time?

cunning mulch
untold eagle
#

Useful, yes. But the bikeshedding that will happen is best done later once the rest is in

hollow maple
#

Agreed

stone dragon
# hollow maple I'm not sure how "voiding" was ambiguous; it means that any insert will succeed ...

You could have every insert succeed, and the handler never contain anything, and have it still not be voiding because it's immediately putting the items somewhere else. Or storing them somewhere not immediately accessible to the thing that put them in but may be accessible from something else. Or processing them slowly and making them visible somewhere else after 20 ticks. IDK. The issue with "voiding" is fundamentally -- what does it mean to "delete" the items? Must they be tossed without side effects? What restrictions are on those side effects?

flat root
flat root
flat root
loud stirrup
loud stirrup
flat root
loud stirrup
flat root
#

Alright another grammar nazi comment: handler's not handlers

loud stirrup
#

Yeah I keep repeating that, since it's the opposite in German 😄

flat root
#

Genitiv hehe

loud stirrup
#

"Deppenapostroph"

#

You btw, commented on the commit 😅 Anyway, pushed the apostrophe

flat root
#

I see that there hasn't been any discussion yet on the methods in ResourceHandlerUtil. Did anyone have a look yet? In particular wrt which methods to keep and which to remove

loud stirrup
#

I just saw a bunch of commented out stuff

flat root
#

Yeah cause idk what I need to fix and what I need to remove

#

To be fair I also didn't really think about them

cunning mulch
flat root
#

Yeah that you can't see regardless

#

But yeah I don't see the use case anyway

#

Is a matter condenser really voiding if it's gonna produce a matter ball? 😄

loud stirrup
#

Well, can be added later anyway

#

Now you brought it up, that was the very example I was also thinking about 😄

#

I'd say technically no, if the definition really is the item disappears without any side-effect

cunning mulch
flat root
#

pup do you have further thoughts regarding int/long? I don't want to design the API in a way that tries to "ostracize" long users

loud stirrup
#

IDK about ostracize. I was a bit indifferent, but the point about checking if something is full only being "correct" when using the long version is a good one

flat root
#

My take is that if using the longs doesn't cost you anything (e.g. if you're doing an amount < capacity check) you should use the long helpers to ensure correctness

loud stirrup
#

An example of where that's on display is a method like ResourceHandlerUtil.getRedstoneSignalFromResourceHandler

flat root
#

Yeah, or ResourceHandlerUtil.isFull

#

Or similarly a BC-like gate that checks for 50% fullness (amount >= capacity / 2)

loud stirrup
#

Okay hm. What is the intention for the extract helper that takes an index?

The javadoc seems incorrect either way (Extracts the first resource from an {@link IResourceHandler} that is not empty.) since it will only extract from that one index.

I can't quite think of a use case for that one

silent root
#

All the ones with a stack factory are methods that were present before in some fashion. This allows us to make one control logic, and have the individual resource types bounce through that with something like ItemResource::toStack or ResourceStack::new. We need the IStackFactory back for this though as the point was to provide a way that you could construct the stack type the consumer needed.

In regards to the extract with an index, I seemingly missed passing the index into the extract method on the handler extract

loud stirrup
#

Hm? I don't follow, the signature of the utility method has the index, it just seems like it wraps IResourceHandler#extract with a filter on the resource, is that the intent?

silent root
#

I meant the handler.extract should be using the index

#

Let me double check where it is used

#

ItemUtil.extractItemStackAtIndex

#

yeah that handler should be passed the index

#

It however constructs a stack from the given index, resource, & amount

#

So something like ItemStack extractedItemStack = ItemUtil.extractItemStackAtIndex(handler, resource -> true, i, 1, transaction); for the hopper logic

#

I just didn't notice the issue in tests, since its logic consumption internally is only doing 1 as the amount

loud stirrup
#

I don't think we're talking about the same method

silent root
#

... ?

#

That is the one

#

The handler.extract.... it needs the index parameter passed through

#

We use that in the hopper logic

cunning mulch
silent root
#

The reason it doesn't have the index on line 376 is because our sole consumption of it atm is the hopper logic, which only takes 1, so I'd never see it misgrab the rest of the indices

#

So I didn't notice the bug

#

But the wrapped aspect is that it constructs a stack of what ever type the caller is wanting

flat root
cunning mulch
#

I think it is a footgun, that there is no real reason having. The main use cases people will have is transferring, and people who just start modding don’t necessarily read javadocs, and more so may not know Java well and will just downcast the long to an int

flat root
#

When do you even use the amount when transferring?

cunning mulch
#

Like I understand it, but I also have seen plenty of people just do the apply suggested fix type stuff before

flat root
#

Most people should hopefully use a move(a, b, r -> true, null) call

#

We could do getAsLong and getAsInt, that I am not against

cunning mulch
#

I don’t remember offhand, I know I use it a decent bit, but that might be the “amount needed” part, which I do understand your point that it is more accurate to use longs there. But that just furthers my suggestion to Soaryn at one point that having a getNeeded helper that calculates the capacity minus amount is very helpful from my experience on my own handlers

cunning mulch
flat root
#

So the thing I feel strongly about is that the long one should be the abstract one

cunning mulch
#

That’s fine

#

I just think regardless of the name of the int one the long one should have AsLong. Though thinking about cases it is probably best for both to just have the AsType so that people are more likely to think/read the docs where we can provide higher level examples of which scenario should use which

flat root
#

For consistency I suppose indeed that having both getAsLong and getAsInt for the names is reasonable

#

Hah, squirrsnipe

#

Agreed then

cunning mulch
#

And I will try to take a look at the state of the slice sometime in the next two or three days and leave a review

loud stirrup
#

That is fine by me too

loud stirrup
#

getAmountAsBigDecimal my beloved! 😍

silent root
#

You are dead to me

#

getAsString

loud stirrup
#

TBH that's not even such a joke K:D

#

that's something to look into later

silent root
#

I'm sorry I don't have that UUID amount of fluid

loud stirrup
#

amount formatting

#

human-readable amount formatting

silent root
#

Formatting likely should be done as a utility really outside of the interface itself

loud stirrup
#

i think there was that fluid amount formatting discussion but that IIRC only pertained to fluids

#

yes, agreed

#

generally there's a buuuuunch of stuff that can be built on this

silent root
#

Can you do me a favor sometime @loud stirrup, you had a fancy "invalid item" codec that you shared at one point, think you could try to make it generic and take in a resource codec to do a similar thing?

loud stirrup
#

Is using Math.addExact and catching the exception on overflow too slow compared to doing the overflow check myself? 🤔

silent root
#

IntMath.saturatedAdd() may be enough

#

OR unless you need the throw

loud stirrup
#

Ah, I was looking at Longs only, not LongMath

#

It'd be good to know if it actually overflows since ResourceHandlerUtil.getAmountAsLong can stop iterating further indices if it does indeed overflow already

#

Although I suppose I can just test if amount >= Long.MAX_VALUE

#

and break

silent root
#

Err...

#

amount is a long?

#

Or larger struct?

loud stirrup
#

w.r.t. the codec: I am not sure it can be that generic, I'd have to think about it. The ItemStack one relies on the ability to actually store the offending ItemStack as a component on an "invalid item" ItemStack

silent root
#

Well we have the IData resource one

#

so in theory we could at least grab those separately

loud stirrup
#

Yes, but you'd need a registered datacomponent to attach it to

#

The primary issue is that given you want a Codec to return List<T>, you need some representation of an invalid T that still conforms to T

#

I did previously think about contributing the one for ItemStack upstream, but NF would have to register an item and data components for that and I think we generally only want to do that opt-in

silent root
#

Unlike the other aspects, that one is less player based mechanic changes and more installation management which I would imagine being fine

loud stirrup
#

No it's about Vanilla compat, I believe

#

Something about the id allocation, I forget the details

silent root
#

COULD possibly have a thing to determine if there was content previously added before initializing that item

#

But I have no idea

#

I'd like to see it upstream, but not super necessary

cunning mulch
loud stirrup
#

It's something to look out for. In the context of resources in general, we can't provide a generalized codec anyway (i mean really generalized) since we have no registry of "resource types" at the moment.
If provided codecs only ever come out of a factory method, I suppose one could add a factory for replacement resources (that get access to the underlying Dynamic that failed to deserialize, for example)

#

But that's something rather advanced

loud stirrup
#

Working on a PR to the PR to restore the helpers...

#

getCapacity is a bit of weird one

#

I'd have assumed that getCapacity(IResourceHandler<?>) returns the "general" capacity, but the current impl will use the resource that is in any given index

#

Not sure it matters greatly, but is a bit unwieldy to explain in the javadoc, heh

silent root
#

"Taking the currently stored resources into account" might be a way to look into wording it, but yeah it is an odd one to specify soarynLOL

loud stirrup
#

true

#

I had initially written something along the lines of {@returns the general overall capacity of the handler} but that wont do

#

people might mistake that to mean it returns the remaining capacity

#

Did you implement it that way with a specific use-case in mind?

#

OH I see

#

We don't actually have an empty resource harold

silent root
#

soarynLOL nope

loud stirrup
#

Yeah ok ok is ee

silent root
#

Hence the second one which is what they can look for

#

which COULD be empty

#

Or should be able to at least

loud stirrup
#

yeah true, i should document the caller can pass empty there

#

to get the general capacity

#

We'll have to write unit tests for all of these harold

silent root
#

We should be able to see the final slice (misc)

hollow maple
#

And gametests catsip

silent root
#

as I did a lot of them

#

(Never got aroudn to flipping some with expect vs actual params)

loud stirrup
#

For the helpers at least there are so many cases you really want JUnit

hollow maple
#

I can see a lot of use in having gametests for e,g, custom-itemhandler-hopper going into a composter

loud stirrup
#

Otherwise it takes aaaaaaaaaaaages to run

hollow maple
#

Oh of course

loud stirrup
#

Probably just overall a good mix of both

hollow maple
#

Yeah

silent root
#

The ones that had a level requirement like actually testing getting the handler I did gametests

loud stirrup
#

yes

silent root
#

A few I made instanced based (though possibly they were unit tests only in my local)

loud stirrup
#

Anything with a level needs a gametest. We have ephemeral minecraft server, but it has no level

hollow maple
#

I was just thinking of more testing the vanilla mechanics we expand on like hoppers and composters

flat root
loud stirrup
#

I think it's probably fine assuming both have the AsInt/AsLong suffix

flat root
loud stirrup
#

aight

#

should contains/indexOf restrict usage of empty resources or at least specify that they're supported (and return the first empty slot / whether there are any empty slots)

silent root
#

Should return first available "open" index

flat root
#

I would disallow empty resources

silent root
#

It is a valid inquiry? Why not?

loud stirrup
#

If it's specified there's a point to it

flat root
#

Maybe it's ok for indexOf to allow empty, but for contains it's really weird

loud stirrup
#

It's okay to generalize over it+

#

contains an empty resource

#

I am fine with it if we mention it in the javadoc, IMHO

#

Prevents us / or the user from accidentally assuming it is not a valid parameter

flat root
#

Adding empty slots would change the result of contains even though the contents of the handler haven't changed. Weird imo

hollow maple
#

contains(empty) should either always return false or always return true - I don't think throwing is reasonable since it's very easy to get an empty resource "by accident"

loud stirrup
#

I mean if you call it deliberately with an empty resource, it's probably what you want though

#

we require non-empty resources on almost all other arguments, Monica

#

and do throw

hollow maple
#

I may be misunderstanding something then - my assumption was that an "empty resource" was e.g. an ItemStack of air with count 0

#

Consistency is generally good, so in lieu of me not understanding go ahead with being consistent 😅

loud stirrup
#

Well generally if the meaning is not well defined, we want to catch errors like that early (which is good imho)

#

but if we can define the semantics, it's fine.

#

I'll just document the current behavior

flat root
loud stirrup
#

Mhmhmhmhmhm

flat root
#

That is the argument for disallowing empty in a bunch of other places, i.e. that yes we could return a noop answer but it is likely that the programmer made a mistake so we might as well report it

loud stirrup
#

So I guess the question is: as a caller, are there situations in which you might have an empty or non-empty resource in a variable and indiscriminately call contains/indexOf

flat root
#

Say you have a filter UI

loud stirrup
#

The semantics of it are quite different if you think about it, OTOH stuff like indexOf(null) works the same way

flat root
#

Calling contains with an empty resource because the filter was not configured, is likely a mistake

silent root
#

I literally use contains(handler,Empty) now

#

It isn't that the filter is a mistake, it tells me the first index that items COULD be received in

flat root
#

Why is that useful?

loud stirrup
#

It can be useful for UI shit potentially

#

But, it might be overall a better tradeoff to have explicit methods for it

silent root
#

UI or if there is no filter at all.

#

There is always a resource present

#

Even if it is empty, that is a resource

hollow maple
#

So, my understanding is that soaryn is using contains(empty) to determine whether there is space in the handler

#

(without necessarily needing a transaction)

silent root
#

Note, a rough estimation of if there is space

#

If on an external's mod I could use this as a pre-emptive don't open up transactions that would just result in rolling back anyway

loud stirrup
#

uh you'll still have to actually try insertion since contains is still just a heuristic for "has it space"

silent root
#

correct

loud stirrup
#

and it'll be super fucking slow for some mods

silent root
#

This isn't for inserting

loud stirrup
#

you're better of using the index-less insert

silent root
#

I don't insert at all here

#

this is a block state control

hollow maple
#

Yeah; I was under the impression that you could view a handler as basically a set of resources, e.g. (a, b, c) for a handler with 3 slots, of which the empty set () is indeed a subset

silent root
#

Technically, the creative player context was also using this if (!ResourceHandlerUtil.contains(handler, resource)) { but that one may need to be redone anyway

hollow maple
# silent root

I can see the utility in being able to ask whether there is space in the container for a visual state like this

silent root
#

(though it looks like that implementation checks for empty before hand)

loud stirrup
#

I can see the utility

#

But as a separate method

hollow maple
#

I would be okay with it as a separate method

loud stirrup
#

The chance to accidentally get a false-positive because a caller made a mistake and checked for EMPTY is not insignificant, IMHO

#

I'd rather have people discover that problem early

hollow maple
#

The main thing I'm concerned about is a potential for confusion if they e.g. are thinking of sets

#

where contains(empty) would technically always be true

loud stirrup
#

Well they'll quickly discover it isn't 😄

hollow maple
#

yeah 😅

silent root
#

ResourceHandlerUtil.dontCallMeCoward()

#

Inverts minecraft

hollow maple
#

lol

loud stirrup
#

Hm. hasExtractableResource -> handler.extract(resource, 1, temp)
That could cause a false negative for handlers that only allow extracting specific amounts, but there's little one can do. Trying to extract Integer.MAX_VALUE could technically work but might trigger rather expensive checks too

silent root
#

That would always be the case no matter the util or not

hollow maple
#

If we really wanted it to be "accurate" we'd probably need a hasSpace or similar method on IResourceHandler

silent root
#

Ehhh that wouldn't go well in implementation I feel

hollow maple
#

Mhm

loud stirrup
#

This is kinda the opposite of hasSpace

silent root
#

We COULD do extract the amount currently present

loud stirrup
#

HMHMHM

silent root
#

So we do a manual iteration

loud stirrup
#

That might actually be more sensible

silent root
#

Likely a decent middleground

hollow maple
#

Right, I thought you were still talking about the has-space case DokiDerp

loud stirrup
#

It already iterates all slots anyway

silent root
#

STILL would have some false positives, but that is still fine

loud stirrup
#

since it offers a filter on the resource

hollow maple
#

But yeah, the "most accurate" solution would be to have additional methods on the interface for querying space/extractability/etc of resources, but as Soaryn said it would be kinda meh in implementation

loud stirrup
flat root
#

ok let me have a look

flat root
loud stirrup
#

Yeah idk about the amount/capacity utils either. Certainly can't use those in transfer itself

flat root
#

I would just delete them for now but if you prefer to keep them that's also fine 🤷

loud stirrup
#

Alright, I checked the large rework PR from earlier and couldn't find any real usages for getAmount/getCapacity (just one in tests)

#

so I'll yeet those

#

One less thing to test, ig

silent root
#

We should still have the stack factories so that we can use those methods to create an itemstack or fluidstack (or other) directly rather than assume always resource stack

#

Otherwise we allocate unnecessarily in the Item and FluidUtils which need the respective stacks

#

@flat root put the interface name back as what it was named It is supposed to be IResourceHandler. That is really aggravating. -.-

#

We had this discussion before, and you "taking over the PR" isn't meant to be you apply all naming as you see fit. You are supposed to be making it smoother...

wind steppe
#

I thought he’d renamed it to something completely different, but IResourceHandler to ResourceHandler is fine

#

I hate the “I” being everywhere

silent root
#

I am definitely not for changes without discussions, but seeing that now we have to waste effort to remove the I in all locations as well as introduce inconsistencies it is not ideal.. As that was the "main" reason we were requiring it to be sliced which lead us here.

copper fjord
#

what's the problem here now thinkies

mossy chasm
#

The I in interfaces

#

The age old discussion between C# devs and Java devs

copper fjord
#

well, not sure how that's a problem

#

the author of a PR cannot be complelled to add nor remove the prefix

#

the discussion ends there

mossy chasm
#

Yup

copper fjord
#

no buts

mossy chasm
copper fjord
#

yes thank you silk now back to #squirrels-🦊 stabolb

wind steppe
#

Should make a note tho to fix the earlier slices that are using I rn

silent root
#

We shouldn't as those ACTUALLY are using I to reduce user contention

#

as Resource is a MC thing already

mossy chasm
#

I am of the opinion of no I

#

But it does not matter

silent root
#

My point was this was a PR that we had finished and needed more review, we had to go through an absurd amount of hoops and suddenly we are not considered the PR authors now

mossy chasm
#

The stance is that the PR writer can be using or not using I as they see fit

wind steppe
#

To be clear, the only reason I wrote my PR with Is in the first place was because Neo did. Otherwise it would have been free of Is from the beginning

copper fjord
mossy chasm
#

And it's worth noting that the vote was for maintainers to take over the PR

#

Therefore the authors of the PR are now the maintainers

#

As they are the ones authoring the PR, i.e. writing the code

loud stirrup
#

Hmmmm. I noticed we do not actually specify that getAmount should return 0 if the resource at the same index is empty. I think that should be a requirement.

loud stirrup
untold eagle
hollow maple
#

Yeah, I think since this originally was owned by a community member we should use their naming as opposed to suddenly changing it

flat root
flat root
copper fjord
#

and we aren't going to spend the next 10 hours arguing over something that isn't left up to discussion. as I said, the rules are the rules, so please move on

cunning mulch
#

I ran out of patience before I finished reviewing the ResourceHandlerUtil class, though I will try to finish doing so later today or tomorrow. But I went ahead and left my comments on the rest of it and what I have reviewed so far of the util class

flat root
#

Personally not a fan of writing 3+ @throws clauses on every method

#

If we write too much javadoc people will stop reading (we're dangerously close to the max I would personally read - we might even have exceeded it in some places already)

silent root
#

But without them, people will assume it won't crash when we use it in other places.

#

While documentation fatigue is something to be wary of, the main point of all of these is to be perfectly clear what you can and cannot do with it, lest we fall into the same trap of the previous handlers where it was the wild west in some cases.

#

Especially in cases where it involves crashing behaviour and not just quirky results

flat root
#

I don't want the implementations to feel forced to throw, especially given that we explicitly ask dynamically sized handlers to be lenient with their index management

#

implementations should however always throw for empty resources or < 0 amounts

cunning mulch
loud stirrup
#

The implSpec is important, but I am not sure the type matters

#

callers really shouldn't catch those, but OTOH we can still specify those if we want to

#

It might be simpler to specifically say "use TransferPreconditions.blahblah to validate the parameters in your implementation" or some-such

flat root
#

Such verbosity 😄

loud stirrup
#

generally we will have the problem of having to specify both what implementors and consumers should do in one method javadoc.

#

that'll bloat it

flat root
cunning mulch
loud stirrup
#

yeah I am somewhat in the camp of: if you dont adhere to the constraints, it's UB

#

but we still recommend implementors to do X

#

But anyhow. We can specify it, but that would also mean we have to actually specify the type of the exception although it doesnt matter

cunning mulch
#

I also am of the view point that when we have these common cases like resource empty or index negative etc, that the docs for the exceptions thrown is trivial to copy to all relevant javadocs so doesn’t really add more maintenance issues if we decide on the wording before applying to all spots

flat root
#

it's a lot of copy-pasting to a lot of places

loud stirrup
#

I mean that's not that much of a problem, i think

#

I'd not copy it to the utils

#

As a consumer it's pretty much useless info though. You'll never be able to rely on it.

cunning mulch
#

Yea the utils don’t matter quite as much

#

But it is helpful to know potential expected cases you can throw for implementers

loud stirrup
#

yeah I also see it as info for implementors, for the most part

cunning mulch
#

And what types of exceptions to throw for certain things.

loud stirrup
#

we'd want implementors to throw these exceptions (well, we'd want them to use the preconditions :D)

flat root
#

that can go in an implNote

loud stirrup
#

with the implSpec saying which ones to throw? 😛

flat root
#

I'm not a fan of these annotations in general

cunning mulch
#

If you are going to add it why do it as an implNote instead of a proper throws thing…

flat root
#

and I meant the preconditions specifically

#

the reason I don't like these annotations is because they split a potentially readable sequence of 3-4 paragraphs into a mess of remarks

cunning mulch
flat root
#

as a side note the implementation of CombinedResourceStorage is difficult to follow

cunning mulch
#

Yes, I spent a bit working out if there was a bug in it because of the doc for bartended being opposite of what it does

#

I think using local variables would help improve the readability of the logic though for some parts

flat root
#

the original is not super clear either:

public class CombinedInvWrapper implements IItemHandlerModifiable {
    protected final IItemHandlerModifiable[] itemHandler; // the handlers
    protected final int[] baseIndex; // index-offsets of the different handlers
    protected final int slotCount; // number of total slots
silent root
#

Basically, an array of handlers, then which index is the starting point for that handler in this set if we were to flatten them all to a single "array" of resources

cunning mulch
#

It isn’t the starting point

#

As said by my comment

flat root
#

each index is actually the ending point

cunning mulch
#

Exclusive*

flat root
#

(or equivalently the starting point of the following handler)

#

btw I'm gonna make all 3 fields private since the handlers and offsets are accessible from the protected methods already

silent root
#

That is fine, I think they are only protected as that is what we were moving from

loud stirrup
#

is the handler count exposed?

#

i mean the number of wrapped handlers

silent root
#

Nope

loud stirrup
#

well if you need it, i guess you could store it yourself in your subclass ctor

cunning mulch
#

Yeah, I also was going to suggest flipping the index -baseIndex[i] < 0 around (in get handler index?) to have it be subtracting the index until I understood that it was end index not start index for the baseIndex

silent root
cunning mulch
silent root
#

That too, if the need does arise

#

Unless you are talking about for subclassing

cunning mulch
#

I mean for subclassing if you need it in the subclass for some reason so that you can avoid storing it yourself. But I don’t think there is a need

silent root
#

Can't think of one off hand

loud stirrup
#

maybe on-commit logic or whatever

silent root
#

Mmmm subclass in constructor save the count

flat root
#

most pointless bound checks ever? 😄

image.png
silent root
#

then you can iterate with getHandlerFromIndex

flat root
#

(the handlers array already has its own bound checks lol)

silent root
#

Looking at the history, apparently there was a point we did

return index >= 0 && index < handlers.length ? handlers[index] : EmptyResourceHandler.instance();
flat root
#

@loud stirrup I believe I addressed or at least replied to your comments on github; do you agree? (i.e. please check 😛 )

#

at least the earlier batch you sent

loud stirrup
#

yes will check

#

and w.r.t. the combined handler methods

#

I was pondering to suggest very verbose names there since "handler index" and "external index" (or public index, we have no real name there) overlaps in name

#

and that confused the hell out of me since some methods take handler indices, other take the external index

flat root
#

so, on the one hand yes, but on the other hand it doesn't really matter... if you look at any of the other methods you'll immediately understand how this is meant to be used

loud stirrup
#

Yeah maybe

#

Or.... you just name them better 😛

flat root
#

yeah this is a bit long

image.png
#

I'll merge implSpec and implNote, not a fan of the current split display

flat root
#

alright, most comments have been addressed; @hidden geode please recheck yours especially, and I am happy to discuss stuff here

loud stirrup
#

Yeah we can't really specify IndexOutOfBoundsException for index

#

Since some don't throw 😄

serene igloo
#

implementations must properly support transactions
no

#

what do I need to do to keep transactions out of neo, rile up an angry mob?

opaque vector
#

Commoble, I know you hate transactions but I think you’re in the minority

serene igloo
#

I feel like there's three devs with mods that can actually use them and they're making everyone else rewrite their itemhandlers to support them

untold eagle
#

did you look at what you have to do or just kneejerk reactioning to the name?

silent root
#

They have definitely improved some of the logic I have

analog sapphire
#

ok so I will start by saying I have zero experience with implementing item transfer mods

that said, how would I (correctly) do something as simple as have a pipe equally distribute its outputs to targets if I don't have transactions?

mental raven
#

There has been countless disucssion on this topic, months of work, and 2 landed PR's (including the start of Transactions) before now. We are not re-discussing this

opaque vector
#

In fact, in Forgecraft, there was a regular modder who said they hated transaction until they used it on fabric and now are pro transaction

silent root
#

For the most part, the implementation changes you'd need:
when you go to set the value, take a snapshot first

#

On the calling side, open a transaction and pass that through. Commit if you like what you got

serene igloo
#

so now instead of just setting an itemstack in a list, I have to implement "snapshots" and "commits"

silent root
#

you don't really implement a commit you just call commit

#

snapshotting is a journal, and is very simple to setup

cunning mulch
silent root
analog sapphire
#

I assume we provide some default framework for simple storage blocks to use that doesn't require every modder to implement their own transactional handling?

silent root
#

You mean templates? then yes there were some

queen wagon
#

idk about the name templates, it sounds more like copy this, than inherit this

cunning mulch
wind steppe
# serene igloo so now instead of just setting an itemstack in a list, I have to implement "snap...

Your implementation would change to just always assuming you’re not simulating, moving the code where you make a final change to a different method (like, moving blockEntity.setChanged) to a different method, and then the snapshots. Snapshots can be literally anything. If you really wanted to, you could just use a compound tag and write to a new tag to make a snapshot and read from one to revert back. You should already have that code made if you’re using a block entity. On items you can just use the data component as a snapshot and do it that way. It’s extremely simple

#

And more than likely, you can just extend an existing template and just use their snapshots and commits, which makes the changes you’d want to make to it a lot simpler since you don’t have to worry about writing code for simulations of more complicated handlers

serene igloo
wind steppe
#

they are good names

queen wagon
#

for me a template is something you look at or copy to make your own not something you inherit from

flat root
#

Template is not a great name, indeed

#

Journal is fine

loud stirrup
#

journal i think is indeed fine, better than the fabric name

hidden geode
flat root
#

ok so @cunning mulch I went through your comments again, but left a few as non-resolved (and non-addressed) as I am not convinced

#

can you go through them again and tell me if there's any you feel strongly about? I think for most I am happy to implement the change if you have a strong preference

#

same for your comments @loud stirrup --- unless you want me to mark them as resolved given that you approved the PR already

loud stirrup
#

They weren't?

flat root
#

and so on

image.png
loud stirrup
#

Well just resolve the ones you fixed directly anyway

flat root
#

I left the ones where we might want to discuss more open

cunning mulch
flat root
#

sounds good, thanks

#

regarding offering a helper to combine handlers or not, we could potentially have ResourceHandler.of(...) similarly to List.of etc but I am personally not a fan. And putting it in the CombinedRH class seems a bit confusing (should one use the ctor or the helper)

#

it's also a behavior difference for dynamically sized handlers whether you go through the ctor or the helper

loud stirrup
#

And I think it makes it way more pleasant to use if you just have a list of handlers that may be anything from 0..N handlers

flat root
#

we could make the helper take a list and the ctor a vararg kek

cunning mulch
#

would definitely be good to get more input on this one though, as maybe it should? Personally from my experience it is helpful for pipes with in transit items, to have some sorts of checks that they can guarentee are not as performance intensive to get a "good enough" estimate of if it is even worth trying to insert. Like sure it might accept some even if it is at the capacity, but imo it is an edge case that isn't worth the performance hit. Which is why I don't really like the stating it is an "estimate". Like sure I can understand having it so if it returns MAX_LONG then it is an estimate and might still accept past that point, but for lower values, imo if they don't have that somehow calculated... let it be an estimated upper bound

image.png
#

taking a list instead of a vararg is less helpful tbh, when it comes to one vs multiple

flat root
#

IMO getting the capacity is not really cheaper than trying to insert (especially if the handler will reject the insertion)

cunning mulch
#

meh, potentially

#

I am thinking from past experience, with things like storage drawers, etc

flat root
#

and if you really care about performance you should use the indexless insert

loud stirrup
#

They will probably not be able to return the capacity in a bullet-proof way either (at least not quickly)

#

I think approximate is not great wording either, it's not gonna guess at it, I think

#

I'd call it a hint

cunning mulch
#

yes hint is better

#

but still having some way of stating that ideally it doesn't return values lower than actually can be inserted would improve it for me

loud stirrup
#

In case of Long.MAX_VALUE it likely would though

cunning mulch
#

I don't care that much if it is exact, but if it can actually fit 100, and just returns 10... that just feels wrong to me

loud stirrup
#

You could special case it, I suppose

cunning mulch
#

yes

loud stirrup
#

(in the docs)

cunning mulch
#

that's sort of what I am proposing

#

I see the benefit for having it not have to be perfect capacity

loud stirrup
#

In that if the returned value is less than {Long,Integer}.MAX_VALUE

#

It is a hint for the upper bound that can fit in that index

cunning mulch
#

but having it be less than amount (when not at the edge of the boundary) just feels foolish to me

#

exactly

loud stirrup
#

I think the current statement of it being a hint is just from experience that it's sometimes not trivial to know if the capacity is 0 or 1

#

(assuming the slot can accept an item based on some more complex calculation)

#

But in that case, you would indeed return 1

cunning mulch
#

I know, but that is why I mainly just want it to be more clear that it is a hint of the upper bound

loud stirrup
#

Which fits with the upper bound hint

cunning mulch
#

anyway, be back in a bit

flat root
#

ok how about this?

     * This function serves only as a hint on the maximum {@linkplain #getAmountAsLong(int) amount} the resource handler might contain.
     * It might overestimate the capacity if computing the exact capacity is expensive or dependent on other state,
     * and the capacity might be larger than the currently stored amount, for example if the capacity was reduced below a previously stored amount.
     * The only way to know if a handler will accept a resource, is to try to {@link #insert insert} it.
#

@hidden geode I don't understand your point regarding CombinedResourceHandler. The behavior is well specified as it is, even for dynamically sized handlers.

loud stirrup
#

I'd be more concise:

{@return a hint about the maximum {@linkplain #getAmountAsLong(int) amount} the resource handler allows at the index}
<p>The returned capacity may overestimate the actual allowed amount, and it might be smaller than the current amount.
<p>The only way to know if a handler will accept a resource, is to try to {@link #insert insert} it.
flat root
#

I mean that's just saying the capacity is potentially too large or too small without giving reasons why 😄

loud stirrup
#

yes because as a caller, it doesnt matter why 😄

#

just that you cant rely on it

flat root
#

as an implementer it matters why though (or maybe not? since these are just examples)

#

I don't care either way so I am happy to go for yours if pup is ok with it

loud stirrup
#

you should already have a reason to overestimate it though, as an implementor

#

so they wouldnt need us to give them one

hidden geode
# flat root <@304594425286361088> I don't understand your point regarding `CombinedResourceH...

No it is not.

Consider a Combined handler with the following inner handler:

  1. Variable sized handler containing 10 different stacks, and one empty slot for the next stack (so index 0 to 10)
  2. One single slot fixed size inventory with say a pickaxe. (so index 11)

If you now extract fully from index 9 the variable size inventory changes (from size 11 to size 10, indexes 0-9, filled on indexes 0-8, empty on 9), any interaction with index 10 is broken and will cause an exception, most likely index out of range, even though the combined handler still reports that 10 is a valid index to use for interactions with it. Which in summary violates the ResourceHandler contract that states that size() indicates the amount of slots that the handler has and that any interaction withit with an index within that range should not throw.

Or am I missing something here

#

Realistically you don't even need the second inventory in the combined handler, but it is there in this example to illustrate that it stays put in the original index list, and that the offsets array is not updated, and the size of the combined handler stays the same.

flat root
#

we ask dynamically sized handlers to be lenient with their bounds check; a properly implemented dynamic handler will just make slot 10 a no-op even if its currently reported size is <= 10

hidden geode
#

Should it also be lenient for slot index 20?

#

Or 50?

flat root
#

it should be lenient for previously reported sizes as the very least

hidden geode
#

That seems a bit vague and really not a great idea

flat root
#

if it never reported a size > 20 it doesn't need to allow slots >= 20

hidden geode
#

Is a good idea

flat root
#

this is not a new pattern, we always do

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

so the handler cannot just start throwing exceptions for indices smaller than the size

hidden geode
#

Additionally it is already broken if you do an insert, except even more.

flat root
hidden geode
#

because now you can not access the new slots of the combined handler

flat root
#

that's fine, it's the behavior that was specified

#

with a ranged wrapper you also cannot access all slots of a handler

hidden geode
#

That is no where specified at all.....

flat root
#

we specify that the size is constant in the wrapper class

hidden geode
flat root
#

no, but it specifies that it treats all internal handlers with a fixed size

hidden geode
#

And making it behave like one, defeats its entire purpose in my opinion

hidden geode
flat root
#

no, we already decided it was out of scope for this PR

hidden geode
#

Else I don't think that adding this class is not a good idea

loud stirrup
#

what? orion, you're exaggerating wildly

#

this is often used for combining machine inventories or whatever

#

saying that not supporting shifting indices for dynamically sized wrapped handlers "defeats the entire purpose"

#

is kinda missing the purpose 😄

hidden geode
#

And if you, you will get subtle undefined and in my opinion undesired behaviours

loud stirrup
#

the default convenience handlers should handle the 80% case

#

and in this case, it's probably the 95% case

#

(at least)

hidden geode
loud stirrup
#

I'd like a food gun though 😄

#

STRAIGHT IN THE MOUTH

flat root
#

depending on what you do it's perfectly fine to use this class for a dynamically sized handler

hidden geode
#

Especially not a foodgun where it is not obvious that the behaviour all of a sudden changes.

hidden geode
# flat root depending on what you do it's perfectly fine to use this class for a dynamically...

I have come up with more cases in my head where it breaks with a dynamically sized handler, then ones where it does not. Yes there are for sure ones where it works perfectly fine. But there are many where it does not. And that seems to me personally risky. Especially if it can be solved by adding a simple bool to ResourceHandler: #isDynamicallySized And checking for it in the constructor

loud stirrup
#

Orion, combining dynamically sized handlers that can change size at any time is already a footgun.

#

Conceptually!

hidden geode
loud stirrup
#

This actually makes it not throw up in your face

flat root
#

OK, I am tired of discussing this. We are not gonna remove the class, and we are not gonna add characteristics at this stage of the rework. What do you suggest we do?

loud stirrup
#

So I am not sure why you'd favor runtime crashes

hidden geode
#

But there is no way to know, that you are holding a footgun at the time you are given an ResourceHandler

loud stirrup
#

Hence the safe way out

#

Just use the size at time of construction

#

Which will not runtime crash

hidden geode
loud stirrup
#

It's not undefined

#

What are you talking about 🤔

#

It's the opposite. It's well defined 😄

hidden geode
loud stirrup
#

I mean, you can have an opinion on it