#Transfer rework (IItemHandler, IFluidHandler, IEnergyStorage)

1 messages · Page 3 of 1

sinful tiger
#

anyway i have never seen anything that legitimately needs to be divisible by any multiple of 125, so it probably doesn't actually need three zeros

opaque vector
#

this is an implementation detail which mean the people actually using it should decide. not players. Not random entity modders

ivory mauve
#

And listing what mod you make is useful to add weight to the opinion. There are plenty of mods that just mirror decisions of other mods. I've even seen some people advocating for 81 on tinkers behalf

#

I'd rather the mod author advocate for their own mod, as they know what compromises are reasonable

opaque vector
#

quarantining this discussion

ivory mauve
#

But yeah, I agree moving this convo makes sense. Its only tangentially related to transfer

#

The one transfer related topic is if every fluid gets its own bucket volume, we'd need to make sure this API rework supports it. But given it works with item stacks having different stack sizes, its probably fine

wind steppe
#

Would anyone stenously object to removing the methods in fluid util that do things without manipulating the itemstack? They seemed to be use before to properly account for creative mode, but this behavior is no longer necessary. But idk if anyone was using it before for other reasons

cursive geyser
#

back in 1.14 I was strongly temptedto just delete the whole of FluidUtil, and let people suggest the necessary helpers as they encounter the necessities. never happened, obviously, but that sense of "this is a royal mess that shouldn't exist" still lingers :P

ivory mauve
#

I think people telling me how much it sucks caused me to just write my own

wind steppe
#

its a horrible mess. Ive narrowed it down to the 4 essential functions: pickup/place liquid, extract/insert liquid in block

#

everything else is just weird or doesnt need to be fluid specific

#

is getFilledBucket legacy? why isnt this just a method on the fluid stack

cunning mulch
#

yeah I haven't touched FluidUtil since maybe 1.12? idk if we used it back then. (except for in some versions the getHandler helper for itemstacks, but now I don't even use that)

wind steppe
#

you dont touch it cuz you dont find it useful, cuz it doesnt work, or cuz your impl is better?

cunning mulch
#

It doesn’t seem useful because it tries to be too generic for certain interactions and then leads to meh behavior

#

Basically it is just too clunky to use, and doesn’t necessarily do what I need

wind steppe
#

Gotcha

sinful tiger
#

the interaction stuff is useful because blocks that use it in their useItemOn method behave predictably with any fluid container in your hand, but it does have limitations

#

Would anyone stenously object to removing the methods in fluid util that do things without manipulating the itemstack? They seemed to be use before to properly account for creative mode, but this behavior is no longer necessary. But idk if anyone was using it before for other reasons

Wait, what is the replacement that still accounts for creative mode?

wind steppe
#

You use IItemContext to manipulate capabilities on items inside the context of their container. PlayerContext.fromHand(hand) will create an item context that mimics creative mode behavior thats used if player.isCreative() is true

#

Since the method creates the context inside the method itself, it will, by default, account for creative mode behavior

sinful tiger
#

if player.isCreative() is true
you mean if player.getAbilities().instabuild right?

wind steppe
#

i guess, i usually use that but i never really thought about why

wind steppe
#

besides tests and going through and documenting things i forgot to, i think the PR is mostly done @flat root

#

I hope to have something reviewable over the next week or so

flat root
#

okay good

#

we can probably try to target this for 1.21.1 (or whatever the next breaking version is)

wind steppe
#

damn, thatll suck not being able to use it basically until 1.22

cunning mulch
flat root
wind steppe
cunning mulch
#

The bigger thing is by the time it is finished and reviewed it will be so far into the beta period that making such a massive breaking change would be more harmful than beneficial

#

While we do make breaking changes as needed, that doesn’t mean we want to break the majority of mods a few weeks into the cycle

wind steppe
#

yeah, that makes sense 😔

wind steppe
#

game tests NotLikeThis

cursive geyser
#

I think it would be a good thing to always target big changes like this to a "tech/preview update" period. so if 1.21.1 was a hotfix, we would target .2, so that "content updates", that tend to be smaller changes, are almost stable straight from release, meaning the entire cycle of 1.21.hotfix is an easy ride for everyone that has kept up with tech updates.

sinful tiger
#

is it / will it be legal for a fluid handler to have 0 tanks

wind steppe
#

i currently have some templates that have a size 0 container

sinful tiger
#

i was asking because i don't know

wind steppe
#

i guess the answer is no, its not currently illegal so

#

should it be?

flat root
#

it's legal

sinful tiger
#

ok thanks just checking

#

