#Transfer rework (IItemHandler, IFluidHandler, IEnergyStorage)

1 messages · Page 1 of 1 (latest)

flat root
#

Let's talk about potentially changing the transfer capabilities of NeoForge - IFluidHandler, IItemHandler and IEnergyStorage. Here are a few things we could do. Other ideas are welcome too!

Unify item and fluid transfer abstractions

Perhaps the most obvious thing is to align IItemHandler and IFluidHandler so that modders who learn one immediately know how to use the other. It would also make the life of some mods that have similar handling for fluids and items easier.

Use something else than stacks as the unit of work for the API

One of the problems with the current APIs is that they use ItemStack/FluidStack as the fundamental unit of work (not even consistently... some methods take or return ints instead). Unfortunately, stacks are mutable and thus working with the transfer handlers involves frequent copies of the stacks that are passed to other functions, just in case.

We could for example use an ItemVariant/FluidVariant that is essentially an immutable count-less stack to represent the resource, and a separate integer parameter.

Did someone say transactions?

The current system is based on simulations. "Could you do <single action>? Yes/No". Transactions are a generalization of that, allowing multiple related actions to be "simulated" and "cancelled"/"executed" all at once. The main benefit of transactions it that they make more powerful transfer behavior possible. For example an AE2-like mod could use a transaction to take all the inputs atomically for a crafting task. (Right now this will fail with e.g. compressing storage drawers).

The flip side of transactions it that all implementers of the handlers need to implement support for them, and while we can provide base implementations and documentation, at the end of the day this is more complicated to get right than the simulation boolean. It would also be very hard to port to it for all the existing mods (whereas the previous suggested changes are not as fundamental).

#

(my personal feeling is that transactions are probably not worth the migration cost and complexity)

sinful tiger
#

non slot based API? (conversely, fluid API isn't slot based and probably could stand to gain the option)

cursive geyser
#

I'm still of the opinion that there should be separate item caps for slots and automation

#

but apparently people disagree

sinful tiger
#

it's easy enough [if a bit inefficient] to implement a slotless automation API in terms of a slot-based underlying container, and that's what's going to happen for the vanilla containers anyway.

river pilot
#

what is a slotless item cap, and what does it mean to have separated slots/automation?

serene igloo
#

you can trivially implement slotless machines with the current system, I have several of these

serene igloo
flat root
#

I'd just add some optional overrides to IItemHandler for slotless invs

cursive geyser
# river pilot what is a slotless item cap, and what does it mean to have separated slots/autom...

so my ideal version of item systems would look like this:

  1. a query interface that lets people know the size and layout of an inventory, if it's built on slots, side meanings, and stuff like that
  2. an inventory interface that works based on positinal slots and has interactions of the type: place, pick up, move, split, ...
  3. an automation interface that works based only on inserting/extracting and nothing else

I know that this basically would remove a whole lot of mods features that rely on thingsworking differently, it's just what would make my life easier :P

flat root
#

You can make enderrift more efficient without that

sinful tiger
#

[also, mutable itemstack slots is a bad idea, even something that does have something that maps reasonably well to 'slots' might not store them internally as an itemstack. the difference between 'slots' and 'slotless' is whether something has a finite, well-defined, capacity. Like, most trash cans only have one "slot", which makes some pipe systems think it can only hold one item until it's actually transferred that item and the slot is still empty]

#

though of course, without transactions the trash can problem is difficult to solve regardless, since it can't know if that one simulated transaction will affect the ability to insert the other items

sinful tiger
serene igloo
# sinful tiger how?

as one of several examples, I have an item tube that doesn't really have a concept of slots, you can try to insert items into it and either it accepts them or it doesn't (if it can't route the item to any itemhandler in the tube graph)

the itemhandler for it is basically just a one-slot ItemStackHandler

#

so, anything trying to insert items into it would basically ask it how many slots it has (1), then try to insert items into each slot it has (the one slot)

cursive geyser
serene igloo
#

it's not exactly slotless but it is using the slot-based interface to implement a slotless machine

round pasture
#

I'm all for your original description

#

Ok to compromise on transactions

#

I do hope we don't keep derailing this proposal with other feature requests as otherwise nothing will materialize

#

(Fabric has this already and it works quite well!!!)

serene igloo
#

I'm fine with nothing materializing though

round pasture
#

The API as it stands now has some issues

serene igloo
#

🤷‍♀️ works fine for me

round pasture
#

Alright

serene igloo
#

so I'm biased against change because any potential change would either A) work less well for my mods, or B) work just as well but I'd need to refactor a bunch of itemhandler graphing code I wrote five years ago and don't remember how it works anymore harold

loud stirrup
#

Yeah I just hope we get a system that generalizes over Items + Fluids in such a way that allows mods like mine (and Raouls :P) to not have to writ everything twice to support Fluids+Items

round pasture
#

Yes and also the passing amounts instead of stacks is vastly superior imo

loud stirrup
#

Yes, I always call that separating the "what" from the "how much" 😄

round pasture
loud stirrup
#

Since the what should be immutable if possible

#

and the amount changes constantly while transfering stuff around (where partial transfers are possible)

round pasture
loud stirrup
#

As long as it doesnt include transactions, it'll probably not even be that bad

#

tedious, sure

round pasture
#

Ehh, probably not worth the effort

#

In my situation

cunning mulch
#

personally I feel like unifying is good (especially say the boolean to explicit action type via enum to make it more readable and easier to keep track of).

Something else as stacks... Maybe? Would need to see how that really would work

transactions? I honestly feel like it is slightly early to switch to. Not really sure how the system works and I understand part of this is to get a better sense on how it will work, but keep in mind how many large changes we have been making that people already are having to rework how they do caps. If say they are going from 1.20.1 to 1.21 if they have to rework both how the caps themselves are provided and also then the entire implementation of the builtin caps... that seems like a pretty large jump/hurdle for some people

round pasture
#

Yeah, I agree doing transactions is a bit overkill. It might not even fit neoforge well

#

Something else as stacks you have to try to feel how it works. I was sceptical at first as well

loud stirrup
#

Transactions were by far the most painful part of porting AE2 to Fabrics storage system

#

w.r.t. something else than stacks: I think over time we've had multiple systems in the various itemstorage mods that crystalize into: trying to make an immutable tuple of (Primary Type, Optional NBT-Data) the key for item/fluid operations primarily to allow for improved caching.

#

Where Primary Type is an identity-comparable registry-object such as Item or Fluid.

round pasture
#

Conceptually I think it makes more sense

#

Also copying stacks all over the place just to modify the amount in the end is really inefficient

loud stirrup
#

Essentially you want to arrive at something that is Object.equals comparable iff it would be stackable

#

We can probably bikeshed for a full year before we arrive at a name for that concept 😁 I never quite liked the Variant name that Fabric used for the concept

#

I think a different system by Grondag used the term "Article", while AE2 internally calls that stuff "Keys" (AEItemKey, AEFluidKey, etc.)

vale birch
#

Hmmmmmmmm
I agree with the need to simplify the IFluidHolder stuff.. that shit is more complicated than my porfessors grading system and that has vector multiplication in it

tranquil crane
#

Because it was annoying and pointless to have to iterate over internal tanks

vale birch
#

I would also argue that maybe a one time huge update cycle might be easier than many smaller, where every update the modders need to do something

tranquil crane
#

The original BC API had slot-equivalents for fluids, since Sengir wrote it for Forestry

flat root
round pasture
tranquil crane
flat root
round pasture
#

And yes. Agreed on the helper function that abstracts away slots. Could even move it to the interface that falls back to a slot based impl. But I think that can be added later?

tranquil crane
#

Like it was always just hypotheticals and fluid-handling things at the time didn't have the same complexity. I think it might be worth revisiting

cunning mulch
tranquil crane
#

I'm still generally of the opinion that you should only be able to ask something for interaction

#

and not grab its internal guts and manipulate them

#

I'm also in favor of changing the simulate booleans to enums BUT I would like that to be unified please, I don't want a SEPARATE enum for each cap

#

if it's just enum SIMULATE / EXECUTE that's fine

flat root
loud stirrup
#

I'd hope the final design uses the same definition generalized over fluids+items

hidden geode
flat root
hidden geode
#

Most pipes really don't give a shit about pipes

#

Either shit fits

#

Or it does not

flat root
#

whether you could that as "internal guts" or not is really arbitrary... the point is that the pipe mods need it 😄

#

the pipes that actually move items inside of them while being smart about not over-sending and stuff need that info, Orion

#

e.g. mekanism's or thermal dynamic's

loud stirrup
#

I think he means simulating that slots are already occupied by in-transit objects

onyx basalt
#

Unify item and fluid transfer abstractions
Question, why not include energy abstractions in this as well, while we're at it?

flat root
hidden geode
flat root
#

unifying energy isn't actually helpful, it just makes the API super boilerplate for anything

hidden geode
flat root
hidden geode
#

I have seen several implementations which do not need to know the slots

hidden geode
#

You can simply keep track of what you are sending

#

And periodically check

#

Sure it would be nice to have a slot

#

But in practice

#

An inventory that is just a "bag of contents with a maximum" is sufficient

loud stirrup
#

The simulation API of the destination is stateless. You cannot know if it would accept what you are sending in total if it's not of the same resource type

tranquil crane
#

So the problem with pipes that try to track sent things via slots and such is that you have to make assumptions about the nature of the inventory, even if you can "see" the slots

#

it's something where on the surface seeing the internals helps but ultimately it kind of doesn't

loud stirrup
#

Unless you define the simulation API in a way that is "would you accept List<Tuple<Resource, Long>>"?

hidden geode
tranquil crane
#

the best way to handle it would be a way of copying the actual inventory, and being able to run simulations on it

#

because then you can maintain a seperate "state" for that inventory

hidden geode
tranquil crane
#

to represent things in transit

hidden geode
vale birch
flat root
tranquil crane
#

ultimately there's no good way to do it because any non-instant pipe is met with "waaah it's not as good as EnderIO"

#

#fuckeverything

loud stirrup
#

Copying the inventory state is almost what techs transactions tend to do 😅

#

Nah, guess why people cry and cry about TD item pipes not being ported

flat root
#

transactions are indeed partially copying the inventory as it is modified so that you can undo the modification

loud stirrup
#

It's just pretty seeing the items travel along the pipe 😄

flat root
spice crater
#

Love it

tranquil crane
#

I get overwhelmingly more complaints about the pipes not being instant

#

but fair enough, squeaky wheel and all

hidden geode
loud stirrup
#

Vocal minority I say! The majority is in silent awe looking at their items traveling! 😛

onyx basalt
#

Mek has it and it's one of the thing I love about Mek's pipes the most tbh

#

I haven't played Thermal in some time, but if it also has it, then great

loud stirrup
hidden geode
#

Like even if AE2's travel is instant

sinful tiger
#

my idea for transactions a while back was to keep the modified state in the world and the undo as, like, a stack of runnables.

flat root
hidden geode
#

It still tracks what can fit

#

And what can't

flat root
#

I wrote both AE2 and Modern Dynamics and I can tell you that slots are very much needed for the latter, but not for the former

tranquil crane
#

AE2 is instant to my knowledge, it shouldn't need it

loud stirrup
#

Well "needed". You use strong words there. Needed to not be as shitty with oversending to the most common containers

#

Replying to Tech, I mean

flat root
#

well yes... needed as in I need basic awareness of whether the target machine will even accept my item and how much of it

hidden geode
#

All item transfer mods, regardless of instant or not

#

Need to keep track of what fits and what not

serene igloo
hidden geode
#

There is no world without that

loud stirrup
#

Uh? No, we basically insert immediately so the next simulation uses the updated state

serene igloo
hidden geode
#

Because simulations are only valid in the call they are fucking made

#

Even a single tick later

#

And the simulation you performed is literally invalid

flat root
#

we just ask the inventory every time, there is no caching of the simulation result

hidden geode
#

So yes tech, any mod in existence, that does not want to oversend, needs to keep track of their shit

sinful tiger
#

if you are a non-instant pipe network and have other items in transit you can re-simulate them before simulating the new transaction, if you have some sort of transaction mechanism you can use to 'simulate multiple insertions'

flat root
hidden geode
#

AE2 is instant

#

It checks once

#

But it still checks

#

None the less

flat root
#

that's not what I am talking about at all

loud stirrup
#

Actually.... do we? 😄

flat root
#

AE2 does not need compound simulations

hidden geode
tranquil crane
#

tbh it's so much of an advantage I'm seriously considering doing it for TD and rendering a fake itemstack

loud stirrup
#

There is no concept of "in-transit" in AE2. But as soon as you have that, and you can have multiple resource-types in-transit, you either need a "batch" simulation API or you need to work around the lack of that API by using a heuristic based on slot reservations.

tranquil crane
#

just do transit after the fact

sinful tiger
#

Integrated Dynamics is instant but can still get confused by Storage Drawers and accidentally end up oversending [even to a normal drawer because of that extra insertion slot] - i don't know what they're doing that AE2 doesn't or vice versa

hidden geode
loud stirrup
#

compound simulation is probably the better term, thanks Tech 😅

flat root
#

the point is that you need slots for slot reservations 😄

loud stirrup
#

Well yes, or a compound simulation API 😛

flat root
#

yeah

#

so either slots or transactions... pick your poison 😄

loud stirrup
#

well,

spice crater
#

Translotctions

loud stirrup
#

compound simulations aren't transactions...

flat root
#

(spoiler: we already have slots)

#

they are exactly the same thing 😛

onyx basalt
serene igloo
#

the way my non-instant pipes work is

  • somebody tries to insert an item into a pipe
  • iterate over previously-cached-and-sorted list of itemhandlers
  • simulate each one until I find one that can accept the item (if I can't find any then reject the insert)
  • queue up the route to that itemhandler as a series of moves within my block graph and accept the insert

(x ticks later...)

  • item actually reaches the endpoint itemhandler, try to insert it again
  • if it fails, try to reinsert the item into the last pipe in the route, or just yeet it into the world if it can't
loud stirrup
round pasture
#

I don't think slots are a good abstraction to check for oversending, as you're making assumptions about the behavior of the inventory

tranquil crane
sinful tiger
onyx basalt
loud stirrup
#

A drawer controller can do a compound simulation against it's own state

round pasture
#

But probably out of scope for this initial proposal

flat root
#

compound simulate doesn't actually compound across multiple inventories

spice crater
flat root
#

which means that a wrapper around multiple inventories is unable to handle a compound simulation

round pasture
#

Hrm, true

flat root
#

(i.e. your basic CombinedInvWrapper cannot handle a compound simulation)

round pasture
#

But is this discussion in relation to whether or not to go for transactions?

#

Because I think that's a no go either way due to the size of the change

loud stirrup
#

It's only meant to capture the case of:

  • You ask the inventory "can you store this 1xoak log"? YES!
  • You ask the inventory "can you store this 1xcobblestone"? YES!
    But the inventory only has a single slot....

What I meant by compound simulation is asking it: "can you store [1xoak log, 1xcobblestone]"?

flat root
#

personally I'm 90% sure that we won't get neither transactions nor compound simulations

#

but yes it is the point of this discussion

serene igloo
serene igloo
flat root
serene igloo
#

I don't think I use that harold

loud stirrup
#

Yes but neither will your slot based approximation in that case 😄

flat root
#

indeed

#

my point is that naive compound simulation is worthless

round pasture
#

Either way, I think everything listed in here, even without transactions, is already a huge improvement 😄

#

Transactions can be added later if we really wanted to

flat root
#

oh right I forgot about one thing... the goddamn IFluidHandlerItem

round pasture
#

Adding transactions from the get go will give you a similar 1.8 model system situation 😄

flat root
#

definitely agree - Fabric only has it because it was a hard requirement and I eventually managed to make it work reasonably

void thicket
#

IFluidHandlerItem is spaghetti ye

sinful tiger
#

ok before i forget to mention it now that we have a thread

#

yeet IFluidTank

flat root
#

ah yes that too

hollow maple
#

i may be late, and while i may not be a fan of transactions, i can see them being useful for the "space reservation" use case to prevent over-sending items: you could ask to reserve the space as part of the simulation, and then the target inventory/tank/etc will pretend it has more contents than it actually does until the transaction is submitted (in which case, realCount == displayedCount) or the transaction is rolled back (in which case, realCount == oldCount)

#

personally, i would like to see the core api be built upon these "transactions" and then an additional layer/api surface on top which provides convenience functions, hiding the transaction from the consumer side

#

i'm not sure if there's a way to cleanly split the "transaction" implementation from the implementation of the inventory/tank/etc but it would be nice if that were possible too

hollow maple
# hollow maple i'm not sure if there's a way to cleanly split the "transaction" implementation ...

my gut feeling for this would be to completely separate the concept of storage from the concept of querying:

  • the interfaces modders would implement to provide custom backing storages for their items/blocks/etc would be something like IStorage<Item> or IStorage<Fluid> (illustrative names only, generics probably aren't the best solution here)
  • this interface would probably optionally expose transactions so that modders who wanted to work with them directly could
  • neo would automatically provide a query interface built upon transactions which stores the in-flight transactions, as well as virtual container counts
  • modders could either query using the transactions directly, or skip them using a higher-level "convenience" method/interface
#

(i'm using "interface" here as a general term for an api, not specifically java interfaces)

sinful tiger
#

have all the common cases [sides that access a subset of slots, and/or separate containers per side that are concatenated together for the null side, insert-only/extract-only/readonly slots, etc] implementable without touching the methods that need to deal with transactions.

#

maybe a class with all transaction methods final and only overridable methods something like int getSlots IItemHandler getTargetContainerForSlot(int slot) int getTargetSlotForSlot(int slot) boolean canInsert, canExtract(slot)

flat root
#

Transactions require native support by everyone, you can't just write a transactional wrapper

sinful tiger
#

no my point is

#

ItemStackHandler would support transactions, and the wrapper class would support them, and most modders wouldn't have to implement any custom classes for their inventories

#

[or at least not any that have to mess with the actual guts of the transaction system - these could be changed to be callbacks instead of overrides in theory to get actually no subclasses]

flat root
#

in practice that's not gonna work

#

because insertion into item handlers has side-effects that the transaction system cannot magically take into account

#

(e.g. even a call to setChanged is problematic)

flat root
#

if we are to change IFluidHandlerItem we can probably make it based on SlotAccess

vestal path
#

It would be nice to have IItemHandler#isEmpty() and IItemHandler#isFull() methods

flat root
#

We need more handler helper methods yeah

serene igloo
#

I'm skeptical about that... let's say you want to insert something into an itemhandler

first you ask it if it's full

the item handler implements this by iterating over all of its slots and checking if each one is full, then it finds an open slot so it tells you it's not full

then you know it's not full, so you iterate over all of its slots until you find an open one and insert your item in it

#

you've just doubled the amount of iteration done

swift glen
#

wouldn't having a getAvailableSlot which returns said slot avoid the double iteration ?

#

with either null or an optional return for cases when the IItemHandler is full

trail fable
#

I'd be fine with getAvailableSlot().if present(...) thinkies

vestal path
trail fable
#

You can't really do that all that well

#

Well, unless you use a slot content -> space in slot map

loud stirrup
#

I thought about it. If an IIH implements a "fast" way to check that it is full (i.e. to allow it to be skipped), it could just as well use that itself to fast-fail in the insert method

#

If we wanted to allow introspection for storage (i.e. for reporting mods or AE2 making use of external storage), that is IMHO a separate capability

distant merlin
#

IItemStorage thinkies

loud stirrup
#

IStorageAttributeView?

flat root
#

Introspection as a separate capability is ultimately a pain 😛

#

Better to find a reasonable set of introspection methods and place that as defaulted methods in IIH

loud stirrup
flat root
#

Not really. An item handler's properties are related to the item handler IMO

loud stirrup
#

Item handler is just about transfering things in/out, not necessarily about state of storage

#

I suppose you could say since a view of the storage is already provided via slots, that's the same thing, I could concede that

flat root
#

Well there is also getItem, isItemValid etc

sinful tiger
#

any helper that we add we should put some serious thought into trying to implement efficiently in ItemStackHandler, and maybe also for InvWrapper for vanilla containers

#

not everything's gonna be able to do it [slotless insert/extract is not impossible, but it is hard] but it could be worthwhile in a lot of cases

loud stirrup
#

Default IIH implementations could, but you can modify stacks external IIRC in that, can you not?

trail fable
sinful tiger
#

it wouldn't require iteration to query

loud stirrup
#

The problem is if a mod thinks it's clever and does:
if (!otherModsHandler.isFull()) {
otherModsHandler.insert(...);
}

It'd actually hurt performance

sinful tiger
#

i believe modifying stacks technically goes against the IIH contract

trail fable
sinful tiger
#

but the gui code does it, and the presence of IItemHandlerModifiable can be interpreted as allowing it for a particular container

#

[the gui code requires IItemHandlerModifiable, at least for the client side handler - i can't recall if it requires it for the server side or not]

sinful tiger
#

the real problem is that it makes proxying harder [even if ISH has an efficient implementation, a wrapper around a range of slots or combination of two item handlers cannot rely on it, and the fact that the underlying handler could be accessed another way means it can't track it itself]

raven nymph
flat root
heavy compass
#

I've always been of the opinion that item handler should have a base handler for things that don't care about slots (barrels, or something with only one input/output) and a subclass that's the equivalent of the current one for those that do. Trying to be smart for the modder just ends up with modders starting to dig in each other's guts (i can't remember the specific mods, but there were a couple during 1.7.3 that did that).

flat root
#

IItemHandler didn't exist in 1.7.3 so that would be a bad example...

#

A barrel can be trivially modeled as a single slot inventory

loud stirrup
#

There should just be slot-less insertion and extraction APIs is all

stone dragon
stone dragon
flat root
flat root
loud stirrup
#

If we split the type from the amount, that should follow naturally

sinful tiger
#

i know storage drawers does, at least. it has an additional always-empty slot used for insertion so that mods that don't assume they can insert to a slot that has more than 64 items can still insert

#

separating it from the itemstack api would let us formalize that, yeah, right now there's still weirdness due to the fact that itemstack network serialization uses a byte for size, etc

#

[typically mods with "more than a stack" storage also allow at least N/64 of unstackable items to be stored as well - i don't know if there's a way to differentiate 'stack size of 1' from 'truly unstackable' in any current api]

late ledge
#

At least judging from how ae2 works, nothing would be truly unstackable. Even the most apotheosised god sword would stack with an exact copy of itself, though it’s unlikely that you could get an exact copy.
I might be wrong, am just smol bean mech e major

round pasture
#

Asking for my own planning, but do you have an idea of when this is targetted?

#

and/or for which MC version

flat root
#

no target yet

#

I think we're gonna lean on the conservative side and avoid refactoring too much anyway

#

if that helps with your decision, I am porting MI to neoforge as is

round pasture
#

hrm, i wonder what remains of this proposal if you avoid refactoring too much

flat root
#

no idea

cunning mulch
#

Actions!

flat root
#

we need to keep in mind that IItemHandler is used in many places as a replacement for Container

#

the fact that a simple extractItem(0, 1, false) just gives you an ItemStack you can use is pretty nice

#

even if it sucks from a transfer pov when you start exceeding stack counts and whatnot

cunning mulch
#

I would be open for readdressing that idea of yours that lex vetod about having method overloads for not clamping at stack limit

flat root
#

yeah that's what I was thinking about

#

for passing stacks, I want to reconsider the guarantees about who gets ownership of which stack

cunning mulch
#

Oh god… that is going to be so bug prone

#

When it comes to porting

#

Assuming things that shouldn’t be mutated previously become mutable now

flat root
#

first order of business is making insert behave the same for fluids and items

cunning mulch
#

Items is better than fluids iirc

#

As someone who uses the item behavior for all including my implementation of fluids (that then gets wrapped into the way fluid expects them)

flat root
#

remainder is really annoying

cunning mulch
#

It is quite useful for a lot of cases especially for distributing across slots as then you take the remainder and go onto the next slot

#

Which for items especially is one of the main use cases

flat root
#

the equivalent would be something like activeStack.shrink( insert(activeStack.copy()).getCount() )

#

but yeah IMO neither pattern is great

#

hence the suggestion to separate the type from the amount, so you could do something like
totalInserted += insert(what, amount - totalInserted)

#

(where what is some sort of countless stack)

#

but that is a massive change compared to the current system, and it forces an allocation every time you want to convert from/to item stacks

#

and it doesn't really play nice with inventories backed by an ItemStack

#

what I really want is the following:

  • unified interface for items and fluids
  • unclamped item handler extractions
  • defaulted method to insert/extract over the entire handler
loud stirrup
#

Another wishlist would be slotless extract-by-filter or somesuch... or just a find-method, i don't know 😅
Given that we just today had a report based on the armory cabinets and AE2 having to iterate the entire thing to find anything in it

flat root
#

yeah that is what I mean by a defaulted method to extract over the entire handler

#

and honestly, I can live without the amount and "type" separation as long as I get that wishlist

stone dragon
#

Some degree of amount and type separation is still needed to make handling larger-than-a-stack stuff less janky

#

Or at least some docs on how you should handle that in whatever system is added

loud stirrup
flat root
#

that's what patches are for

loud stirrup
#

but i also agree with what you highlighted w.r.t. IItemHandler being used in place of Container

#

which is a problem for UIs that just mutate stacks

stone dragon
#

So long as it's intuitive how to build larger-than-stack stuff or slotless handlers, I'm happy

flat root
stone dragon
#

Cause those are the biggest pain points for me with the current system. Beyond that I'll adapt to whatever folks decide they want

flat root
#

I don't want any of that bullshit

image.png
#

😅

stone dragon
#

Christ, that's quite something

loud stirrup
#

Well yes, but assuming you then have to store as ItemStack internally, for UIs that's usually not a high-traffic invenetory

#

so conversion at the edge is fine. assuming you then have an implementation class that stores ItemStacks internally, you could just access that impl-class from the mods menu

#

Similar to how we have IItemHandlerModifiable

flat root
#

storing key and amount separately is definitely a mistake if you need simple GUI interactions

#

the problem is that if you store stacks then you are converting keys to/from stacks every time there is an interaction, and that can have quite the traffic

#

which leads me to think that working with stacks directly may not be too bad

loud stirrup
#

Except for the wonky amount=0 -> convert it irreversibly to EMPTY and other problems

flat root
#

can we list out these problems? maybe we can fix them

#

I wonder if there would be a way to temporarily "freeze" a stack to limit uncontrolled mutation

loud stirrup
#

The other question is: can UI mutation be detected and handled safely? Probably not?

flat root
#

UI mutation happens via slots so it's mostly ok

#

ItemStackView 🤔

#

the problem is that the nbt is also a mutable mess 🙃

loud stirrup
#

Well that's what I meant, if it mutates and you don't get a chance to sync it back to the inv, we're a bit out of luck. In those cases, you can only really store ItemStacks internally

flat root
#

yeah

loud stirrup
#

But those are not "large" inventories and converting to a "type" seems unproblematic?

#

I mean for each request. For the NBT-Less items we'll have it on the Item instance anyway

flat root
#

a naive getResource(int slot) call will fuck you over regardless of the inventory

#

people might have a lot of double chests

#

I mean that if you iterate many double chests and check the "type" in each slot it might be quite slow

#

what the fabric transfer API does is that insertion doesn't actually query getResource - it uses the method to compare an ItemVariant against an ItemStack directly

flat root
#

ItemVariant.of(damageableItem) will not actually match new ItemStack(damageableItem) I think

hollow maple
#

Isn't ItemVariant supposed to be an ItemStack minus the stack size?

#

So like, it stores NBT data and stuff

flat root
#

indeed

image.png
#

(needs attachment support as well)

sinful tiger
#

IItemStack [to be implemented by ItemStack and by a new better type] [half joking]

serene igloo
#

can we keep the insert/extract methods that just return an itemstack

flat root
#

maybe

serene igloo
#

that's not a maybe, don't get rid of them

#

if you have to get rid of them to rework the itemhandler api then don't rework the itemhandler api

flat root
#

maybe, maybe not

#

your comment doesn't help with design considerations at all

#

some easy to use method to easily insert/extract stacks is definitely needed, whether it will be the primary API that must be implemented or just a helper function is yet TBD

serene igloo
#

the current api is fine, we don't need to redesign it

flat root
#

you said the exact same things about caps and you were wrong

serene igloo
#

no I wasn't

flat root
#

feel free to ignore problems that others have with the APIs I guess

#

for a quick tldr:
what I really want is the following:

  • unified interface for items and fluids
  • unclamped item handler extractions
  • defaulted method to insert/extract over the entire handler
serene igloo
#

unified interface for items and fluids
please don't, items and fluids aren't the same thing (having more similar APIs for itemhandlers and fluid handlers is fair)

unclamped item handler extractions
add an extractMany method that returns an ItemGlob or whatever

defaulted method to insert/extract over the entire handler
ItemHandlerHelper#insertItem exists, though I wouldn't be opposed to the defaulted method

flat root
#

being able to override the defaulted method is essential for performance

#

please don't, items and fluids aren't the same thing (having more similar APIs for itemhandlers and fluid handlers is fair)
why not make them have the same core API?

#

we can add a few item-specific defaulted methods on top of a common core

serene igloo
#

like, an interface that has methods for both item handling and fluid handling?

loud stirrup
#

no, you have a common base interface

flat root
#

no no... more like IItemHandler extends IHandler<ItemStack>

serene igloo
#

ahh

loud stirrup
#

The concept of transfering countable resources between game objects can be generalized to some degree, with resource-specific helpers built on top of that concept

#

such as the methods that return ItemStacks directly on extraction

serene igloo
#

why do we need a common superinterface though

loud stirrup
#

I just explained why 😅 It is literally impossible for mods to handle resource transfer using common methods at the moment due to the lack of a common root

serene igloo
#

you'd still need to define and expose a different capability for each type of resource you want to handle

loud stirrup
#

Yes but a capability is an object

#

Think of Nano's tunnels for example

#

for simple resource transfer caps, he could conceivable proxy it generically

#

AE2 can transfer any type of resource i.e. via P2P tunnel without having to implement functionally the same code over and over

serene igloo
#

you can already do that though

#

I do it in hyperboxes

loud stirrup
#

Proxying all caps is dangerous as Nano found out 😛

#

But in any case, AE2 on Fabric uses the same code for Fluid and Item handling, while it requires split implementations on Forge

serene igloo
#

even if you have the superinterface, you'd still need to split the implementations?

#

because you have one capability for item handling and one for fluid handling

flat root
#

it'd be trivial subclasses yes

#

like ItemHandlerProxy extends HandlerProxy<ItemStack> or something like that

#

@loud stirrup do you think there's a chance we could use the exact same interface? thinkies

#

as in, IHandler<ItemStack> without needing an IItemHandler subclass

loud stirrup
#

Well as we were saying, the IIHModifiable exists for GUI reasons

#

which only applies to items

#

I don't see a major issue in having the Cap be for the sub-interface. What matters is the implementation code can rely on the superinterface

flat root
#

it means that we'd still need to duplicate implementations so they can have the proper type, right?

loud stirrup
#

Unless I am missing a major hurdle here, I don't see a problem?

#

I am talking about the consumer side btw

flat root
#

as in

public class RangeWrapper<S> implements IHandler<S> {
   ...

   public class Item extends RangeWrapper<ItemStack> implements IItemHandler {}
}
loud stirrup
#

RangeWrapper is such a bad name 😄 😄 😄

#

Oh you mean you could just keep the old interface around and implement it on top of the new one or what?

flat root
#

I was thinking of a subinterface with more helper methods

loud stirrup
#

What is RangeWrapper here? an implementation class on the provider-end?

#

I am confused

flat root
#

yes, it's on the provider end

flat root
#

in principle the various combiners like "slot range", "aggregate", etc... would have essentially the same code for item and fluid handlers

#

would be a bit weird to require an explicit subclass just to add the "marker" interface

loud stirrup
#

Well if we can provide the convenience without the subinterface -> fine. But that means splitting item-type from amount is impossible

#

If you had a subinterface you could conceivably do both

flat root
#

the more I think of it, the less I am convinced by splitting item-type from amount

loud stirrup
#

What really remains is that ItemStack cannot have a long amount, and the internal handling of maxStackSize can be really problematic

#
  • the type-change when amount is reduced to 0
#

lots of footguns with that one

flat root
#

for max stack size, I want it moved out of the "core" API and into helper functions

#

is the long amount something that we want? given that IEnergyStorage has no chance of using longs anyway

#

on Fabric it was needed due to INT_MAX / 81000 being too small

copper fjord
flat root
#

so I'd argue that paying attention to proper handling of empty stacks is needed regardless of what you do

loud stirrup
sinful tiger
# loud stirrup I just explained why 😅 It is literally impossible for mods to handle resource t...

why do they need to? it seems like this is designed to support mods like AE2 at the expense of other mods, except it's not like this gets you out of the need to write an AE2 addon to support a new resource type since you still have to support stuff like rendering the resource in a gui, dropping the resource into the world, design considerations like how many types per cell and how many units per byte, etc

loud stirrup
#

But, you misunderstand. It's not supposed to be at the expense of anyone

#

But ItemStack just has problems™️

sinful tiger
#

if we're doing long amounts should we revisit fluid stack amounts? iirc that was rejected mostly because the amounts that would make everyone happy would require long amounts to support reasonable quantities, and Lex said no to long amounts

#

[i think the ultimate compromise people ended up with before that was 648,000 - 81,000 isn't divisible by enough powers of 2]

loud stirrup
#

If we use a unified resource and dont split type+amount, then it's int amounts anyway due to ItemStack

#

I mean ultimately you'd introduce some base-interface for the "Stack" and patch implement it in ItemStack
It would have some methods for: key [Item/Fluid], nbt, amount, data attachments
and you could generalize over ItemStack/FluidStack then

serene igloo
loud stirrup
#

So. If you go and fix those, might as well fix the rest of the issues 😛

sinful tiger
#

In principle, we could have two APIs [three if you count Container], and have wrappers that allows implementing either in terms of the others.

flat root
#

the more I think about this, the more I am leaning towards just adding the few extra methods that we need to largely improve AE2 performance performance of every large storage mod

loud stirrup
#

ahem, every large storage mods, please 😛

flat root
#

😛

flat root
#

my experience porting a big mod to neoforge is that the existing fluid helpers are nice to work with and cover most use cases if you design your code around them

meager nexus
#

can I see your porting work ?

flat root
#

most of it is not transfer though

silent root
# 
public interface ITransaction<T, R>{
    List<R> extract(T object, int amount, boolean simulate);

    List<R> insert(T object, int amount, boolean simulate);
}

public interface IItemHandler extends ITransaction<Item, ItemStack> {}

Something I thought about

loud stirrup
#

no transactions

#

oh wait you are not actually suggesting that 😄

silent root
#

Hm so no returning a List?

loud stirrup
#

don't call it that then!

silent root
#

I used Transaction as a placeholder

#

can be named anything

serene igloo
#

IItemsHandler

silent root
#

hm

distant merlin
loud stirrup
#

I mean they called it transaction 😄

#

then that's where my mind goes hehe

serene igloo
#

tbf I call my clientstuffdoer ClientProxy even though it's just a place where I keep a bunch of static methods

flat root
sinful tiger
#

also where's nbt if T is Item

#

like that's not nearly the largest issue here but it is an issue

silent root
#

Yeah

flat root
#

I keep running into issues because you cannot compose item handlers without transactions 😦

flat root
#

notice the lovely

                    if (!leftover.isEmpty()) {
                        LOGGER.warn(
                                "Trying to move up to {} items from {} to {}, but destination rejected {} that could not be inserted back in the source.",
                                maxAmount, from, to, leftover);
                    }
stone dragon
#

Honestly I wouldn't mind a neo transaction API... as long as there was an abstraction of some sort over it to not make you have to deal with all the complicatedness if you don't need it

#

Not sure how feasible that is

loud stirrup
#

It's not feasible IMHO

#

One could provide a per-inventory batch simulation API that at least would help me a tiny bit 😛

flat root
#

Because it doesn't compose across inventories

flat root
stone dragon
sinful tiger
#

i still think my 'undo stack' idea would have worked

stone dragon
sinful tiger
#

basically implement 'transactions' by having the 'uncommitted' operations affect the actual world [rather than needing a special data structure to store the intermediate state], and maintain an append-only list of lambdas that are executed if you want to roll back the transaction

stone dragon
# sinful tiger basically implement 'transactions' by having the 'uncommitted' operations affect...

That's an idea that I don't think will hold up in practice. Mods then have to be able to undo the full effects of any insertion operation. That can be... difficult, at times. Could have all sorts of side effects related to that insertion, and you now need to write those both forwards and backwards. If you have any appreciable logic to do on insertion, you've just doubled the amount of code there. It's also super easy to mess stuff up with this.

For an example of where this would be a terrible idea: imagine a block that, when an item is inserted into it, immediately drops it as an entity. Now the block has to keep track of the created entity, be ready to undo creating it, and somehow make sure that it doesn't get synced to clients before it's possibly deleted. Same with, say, a block that, when an item is inserted and it's a block, places it...

#

The issue with undoing that sort of mutable state is that, well, mutating stuff out in the wild world can have side effects that propogate quite a ways

#

And if those side effects go on to interact with other people's stuff (not unlikely!), you may not be able to sensibly undo stuff

#

It's just infeasible for anything besides a dead-simple vanilla-like inventory

#

I can think of a ton of situations off the top of my head that would either be completely infeasible with that sort of system, or would involve performance degradations to make them work with it, even ignoring the fact that it's so fragile

#

The only way to get it to work, in practice, in a way that isn't exceptionally fragile and prone to being implemented wrong would be by adding some sort of "finalize" method to let the handler know it can go execute side effects now, and then having it store its changes in some internal state instead till then and- Oh wait, I reinvented transactions, or something really dang close

silent root
#

IItemHandler Idea: (for Slots) Like Chests?

  • SlotContainer
  • Controls what Goes into it, and how much can be added/removed at a time
flat root
#

see the long discussion in #1184607569629675530 as well

loud stirrup
#

This is not really just transfer rework. It also serves recipe viewers that have to handle fluid & item & other resource ingredients in a unified model

flat root
#

that is true

#

I'll open another thread then

#

#1218234714431688714

silent root
hidden geode
#

One thing I would like to see is "Search"

#

Aka being able to request different types of resources

#

Aka: Give me a "Tool" of Type X

#

Or Give me 64 of Tag Y

#

Etc

silent root
#

Yea

hidden geode
#

I had a redesign that has this

#

Where it has 2 options:

#

Fully known: Things like tags, or if a specific item is need

#

And one where it is not fully known

loud stirrup
queen wagon
silent root
#

Well I havent updated License

#

xD

loud stirrup
#

Introspectable filters are a can of worms

silent root
#

I will make it MIT

queen wagon
silent root
#

You can do whatever the heck ya want with it idc

loud stirrup
#

In AE, we ended up going essentially with Predicate<T>

hidden geode
loud stirrup
hidden geode
#

Then that would speed up the lookup in forexample AE by a million tons

silent root
loud stirrup
#

That's what I mean by introspectable. In some cases the filters are so generic you can't really do any speedup

copper fjord
loud stirrup
#

In other cases, yeah, using an index or similar would be possible

hidden geode
#

Which is really terrible

#

Or needs specific interaction logic for it

loud stirrup
#

That's the primary reason we do not offer a slot-based interface to access the AE content from outside of AE

hidden geode
#

Like I am here specifically talking about Minecolonies here

#

in 99% of all cases we always know exactly what we can accept

#

And what not

loud stirrup
#

Usually interfaces are not really indexed on tags either, tbh

#

AE has an even more annoying problem of wanting to extract items only if it can re-insert them somewhere else. Hence the Predicate 😄

hidden geode
#

What Imean with tag is that the predicate is based on the tag values

silent root
#

What License do you pefer for stuff like API

serene igloo
silent root
#

Well yeah

#

But for Prototype ideas xD

flat root
#

doing away with the concept of a slot entirely, and instead having something like a metadata capability that returns information about slots. It could then have a method which returns a handler for a specific slot, or range of slots
@hollow maple
I think this tends to be very complicated for little benefit

#

multiple capabilities in particular are very annoying for proxies etc to deal with (there's more proxies than you think! many mods have their own internal "proxy" system)

hollow maple
#

That's reasonable; I was just thinking that a lot of things like tanks don't really support multiple "slots" so it might be possible to think of slots as a layer on top of a slot-agnostic system

flat root
#

if a handler has a single slot then it is a trivial case of a slotted handler (with a single slot, 0)

#

in general, the problem is that only increasingly niche mods benefit from more advanced item handler features

#

so any feature that "seems useful" must be balanced against the need of so many mods to implement it

hollow maple
flat root
#

(modern dynamics would benefit from a way to mark multiple slots as "logically equivalent" but most mods wouldn't have a use for such a feature)

stone dragon
#

As someone with inventories that don't really have normal "slots" at all - or rather, the ones it has aren't well behaved - I'd love the system Monica described actually. Currently I keep running into issues where the "slots" I expose... can sometimes break expectations

#

It's one of the reasons I've avoided updating that mod

flat root
#

can you describe what your inventory does?

stone dragon
#

From the perspective of things interacting with it: you can attempt to insert stuff, which it may or may not consume. You can attempt to extract stuff; it can expose any number of different things for extraction. The extraction and insertion ends of it are unrelated.

#

In terms of what it is - highly configurable "buffer" block

#

Basically, stuff is inserted but not into any particular slot - and the number of slots exposed for extraction is not fixed and may change after any operation is done on it

#

(Which is bad because some BEs that do multiple extractions in one tick assume that the inventory size they saw the first time around will still be the case the second time around)

#

Additionally, I want to be able to expose items as "present" in the inventory without them being marked as "extractable"

#

(For the use of mods like WAILA or whatever the new equivalent is)

flat root
#

it is a bit rough but it's doable with the standard IItemHandler interface

flat root
flat root
stone dragon
stone dragon
# flat root that is supported by simply not letting `extractItem` extract them

Well, there's the fact that there's a lot of slots that might be exposed to be viewable but shouldn't be extractable - and that's a performance impact, because it means anything extracting stuff might need to iterate a bunch of non-extractable stuff to extract something. I can try to make that a bit nicer by ordering the "slots" as (input slot) (extractable slots...) (view slots) and hoping that people iterate in that direction, but it's still not optimal

flat root
#

well you don't really know what people are going to do with the content in any case

#

there could be some "inventory watcher" that wants to monitor the non-extractable contents of the inventory too

#

on the other hand, inventories with a lot of slots are generally going to be slow anyway - there isn't much to be done about that for extraction since we need to know what to extract

stone dragon
flat root
#

insertion is a different story, and could use a slot-independent "bulk insert" function

stone dragon
#

The issue is, basically, that slots are matrix-like, and this setup isn't

flat root
#

sure - but either you make some compromises, or all mods that deal with items need to handle various possible "shapes"

stone dragon
#

Then, "slot" stuff is just an implementation of that - or maybe a further subinterface with implementations of those methods in terms of slot stuff

flat root
#

that is mostly the fabric system - the problem is that without an explicit notion of where you are in the inventory you cannot do more advanced tracking (like Modern Dynamics is doing to keep track of slots that it thinks will be occupied)

stone dragon
flat root
#

I really don't want a system where some blocks work and some blocks don't, that is quite bad from a user perspective

#

of course here the item transfer will be a bit random but at least some prediction can be made thinkies

stone dragon
#

So you obviously can't depend on it anyways

flat root
#

well the items do get sent and they will just come back to the source if they can't fit

#

the only other solution I know of is transactions (which do solve the issue because they just feed all the pending stacks to the inventory and see what it says), but I am pretty sure that introducing them in neoforge would be a bad move

stone dragon
flat root
#

well your system doesn't really solve an issue I think

#

it does complicate the API a bit because it requires the introduction of StorageView or a similar abstraction

#

for the variable slot count I am not sure of the best solution

#

it should be acceptable to cache the slot count for one tick, I would say?

stone dragon
stone dragon
flat root
stone dragon
#

At present my solution is "fuck it, I hope nobody relies on the slot count being somewhat constant" and just letting it change around all the time, but that's... not great

#

Cause mods do shit like ```java
for (int i = 0; i < handler.getSlots(); i++) {
maybeExtractOrInsertStuff(handler, i);
}

Or the like
#

My inventory still has ordering, so I want to exose slots in order so that WAILA gets sensible stuff

#

But that means putting extractable slots at the end

#

Which isn't optimal if the size is shifting around and those need to all be exposed

flat root
#

there's a reason why most helpers only query the size once 😛

image.png
#

technically it should be fine for the size to change at any time provided that you are lenient with invalid indices

stone dragon
stone dragon
#

I can make stuff that works well enough, sure - but it all points to an underlying flaw with the API

#

And that's that you're assuming everything is a square when some of it is circles. I'm not saying you need to support every different shape out there - you just need to make a generic interface that any of them can conform to, and leave the more specific stuff to subinterfaces

flat root
#

but I don't really see a problem there?

#

I could also move the check to the loop, I don't mind

stone dragon
#

The problem is that the block in question is meant to be a buffer that allows fine control over the flow of items through it. If it's moving stuff in two ticks that it's meant to move in 1, that can lead to some otherwise unexpected behavior.

#

You're placing all the burden of implementaiton onto people with non-conventional inventories, forcing us to implement interfaces that don't make any sense with the inventories. Additionally, the IItemhandler inventory implies things about the underlying inventory that may not hold true, so the inventory might do all sorts of caching that don't work in practice - caching that it thinks will work because of the interface structure. Splitting the interface up in the way I propose is more informative for consumers of the inventory too - it lets them get some additional information about what they're interacting with - namely, "this is something I can treat as vaguely array-like" or not - and then make decisions about what sorts of caching or optomizations to do based on that

#

If an inventory has no concept of slots, iterating slots to find an empty one and then doing a slotted insertion is inefficient. Caching that information on empty slots is really inefficient

flat root
#

slotless insertion is easily added to IItemHandler

#

and in fact would be a good feature for performance in general

#

for extraction you generally can't get around iterating slots/views

#

since you typically don't know what you are going to extract

flat root
#

other mods may choose to stop after 1 successful extraction for whatever reason

hollow maple
#

maybe we should have an extract overload which extracts a specific item/fluid from any slot?

flat root
#

that is really awful

#

I personally hate that pattern with a passion 😛

hollow maple
#

fair, but it's mainly aimed at the "I only want coal" kind of filtered extraction; and it'd be down to the implementor where it actually extracts from

flat root
#

"I only want coal" is not really what you want to do in many cases

hollow maple
#

and if you didn't actually care, you could presumably just pass "null" to that parameter

flat root
#

it's more like "I only want an item that matches these 30 Ingredients and can fit into the machine"

hollow maple
#

yeah, I'm just thinking of items like the modern dynamics extractor set in whitelist mode

flat root
#

with or without NBT? 😛

flat root
hollow maple
#

But yeah, when extracting you don't really care about the slot you're extracting from, by default; just that you get an item that matches some precondition (that condition can just allow any item)

flat root
#

sure, but if you want to extract multiple items it's important for performance to have an iterator-type pattern

#

otherwise you have to re-search the entire inventory with a new (or the same!) predicate

#

this is why Storage<T> has an Iterator<StorageView<T>> iterator() method

#

I'd say that in general the caller is more suited to do the iteration than the inventory itself

hollow maple
#

Here's a potentially silly idea:

default ItemStack insert(ItemStack stack) { /* for slot in slots { insert(slot, stack) } */ }
ItemStack insert(int slot, ItemStack stack);
// and likewise for extract

that way, if the provider of that API has a more optimal API for slot-oblivious insert/extract they can implement that, and then by default it just does an iteration over all the slots

flat root
#

that should exist for insert yeah

#

I think that there is some merit for an unslotted extract that accepts an exact stack to match against

hollow maple
#

and this way, we can also add a note in the docs that users should probably prefer the slot-oblivious methods unless they have a reason to target specific slots

flat root
#

the only problem I have with this approach is that using these bulk-insert methods is more prone to item voiding

hollow maple
#

@stone dragon would this work for you?

hollow maple
flat root
#

I am concerned about aggregate item handlers that delegate to the same underlying handler multiple times

#

a simple example is a chest connected to pipes on multiple sides

#

well I don't think there is much to be done about it - but I think that MI pipes can void items on neoforge where they couldn't on fabric

#

I should try it

hollow maple
#

My understanding is that the tick is done on one thread serially for each ticking block entity, so as long as the operation for one pipe leaves the inventory in a consistent state, all three should behave too

flat root
#

the way it works is that the pipes build an "aggregated item handler" for the entire network

#

then they just do moveAll(source, aggregate)

#

(a bit simplified but this is the general idea)

#

look at that logger call 😛

image.png
hollow maple
#

Yeah, as long as your state tracking in that aggregate is correct, it SHOULD be fine; we should probably have a parameter for whether it's valid to split a stack though (e.g. when inserting 2 dirt into a slot that only accepts 1 dirt)

flat root
#

I just managed to trigger the logging statement

#

[22:38:03] [Server thread/ERROR] [Modern Industrialization/]: Discarding overflowing item {}, extracted from block at position {} in {}, accessed from {} side

#

at least I realized I have to fix it lol

flat root
hollow maple
#

ah

flat root
#

that's the setup 😛

image.png
#

there we go:

[22:40:34] [Server thread/ERROR] [Modern Industrialization/]: Discarding overflowing item 6 modern_industrialization:iron_plate, extracted from block at position BlockPos{x=87, y=74, z=-81} in ResourceKey[minecraft:dimension / minecraft:overworld], accessed from south side
#

the "only" way to fix the issue is to not do a bulk insert, but instead iterate over each slot and try the transfer that way

#

(which sucks because it's so slow)

#

the alternative is that aggregates don't override the bulk-insert function

#

but sometimes you don't have the choice, and you have to aggregate slots from unknown inventories (e.g. ae2 p2p tunnel)

#

anyway

#

you can't solve these issues without transactions, however transactions are also very complex 😩

hollow maple
#

So, I'm guessing the issue is that sink.insertItem() in this case multiplies the amount that could be inserted, because it thinks those chest sides are different inventories entirely?

flat root
#

I setup the chest very carefully so that it would only accept 1 item

#

but if you simulate it will accept 1 item 4 times

hollow maple
#

Yeah, that makes sense

flat root
#

the way transactions work is that the mutation is always carried out, and can either be reverted or committed (i.e. acknowledged/validated) later

hollow maple
#

The solution would be to do { simulate(); insert(); } for every slot, rather than { simulate(); } { insert(); }

flat root
hollow maple
#

which is fortunate, because the code I was going to suggest was something like for (int slot = 0; !stack.empty() && slot < size(); slot++) { if (simulateInsert(slot, stack)) { stack = insert(slot, stack); } }

#

I don't think it'd be possible to simulate a bulk insert, at least in aggregate like your approach

#

but I could be wrong

flat root
#

it's not possible

#

there's two layers of aggregations (which depend on the itemstack being moved) on top of the item handlers 😄

#

it's just unfortunate that this code will never be as clean on neoforge as it was on fabric 😭

#

anyway I'll stop my rambling

loud stirrup
#

For the source inventory that is 😛

#

OTOH doesn't help I suppose since you cannot partially rollback

#

we probably do not need to rehash the months and months of discussion on this from the fabric disocrd

flat root
#

I think you meant "years" 😄

#

there is no solution other than transactions, I am pretty sure of that

#

the only option is either transactions, or finding "good enough" workarounds such that the problem doesn't happen too much in practice

#

unfortunately, "bulk insert" methods will make these issues more likely to happen for aggregate inventories

stone dragon
# flat root other mods may choose to stop after 1 successful extraction for whatever reason

Yes but that's fine - they should choose to stop because of their normal logic, not because of a limitation in the IItemHandler API.

The issue at hand is basically this: the majority of item insertion operations looks like: "Look for the first empty slot, insert into it". The majority of item extraction operations look like: "Look for the first slot with items matching this condition. Extract from it".

Currently we define those operations in terms of the slot operations. I am simply claiming that this is backwards; we should define the slot operations as a special case of something with those two fundamental operations, of "insertion" and "conditional extraction"

stone dragon
# hollow maple <@824138021854117939> would this work for you?

It's not just insert, it's extract - and, more than that, the necessity of defining stuff with slots at present. The "fundamental" ones should be the slotless ones, with the slotted ones being a special case of that, not the other way around - that's just backwards

stone dragon
waxen dawn
#

@flat root are we proceeding with the IResourceStack abstraction?

flat root
#

Not on the snapshot branch anyway

swift glen
#

then you'd use it this way

Transaction myTransaction = new Transaction()
    .insert(myItemHandler, mySlot, myItemStack)
    .extract(myItemHandler, myOtherSlot, 12);
    
if (transaction.commit()) {
    System.out.println("success");
} else {
    System.out.println("transaction failed");
}

(My java is probably rusty and I never done moded minecraft please excuse potential code error, what's important is the logic)

queen wagon
#

It does not since the changes made in one step do not affect the next step if simulated so the handling of that temporary state needs to happen on the handler

sinful tiger
#

the problem is each inventory still needs to know if the state it's in at the point of the operation would allow the operation to succeed

#

like, you can't take 64 cobblestone out of a chest because there's no cobblestone in the chest

#

but if an earlier step of the transaction put 64 cobblestone in then it would work

#

or, there's plenty of room for items in the chest so inserting would succeed, but what if earlier steps in the transaction filled up all the slots

swift glen
#

I see, it's effectively harder than what I had in mind

sinful tiger
#

handling transactions by saving the entire world to disk, disabling all other saving, running the operations, and reloading the world if one of them fails

waxen dawn
#

I need your thoughts on this

flat root
#

I would focus on the immutable "resources" as a core concept

waxen dawn
#

I would request a consensus from the maintainera

#

cause Im here for abstracting stacks

#

poll?

#

in #maintenance-talk ?

flat root
#

I think it's still too early

#

Abstracting stacks resolves the code duplication but not the other problems of IItemHandler

loud stirrup
#

Unified immutable resource representation is kinda the prerequisite for any kind of stack that builds on that (my $.02)

flat root
#

Mutable stacks do not nicely build on top of immutable resources

#

(problem 1)

#

And generic immutable stacks like I did in my PR are clunky because they are not specialized to the resource itself

#

(problem 2)

void thicket
#

an immutable superclass to itemstack should fit in fine imo

flat root
#

It doesn't solve the problems with IItemHandler unfortunately

loud stirrup
#

you mean where it has to return a tuple of (resource, amount)?

flat root
#

It doesn't solve the stack lifetime problems

void thicket
#

the what thonk

flat root
#

As long as there is mutability there is the problem of "when is the stack going to change"

paper merlin
wind steppe
#

Hello o/. I've been developing stuff with both loaders transfer APIs for a while now and i'd like to give my 2 cents on this.

TransferVariants are very useful, and great for inserting into containers. But they arent the best for extractions. Instead, I think a solution using a predicate of some kind would be better here.

Extracting any fluid from a container, most of the time, requires a query to that container. This is best for mod compatibility imo. For example, if Ad Astra wants to drain 500 mb of Oxygen from tank, it'd need to know what oxygen is inside the tank, because it allows any fluid of the tag c:oxygen to be input into the tank. It wouldn't know what fluid might be inside the tank, so AA needs to query it, get the fluid in the tank, and then do extract(oxygen-variant, 500mb, no-simulate). This is alot of steps to do something that should be quite simple. A query to know whats in the container is almost always needed to extract from the container for these reasons.

The simple task of draining a non descript liquid from a tank requires a query as well, because we need to know what to extract. To just extract the first non empty fluid in a container, id need to loop through all the fluids in the slots, get the first non empty one, and then extract that from a container. Forge solves this by requiring devs implement their own nondescript method for extracting, and just not having a unit needed for items, instead opting for a slot based system where you have to match whats in the slot itself. These are not elegant solutions, and both ultimately are work arounds for ItemStacks/FluidStacks being terrible units for matching for extracting, and TransferVariants dont really solve this problem either.

This change would require some kind of Variant + Components + Amount object being the return type of extract. The transfer system on fabric returns the amount extracted because it requires an exact unit to be passed into extract, so you already have enough information there to form the stack or do what you need to do. Not really sure what would be the best thing here. It could be a VariantHolder interface thats implemented onto FluidStacks and ItemStacks, that allows you to grab the variant, the components, the amount, and, for the purposes of transfer, allow you to make a TransferVariant of that type from it.

TL;DR TransferVariants are good for inserting, but bad for extracting, and a predicate should be used instead.

#

Also not sure if i should have stated it, but i am all for TransferVariants as a whole. Not sure if it was discussed much here, but bringing in Fabric's ContainerItemContext would also be something id like, as its extremely helpful with doing certain things

#

this might look something like this:

public interface BasicContainer<T extends TransferUnit<?>, U extends UnitHolder<T>> {
    long insert(T unit, long amount, boolean simulate);
    U extract(Predicate<T> predicate, long amount, boolean simulate);
}

public interface UnitHolder<T extends TransferUnit<?>> {
    T getUnit();
    long getHeldAmount();
}

and an impl would look like this: (In this example, FluidHolder implements UnitHolder, returning a FluidUnit/FluidVariant. Its my equivalent of a FluidStack)

public class FluidContainer implements BasicContainer<FluidUnit, FluidHolder> {
    @Override
    public long insert(FluidUnit unit, long amount, boolean simulate) {
        return 0;
    }

    @Override
    public FluidHolder extract(Predicate<FluidUnit> predicate, long amount, boolean simulate) {
        return null;
    }
}

This would still allow you to make completely generic transfer methods, while making extract more flexible

#

tho i guess you could also return U in insert if you wanted to as well, tho im not sure thats entirely necessary or even useful

flat root
#

@wind steppe fabric has some helpers for this in StorageUtil

#

And no, Predicate has no business being in the core extract method

stone dragon
#

I dunno, whether or not that design is the right way to do it, telling something what you want to extract instead of where you want to extract from -- at least in the base interface -- seems pretty sensible. I've maintained, and will continue to maintain, that slots should be an optional layer on top of a slotless setup

wind steppe
flat root
#

Because of performance reasons basically

stone dragon
#

Predicate means you have to check every thing in it. Variant can have more efficient implementations

#

If you've got a large storage of some sort, you don't want to linearly search till a predicate matches, you want something you can look up in a map or somesuch

wind steppe
stone dragon
#

Like, say a container stores info internally as an Object2IntMap<ItemVariant>

#

(Which isn't an unreasonable case)

#

With a predicate, a lookup is linear time; with a variant, it's constant

flat root
#

Adrian there's rarely a good reason to use extractAny

#

Usually one should use move(A, B)

stone dragon
#

I see your point on having to do linear time searches on the extracting side; but, using variants, not predicates, to lookup means that when you don't have to do this you get constant time lookups (and there's plenty of cases where you don't; I've got some in my own mod). If you back it with a predicate lookup like that, you force it to always be linear time, not constant-but-sometimes-linear

wind steppe
#

Im not trying to say that this is faster than just using the exact variant, i understand the performance drop off here. Im saying, that the most common scenario for transferring is that you have no idea what you can transfer out of the container. Short of having a container where only 1 variant is ever allowed in the container (which in that case, you can just ignore the predicate entirely), there is almost always going to be a call to look at the variants, find one, and then use that to extract, which would lose performance anyways

wind steppe
flat root
#

Because fluid handler is suboptimally designed

#

Some of the helpers in FluidUtil are "broken" because they use "extract any" instead of looking for a fluid that will fit in the target

#

tryFluidTransfer_internal if I got the name right...

#

Almost always people have logic that looks like

if (i am empty) myStuff = extractAny();
else myStuff += extractSameAs(myStuff);
#

If this is what you want "extract any" for, consider using moveAll(..., myStuff)

wind steppe
#

extract any i mostly use for internal consuming of liquids for recipes or ticking down fuel etc. If its allowed in the container, its allowed to be consumed

#

I guess having utility methods to do that searching would be a good solution as well

flat root
#

Yeah

#

When working with a single internal slot I like to just "hardcode" index 0

stone dragon
sinful tiger
#

I don't think a predicate is inherently bad per se - you can't just use an opaque lambda, but having a class that could be called something like VariantPredicate that implements predicate but also can be broken down into "set of items and/or tags + predicate on data components" for introspection by handlers that can make use of that information could be useful.

stone dragon
sinful tiger
#

I mean, I'd have it take the concrete class directly, just like how recipes take an Ingredient instead of a Predicate<ItemStack> because it has to be serializable

#

my point is allowing the InvWrapper/ItemStackHandler impls to call a .test() method, mainly, i guess

#

and other handlers can lookup the item in a map and then call the test method [with maybe some special casing for 'wide open' and 'exact match']

stone dragon
sinful tiger
#

My point is

#

that advanced containers could still benefit from "use a map or whatever to find the right item, then use an arbitrary predicate to filter on components/attachments/capabilities/nbt/toolactions/etc beyond that"

#

i just realized tool actions probably aren't a good fit, but the rest are

stone dragon
sinful tiger
#

no

#

no it wouldn't

#

an item variant is "item + exact components/nbt/etc", there's no way to specify "any undamaged sword [but with arbitrary enchantments]" or "any sword with these enchantments etc [but arbitrary damage]"

stone dragon
sinful tiger
#

i thought the proposal for requiring a variant was to not allow consumer-side filtering at all

stone dragon
#

Stuff must be able to see what is available, pick something, and retrieve it

#

You'd have huge issues otherwise

#

That filtering just happens on the side of the thing querying the inventory, not as a predicate passed into the query, because that has a ton of issues.

#

But the workflow of "get list of things in thing; iterate list, find the thing I want; query that thing" would be common for things not wanting a specific variant, yeah -- and is currently what happens all the time

#

A lot o this discussion was coming out of discussion of the fabric transfer API, and their ItemVariant/TransferVariant

wind steppe
#

so i did end up trying out predicates in my project. ultimately, i think utility methods on top of the transfer stuff like what fabric has for storage utils are more effective in doing the same thing. Im not sure i agree with the performance drop offs in all situations, but ultimately they can both do the same thing, but putting a predicate in extract prevents someone from doing an exact get even if they wanted to.

stone dragon
#

There are performance dropsoffs for predicates in many common situations. There are no performance dropoffs for filtering on your own end that a predicate wouldnt' also have

waxen dawn
#

@flat root are we still interested in this

flat root
#

Yes

#

This is longer term work

wind steppe
#

@flat root is this what you had in mind for storage? Fabric does slotted storage with a seperate class for each slot, and thats how i currently implement storage in my mod but this seems simpler on mod authors to implement to much the same effect, albiet without a generic slotless extract/insert method.

image.png
flat root
#

that seems good enough yes; we can potentially add supportsInsertion and supportsExtraction

wind steppe
#

ah yeah, i forgot bout that

flat root
#

also please do not call it IHandler 😛

wind steppe
#

i was gonna call it IStorage but i thought handler made more sense for the context of forge

hollow maple
#

IStorage please

#

handler is... blegh.

wind steppe
#

i storage it is then lol

copper fjord
#

getResource vs getAmount, what's the difference

wind steppe
#

oh shit

flat root
#

🖐️

wind steppe
#

ones supposed to be an int mb

flat root
#

where are my longs 😭

wind steppe
#

i dont think longs are what people want on neo

flat root
#

this is potentially a topic of bikeshed

#

in reality there's no reason not to use longs but people are stupid 😛

copper fjord
#

lol I was about to mention

#

I was thinking about that just as you wrote the message

flat root
#

it's fine if this is int-based, doesn't matter

wind steppe
#

crisis averted

image.png
flat root
#

isBlank should probably be isEmpty and also check for the amount

#

(arguable)

copper fjord
#

isBlank seems quite meh

wind steppe
#

ah yeah. also can we rename ResourceAmount to ResourceStack

flat root
#

🤷

#

names can be bikeshed at a later time but why not

wind steppe
#

niiice

copper fjord
#

I mean what's the usecase over getting the amount in the slot and checking for empty

#

also, since most of those methods take in an int, can't we just have a slot iface

wind steppe
#

i mean, we can. thats what fabric does but its a little bit cumbersome to implement

#

more so than just these methods

flat root
#

we shouldn't have a slot interface cause everyone would be forced to either cache the list of slots for their inventory, or allocate new ones constantly

#

on fabric it makes more sense because each slot probably wants to extend from SnapshotParticipant anyway

copper fjord
#

I think that getslors.foreach would be more ergonomic than a fori, but w/e

wind steppe
#

this is what i do in my lib, its nice and also makes it so the base storage class has a slotless insert/extract method. This is admittedly more complicated tho, but i do it for fabric

image.png image.png image.png
copper fjord
#

IT'S A LONG!!!!

wind steppe
#

indeed its a long lmao

#

ive committed forge sin

#

I think energy storage works as is without changes

flat root
#

ah right, we do want to add slotless insert and extract methods at the very least

wind steppe
#

yeah agreed, they are nice

void thicket
#

long bad™️

flat root
#

you're bad™️

wind steppe
#

we want the existing handler classes to extend this one right? but the caps to return this base storage class with the generic of the resource type

hollow maple
#

honestly, my only problem with using slots-by-default is that it just doesn't make much sense in the context of energy

#

it's whatever otherwise

wind steppe
#

energy is slotless

hollow maple
#

yeah

wind steppe
#

energy does not use this class as it does not use a resource for transferring

hollow maple
#

I mean that makes sense given the API surface

#

but still sadge

wind steppe
#

the thing im most excited for is to delete the FluidHandlerItem class. context all the way baby

hollow maple
#

yeah, I kinda wonder if "context" would be a useful parameter for energy. Maybe extracting 10 neoforge:energy per tick vs 10 mymod:mana while still using the same interface? 😜

wind steppe
#

oh lord no

#

god no please

hollow maple
#

LOL

wind steppe
#

you could make energy completely generic, i dont think that would be nice tho. Theres nothing stopping anyone from just using the energy class as a capability for mana tho and just registering it as a separate cap

hollow maple
#

it'd certainly be interesting, though, having a potato battery that stores magic energy because a mod author forgot to check

flat root
#

we don't want the new API to extend from the old one or vice versa; right?

#

well we could do it

#

would that make migration the easiest? probably

wind steppe
#

well i think itd be fine to make the old api extend the new one

flat root
#

yeah I think it's sensible enough

wind steppe
#

you had mentioned you wanted to do it in a way thats backwards compatible, is that not what you had in mind?

#

i guess you could register a second capability and do funky conversion shit but god that sounds awful

flat root
#

we will have to do that either way

#

and yeah I think it's sensible that IItemHandler implements IStorage<ItemVariant>

#

that will allow reasonable code migration, and directly proxying the capability in one direction

#

IMO the plan should be something like:

  • 1.21: add the new replacement API alongside the old API; the old API will keep working (except maybe for IFluidHandlerItem?)
  • 1.22: delete old API
wind steppe
#

makes sense. How would we handle the Void context on the older apis? have a defaulted one? or change them but convert the storage class

flat root
#

ah; well the ItemCapabilities won't be backwards compatible

#

I don't think that there is any alternative

wind steppe
#

You could do what I do in Botarium and have some very limited contexts wrapped around the item stacks be provided, but that would suck

#

And would inherently limit anyone using the new api to the constraints of the old one. Might be best to yeah just not make them backwards compatible

vale birch
#

So what, I can only have Int.MAX number of slots?

Also did I read it right that this will only support storage with slots?
Why not add the interface for slotless stuff as well?

echo saddleBOT
#
technici4n

[Reference to](#1183818213134446742 message) #1183818213134446742 [➤ ](#1183818213134446742 message)ah right, we do want to add slotless insert and extract methods at the very least

lukebemish referenced ·
flat root
#

thinking that you need a slotless interface is often a mistake 😛

#

the slotless methods exist purely for "optimization"

vale birch
#

Having a separate slotless interface would mean support for energy like setups

#

IMO

#

Also what about my 6 billion slot chest?

flat root
wind steppe
#

You wouldn’t be able to use a slotless storage for energy anyways, as that’s also resource-less

wind steppe
#

This is the item context class. Fabric's requires you give them the storage view of the main slot and the storage views of the outer containers slots but i dont think thats necessary. insertOverflow was also removed as i dont think its necessary for a mod to call those methods directly.

image.png
flat root
#

Would it be worth replacing boolean simulate by a TransferAction enum?

#

(IMO yes)

#

Looks quite reasonable otherwise. I think that it's a good starting point

wind steppe
#

i mean maybe? i tried it, i thought it wasnt necessary tbh

#

Another thing im taking a look at, in your github draft you wanted to be able to do a few things with resource stacks. I was about to do most of them, but the changes to the components in the resource stack required some changes to the iresource. i dont mind the changes but they are worth discussing

#

The interface would look like this now

image.png
#

with that, the dream of:

ResourceStack<ItemResource> stack = getStack();
stack = stack.shrink(16);
stack = stack.with(resource -> resource
        .set(COMPONENT_1, someComponent)
        .remove(COMPONENT_2));
setStack(stack);

is possible

wind steppe
#

Although, should this take a consumer? is this really different from:

ResourceStack<ItemResource> stack = getStack();
stack = stack.shrink(16);
stack = stack.with(DataComponentPatch.builder()
        .set(COMPONENT_1, someComponent)
        .remove(COMPONENT_2)
        .build());
setStack(stack);
flat root
#

Please no self type like that

wind steppe
#

i guess, but short of like, an unsafe cast, not sure how else you're meant to do that

flat root
#

Well the former doesn't require an unsafe cast 😛

#

Idk about the immutable stacks myself

wind steppe
#

itd have to look like this without the self type

image.png
flat root
#

That's why I used a lambda

wind steppe
#

that would still cause the same problem tho, how are you supposed to return a new T without something in IResource that accepts the changes and returns a new one of itself

#

oh i see what you mean now

#

hm

#

the param in your method is a UnaryOperator of T

#

yeah that works

wind steppe
flat root
#

It's a lot clearer than magic boolean values

#

Especially on the caller side

wind steppe
#

Hmm. i guess. i think this one is a matter of personal preference, maybe we make a separate comment on that in the PR and ask what people would think is better. Cuz on the one hand its definitely more clear, on the other hand the boolean is slightly easier to work with

flat root
#

The enum would have isSimulate() and isExecute() functions or similar

#

Basically FluidAction

wind steppe
#

do people like fluid action

flat root
#

More than a boolean? Probably

#

The main annoyance is how it's nested inside IFluidHandler

wind steppe
#

yeah definitely annoying there. i tried searching in FC to see if anyone complained about it. just lemming complaining about how fluid action is inconsistent with the other handlers, which is 100% fair, but then he said: "it should either be ForgeAction for everything or just true/false for everything" which is not a definitive answer waaaaaaa

#

ill just add it

flat root
#

do we want to organize everything under net.neoforged.transfer?

wind steppe
#

transfer works, i didnt realize you had used transfer initially. When you mean 'everything', you dont also mean putting the resources in there? i honestly dont think itd be that bad to move like, the fluid resources under net.neoforged.transfer.fluids and keep the new examples in there too

flat root
#

yeah that's what I mean

#

then we can try to have a sensible package organization, e.g. transfer.storage, transfer.items, transfer.fluids

wind steppe
#

sounds good

noble latch
#

Hmm, would it make sense to introduce a IItemStorage specialization that has a method for requesting an IItemContext for a given slot? The idea would be to make cases like capability operations on a player's held item (i.e. extracting fluid from a held bucket) trivial if no special handling of slot mutations is needed on the caller's side

flat root
#

There should be factory methods for common use cases already

wind steppe
flat root
#

E.g. IContainerItemContext.forPlayerSlot(player, slot)

#

Well they're not written yet but I assume it's planned 😛

wind steppe
#

Yeah that’s the plan. I don’t like calling it container item context tho. I like just IItemContext

noble latch
#

That seems reasonable as well 👌

vale birch
#

How bad would it be if someone extended the ResourceStack to support negative amounts as non blank?
Sounds like something that COULD happen...

wind steppe
#

I’ve been mostly looking at the existing classes for the handlers and making versions without the handler classes. Some of these classes can be converted to be completely generic as long as an empty resource is passed to it. Empty, Creative, and Void containers function pretty much the same no matter what resource you’re using.

Also question for impl details, in my mod I use the same container for all cap types and just have different constructors and set a runnable to tell it how to update itself. I think that would work here as well. I also think it would be good to encourage people to use data attachments in the examples

wind steppe
vale birch
#

but what if I actually want to have negative of something?

#

I mean it is an edge case but also not something un-immaginable

wind steppe
#

Then I’d probably say make your own resource stack? I think amounts should always be nonnegative to other mods during transfer

vale birch
#

Sure justmaking sure it is an intentional decision

#

I don't fully understand where this ItemContext comes in?

  • It doesn't have slot params so I guess this is a View onto a single slot OR just a slot agnostic view to a whole chest???

  • Why is it Item specific and not a IResourceContext? where the getCapability would be the only thing specific to items (? maybe not event that?)

flat root
#

i.e. items that contain resources

noble latch
flat root
#

At least in the original design

wind steppe
#

You don’t really need context for anything else unless you for some reason decided to make FluidStack capabilities. It’s a view of a single slot with slot agnostic methods for inserting into the container. Extract extracts from the main view, exchange extracts a given amount from the main slot and then inserts the same amount of a new value into the container, first trying to insert into the main slot, but then inserting into an overflow if it can’t.

vale birch
wind steppe
#

I might have explained poorly what it does in the PR, I’ll try to update that when I get home

vale birch
#

well funny you would mention fluid caps....
The last time i was doing modding i was doing something like that... well not caps but storing data on fluids (I was saving the mod id for the blood to have sheep blood)

#

No i think your description was good now that I read it again

wind steppe
#

Fluid caps is honestly not a terrible idea i just don’t think we should expose any of the default caps to fluid stack

noble latch
noble latch
flat root
#

Interactions with IFluidHandlerItem are unbearable atm

wind steppe
#

Fr

flat root
#

It's horrible to use

noble latch
#

Yup

vale birch
#

Yeah that is where i gave up in 1.16 doing the data based mixing and all the simulate trying stuff was stupid

#

and that is why I'm interested in this PR

wind steppe
#

Fluid handler item is also kinda stupid because the handler dictates to the container what the slot is going to be changing to, doesn’t allow the container to say “no you aren’t allowed to digimorph into that” while also being more restrictive because the fluid handler item has no place to put any extra items if it decides to pop out an empty container from a bigger stack

vale birch
#

Ohh yeah I was making a vial for my blood as well....
getting those to empty into a mekanism tank took way too many days XD

wind steppe
#

ItemContext also has some potential to be super useful in an ingredient, I plan on also introducing a PR after this is merged adding something new with that

vale birch
#

I was reading the fluid resource stuff and I'm a bit confused.... Why does it wrap the fluid stack isn't this about getting rid of that?
Is this just a transition and later will be removed or is this to stay?

wind steppe
#

I honestly don’t know the answer to that question, that was implemented by Tech. Mutable resources aren’t completely useless, and having FluidStack as an analogue to ItemStack is nice. Ideally tho it’d be phased out in favor of just using FluidResource and ResourceStack everywhere

flat root
#

I implemented them as a stack wrapper because it's so easy to do, and makes conversions to/from stacks very efficient

sinful tiger
#

[i forget, did we end up getting rid of item stack attachments?]

void thicket
#

Though the context isn't necessary unless modification of the item is needed

#

like it is for buckets

sinful tiger
#

ah so we can still replace data component values?

void thicket
#

Yeah

sinful tiger
#

actually

#

if you want to handle modifying an item that is inside another container you'd still need the context because the container might not maintain its contents as live itemstack objects [in particular, if that container is an item, even if it does contain itemstack objects those likely should not be mutated]

#

also raises ownership questions for the usual case [in some cases mutating the itemstack may simply have no effect, in other cases it may violate the immutability of some object containing it]

wind steppe
#

Yep, I noted that in the interaction changes in the pr

#

Technically we could change the item cap to be a cap of the IItemContext and rename it to something slightly different. That would solidify “hey the stack is read only, do not mutate it”, since the item context provides the same information that the itemstack provides

#

But yeah, if you want to make any changes to the item, component or otherwise, you have to use context. ItemStack doesn’t really serve a purpose at all anymore

wind steppe
#

Considering ResourceStack doesnt really have any kind of stack limitation, i think it would be really helpful to have a public ResourceStack<T> growToCap(int amount, int cap) method in ResourceStack that containers can use and pass in their limits to make it easier to work with.

flat root
#

idk, are people really going to use ResourceStack that much?

#

it's such a pain to get a Slot to work with immutable resources, sadly

flat root
#

you want to change it from ItemCapability to something else?

wind steppe
# flat root you want to change it from `ItemCapability` to something else?

Maybe, I was saying it was something we could do since it’d help with people misusing the provided ItemStack. But then other people using ItemCapability might want a more involved context. Like, maybe Curios wants to have “WearerContext” that extends the item context to add the entity wearing the item

flat root
#

yeah this should remain separate context I think

#

on fabric you typically just ignore the stack and store the context in the storage

wind steppe
#

yeah, we’ll just have to warn people about it in the documentation. And I assume there will be a blog post about the changes with how much this changes about stuff. Could let people know there too

flat root
#

And I assume there will be a blog post about the changes with how much this changes about stuff.
you want to write it? LUL

wind steppe
#

I could if you want lol

wind steppe
#

AAAA not having transactions is gonna kill me on item context. I have no idea how to properly handle a container that wants to do multiple exchanges in 1 shot. There’s no proper way to simulate this happening

#

I sent that an hour ago. Man airplane WiFi sucks

stone dragon
#

I, unfortunately, have exactly such a container and have been thinking the same thing for some time now

wind steppe
#

I was trying to think of something in between transactions and simulations but im not sure what. I was considering maybe builders could work but theres some tricky things with usability in that sense and im not sure if that would solve all the issues. Itd change storage to be immutable. Storage would have read methods for seeing whats in it, but if you want to change its state you do .builder() on it and itll make a thing you can make changes on. The builder would be built on the current state of the storage, and then changes could be made and stuff. There wouldnt be any simulation, you just make changes, check if they are good, and then do .commit() at the end, or just dont if you dont like the result. I feel like builders would probably be more foolproof than transactions, as they are a separate thing altogether from the main class so itd be easier to understand? idk, this definitely has its own issues, but maybe something worth considering :P

stone dragon
#

Honestly I just want transactions

#

But I suspect that might not be the most popular

wind steppe
#

yeah transactions is a no go

stone dragon
stone dragon
wind steppe
#

just not something people seem to want

stone dragon
#

I mean, who's "people"?

#

Modders in general, or maintainers, or both, or whatever?

#

Could make a transaction API wrapped up in a simpler API for non-simulated cases

#

Then the only time you'd have to touch transactions is chains of stuff or wanting to simulate stuff

#

And doing stuff sensibly in those cases will always require complicated stuff

#

The bigger pain is on the side of inventories implementing it I guess, so yeah, maybe that's an argument

wind steppe
# stone dragon This feels like slightly refactored transactions. What's the difference?

The main difference would be there isnt really any state management per se. And it changes fundamentally how the storage class is structured. I think one of the most confusing parts of storage on fabric is that we're meant to look at the the class that doesnt look all that different from what we're used to but transactions have to completely change how one is supposed to develop within the class. Making it a builder kinda makes it more obvious, the state does not change within the storage, changes are made in the builder, and only when the builder is built do the changes apply.

stone dragon
#

Can you give me an example of what using the builder would look like?

stone dragon
wind steppe
#

it is, just in a different form factor that makes it easier to wrap your head around

stone dragon
#

Cool. I'm totally in favor of a transactions-but-more-intuitive solution

wind steppe
#

its a little more intuitive on the implementation side, im not sure how it is on the dev trying to use it side

stone dragon
#

My biggest issue is, basically, I just need to be able to sensibly represent this one particularly annoying inventory of mine. If I can actually do that (current system doesn't allow me to in a way that doesn't have crappy edge cases) and if it's not too ugly (using transactions for this particular one sucks to implement) then I'll be happy

stone dragon
wind steppe
# 
Storage fluids = item.getStoredFluids(); // fake method
fluids.getResource(0); // read only methods
fluids.getAmount(0);
Builder modifier = fluids.builder(); // builder used for modification
modifier.insert(fluid, 10);
modifier.extract(otherFluid, 5);
if(changes.isGood()) {
  modifier.commit();
}
stone dragon
#

👍 love it

#

The one potential limit is stuff involving, like, multiple machines -- what happens if committing one builder makes another builder for another BE invalid? Heck, what happens if someone has two builders for one BE open at once?

wind steppe
#

that would be a problem yeah. this is just something i was thinking about, it might ultimately not work as a solution

#

it might end up needing so much modification that it ends up just being transactions and we're back at square one

#

it makes doing certain things that transactions make possible possible but could also end up just being more error prone

flat root
#

I really dislike that sort of transactions-at-home pattern because it doesn't solve composability

flat root
#

Even with single exchange this is a massive improvement over the existing APIs

#

Transactions will only help fixing edge cases, but here we're targeting basic usability IMO

cursive geyser
#

if transactions end up existing eventually, they must not get in the way of modders. there's already people questioning why we need a custom API for items instead of using vanilla Container stuff and wanting to use it instead of iitemhandler. if implementing the item API were to become a lot more complex than saying "fuck it I'll use Container I don't care about pipes", more and more people will do the latter

flat root
#

Nah they won't use Container cause they'll want to be compatible with the more well known mod

#

"they must not get in the way of modders" you can't have your cake and eat it too 😛

cursive geyser
#

honestly I don't have a use for transactions myself. I'm one of the people that doesn't want additional work :p

serene igloo
wind steppe
serene igloo
#

they might if I put container capabilities in neoforge

wind steppe
#

again, stubbornness

serene igloo
#

what makes you believe transactions would ever exist

wind steppe
#

i dont believe transactions would ever exist, which is why i was thinking about a solution that involved something that wasnt transactions

#

but as tech rightly pointed out, this is not much gain just to have 'transactions at home'. it was just an idea

wind steppe
# cursive geyser honestly I don't have a use for transactions myself. I'm one of the people that ...

yeah i dont think most people would. problem is that transactions are more useful in other cases. like, its far more likely you'd be doing multiple transfers when you're doing item capability related stuff, and transactions would be more useful there. You'll do a transfer on the item's storage and that the storage will do a transfer itself. maybe the storage will do more than 1 transfer. I dont think itd be super uncommon to have something like: a stack of full storage that, when drained, will drain as many full items as it can and then also drain 1 container to be partially full. so itd do 2 transfers, one to fully empty x number of items, and one to partially empty 1 item. if all that happens in 1 single extract method, itd be impossible for simulate to always get that right short of like, making it a required part of the spec that items always have somewhere to go (which would be bad imo, itd force every mod to implement some kind of overflow drop on floor fallback thing)

serene igloo
#

harold my non-instant tubes already do overflow drop on the floor fallback

wind steppe
#

yeah but that may not be desired all the time

#

a modpack might want a pipe mod that doesnt do that because thatd be really bad for like, a skyblock pack

serene igloo
#

it tries to reinsert items into the pipe network before it does that, so you can use a shunt to catch overflow and redirect it back into the pipes or into an incinerator or whatever

sinful tiger
#

what if we merged in IItemHandlerModifiable and forced all item handlers to implement setStackInSlot

flat root
#

Ah yes, why provide proper APIs when people can just force-set a stack

#

This sort of suggestion just ignores the reason why IItemHandler exists :/

cursive geyser
#

please no

#

setStack should never be publicly exposed where people can call it without realizing it's not the proper way to use the api

#

additionally, it would be extremely annoying for large-inventory mods

#

since eg, Ender-Rift slots don't necessarily contain what getStack says

#

due to the internal storage being a long