[i was, in fact, going to suggest it should be legal if it wasn't currently]

serene igloo
#

okay I take back everything bad I ever said about transactions

#

I don't need them for inter-block automation

#

but I'm rewriting jumbo furnace to be able to handle recipes that can take x different ingredients with xy count each to produce z outputs and also handle crafting remainders from consuming inputs and fuel

#

and transactions would make my intra-block automation much easier

#

right now I'm like... taking an itemstackhandler, making a copy of it, doing a bunch of inserts/extracts on it, and if they're all valid I update the original one

#

which I guess is probably just how transactional itemhandlers would end up being implemented anyway

flat root
#

I had the same realization as you 3.5 years ago when I realized that transactions would make my internal logic simpler 😄

#

(turns out my internal logic is still doing an inventory copy)

serene igloo
#

ItemHandlerStack harold

wind steppe
#

Hey he gets it! And yeah, I was having the same concern when implementing item context for this

#

Maybe if we wait a few months everyone can come around to transactions kekw

wind steppe
#

What are the thoughts on making a canInsert(resource) method to check if the handler, regardless of the contents, can insert this resource into any of its slots. If its blank, it checks it returns true if it can accept an item at all, even if it only accepts a specific one

cunning mulch
#

Isn’t that what currently isItemValid is?

wind steppe
#

yes, but that method checks a specific slot

#

basically have a slotted and unslotted variant of isItemValid

wind steppe
#

oh actually, question. Since this isnt being implemented in 1.21 but rather 1.21.1, can we just... delete the existing item handler and fluid handler? they were kept around for people to have time to transition but now you basically have until 1.22 to transition, every version after 1.21 is pretty much going to have no usage anyways

flat root
flat root
#

At least it seems easy enough, and would significantly ease migration

wind steppe
#

So thinking over it more, i think the argument for keeping Energy as a single slot container is kinda weak. With indexless insert/extract methods itd be pretty trivial to use it in the same way people were using it before, mods like Jade could more accurate represent how mods that could show energy in multiple slots do so, and it would not be more difficult at all to use this from a dev perspective, we can provide a simple ISingleEnergyHandler class thatd make this easy. Itd open up the potential for mods to say, an energy block with each index being a battery you can replace or something.

You could argue it complicates energy for no reason, id argue energy has always been simple and no one has really had the opportunity to do anything more complex with energy because the interface is always limited to 1 slot. The decision was made 10 years ago with regard to how energy was handled by the dev who implemented it and ever since the interface has limited how we're allowed to interact with it. Energy doesnt have to be this way.

Also it should be called IValueHandler not IEnergyHandler so if we add move helpers itd be usable with other mods using it for Mana or something without having weird looking code

void thicket
#

Energy is not within scope of this transfer system whatsoever imo

#

it does not share the characteristics of the other objects

wind steppe
#

no yeah, thats fine, itd still be its own handler

#

because adding a resource probably would be more complex than needed

#

but im saying making IEnergHandler being its own class with indices

void thicket
#

That sounds incredibly annoying for no reason

wind steppe
#

it wouldnt be more annoying for practically anyone

#

whats a usecase that would be harder with this that wouldnt be solved by having a simple ISingleEnergyHandler for people who still want to use energy in the same way they were using it before

void thicket
#

What is a real usecase that is permitted by this added complexity

#

There are already blocks that compose multiple internal battery items

#

they do not need this

wind steppe
#

those containers are not able to properly express that they have that many slots. So a mod like Jade or something would still read their multiple batteries as a single buffer

void thicket
#

No, that's already happening correctly

#

the buffer expresses the sum

#

and/or jade can be told the underlying item slots and display that - the handling is provided by the mod (not jade)

#

You will need to provide a very compelling reason to justify this change, not a theoretical usecase

wind steppe
#

why "very compelling". This is not a complex change, and makes it more flexible. Say a mod has a pipe and it wants to control which slots theyd be inserting energy into a handler with. Rn they wouldnt even be aware that the slots exist so theyd insert it and let the mod who made the handler handle distribution. You could argue thats fine, but we currently have index based insert/extract methods on the normal handler to allow for this kind of behavior, so why not allow it here as well

void thicket
#

Say a mod has a pipe and it wants to control which slots theyd be inserting energy into a handler with.
How do you express that meaningfully? That seems like you would either be inducing a for loop (which means the slots are useless), or you have to expose slot metadata to the player for micromanagement

#

As for why "very compelling"

#

You would be sending a breaking change to everyone using the energy system

#

we aren't doing that on theoretics

wind steppe
#

we are already sending a breaking change to everyone using the energy system

#

the energy handler got a refactor with its name and its methods, and anyone who used energy handlers with Itemstack have to rewrite them to account for IItemContext

wind steppe
void thicket
#

That doesn't have any tangible benefit though

#

its just complexity for complexity's sake

wind steppe
#

whether or not you think this feature is useful in a mod is irrelevant, you asked for what a mod dev would use this for, i explained

#

it essentially just allows more complex handlers to explain their increased complexity. it would not affect less complex handlers, they can use the simplified ISingleEnergyHandler that handles explaining the basic things that are the same across all single slot handlers. You could argue that the fluid handler is also complex for complexity's sake because besides a handful of mods no one really uses fluids with multiple slots, the handler itself doesnt even have slotted methods for i/o

void thicket
#

The complexity increase isn't just "they can use the simplified version". Docs have to update, code has to handle the non-simple case, questions will have to be answered about the decision. This is not worth it for a theoretical usecase.

#

Fluids do not express the same ability to be flattened

#

Despite the fact that people often only store one, the inability to store multiple would represent a reasonable problem

#

But with energy you can literally only ever hold one, there are not multiple types

wind steppe
#

you have the freedom to express multiple indices of the same resource type in a normal handler as well without having it be flattened

#

we are making large breaking changes to how handlers work in general that also affect how people use the energy handler. If a mod really wants to treat energy as simple then we can provide flattening methods in a helper. But Im not convinced the base interface should limit how a handler can be expressed by flattening it from the get go.

vernal dust
#

More discussion on energy: #1254214554842300477

wind steppe
#

hello, reviving the dead conversation here. @cunning mulch @flat root would one of you mind taking a look at the templates in transfer/handlers/templates and seeing what you think? I know Tech isnt a huge fan of this kind of thing but i think the implementations are simple enough so people can still understand how they work while providing a valuable utility for devs who want to make use of the newly abstracted storage system for creating new containers holding different kinds of values.

cunning mulch
#

I will try to take at least a basic look this weekend

cunning mulch
#

meh

#

I left a few comments on the specific files you asked about

#

some I didn't bother duplicating even though the code is the same in various places so I could have copy pasted them

#

I also didn't bother checking all the math

#

my initial intuition though is the stepped template might be better as a follow up PR that then gets some insight on how useful it actually is to people to have a default provided that can do things in steps.

wind steppe
flat root
#

I want a transfer rework and I kinda don't want to review it at the same time, but I should really review it harold

#

the duality of the lazy reviewer

cunning mulch
#

it has merge conflicts, lack of docs, and unaddressed spotless things (such as imports in patches)

#

I am holding off until then on attempting to give it a more thorough review

wind steppe
#

thats reasonable

#

i just wanted some feedback on the handlers in this package to know if that was something that id be asked to completely change later

flat root
#

ok let me have a deeper look

cunning mulch
flat root
#

ok I had a quick look and left a few comments

#

you can probably ignore most of them

#

it's a bit different from what I was expecting but that's not a bad sign

wind steppe
#

wha, i thought you said you didnt want insert to have a default impl

#

i discussed this with you >:(

#

i even wrote one!

#

did you mean like, you didnt want the default impl to be stacking, so the default impl would just go over the slots once

flat root
#

yes

wind steppe
#

gotcha alr

flat root
#

tbh I might have forgotten things 😛

wind steppe
#

also for this function

image.png
flat root
#

realistically we could say that passing a blank resource gives you the "default" capacity

wind steppe
#

yeah thats what we discusssed when pup brought it up

#

but i think an argument could be made that thats confusing

flat root
#

less functions = good, there is already a large increase

#

(compared to current handlers and to trapi)

#

I need to actually try out the API, but I only had 20 minutes for a review 😄

wind steppe
#

that is true, tho the amount of implementation is about the same

#

cuz you'd have to currently check with an if statement is the resource is blank to return the maximum, otherwise return the resource based one

#

the argument for separating them was that it kinda gives the blank resource a dual meaning depending on where its being used. In the capacity method it acts as a "universal" resource, of all resources in game, what is the theoretical maximum of at least one of those resources that you'd accept. But in everything else it acts as "blank". This duality may confuse users. It could imply that the extract method should perhaps accept the empty resource as "universal" and extract any. You and I know why that doesnt make sense, but thats what a user would think itd do, and itd make the api more confusing

#

hence why i separated the querying of the theoretical maximum into its own method

#

Oh also on another note, i compeltely agree that the utility classes being static inner classes is kinda bad, but given how other forge classes have been doing it i did it that way as well. if we dont want it that way i would be more than happy to make them their own classes 😄

flat root
#

i.e. the getCapacity(int, T) overload is already weird if you pass a blank T

#

so might as well say that means an "approximate"/"estimate" capacity 😄

wind steppe
cunning mulch
#

and what I had requested/suggested when I initially said we should have the method

wind steppe
drifting wolf
#

Hi, would it make sense for you to add itemHandler also for entities marked on picture (crossed are already added by neo)?
I would like to see at least for static entities like armor stand or item frame

image.png
silent root
#

For item entity, doesn't exactly make sense, dunno. For everything else, looks good

stone dragon
#

I can see the appeal of the item entity one honestly. It doesn't not make sense, and with how capabilities work nowadays a capability that on extraction, removes the entity isn't that weird

silent root
#

For that it makes sense, for extraction

stone dragon
#

Extraction and also insertion, which is just adding more items to a dropped stack

silent root
#

Yea

drifting wolf
flat root
#

I think it's really weird to implement IItemHandler on ItemEntity

#

then again, it's not really clear what IItemHandler really does for entities

sinful tiger
#

i think there's at least one mod out there that proxies some/all item capabilities to ItemEntity

#

which would imply that IItemHandler should, like, specifically work on ItemEntities where the itemstack is a shulker box or the like

#

regardless of its merits, it's certainly contradictory to the idea of treating it like a single-slot container containing the itemstack itself

#

fwiw i never considered ItemEntities in my proposal to otherwise build out inventory capabilities on entities [something IItemHandler itself really isn't a natural fit for except for things like chest minecarts/boats]

#

which ended up turning into ItemHandler.ENTITY_AUTOMATION

#

the problem with a generalized inventory system for entities is that they're not interchangeable and any given entity may implement more than one "kind" of inventory [e.g. every living entity has armor and hand slots, though even there you really really don't want automation to be able to pull from villagers' hand slots, and for most other entities they should probably be affected by drop chance somehow]

drifting wolf
#

My itemHandler usage is just for querying content in somewhat sized AABB, just my two cents: I personally think of itemHandler as generic inventory interface, nothing more nothing less, so I expect it to provide full access to entity inventory. It might make sense to say this is main inventory part, or this is sth special like armour slots, but for that there is already isItemStackValid(slot)

#

For me then itemframe with one one-sized slot or itemEntity with one normal stack are also inventories tho very special one

sinful tiger
#

The problem is isItemStackValid doesn't say what having an item in that slot means. And anyway, while I think item frames are fine, I think it's reasonable to expect mods that are interested in item entities in that way to special case instanceof ItemEntity - otherwise there's two contradicting interpretations [the item itself vs the contents of an item that has its own item capability item handler]

#

at one point i had a proposal for a capability specifically for entities that would explicitly let you get multiple item handlers for different kinds of inventory [and even allow multiple different ones to be attached by different mods - something that's honestly otherwise kind of missing from the capability system]

wind steppe
flat root
#

somehow I found this again

#

with title "This is what happens if we overdo it:"

void thicket
#

screms in overengineering

loud stirrup
#

Moarrrrr strategies
Moarrrrr factories
Moarrrrr adapters

wind steppe
#

i am back

image.png
boreal plover
#

🔥

queen wagon
#

sooo transactions when?

#

/s

flat root
#

transactions are sadly the only sane way to implement helpers such as "move all items from A to B"

cursive geyser
#

so long as implementing/using the transaction system isn't a hard requirement to make a basic chest or hopper :P

sinful tiger
loud stirrup
void thicket
#

A more basic solution would be to enforce that IItemHandler have a deepCopy method to do multiple simulations on

Transactions need this functionality anyway, so we could use it as a stepping stone

flat root
#

deep copying the giant chest 😍

void thicket
#

stack copy is cheap now :p

#

and presumably you only need to bother when doing large ops

flat root
#

hmmm yes

#

it might be tricky (and lead to problems) to have multiple "live" copies of the inventory though

#

you can't really copy a cauldron handler without copying the entire Level if you know what I mean 😄

void thicket
#

How would that even work with transactions then thonk

#

I guess it's a different contract

#

The impl is responsible for retaining uncomitted state

cursive geyser
#

no idea how "deep copy" would behave if it encounters a very full Ender-Rift 🤔
I guess I'd have to implement transactions for that either way

#

which would take a long time to get around to doing thinkies

#

unless this happens after my post-doc contract ends and I don't have a job at the time lol

stone dragon
flat root
#

the Fabric impl for cauldrons directly mutates stuff in-world, without sending any update; when a change is committed, it the neighbor update is sent

#

to ensure that this remains sound, there is a weak hash map that keeps track of the cauldron wrapper for a given position

stone dragon
flat root
#

hasn't caused issues in 3.5 years so it's a reasonable approach

#

not necessarily

stone dragon
loud stirrup
#

gotta ask lex about forge 😄

#

Yes ofcourse, but let me have my fun 😛

swift summit
#

and this is why the project should have been called NeoFroge!

fossil cave
#

🐸

wind steppe
#

i thought everytime we tried suggesting transactions it was a resounding "no"

stone dragon
#

Not... quite, from what I remember. It was a no from some folks, and a "I don't wanna bikeshed right now" from others, and something other folks liked, at least from what I saw of it -- plus, the new capability system has had time to mature now, it's not like earlier when folks were already reeling from all the changes to capabilities in hat first round

median cedar
#

it would be quite cool to see the transaction system as it allows more complexity (if needed) to some mods, without really forcing complexity when you don't need that extra functionality imo, my mod highly depends on simulated energy transfers and when trying to do some kind of "fusion" for fabric/neo I ended up making an ugly system of transactions for fabric and a transaction -> book for neo, plus all of the benefits I told before it also allows for a cleaner bridge between both loaders but I guess that isn't an important point

wind steppe
#

I guess the question is, how do most people use the storage capabilities. If they mostly use the built in options, making it more complicated might affect only a small subset of people. Would be good to maybe do a poll to see? @flat root

flat root
#

perhaps yes - I feel like it's really hard to judge

wind steppe
dire hazel
#

I feel like for items specifically most mods are able to just expose a Container and transfer using a helper. Similar things could be set up for fluids so that most mods don't need to write their own storages. Transfer using transactions isn't really that confusing, users just need to use raw transfer within one.

The main issues I've ran into on fabric as the author of a, fairly popular, transfer api only storage mod have been related to other mods copying hopper code without fabrics mixins and people iterating the slots of a store with one transaction each (as opposed to having an outer one) causing issues with reordering. The first doesn't apply to neo and the second can be fixed with proper javadocs (which were added to fabric and I haven't had an issue since then)

cunning mulch
ivory mauve
#

the builtin ones are good for normal things. Tanks with single fluids. Item handlers that have a fixed slot count and stack to a fixed size, etc.

#

But I also have unusual handlers. Like our multitank that has a fixed capacity but stores any number of fluids, or the casting tank with a variable capacity based on the current recipe being performed

#

Its important that any changes to the API still support those concepts

#

Was having a discussion on a different server that raised a point I'd want to ensure is addressed. I'm told the the proposed API has a size() function that determines the number of distinct fluids in a tank, and that in the current design its expected that a change to that would dictate invalidating the tank. This is not viable for my usecase as our multitank changes its number of fluids any time someone attempts to pipe in a new fluid

#

Vanilla has a handler that is similar, the bundle. Pretty sure they lack a concept of items that are containers but that would need to be adapted to this system

#

I basically have two options for how to make my thing work with the system:

  • Option 1: I do what I did before, report the number of fluids as size and assume no one is relying on that being fixed for their caches.
  • Option 2: I treat my tank as having a fixed size by just mapping fixed indexes to key fluids (e.g. 0 is just the bottom most fluid in the tank). This would require that mods attempting to do filtered extraction (e.g. a fluid or a Predicate<FluidStack>) are asking my handler how to extract it.
#

I think option 2 sounds like the better path forwards from the other conversation I had as apparently "people want to cache fluid handlers based on slot indexes" despite that never being a thing before.

Problem is supposing I wish to do a predicate based filter, HandlerUtil#extractFiltered assumes the best way to extract is to iterate the indexes in my handler based on size() then try each one against the filter

#

My proposed alternative is adding handler.findResource(Predicate<T>), which allows me to return the first resource my tank contains that matches the predicate, or empty/null (whichever makes more sense for the API) if the predicate does not match. This would then allow the following:

public static <T extends IResource> ResourceStack<T> extractFiltered(IResourceHandler<T> handler, Predicate<T> filter, int amount, TransferAction action) {
  T resource = handler.findResource(filter);
  if (resource == null) {
    return null;
  }
  int extract = handler.extract(resource, amount, action);
  if (extract > 0) {
    return new ResourceStack<>(resource, extract);
  }
  return null;
}

and ensure we maintain the previous behavior where such an extraction can know to pull from fluids beyond index 0.

#

default impl of findResource can just iterate the fluid handler by index.

#

The one flaw in this approach is the resource existing doesn't guarantee its extractable, though that could be resolved by making it extract specific (e.g. findExtractableResource(Predicate<T>), or alternatively giving it an enum argument specifying the goal of the find (insert, extract, or inspect, findResource(Predicate<T>, HandlerAction))

#

I can't think of a usecase for insert personally, but maybe someone else needs it

#

If all this seems overcomplicated/finicky, the other approach is to just not assume handler sizes are static apart from invalidation, though that gets messy with index based canExtract/canInsert. You'd probably have to implement a protocol where the extraction/insertion status is stored as a record with default values and index specific overrides, so the number of indexes changing is not a problem provided they use the defaults

#

e.g. have a record HandlerPermissions(boolean anyExtract, boolean anyInsert, Permission defaultPermission, Map<Integer,Permission> slotPermissions) sort of thing, where Permission can be insert, extract, both, or neither. Booleans there can easily be computed by just iterating slotPermissions/checking defaultPermission

#

key thing overall is I want to minimize the number of assumptions callers required to be making about handlers. Non-standard handlers are the whole reason the API exists and we don't just all have mutable List<FluidStack>

#

sure, slots are neat and all. But most handlers don't care about slots unless you are doing something very fancy. Usually they are more concerned with the standard "I have resource, you want resource" or "I want resource, you have resource?" style of interactions. I want to ensure people doing the latter with a predicate instead of a single resource are not forced to resort to slot indexes to accomplish that as it wouldn't be compatable with how I must implement my tanks from how this PR has been presented to me.

stone dragon
# wind steppe I guess the question is, how do most people use the storage capabilities. If the...

My take as somone with a storage-capability-using mod I have yet to port due to having some obnoxiously complex custom implementations -- I use both, but mostly my own implementations. That said, I would definitely love a transaction system or the like since it would make some wackiness I've ran into a bit less likely to cause issues. However, I also agree with KnightMiner that having containers have to be invalidated on size change is a no-go -- I have containers where transactions may change the container size and this would be a pain

median cedar
#

I feel as this has been forgotten a little bit, any new info about it? What does the neo team think right now about it

void thicket
#

I've always thought this level of refactor was overengineering and nothing short of implementing transactions will fix the real issues people have™️

But nobody really wants to rewrite their code to support transactions, lol

stone dragon
void thicket
#

I think we could get there at some point, there's just been so much "rewrite everything" recently that more churn isn't likely to go over well

stone dragon
void thicket
stone dragon
#

Fair enough, I suppose? Though this doesn't have to be a short-term thing

#

I would definitely say that -- personally -- I think it would be awesome to have transactions as a long-term goal in some form

flat root
#

My current thinking is to write a form to gather feedback on the current APIs

echo saddleBOT
#
technici4n

[Reference to](#1183818213134446742 message) #1183818213134446742 [➤ ](#1183818213134446742 message)perhaps yes - I feel like it's really hard to judge

vyrek. referenced ·
median cedar
#

i think it would be cool to have a poll

stone dragon
#

That's fair -- knowing how people use stuff and what the bottlenecks are exactly is always wise

ivory mauve
#

Minimize the number of instances of objects created; use as many integers as possible during transfer

#

Combined with just unifying transfer assumptions for different resources

#

It's not going to fix every possible mistake, but minimizing mutable parameters to things minimizes a common risk point

flat root
#

We have been discussing this a bit internally and we're not sure what to do. So we'll play around with the options.

Minimum commits to show what transactions could look like. See https://github.com/neoforged/NeoForge/pull/2270. I just pushed:

  1. a setup commit introducing what the API could look like with simulations + the transaction manager
  2. an outline of converting ItemStackHandler to a transaction-based APIs: https://github.com/neoforged/NeoForge/pull/2270/commits/4e74faae1defbcab1a8f063f011a90174bd18b3b

cc @cunning mulch in particular

cunning mulch
#

I will try to take a look at it later/sometime in the next couple days. Only initial comment is that updating our file that makes hoppers support ItemHandlers, might be useful as an example of what changes for usage there is (rather than just the commit linked above which shows changes for implementation of the handler itself)

flat root
#

fair

#

one thing here is that it might be hard to IItemHandler -> Storage changes from simulation -> transaction changes unless I do both separately

cunning mulch
#

do you mean hard in terms of having it in a clear individual commit? Or do you mean without making a bunch of other changes to things?

flat root
#

Yeah as a clear commit

wind steppe
#

wait you actually want to go ahead with transactions?

#

I mean im all for it but I thought I was in the minority here

cunning mulch
cunning mulch
flat root
# cunning mulch I will try to take a look at it later/sometime in the next couple days. Only ini...

added some "made up" examples, see ItemHelper here: https://github.com/neoforged/NeoForge/commit/1c990738d2d7549b36c781cc9d75617cc79c3aaa

the examples are:

  • add up to 10 apples and return how much was inserted (i.e. simple non-simulated insert)
  • implement insertItem with the signature from IItemHandler
  • a coalToDiamonds which tries to both extract 16 coal and insert 1 diamond

each example has both a transaction and a simulation version

GitHub
Add some usage examples · neoforged/NeoForge@1c99073
cunning mulch
#

I will try to take a look at it this weekend

silent root
#

What if you did transactions like

-> Insert Item
-> Check Response
-> Confirm Transaction

public ItemTransaction insertItem(int slot, ItemStack stack);

var transaction = insertItem(...)
if (transaction.getRemainder().getCount() > 0) transaction.complete();
dire hazel
#

That removes one of the main benefits of transactions: the ability to group multiple actions into one atomic operation.

Your proposal also can't work well, because it'd require the changes to be rolled back within the insertItem method, as the transaction doesn't know when it fails.

The way fabric and the current proposal does transactions, is that almost all of state changes from insertion happen immediately, and are rolled back upon failure. This means that you can't extract the same resource twice through different paths in simulation.

cunning mulch
#

xsmh I accidentally commented on a commit instead of on the PR as a review because I have multiple tabs open and got which tab was which mixed up

loud stirrup
cunning mulch
#

because I deleted my comments to move them into my review!

#

but #neoforge shows them xlurk

cunning mulch
#

there you go @flat root an initial review of stuff. I will have to look again afterwards, but I suspect with at least some of the changes I suggested, it will lower the added complexity that transactions have over the current system. I can even see with those changes how it has a potential to on average for usage to be less complex than the current system is now.

Though I still need to do more evaluation at some point to see if there is a better way to handle things like the changes to ItemStackHandler to make them more transaction friendly implementation wise so that it is harder for people to screw up the implementation of handlers if they have their own custom ones (namely thinking about if we can streamline the handling of making sure people don't forget to be updating their snapshots

#

but overall I think there is hope of getting it to a point where in general the system will be simpler to do things with than the current one, without quite as much of the boilerplate that the MVP has being necessary.

flat root
#

Thanks a lot. I didn't have time to respond to all comments yet let alone write any code, however I don't think that complexity is getting reduced

#

Since it seems to me that only changing how you open the transaction as well as adding some helper methods would be user-visible changes

#

The hard part is always making sure that the internal state of inventories is transactional, not so much the cosmetic aspects of the API imo

cunning mulch
#

mainly make it so that there are fewer calls needed by adding helpers

#

for users interacting with it

cunning mulch
loud stirrup
#

@flat root was there as specific reason it's rollback-by-default vs. commit-by-default-on-close ?

#

I don't remember, actually

flat root
#

It would also be weird to commit on exceptions? Although arguably exceptions will crash the game anyway

loud stirrup
#

and auto-rollback on exception

#

With the code construct used here that doesn't work, granted. 😄

flat root
#

This is also not an enterprise transaction 😄

#

But how does it work? Wrap in a lambda?

loud stirrup
#

No they're just method annotations that wrap the entire method in a transactional context

#

if the method throws, TX is rolled back, if it doesn't it commits.

#

at least that's what Spring does. But as I said, that doesn't really transfer. So yeah, the exception behavior kinda makes it necessary to explicitly commit.

flat root
#

I don't think it's a big deal but maybe people are being put-off by the try-with-resources + having to commit?

#

It's more verbose but I don't find it particularly hard, conceptually

loud stirrup
#

Lambda would be worse, IMHO.

#

Method annotation wrappers are a shitton of magic and I'd stay away from these too

#

So, it's probably ok.

#

@flat root do we require people to explicitly commit or rollback so that missing it results in an exception?

#

I don't remember

flat root
#

No, doing nothing results in a rollback

loud stirrup
#

Might be worth considering since right now, it's very easy to miss and then wonder why nothing's happening

#

Although it does make it more verbose overall, yeah

cunning mulch
loud stirrup
#

If you're just fire&forget insert into a single target and just wanna know the left-over, I'd agree

#

that doesnt need to be a try-with-resource and commit to do that

dire hazel
#

There should probably be some util class for transfer and simple insert/extract operations

flat root
#

I disagree with any helper that will commit the transaction for you

#

But I can definitely see a non-transactional function making sense

loud stirrup
#

The helper opens the transaction itself

flat root
loud stirrup
#

it should contain the try-with-resource + operation + commit and return the result of the operation

flat root
#

Yes

#

I wonder if it would be best in the Storage class or in another one as a helper

loud stirrup
#

I do think not all mods are pipe mods, so you'll have a bunch of interactions with IItemHandler that just wanna shove something in there without any simulation.

#

It could be a static on Storage, I wouldn't make that an Instance method, too much trouble if someone overrides it

flat root
#

I think we should provide a helper class with the same signature as the current iitemhandler methods tbh

#

That will make transitioning to this API much nicer

loud stirrup
#

Yes indeed

flat root
#

What I have in mind for the migration story is that we keep IItemHandler and all its implementations, but deprecate them for removal

#

The IItemHandler capability however we will remove entirely

#

And I would provide a temporary helper that wraps a Storage<ItemVariant> as an item handler?

loud stirrup
#

I just had that thought... can you implement IItemHandler on top of the transactional API by treating every method invocation as a standalone TX?

#

It'll be a problem if someone recursively tries to invoke an IIH from a transactional implementation

flat root
#

Yes that's exactly how you do it

loud stirrup
#

So hm. Make it sealed? 😄

#

You only get to use it as a consumer

#

Doesn't really help though 🤔

flat root
#

Yeah idk

#

Realistically we need some help with migration

#

Assuming we make it on time for 1.22, we'll have some of these migration helpers for the entire 1.22 lifecycle

#

(i.e. for the one 1.22 version that people will actually support)

loud stirrup
#

code mod, code mod, code mod!

#

(last time I tried openrewrite it sucked and was unsuitable for our purposes, sadly)

flat root
#

So what's the plan? Address the comments that can be addressed now and start fleshing out the whole API?

loud stirrup
#

I'd say so

#

ask ppl again to look at the usage examples in particular

flat root
#

And discuss whether they're good/bad

loud stirrup
#

@people lol 😄

distant merlin
#

ya gotta do a proper ping kek

loud stirrup
#

yes but at whom! people might not be in here 😅

#

So. How does a @here ping work?

distant merlin
#

most of what I search online says it'll only ping the people in the thread

#

but I am still nervous kek

copper fjord
#

it pings online people in the channel

distant merlin
#

i mean, I was concerned whether it'd ping the whole server or just in the thread

flat root
distant merlin
#

I'm so tempted to @here

flat root
#

Then go for it

#

Else I will

distant merlin
#

...safe

old meadow
paper merlin
#

looks good

loud stirrup
#

In a thread it only pings "participants", I think? But in any public channel, would it be the entire server? (Or only the entire server that's online, I guess)

flat root
#

So the goal is for people to comment on what transactions actually look like

#

At least with the design I am proposing

#

The linked commit shows a few examples comparing both options (simulations and transactions), please comment on the API

distant merlin
stone dragon
#

Ahh yeah I see a usage example where you have multiple operations. I like it

#

Mostly because you avoid the issue with the simulate system when you have a secondary operation fail but the first part succeeded or whatever -- see the ```java
// TODO: Here something failed and we need to give the coal back if possible, or buffer it...

flat root
#

For the transactions at least

past lodge
#

The switch to fabric-like transactions seems pretty good

fossil cave
#

I wonder how the API will differ and what the reasons for those differences will be

past lodge
#

it would be nice (more so for less experienced developers) if there was also something simplier ontop that could be used if you don't need to do anything too complex

wind steppe
#

i think the idea is that less experience devs would just use the templates

#

like, ItemStackHandler

silent root
#

Is the idea of this system to have a "commit" history of the entire storage of something like a chest?

sinful tiger
#

if the intermediate state is in the real world [i forget if that's the plan or not] then i'd be concerned about the possibility rollback-by-default meaning it gets rolled back at an indeterminate future time if the transaction leaks

sinful tiger
sinful tiger
#

given the single-threaded nature of minecraft, waiting isn't a solution, so we'd need to throw an exception

wind steppe
#

pretty sure fabric just crashes the game

past lodge
silent root
#

Ahhh

loud stirrup
# silent root Is the idea of this system to have a "commit" history of the entire storage of s...

No, it keeps snapshots of an inventory (or machines) internal initial state while the transaction is ongoing and either reverts to that when you roll-back or throws it away when you commit. Imagine a machine that has 2 inputs slots, but you can only insert into one, or the other.
Without transactions, you'll simulate insert into slot 1, then simulate insert into slot 2, and both will succeed.
With transactions, you'll see after inserting into slot 1 that your insert into slot 2 fails, and then you can roll it back.

stone dragon
#

It's great, especially for situations where inserting or removing an item can have wacky effects that would prevent other insertions that would have been fine before (the case in question for me)

flat root
#

What's with the non-indexed storages? Just make an index system by resource if needed

#

I'm not sure you can make an equivalent to getUnderlyingView() with a slotted system, but I also don't think that it's a problem

loud stirrup
#

Emulating an index based system when you just index by resource is expensive

#

HOWEVER; mods already have to have that code now.

flat root
#

It's not expensive if you're iterating over the inv anyway

#

It's expensive if you already know what you're extracting, but that's covered by slotless extraction methods

loud stirrup
#

I mean if the implementation has that

#

I was refering to the implementation having to provide indexed access

#

ultimately yes, you can site-step this by also maintaining an ordered list of keys separately and making sure it's synchronized, then you can use that list to satisfy the index access.

flat root
#

It's a bit of effort but the upside is that you get pretty good performance I would say

loud stirrup
#

Well yes, my point is also that mods already have to do this now, so migrating over to this new system shouldn't incur additional implementation effort, right?

flat root
#

Not on that front at least

vernal dust
#

Would it make sense to borrow AE's hashing algorithms for Neo?

#

To use as keys/indexes

flat root
#

I don't think we do anything special there, outside of sorting the hash inside the "resource" (AEKey)

loud stirrup
#

Do we no longer intern the hash? I don't remember

flat root
#

Ah right. Not sure after the data componentening

cunning mulch
flat root
#

Yeah that's what I meant

#

The resource <-> index mapping can be handled internally by the "non-slotted" storage

cunning mulch
#

yeah I am just agreeing with you

flat root
#

this is just a port of what is in Fabric... I am not sure how helpful some of these are

#

e.g. StorageUtil.simulateInsert(storage, slot, resource, maxAmount, null) is quite verbose 😄

#

some are quite powerful though, e.g. StorageUtil.move(from, to, iv -> true, 10, null) will move (up to) 10 items of any kind from one storage to another one

flat root
#

wouldn't it be nice if insert with a null transaction just opened a transaction, did the insertion as usual, and then committed the transaction 🤔

#

not sure that's feasible in practice though

loud stirrup
flat root
#

We can but I'm worried about putting even more stuff in it

loud stirrup
#

Sure, but discoverability is certainly easier if you're looking at Storage. I guess in other areas we've already tended to use separate util-classes (FluidHandler stuff had utils externally IIRC), so I suppose having it separate is consistent with existing design.

past lodge
#

porting lib has a number of helpful methods for transactions that might be worth looking at

flat root
#

Last time I had a look I wasn't convinced. Do you have specific examples in mind?

loud stirrup
#

Hm, a defaulted contains(Resource), default count(Resource) might be interesting, tbh. that's independent of transactions though and goes more into the slotted vs. non-slotted thing (and underlying optimization)

flat root
#

We can add more defaulted functions but I'm really worried about ending up with 20 of them

past lodge
#

a lot aren't needed but there are some neat ones

#

especially getTransaction

loud stirrup
#

That's likely the criteria

flat root
#

Even then, I would be very careful

flat root
loud stirrup
flat root
#

Yeah that's what I have in mind. Mostly utils and maybe promote some later

loud stirrup
#

I do think there's potential for quite a few nice helpers though

#

Counting items, counting items up until a threshold, etc. etc (or canExtractAtLeast(Resource of, long amt))

#

Hm however, should be considered if those helpers would actually need to be used inside of a transaction for their actual purpose, which reduces the need for an version that includes the TX logic

past lodge
#

there are some good helpers and some bad helpers

#

most of these need to be reviewed to see how many mods will actually use them and if they are actually good

#

create fabric is the good example for that since everything that it uses transfer api for is a direct port from the existing forge code

loud stirrup
#

ya with porting an existing mod you always have the problem of not being able to pass through the Transaction easily without changing a lot of methods

#

We dont necessarily have that problem anymore if we have the same on NF, you can just pass them through

#

(I ran into that on AE2 as well and IIRC I also played with thread local TX objects at some point)

flat root
#

It just occured to me that the PR should probably be worked on in 1.21.1

#

Otherwise testing it is really difficult for most of us

cunning mulch
#

yeah that might make sense tbh, given how in theory the majority of the PR is kind of abstracted away from super directly interacting with MC so rebasing it shouldn't™️ be that hard

flat root
#

yeah exactly

#

I don't want to port MI to 1.21.5/6 just to try this out and I'm sure you feel the same with Mek

#

AE2 is kinda ported, but it would be nicer to be able to test the PR on a few 1.21.1 mods and see how things work out (and also if they work together)

past lodge
#

i'd be willing to attempt to port create to this, there's some groundwork already with how create fabric uses the transfer api

loud stirrup
#

Yeah 1.21.1 is probably a good idea

flat root
#

found some funny archives 😄

image.png image.png image.png
flat root
#

ok the PR is now on 1.21.1

#

this is good because we can now focus on getting something testable ASAP and then fill out the extra details later

loud stirrup
#

you should probably pin it separately (the PR link), since the only pinned thing is a direct commit link, which makes it a bit annoying to get

clear tusk
#

also looks like you need to run applyAllFormatting so that the pr can actually build and publish for pr-publishing

flat root
#

it's still a bit early for publishing, no?

clear tusk
#

oh i just saw you toggled it on

flat root
#

ah right I did; still, I think the PR is not usable yet

flat root
#

now let's see how bad the transactional Container wrapper will get

#

ok, pretty bad 😄

loud stirrup
#

BTW should we clarify what a storage should do when the slot index is out of range?

#

error? return blank sources/noop?

#

or undefined behavior?

cursive geyser
#

I think defined is best if it's possible to define something

#

if that is an error or a noop 🤷‍♂️

#

people should range-check their inventory usage

loud stirrup
#

Yeah I bring that up because our cauldron wrapper doesn't 😄

#

It just ignores the tank completely and still operates on the cauldron

cursive geyser
#

ew

#

that's not nice

#

we should at least TRY to be consistent smh

#

even if we give both as options, it should still be either noop or error, never using the contents of a different slot

sinful tiger
#

fwiw one of the mods i'm working on has an item-based handler that has multiple tanks of different fluids that are all entangled together [e.g. all registered xp or potion fluids] such that emptying one empties them all.

#

mine's not precisely an 'oredict converter' [it's an attempt to make bottles actual fluid handlers without making any other compromises] but the same concept could apply to one

#

so 'operations on one slot affecting another slot' is arguably legitimate

loud stirrup
#

Uh, that's not what we're talking about 😛

#

Ghz means that for an inventory with a single slot, accessing slot 123 shouldn't operate on that one slot

sinful tiger
#

regardless, i don't think it's necessary to specify beyond "shouldn't be an error"

#

as long as extracting from slot 123 doesn't cause a dupe glitch i don't see the problem

loud stirrup
#

Well that's the case where the API just says it's undefined behavior, do whatever

sinful tiger
#

for item handlers, the ability to e.g. make a gui with 27 slots bound to a container which may have fewer [or which may change its size dynamically] without causing exceptions i think is important

loud stirrup
#

I still don't think you understand the point we're making 😄

sinful tiger
#

we could mandate 'treat out of range slots as always-empty'

loud stirrup
#

If your IItemHandler has size() == 1

#

what happens if you insert(1, ...)

#

or insert(123, ...)

sinful tiger
#

I am advocating that whatever happens should not be an error.

#

either it does nothing or it does something is fine

#

i think we want to permit implementations that do not check the slot number at all for one logical slot, and also implementations that change their size dynamically and something accessing them may not be aware the size have changed.

loud stirrup
#

@flat root So my 2 cents: readSnapshot is kinda meh in terms of naming. Isn't it revertToSnapshot?
Also: in your cauldron implementation, you remember the last state released by releaseSnapshot and consider that the original blockstate before modifications were made in the TX. To be honest, from reading the Javadoc I would have never gotten that.

cursive geyser
# sinful tiger I am advocating that whatever happens should not be an error.

I partially disagree. Accessing slot numbers outside 0..size() should either be a hard error or no-op, but never undefined. I get not wanting errors at runtime as a player, but as a dev I rather get hard errors with clear explanations than have no error and cause data loss that the user doesn't know about and doesn't know to report.

drifting wolf
#

my 2 cents would be that index based access with size limit should throw ArrayIndexOutOfBoundsException, to be consistent with default java index access design
(especially in-dev, in prod doing noop might make more sense, as some ppl tend to not interrupt UX where possible)

loud stirrup
#

I mean the problem has been a problem since... forever?

#

I don't think IItemHandler specified what should happen

#

How the hell do I register a generic capability interface 😅

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

We could give the original state in onFinalCommit if that helps

loud stirrup
flat root
#

Wtf dude

#

😄

loud stirrup
#

Eh, wrong reply

loud stirrup
#

As in, it's actually easily done and allows for safety of two mods registering the same cap for List<T> vs. List<R>, in principle

flat root
#

Please don't do it 😛

#

You could have checked Fabric, it has the same problem and solution

loud stirrup
#

You realize it's not actually safe to do that, yes? 😄

#

I can live with it for our own storage caps, since no one else is going to register those but you should at least consider changing the caps system to something that actually does support generic signatures 😅

#

It didn't seem that involved, honestly.

flat root
flat root
loud stirrup
#

You don't actually remove the existing methods, you just add overloads with TypeToken<T> (we just reuse the one from guava :-P)

anyway. different topic, really, but should at least be considered at some point.

flat root
#

I dislike it out of principle

#

Doesn't mean we shouldn't do it, but my (somewhat uninformed) opinion is that it hasn't been necessary in the past

loud stirrup
#

It does allow us to distinguish between two mods trying to register a Storage of different content types under the same name.

Sure you can say this is "never" going to happen, but the safety is nice, still. And TBH. All the heavy lifting is doine by Guava in this case. We can think about it later, it has little to do with the transfer experiments, anyway.

#

The generic interface just causes us to run into it

flat root
#

It's just not something I would usually write. But maybe it would be a good thing to do

#

In any case if we do this we'll split it in another PR and get that merged first

loud stirrup
#

Yes, definitely

void thicket
#

out of bounds slot indicies should throw

flat root
#

I agree but we should think for a second how dynamic handlers work

#

These will need to be a bit more lenient by necessity

loud stirrup
#

There's a case to be made for them to behave as if empty

#

(and readonly)

flat root
#

(i.e. if you suddenly go from 10 to 8 slots, a query at position 8 or 9 should not throw but probably just do nothing)

loud stirrup
#

But in any case, it'd be nice if we specified expected behavior

sinful tiger
#

like the solution to "I rather get hard errors with clear explanations than have no error and cause data loss that the user doesn't know about and doesn't know to report. " is simply not cause data loss

#

like what does it matter if inserting into a trash can or extracting from an infinite water source ignores the slot number

sinful tiger
#

anyway my point is there are reasonable behaviors other than no-op - and having all slot numbers alias the same real slot [whether that is a trash slot or an infinite source slot or something else] is just the most obvious one.

flat root
#

all slot numbers aliasing the same slot is probably the worst thing you can do 😛

#

it feels very unexpected to me at least, and can easily lead to "duplicate" transfer

loud stirrup
loud stirrup
flat root
#

did I do that? probably

#

😔

loud stirrup
#

That's why I even brought it up lol

#

BTW: what is the semantic of passing blank variants to extract/insert?

#

Should that crash?

sinful tiger
#

like, proxies are legal, and a 'proxy' that combines two inventories [themselves possibly proxies to the same inventory] are legal, so we already have to deal with actual aliasing in non out of bounds slots

#

compacting drawers is another case where extracting from one slot has the side effect of reducing the count of other slots, or vice versa

flat root
#

I'm in favor of crashing

loud stirrup
#

Since we only return the amount, we can't emulate the previous IFluidHandler#drain that didn't take a slot or resource anyway

#

So I am fine with crashing, I guess

flat root
#

well no, the alternative is to return 0

#

but I think that usually inserts with negative stack sizes or blank variants are a bug

loud stirrup
#

Yes, I know, but that still wouldn't emulate the old method (since it obviously can't)

flat root
#

which one? the way you did it seemed fine to me

loud stirrup
#

Eh just ignore me, it was just a thought I had when I implemented it 😛

past lodge
#

mods mods definitely should be making sure they are accessing slots that exist, having undefined behaviour for this wouldn't be a good idea

flat root
#

then it should probably throw to make sure that mods who perform out-of-boudns access get issue reports

past lodge
#

yeah

#

also readSnapshot sounds a bit weird

#

something like rollback would sound better

flat root
#

#1183818213134446742 message hehe

echo saddleBOT
#
gimpansor

[Reference to](#1183818213134446742 message) #1183818213134446742 [➤ ](#1183818213134446742 message)@flat root So my 2 cents: readSnapshot is kinda meh in terms of naming. Isn't it revertToSnapshot?
Also: in your cauldron implementation, you remember the last state released by releaseSnapshot and consider that the original blockstate before modifications were made in the TX. To be honest, from reading the Javadoc I would have never gotten that.

technici4n referenced ·
flat root
#

revertToSnapshot sounds good to me

past lodge
#

alright

#

is the PR in a state where mods can play around with it or is there still more work needing to be done before that?

flat root
#

there's still more work I think

#

but thanks to shartte's help it's going faster than if I were alone working on it

#

we haven't figured out the IItemHandler migration story yet, but that can probably come later

#

fluid-containing items are still not done at all (ugh)

past lodge
#

good to hear, i'll have some more time during the summer so i'll look into playing around with the changes and make a few notes/reviews of my own about the changes

past lodge
flat root
#

you mean with setStack?

#

yeah well, how many times will I have to explain that this is a bad idea idk 😄

#

should the Item be changed, the initial stack will be cleared entirely

#

ok it looks like you copy the stack back over, that's good

#

@loud stirrup re empty storage, if you make it always have 0 slots it can simply throw in almost every method

#

which means you can also remove the annoying blank variant and ITEM and FLUID versions

#

that would be a good argument to make OOB access throw imo 😄

loud stirrup
#

Well yes, I had that exact thought lol 😄

#

"if this was allowed to throw, I could use the same object for every type of storage!" 😄

#

It's a pretty weak argument though

flat root
#

I think that there should not be item- and fluid-specific code in the base Storage classes

#

otherwise it means that the API is not maximally generic, given that we had to special case some types

loud stirrup
#

Well, I could just make it abstract + template-method style instead

#

and EmptyFluidStorage extends EmptyStorage / same for items

#

and put those in the other packages

flat root
#

Fabric has a simple Storage.empty() method since there the implementation of the empty storage manages to be generic

loud stirrup
#

They throw then, I'd wager?

flat root
#

it simply has no StorageView

loud stirrup
#

Ah no it's due to the storage views, yes

flat root
#

tradeoffs! The storageview concept is not bad, but overall not necessarily better

#

(e.g. the filter wrapper has to wrap every single storage view 😭)

loud stirrup
#

But I do generally agree with the thought of keeping storage free of references to concrete variant types

flat root
loud stirrup
#

On the case of accessing the "first" (original?) snapshot on final commit

#

Since it requires storing the snapshot, we might make it a special snapshot variant subclass that offers it?

flat root
#

we could simply always offer it to onFinalCommit as a parameter

#

it just means that it needs to be released at a different timing

#

e.g. protected void onFinalCommit(T stateBeforeTransaction)

loud stirrup
#

I haven't quite figured out when the release call is made. If that's possible then yeah that'd be a lot nicer.

#

API wise it seemed a bit odd to "abuse" the release call for this since the javadoc says absolutly nothing about it's relationship with the transaction order

#

VoidFluidHandler doesn't seem terribly useful on its own, to be honest. At least not useful enough to be a public facing template

flat root
#

I would use it for my trash can 😄

silent root
#

Read a lot about the slotted stuff, would be dope if mods took advantage of that and incorporated it into pipes as an advanced setting 🤔

#

Would be amazing

flat root
silent root
#

Some mods use a SINGLE IFluidHandler thing, and have multiple tanks

#

Uhm, Industrial foregoing I think does it?

flat root
#

So, I need to add a few methods to Container to make the vanilla implementations fine to use in a transaction, with the following extra methods:

  • side-effect free version of setItem(int slot, ItemStack stack)
  • function to apply side effects after a transaction: onFinalCommit(int slot, ItemStack oldStack, ItemStack newStack)
  • function to do "stuff" after a successful transfer: afterTransfer(int slot, TransactionContext transaction). This is only used by chiseled bookshelves to correctly update their lastInteractedSlot (i.e. in a transactional way)

My question is: do we want to make these available to modders, or do we keep those purely as an impl detail for vanilla Containers and assume that modders with complex needs will stop using Container?

loud stirrup
#

Since it's an interface, you kinad need to allow modders to implement this

flat root
loud stirrup
#

That makes it a loooooooooooooot easier to understand whats going on

flat root
#

I agree; thanks for the feedback ^^

#

this one change already makes the transaction system a lot nicer than on fabric I would say heh

flat root
analog sapphire
#

hmm, I'm not sure how I feel about this API design

#

changing the meaning of the vanilla setItem only works if everything implementing Container is patched with that in mind

flat root
#

the default doesn't change

analog sapphire
#

oh I misread

#

nevermind

flat root
#

in practice, you can ignore the new overload unless you are trying to use ContainerStorage AND side effects are getting in the way

flat root
#

ok, not doing too bad on items I think; important next steps are:

  • fluid base implementations
  • fluid-containing items
loud stirrup
#

I can commit the bit of prototyping I did (not much), I did help out on the pre-release though

flat root
#

I think the pre-release is the priority

#

I don't think I'll be working on this over the weekend anyway, so there is no rush to push your stuff 😄

loud stirrup
#

@flat root now back on transfer

#

Thoughts on class InvalidSlotException extends IndexOutOfBoundsException to fully specify that this should be thrown when the slot is out of bounds?

#

MIght not really be an improvement, but might also make testing easier

loud stirrup
#

Eh... probably no real point in introducing it

flat root
#

Idk, IOOB is already descriptive enough, no?

hidden geode
#

Normally yes.....
But the question is does it make sense to include other information in the exception that would allow a caller to recover easier. Things like the transaction or inventory information?

flat root
#

I think slot index and size of the inventory are the most important

#

From the stack trace you typically know which mod messed up 😄

hidden geode
#

Sure from the stack trace yes.
But my point is a special exception here could be usefull in a Catch block to handle errors better.

So in the "we do not crash the game" scenario.

If we crash the game then an IndexOutOfBounds is nice enough yeah

flat root
#

We'd crash the game usually

loud stirrup
#

You're using IllegalArgumentException 😄

flat root
#

Ah oops

loud stirrup
#

It's fine

flat root
#

Even the StoragePreconditions helper?

loud stirrup
#

It's not really a recoverable error, imho

#

Yes

flat root
#

But maybe some state got inconsistent, idk

loud stirrup
#

Well, what I mean is: it doesn't matter which precondition you fail to satisfy, no?

flat root
#

Yeah you should not do it

#

If you really want to "catch" OOB access just do a check before calling functions on the storage

loud stirrup
#

yup, exactly

loud stirrup
#

I'll move the description of the slot bounds to the class-level doc of Storage

#

since it's a concern across many methods

loud stirrup
#

In terms of docs. "(or would have been, if simulated) "

#

That's not really a concept we explain in the docs anymore, right?

flat root
#

Yup

#

Docs are a little outdated

flat root
#

I'm not convinced by the stack size handling of the bucket storage impl

#

@cunning mulch what are your thoughts on handling item capabilities for a stack of size > 1?

#

It's fairly easy to create a wrapper context that exposes a single stack at the time

#

@loud stirrup your bucket storage impl is a bit inconsistent: it exposes the amount and capacity of a single bucket, but still allows I/O with multiple of them

cunning mulch
#

Or more accurately I guess try to split between?

flat root
#

The way it works on Fabric is that stack size is ignored, and a single item is processed at the time. This means that the behavior of "and stow" happens automatically

cunning mulch
#

I am confused

#

so where does the data get saved when it is still in the process of being filled so not moved to an output slot yet?

#

or is there a third slot that then things get moved to

flat root
#

The storage impl will typically use exchange to swap a single item

#

Which will:

  1. Extract from the current slot
  2. Insert into the current slot
  3. Any leftover gets sent into the player inventory
cunning mulch
#

I am talking about when there isn't a player

#

say a tank filling items that get put inside of it

flat root
#

Then it's up to the context to handle it. (The filling machine provides the context) I suppose in that case it would either fill in place (so any stack of size > 1 will fail to exchange and nothing will happen), or move to a second slot (so stacks will be exchanged from the input to the output)

#

From the POV of the storage, ctx.exchange is called and if that succeeds the transfer is successful. If it doesn't, the transfer is not successful

cunning mulch
#

and in the case you don't want to move to "output" until it is done filling, need a third intermediary slot to hold the partial filled one in until more fluid is available to finish filling it

flat root
#

Ah yes exactly. Or continue filling "in place" and then handle moving out of the "working slot" separately

cunning mulch
#

tbh part of me wonders if we should leave it up to the item more, and provide a way for the item/cap to say that it only will function at x items (when x <= current), that way the inventory only accepts that many and then just acts upon it as normal

flat root
#

What I want to avoid is making the system underspecified such that everyone has to copy the stack with a size of 1 and handle stowing manually (i.e. the current status quo)

#

But I didn't quite understand what you meant

#

I think from the POV of the storage impl the only thing that matters is "empty bucket gets replaced by full bucket", no? And we leave it up to the context to handle that conversion?

cunning mulch
cunning mulch
flat root
#

The question is then whether the storage impl should even look at the current amount or just pretend it's 1

cunning mulch
#

storage impl being the tank that is filling the buckets or the buckets themselves?

flat root
#

Oops I mean the buckets

cunning mulch
#

just pretending it is 1 feels iffy

#

and like it might make it easy to introduce bugs?

#

though unsure

loud stirrup
#

Buuuuut, that is not inconsistent 😄

#

My next stop would actually have been implementing tests for it

flat root
#

I would consider removing getAmount from the context entirely, btw

loud stirrup
#

Hm, isn't that even worse?

flat root
#

On fabric you typically always exchange a single item anyway

#

The biggest annoyance would be emptying an entire stack, because you'd have to loop over the extract method until the stack is empty

loud stirrup
#

Well, if you grab a storage for a stack of empty buckets. does that have capacity 64B or 1B 🤔

flat root
#

On Fabric 1B

#

And you can only insert 1B at the time

loud stirrup
#

And you're moving out the filled buckets as overflow, right?

flat root
#

I would say that's how inventory interactions usually work on forge as well

loud stirrup
#

if the underlying amount is > 1

flat root
loud stirrup
#

let's call it bucket count

#

You could insert 64B at once using this handler, though 🤔

flat root
#

Sure but I am more concerned about the default behavior when you're getting started with the API

#

I claim that in most cases you would probably want to be filling one bucket per player click

loud stirrup
#

Well, knowing that you can iteratively fill it is as unintuitive as you claim knowing how to fil lit one-by-one is 😄

#

We should think about this some more, I guess

#

Ultimately you have storages that have a minimal insertion/extraction amount (or discrete?)

#

But others don't

#

We may actually wan to formalize this

flat root
#

I think that removing getAmount from the context saves every implementor from having to think about this problem 😄

loud stirrup
#

examples are cauldron, bucket

#

there are probably others

flat root
#

"bucket count is ignored by getAmount and getCapacity, but taken into account by insert and extract" seems quite confusing to me

#

Whereas if you don't expose the "bucket count" at all it's impossible to get it wrong

loud stirrup
#

You shouldn't consider this fully implemented yet 😄

flat root
#

I am thinking ahead

loud stirrup
#

Well yes, but assuming you do return FluidType.BUCKET_VOLUME * context.getCurrentAmount()

flat root
#

Unless you have a brilliant idea that you haven't presented to me yet? 😛

loud stirrup
#

it'd report 64B of capacity

flat root
#

Yes

loud stirrup
#

The problem that exposes is that the API doesn't allow the storage to communicate there's a discrete amount for insertion or extraction

flat root
#

That's a bit odd, but it's consistent with how much it can accept on insertion

loud stirrup
#

which you'd not only use for cauldrons but also for UI interactions

flat root
#

But why does that matter?

loud stirrup
#

I don't think it's any more odd than the Fabric behavior. You can easily argue if you're holding 64 (hypothetical stackable) filled buckets you are holding 64B of that fluid

#

😄

flat root
#

I'd expect a stack of "continuous" tanks to behave the same as a stack of "discrete" buckets

loud stirrup
#

You're refering back to the behavior of UIs that would iterate

#

you don't really know what to iterate with 😄

#

And no, the fact is that you can only extract in increments of 1B

#

regardless of amount

#

Which is what I mean by discrete

#

Same for cauldrons

flat root
#

I know but I don't see the relevance

loud stirrup
#

The relevance is that an API consumer cannot know how to extract from it properly?

flat root
#

Why not? Just extract however much you want and it will return how much it managed

#

If you try to extract less than a bucket, then 0 is returned

#

I would say that's fair game. You might want to buffer extract capacity across ticks to make sure you'll eventually empty the bucket though. (MD does this)

loud stirrup
#

Eh... I don't know

#

The downside I can see with exposing the entirety of the stack is the fact that a UI would need to know what to do when you try to "empty" a bucket that way

#

but the same applies to holding a 64B mekanism tank. Would you want to empty that all at once or not

#

no idea

#

but oh well, i dont see why we'd remove the amount from the context when ignoring it is enough to do what you want to do for that particular implementation 😛

#

doesn't mean all implementations have to work like that

flat root
#

I claim that all implementations should work in the same way

#

At least for the standard capabilities

#

Because when you use the API by grabbing a storage from the capability you want the behavior to be predictable

loud stirrup
#

Well, it's arcane 😄

#

Predictably arcane

flat root
#

If it's undefined behavior it just sucks for everyone

#

You'll end up always using a context with a bucket count of 1 to avoid the undefined behavior

#

It's precisely the status quo and I don't like it 😛

#

(why would we let tanks from mekanism and MI behave differently by default? Players notice these inconsistencies)

loud stirrup
#

your approach isn't written in the spec either

flat root
#

If you don't get a bucket count you can't misuse it

#

I don't think "misuse" is quite correct... Rather you don't get to use it inconsistently

loud stirrup
#

You want to limit what a storage cap can do for a stack of items

#

There's no consistency to begin with!

#

That's why I am so confused 😄

#

You have to adhere to the Storage api for the cap you return and that's it, really.

#

As a consumer you do not know it behaves like you describe

#

or whether it behaves like I describe

#

since practically you can't know that you had a bucket to begin with

#

unless you special case for it which you shouldn't do either...

flat root
#

Since we're writing an API we can choose which contract implementations have to adhere to

#

But in any case I don't think there is much point in discussing this further. I suspect that my point doesn't quite get across, and it would be easier to reason about code

#

So I'll make the change I have in mind tomorrow and we'll see how it turns out in practice (for energy I suspect we'll want a way to fill up an entire stack at once, so it will be interesting to see how that turns out)

loud stirrup
#

Ahem that is quite heavy handed 😛

#
  1. which contract are you even talking about specifically. Storage?
#
  1. even on Fabric you have access to the amount, just in a convoluted way.
flat root
loud stirrup
#

But those are still just Storage<ItemVariant>

flat root
loud stirrup
#

And my process is that if you have an item that naturally can work with an amount multiplier

#

we shouldn't prevent that

flat root
loud stirrup
#

i.e. if it's an item that simply evaporates when you extract from it

#

can trivially be made extractable to times its stacked amount

flat root
#

Every storage that I know of except the ender ones can somewhat trivially handle the bucket count

loud stirrup
#

okay wait we need to be precise here 😄

#

when you say storage, what do you mean exactly?

#

the in-item-storage capability implementation for Storage<T>?

flat root
loud stirrup
#

Okay sure, but in those cases, they can chose to ignore it, or can they can work with it? I don't see the harm in giving them access to the stack amount since they have to more or less interact with it through exchange/extract/insert anyway

#

if they just assume it is alyways count=1, it will behave "odd" for the consumer of their Storage implementation (odd in the sense that it behaves as weird as Fabrics)

#

but the implementation would be correct in terms of the Storage interface

#

on the other hand, the same is true if they do use the stack size

cunning mulch
loud stirrup
#

that's also what I meant by discrete transfer amount

#

but are you talking transfer size or size of the itemstack you're querying the cap for

cunning mulch
#

Size of itemstack you are querying the cap for

#

So that then inventories can only accept the amount of items into the slot that it will still be able to act upon

loud stirrup
#

Ah, the Fabric impl will work for any size of stack

#

It's just that only the content of one stack are visible at most via the Storage interface

#

when you extract from it, if there was more than one item in the underlying stack, it'll still show the content of the second one

cunning mulch
#

yeah, so as I said earlier to tech, basically just requires inventories to have a semi third slot for what is currently being acted upon

#

but that is probably fine(?)

loud stirrup
#

Was it always open(@Nullable parent) for opening a transaction? Weird. I remember there being an openOuter/openNested but I may be hallucinating that 😄

#

Wait. No. The example code actually still has that pattern 😛

 * try (Transaction outerTransaction = Transaction.openOuter()) {
 *     // (A) some transaction operations
 *     try (Transaction nestedTransaction = outerTransaction.openNested()) {
past lodge
#

open(null) feels a bit weird, openOuter and openNested feel a bit more natural

cunning mulch
#

as it had a param

flat root
#

There were 2 versions of openNested

cunning mulch
#

Yeah, I think a single one is just clearer overall for intent when it comes to nested ones being involved

#

Granted that is what I said initially too and why you changed at least one of the two

flat root
#

Which of the following cases are reasonable?

  1. Add 3500 mb to 4x 1 empty bucket -> Fill 3 buckets
  2. Add 3500 mb to 4x 1 empty bucket -> Fill 1 bucket
  3. Add 3500 mb to 4x a "continuous" tank that can hold any amount between 0 and 1000 -> Fill 1 tank with 1000 mb
  4. Same as 3. -> Fill the 4 tanks with 875 mb each
  5. Same as 3. -> Fill 3 tanks with 1000 mb each, and fill the last one with 500 mb
cursive geyser
#

4x here means there's 4 slots?

#

assuming you specify slots explicitly:

  1. doesn't seem reasonable because you specified 1 slot so the others shouldn't be affected
  2. does seem reasonable
  3. does seem reasonable
  4. does not seem reasonable
  5. does not seem reasonable
    however if the interface exposes only 1 slot, then
  6. does seem reasonable
  7. does not
  8. does not
  9. does
  10. does
    in the second case I would think 4 and 5 are both equally valid depending on implementation details
#

additional note: this I'm thinking of 1 insert operation from the point of view of API, not user action. ifit's user action then all of them are valid depending on implementation detail of the destination

loud stirrup
#

I don't think having to pass null to a method all the time is good

#

re: open(null)

#

Especially not since transaction context usually doesn't accept null when it's a method parameter in all other cases

cursive geyser
#

additional note 2: if a "helper" method of the kind insertAll(fluid) is what is being talked about, then the assessment for 1 slot applies except that I would expect 5 and never 4

flat root
flat root
cursive geyser
#

oh!

#

in that case I would expect it to fill items until it runs out

#

0->1000 for the first bucket/tank, then the second, third,

#

and finally buckets would reject the last 500 but not the tank

#

so 1 and 5

#

adding to the existing reaction votes

#

but

#

in my mind this would happen once per tick, so 1 bucket on tick 1 + 1 bucket on tick 2 + 1 bucket on tick 3 + leftover
however I assume we are talking about a transaction / result of an operation

#

so that becomes an implementation decision on the host: do they allow batch filling in a single tick or too cheaty?

flat root
#

I am talking about a single insert call to the capability returned by the bucket stack

cursive geyser
#

hm

#

in that case 2 and 3. a single insert on the bucket should never return more than 1 because the result isn't stackable

#

but the answer could be different for tanks or cans or such, because in some mods those are stackable

loud stirrup
loud stirrup
cursive geyser
#

(4x bucket).insert(3500) -> (1x filled bucket), 2500 leftover
(4x 1B sized can).insert(3500) -> (3x 1B sized can), 500 left over
(4x stackable 1B tank).insert(3500) -> (3x stackable 1B tank), 500 left over (because the leftover wouldn't stack)

#

IMO

queen wagon
#

I'd say a stack of multiple buckets should only accept anything if the result can move to another slot otherwise transfer 0

loud stirrup
#

Generally, we're "moving out" created items when they can't fit into the slot the storage containing the "container item"

flat root
#

Consider that the insert on the bucket is transparently modifying the rest of the inventory automatically. Multiple buckets can be converted at once if the capability implementation so desires

flat root
#

If you implement 1 or 5, you can get 2 or 3 by introducing a wrapper context that exposes a stack at the time

#

If you implement 2 or 3, you can get 1, 5, 6 by iterating

#

4 however is really hard to emulate IMO without making assumptions that will break with ender buckets

#

And for a stack of batteries I think 4. is the behavior you would want

#

And I probably managed to confuse everyone now? 😄

queen wagon
#

for anything but energy I'd say never do 4

flat root
#

Ideally you would always do the same, and have a wrapper that will implement 4. on top of whatever

queen wagon
#

yea

flat root
#

I think I need to write code for all these cases to see what such a wrapper will look like in practice, and if that can be implemented without making assumptions

#

The absolute worst case is a hybrid "ender battery" that has both "ender" capacity and "nbt" capacity

vernal dust
#

I think we need to consider the filled bucket scenario more. Filling a bucket doesn't just work on fluids, it works on item stacks too - so what if the BUCKETS are willing to be filled (fluids) but the ITEM storage cannot handle it (say a drawer with 1 slot)

#

You'd need to have the fluid handler interop with items, or have some kind of weird nested transaction

loud stirrup
#

That is already covered

#

That's pretty much why the context object giving access to where the bucket item is stored is only offering mutation access to the item using transactional APIs itself

#

If the exchange operation (transactional extract+insert) fails, i.e. due to unavailability of enough slots, the outer fluid insertion will also fail
(fail meaning it returns 0 for no amount extracted, or filled)

sinful tiger
#

....does that mean we need nested transactions? like what if someone wants to roll back their intended "fill a bucket in an inventory" operation without rolling back a larger operation that it is a part of?

#

you'd need to do that in the implementation of a fluid handler that wraps a bucket(s) in an arbitrary item handler wouldn't you?

#

[if you're doing it in an item handler you control you can at least precheck that the item operations will be valid]

flat root
#

Nested transactions are in the PR yes

#

They are not particularly complex (compared to a single level of transactions), yet very important to keep the API composable

loud stirrup
# sinful tiger ....does that mean we need nested transactions? like what if someone wants to ro...

Yes, the default implementation for exchange on the in-item-storage context uses a nested transaction

    default long exchange(ItemVariant newVariant, long maxAmount, TransactionContext transaction) {
        StoragePreconditions.notBlankNotNegative(newVariant, maxAmount);

        try (var nested = Transaction.open(transaction)) {
            long extracted = extract(getCurrent(), maxAmount, nested);

            if (insert(newVariant, extracted, nested) == extracted) {
                nested.commit();
                return extracted;
            }
        }

        return 0;
    }
flat root
#

I'm thinking it might make more sense to move the fluid-specific code to net.neoforged.neoforge.fluids and similarly for items

#

even though I like to have "everything" under net.neoforged.neoforge.transfer

loud stirrup
#

Pretty much should support best of both ideas (have to put it through some real-world testing, ofcourse)

loud stirrup
#

Oh and about this check: "if (!context.getCurrent().is(Tags.Items.BUCKETS_EMPTY)) {"
I think I copied that from the current handler, but is that really sensible?

#

We do not consider the specific empty bucket when we decide which item will be used for the filled item, so this is really a one-way transfer

flat root
#

yeah it's one-way and it only works for the standard bucket IMO

flat root
#

for items however all of net.neoforged.neoforge.items will be getting deprecated except for StackCopySlot

#

btw @loud stirrup should we already start deprecating item handlers or is it too early still?

#

btw this is really bad

image.png
#

we really need a unique wrapper instance per position since it stores transactional state

#

not a new one per query

#

ok fixed

loud stirrup
loud stirrup
flat root
#

I think it's not a problem for the fluids package

#

I guess we'll do it for items too heh

loud stirrup
flat root
flat root
#

alright, ported over the composter wrapper

#

that one is a beast 😅

loud stirrup
#

So I'd replace this by checking for the vanilla bucket explicitly... Or potentially just passing it the empty bucket item via ctor if someone wants to reuse it for some weird modded bucket 😄

#

this one, I mean if (!context.getCurrent().is(Tags.Items.BUCKETS_EMPTY)) {

#

OTOH it's a 120 line class 😄

fossil cave
#

How many changes have been made to the API compared to Fabric?

flat root
flat root
fossil cave
#

The goals of those systems seems almost identical, but the APIs are different

loud stirrup
#

Yes

fossil cave
#

Or actually maybe not? Some method signatures between them are exactly the same

loud stirrup
#

Sure

#

Are you advocating for a verbatim 1:1 copy? 😅

#

At least it sounds like that

fossil cave
#

I like unification and that is generally my approach but I do not know much about either API so am only wondering how diverged this version is

#

It will have an impact on mods I help develop, even if I am not the one using the APIs

loud stirrup
#

The big BIIIIIG point is that should it come to fruition, it'd offer transactional design which is the big differentiator

fossil cave
#

Is Fabric transfer API not transactional?

loud stirrup
#

No that's my point. It is

#

And this would bring that to the table too

#

While currently, you cannot implement the same logic on NF

fossil cave
#

Yes, that's good

#

Perhaps you misunderstood though; here #1183818213134446742 message I meant goals of capabilities vs. API lookup, not goals of current Neo transfer vs. this new API

loud stirrup
#

Ooooooooh

#

I did indeed misunderstand

#

Although the APIs are not that different

flat root
#

the biggest change so far compared to Fabric is adding slots to the main interface (Storage)

loud stirrup
#

Mhmmm, this may be overengineered but it's very easy to reuse by mods

# 
public abstract class BucketFluidStorage implements Storage<FluidVariant> {
    protected abstract ItemVariant getEmptyItem();

    protected abstract int getItemVolume();

    protected abstract ItemVariant getFilledItem(FluidVariant fluidContent);

    protected abstract FluidVariant getContainedFluid(ItemVariant filledItem);
#

And for Vanilla buckets that is extended as:

public class VanillaBucketFluidStorage extends BucketFluidStorage {
    protected ItemVariant getEmptyItem() {  ItemVariant.of(Items.BUCKET); }
    protected int getItemVolume() { return FluidType.BUCKET_VOLUME; }
    protected ItemVariant getFilledItem(FluidVariant fluidContent) { return FluidUtil.getFilledBucket(fluidContent); }

    protected FluidVariant getContainedFluid(ItemVariant filledItem) {
        var item = filledItem.getItem();
        if (item instanceof BucketItem bucketItem) {
            return FluidVariant.of(bucketItem.content);
        } else if (item instanceof MilkBucketItem && NeoForgeMod.MILK.isBound()) {
            return FluidVariant.of(NeoForgeMod.MILK.get());
        } else {
            return FluidVariant.EMPTY;
        }
    }
}
flat root
#

same idea, but yours is discrete whereas this one is continuous

loud stirrup
#

Well not quite the same since it converts the hosting item to and from that

#

Oh

#

down there

#

Huh, weird, I didn't expect this to actually have that functionality

#

Ok yeah one of the differences would be "The item may not change, so the data has to be stored in the components of the stacks."

flat root
#

this class is what MI tanks are implemented with - it's actually quite powerful and I would like a similar helper in NeoForge

#

we need a helper for discrete and a helper for continuous - I don't think it makes sense to combine both

loud stirrup
#

Yup

flat root
#

this is also similar to FluidHandlerItemStack and FluidHandlerItemStackSimple

#

former is continuous, latter is discrete

fossil cave
flat root
#

it's more complicated since there is no concept of StorageViews or SingleSlotStorages anymore

fossil cave
#

I see

wind steppe
#

The PR compiles and theres tests now and stuff. I know that the last time it was discussed there were a bunch of implementation changes we wanted to discuss? Would be helpful to have those now that its in a reviewable state

loud stirrup
#

We're not going to merge this into 1.21.5 (which just went stable)

#

And will more likely go ahead with our own implementation for 1.21.6 and beyond

wind steppe
#

well of course we'd just update it to 1.21.6 when thats out, but this implementation is much further along than the experimental one you're making, why not just discuss what you want to change and then make those changes..

loud stirrup
#

We have to maintain it and I'd rather do that with an implementation we already know

wind steppe
#

seriously..?

loud stirrup
#

Uh... yes?

past lodge
#

the focus should be on getting something that works well and is good out quicker

#

the current state of things is horrid

#

the experimentations pr getting off track a bit and time is being wasted on simple stuff like method/class names etc

#

soaryn & adrian's pr is quite good, and efforts should go towards that instead

#

it's more far long

#

this feels a lot more like "whatever we say is final" rather then "lets all work on something modders (we) will ultimately use"

#

it's not that cool to shoot down contributions like that

#

there's no real reason to nitpick and overengineer stuff so much, it's better to have something that we can use, and then from that make changes when stuff can be improved

loud stirrup
# past lodge it's more far long

So, the PR that was closed before a maintainer led PR was even opened. But even if you simply ignore that for a second. There was no transaction support in that PR that I know of, but without transactions rewriting these systems is a non-starter. Now I see that the PR today apparently introduced those, but then saying it is far along after adding this last minute is a stretch.

#

What you're also completely ignoring is that maintainers have to maintain this system going forward. Likely for years. So being annoyed at maintainers preferring a solution they know is understandable but ignores the boring realities of maintenance.

#

And to the same effect there's no reason to rush something out of the door now.

flat root
#

And beyond that, it should be no surprise that maintainers want to have control over the design, rather than letting someone who is not even in this discord server and has completely ignored the discussion in this thread

wind steppe
#

the repo is still mine, soaryn just brought it up to date

#

you are also under no obligation to accept a PR without having the person writing it make the changes you want, so I dont understand the point you're trying to make here

past lodge
#

yeah this is not "you must merge this" situation, and rather "lets contribute and work together on this instead of it being lead by only maintainers"

loud stirrup
#

I would have objected to that PR without transactions. Adding those weeks after we started a maintainer PR is open feels like it was aimed to create this exact drama.

#

And now we're having proxy discussions

wind steppe
#

I was under the impression that the PR written by Tech was an experiment. Its was written on 1.21.1 and afaik doesnt compile or pass the game tests. This was not to cause drama. I would have written the intial PR with transactions had I know that was going to be accepted by the maintainers. I also dont understand this "familiarity" argument. You guys are going to need to review this PR anyways, this wont get merged without the maintainers being familiar with how the new codebase works

loud stirrup
#

It is deliberately targetting 1.21.1

#

Which was discussed... in here if I recall

flat root
past lodge
#

its targeting 1.21.1 so it can be tested with larger mods

flat root
#

I changed the target to 1.21.1 precisely so it could be tested once it's ready for that. We'll need real world feedback before we merge anything

loud stirrup
#

I actually think retargetting it to 1.21.1 to enable a feedback loop that could otherwise not be had is a great idea

flat root
#

I'd even say it's a necessity tbh. Otherwise we can't really do any proper testing

wind steppe
#

even in its not transaction state it was further along, my impression of the PR was that it hadnt changed any of the underlying systems that would be changed. Theres alot of other stuff that needs to be changed if IItemHandler and IFluidHandler are planned for removal

loud stirrup
#

Their removal is unacceptable if there's not a major functional addition that justifies it. And in my opinion only transactions pass that bar.

wind steppe
#

which the PR now supports

loud stirrup
#

Since yesterday

wind steppe
#

Yes, it wasnt that difficult of a change

flat root
#

I mean sure, once you can copy-paste all the work we've been doing it's not that big of a change. But that also shows how much we have actually done with them

flat root
#

To be clear, I have looked at your PR for some things, and it is useful prior art. I'm happy to consider bringing some things over (let me know which parts exactly), but there are many things I deemed suboptimal and chose to do differently, partially because transactions change the picture and partially because I think that some things ought to be done differently and getting them changed would be an uphill battle. TLDR if you think there's a specific part of your PR that we overlooked just let us know and we'll have a look

wind steppe
#

we could have discussed that, we could have discussed everything about the PR. Your PR wasn't even labelled as targetted for actual release, it was labelled "experiments", and it had a simulation based API in there too just to compare it. None of this was intended for drama, I didnt even see this as displacing your PR. There was a world where we could have continued to use your PR to test with modern mods and then applied the feedback on the larger PR that has more of the underlying changes and was further along. But instead this happened :/

vernal dust
#

Wow. All I can say to this is wow. Tech, shartte, BOTH of you knew Soaryn and Adrian were working on this PR for MONTHS now, and were extremely open about it in the Forgecraft and Twitch communities. The fact that you both are this hostile to an alternative solution when you HAVE been talking about your transaction and transfer changes as experimental is, frankly, damning.

#

Add on the fact that the second I brought up codecs for attachments and was INSTANTLY shut down, as someone passionate about the codebase and wishing to learn more, is pretty telling of how you feel about the project.

opaque vector
#

From here on out, please remain civil. The brainstorming can continue but only if everyone stays cooperative.

west fulcrum
#

I think it might be better if instead of denying the PR that was worked on a year could be read and take the part that are useful, OR, instead look at the PR, give criticism of it and say something like, “Hey, this could be better”, or “this method can be removed/replaced” instead of just saying “no” because it's too big or might take too long because, the “official” API for this is not even marked has will go to X version, it's marked as experimental.

I understand the first thing that the version was recently marked as stable and the PR won't be merged because of that, but then, just no because you're working on your API I think is.. bad.

Again, I think it would be better to give constructive criticism to the PR and try to make it as good as possible and something that the maintainers are not k1lling themselves the update instead of denying because the API you were working on.

wind steppe
#

I do want to clarify that Soaryn and I more than understood that the PR was not going to be merged this version cycle. We are like, a few weeks away from 1.21.6, I doubt we would have finished the review and editing process before 1.21.6, the goal was always to get feedback, make changes, and prepare the PR for 1.21.6 when it releases

flat root
# wind steppe we could have discussed that, we could have discussed everything about the PR. Y...

There was not much to discuss since the PR didn't implement transactions, which have a fundamental influence on the base implementations, in-item API, and the various helpers. And by the time Soaryn came along with the transactional version (yesterday), we already had done a lot of the groundwork. And let me stress again that this could only be implemented in one day because of all the work we have been doing.

Beyond that, it makes no sense to design a full testable API in 1.21.1 and then copy over improvements to a separate PR? It's a lot more logical to just rebase the final version onto 1.21.x and merge that in.

There is still a lot of room for discussion and considerations, just look at my PR and open conversations. I am currently focusing on the most important bits so naming issues will have to wait. Or you can also just post links to parts of your PR that you think we should consider for inclusion.

#

(as an example, note that the original version of the Storage interface comes straight from your PR)

opaque vector
#

If you are taking some of their work, can you add them as authors or credit to your PR as well

#

Both Adrian and Soaryn

wind steppe
#

Whatever tech, just do whatever you want, since that’s clearly what you were going to do anyways. That’s evident with the class that you took from my PR but also decided to rename to Storage despite the fact that we all agreed on IResourceHandler months ago. You could have very easily just said the PR needed to be migrated to using transactions, or you could have just forked ours and migrated it yourself. Why you started your own entire separate PR is beyond me, but idc anymore. If Soaryn wants to continue working on this he can. I’m done

queen wagon
opaque vector
flat root
#

For a merge in NeoForge I would say 1-2 months if we get sufficient feedback

flat root
# wind steppe Whatever tech, just do whatever you want, since that’s clearly what you were goi...

Many things in your PR are badly named, and the same thing happened again when Soaryn copy-pasted the transactions but also renamed/refactored a few functions in a way that makes the API worse. This is not the first time such things happen, and getting these bad changes reverted has proven to be difficult in the past. So no it's not just a matter of "just ask us to change things", it will actually be a much more painful process than just doing it in a more controlled way in the first place.

Beyond that, transactions are not a requirement that appeared out of nowhere. It is an ongoing experiment which involves finding an API structure that compromises behind migration ability, ease of use, and functionality. This is still something that will require further feedback, and the biggest reason to get the PR in a testable state. Design is an iterative process and there has been no contribution or comments regarding that from Soaryn in this server, which means that convincing maintainers that your design is good is almost impossible. This is the second reason why we are writing the PR again: to make sure that we are carefully considering the many design decisions and their tradeoffs.

I think it's pretty clear overall that your original PR would not have been merged, and that my PR is the best shot we have. And there is still a lot of designing and convincing to do. Whether you want to help with that or not, is up to you.

opaque vector
#

Beyond that, transactions are not a requirement that appeared out of nowhere.
correction, it did appear out of nowhere

#

brainstorming started with us maintainer saying NO transaction. Adrian's PR was made to match our wishes. Later on down the line, mindsets where changed and transaction became a requirement

#

This was not communicated back to Adrian to update his PR

#

Essentially, we doomed his PR from the start by our wishes

flat root
#

No, the PR was doomed without transactions because the cost/benefit ratio simply wasn't there

#

That wasn't necessarily agreed upon or communicated back, it's just a shared implicit feeling I think

opaque vector
#

And it didn't have transaction because us maintainers said no transaction. We DID doom it

#

it was us

flat root
#

No, it is doomed without transactions because it's not worth it

#

The question then became how bad can transactions really be, and the answer is an ongoing experiment

wind steppe
#

getting these bad changes reverted has proven difficult in the past

You mean getting everyone to agree with you on naming has proven difficult in the past, so it’s just easier for you to charge ahead with your implementation with your chosen names than have to actually argue why your names are better. Your “more controlled way” is just your way. I’m sure it would be more painful to yknow, actually listen to the community and convince them to accept what you want.

Soaryn has pretty much only worked on updating the branch, migrating to transactions and adding the game tests needed for the final leg of the PR because frankly I was exhausted and didn’t want to do those parts. I have been here the whole time if you wanted to have a substantive discussion about the design. But as TG said none of this was communicated to me or Soaryn

opaque vector
#

I'm gonna kill this right now. Do not reply technician unless it is specifically about design and pieces of code. I still yet to have a final one-to-one with everyone involved

hollow maple
#

Heyo. Still feeling quite unwell, but I'd like to say a few things:

  1. I was unaware that the branch shartte and tech were working on was "official"; it seemed like they were just doing their own thing (hence the name "experiments")
  2. I genuinely stand behind Soaryn and Adrian's PR; I think the naming and implementation is generally better in their PR, combined with it being more or less fully tested already.
  3. The way you two have been speaking about "we maintainers" makes it feel like you two are the only maintainers and everyone else is below you. I'm sure you didn't mean it that way, but it feels like there was a discussion, again, behind closed doors and vote from the larger cohort of maintainers were not included.