#Transfer rework (IItemHandler, IFluidHandler, IEnergyStorage)

1 messages · Page 9 of 1

opaque vector
#

It’s a “it gets done when it gets done” at a regular pace. If the pace ends up making the current Mc release, nice, if not, it’ll be some future version

#

No specific target Mc version

flat root
#

I think we are on track to have it released on Tuesday next week

queen wagon
#

including a round of final reviews that makes next friday

flat root
#

final reviews for what?

queen wagon
#

the whole thing if we forgot something in a slice

flat root
#

then we'll fix that after the release

#

nobody is gonna review the full 10k lines or so diff

opaque vector
queen wagon
noble latch
queen wagon
opaque vector
queen wagon
noble latch
opaque vector
queen wagon
opaque vector
#

I’m saying when ALL slices are in, some one or so should try and use it in their mod dev before the whole pr is merged

#

Just a smoke test

flat root
#

not worth it if we have to delay the refactor for too long

#

and given the amount of tests we already know that the PR is mostly usable 😛

queen wagon
flat root
#

there was already plenty of feedback from the reviewers, Schurli

silent root
#

I may be able to use that version to test the changes since I have the old system already fully migrated to. As long as it is up to date in terms of the rest of neo code

flat root
#

I don't appreciate people who were not involved with the rework at all coming in and trying to delay it

queen wagon
queen wagon
opaque vector
# flat root I don't appreciate people who were not involved with the rework at all coming in...

I had experiences where I gotten burned not testing the finalized version of something before releasing and realizing a specific thing was missing. Easier to fix it in pr stage than hotfixing/bc after. I don’t foresee a super long delay either for smoke test in a mod dev environment. Is there a reason you think having just one person try and build their mod against the pr would delay it by months or something?

flat root
#

I simply think that this PR must land when we go live with NeoForge 1.21.9, that's all. We'll do the best we can to deliver something reasonable (and given the amount of discussions and tests we had I don't expect any major problem to come up), but hitting modders with such a huge breaking change 1-2 weeks into the BC window will make modders much more unhappy than if we forget a minor detail

#

In any case we'll probably receive a number of issue reports and PRs that suggest various improvements (e.g. new more advanced provided implementations, new helpers, etc...)

queen wagon
flat root
#

you are allowed to have that opinion but I disagree

queen wagon
#

well then I think we'll need a maintainer vote on this

silent root
#

I would recommend (let is assume the pr is done release ready) to have a beta 1 or 2 before we push it live

#

It was really helpful to migrate to port then do the resource handling port in 21.6

#

As a LOT of people will need to sort data gen first

queen wagon
#

well tech seems to be of the opinion that directly porting is easier

opaque vector
flat root
silent root
opaque vector
#

Fapi has experimental/unstable annotation for APIs they deem new and could change

queen wagon
noble latch
#

We have the BC phase for that

flat root
#

that's useless in the NeoForge context, all APIs are "experimental" in the BC window

flat root
silent root
#

I will speak from experience when I did the 21.6 migration, it was easier in two phases in the same version

#

Having to do it all was not a burden I wish on anyone, so this opens a choice to devs

#

Go all in on latest or choose beta1 for a stepping stone

flat root
#

beta1 will probably have some issues, some potentially critical for mods

silent root
#

So beta3 or 4 is when you should likely target

flat root
#

they will also have issues

silent root
#

It will always have issues

#

It is software

#

But that shouldnt be the argunent for not having a stepping stone

noble latch
#

I think pushing this as beta-1 is a good idea but it should be available day one to give people both options from the get-go

silent root
#

I wont personally need it, but I know from experience having to redo data gen/ models and other interactions at the same time was not a fun time

#

Luckily I could do 21.6, then add the test pr version we had

cunning mulch
#

I skipped some messages, but personally I would like at least the initial beta to not include it. As someone who hasn’t finished porting to 1.21.8 yet, it would be nice to be able to target 1.21.9 given vanilla might have changed some of the rendering things that I haven’t adapted to yet anyway. And that way I don’t have a few thousand more conflicts to deal with before I get things compiling xlurk

silent root
#

Yeeeep

cunning mulch
#

Same day is fine

silent root
#

Also yep

cunning mulch
#

But initial release, meh

flat root
#

OK we can merge it right after 21.9.0-beta is built

queen wagon
flat root
#

I don't think anyone is seriously considering not releasing NeoForge on the same day as 1.21.9 😛

queen wagon
#

why should we put that stress on ourselves? it should go out when it's ready, no deadline

flat root
#

I think it's nice to release on the same day and get something out into the modders' hands (unless it's horribly broken, which it currently doesn't seem to be on pre2). Fixes can and will come later anyway

#
  • it's how we have always operated, so if you want this to be reconsidered you should open a broader discussion in #project-talk
queen wagon
cunning mulch
opaque vector
#

21.9.1-beta for transfer, and if broken, we tell people to use 21.9.0-beta in meantime?

flat root
#

no we tell them to send fixes stabolb

queen wagon
silent root
#

Or elaborate on the problem and use beta0 in meantime

flat root
opaque vector
noble latch
flat root
#

there is no hard deadline but there have always been enough people to release on same day it seems

#

ah yes of course

queen wagon
flat root
flat root
#

I already reviewed FML itself, but still need to review the update PR for NeoForge

silent root
#

I hopefully will be able to provide some quick feedback on things missing/not working but I think I managed to isolate a lot of my helper use out from when we changed the util a bit, so some feedback may be missing soarynLurk

noble latch
#

I see no reason to delay the initial 21.9 release, everything (except one thing, oops) that is currently known to be broken or problematic has open PRs waiting for review. Those can all be merged post release though if they don't get reviewed in time (maty, go merge keybinds /hj).

cunning mulch
#

Not the Neo release

copper fjord
noble latch
#

Am I though? Read that again kek

loud stirrup
loud stirrup
#

Although I admittedly will probably fall on my face with 500 GUI compile errors or sth along those lines when switching to 1.21.9

cunning mulch
cunning mulch
noble latch
loud stirrup
#

I don't think I even use that

#

Oh wait, but 1.21.9 was the BE renderer submission changes, right?

noble latch
#

Yes

flat root
#

yup

loud stirrup
#

Ugh. 😄

#

Uuuuuuuuuuuuugh.

noble latch
#

I've already implemented that for two mods, it's not that bad

flat root
#

can someone prepare a short list of the important changes in 1.21.9 for the blog post already? I'm happy to write text but you have to give me bullet points + some simple migration diffs

#

ok this is getting off-topic 😄

cunning mulch
#

Either way ping me when the energy handler slice is ready to review

silent root
#

Shartte, how likely could I get a 21.9 (when able to of course) of ae with the resource stuff to test interactions?

#

Not a "user ready jar" but a "hey the handlers work" one

loud stirrup
#

Or better yet... "FML? fml....!"

loud stirrup
flat root
#

meh most FML changes are internal but we need to document that additionalRuntimeClasspath is not needed anymore + where to find dist and prod

loud stirrup
#

It's probably writing docs for 10 ppl in the entire community, to be honest

#

But still 😄

noble latch
silent root
loud stirrup
#

It is true, the FML changes have barely any effect for normal modders

flat root
#

oops, forgot to multiply by the amount in the capacity it seems

image.png
past lodge
#

it's sorta like how rust's git & branch option work with crate dependencies

flat root
past lodge
#

interesting, why's that?

flat root
#

cause it's really not a normal dependency

#

assuming that what you want is reusing the mc sources from the neoforge project

past lodge
#

understandable, i guess there's always the option of using a sub module & dependency substitution

flat root
#

sure, but you probably don't want to rebuild the full userdev & installer jars + do a full decompilation/recompilation cycle every time you want to run the game

past lodge
#

oh i think we aren't talking about the same thing

#

i was talking about using the 1.21.9 branch of ae2 in your own project

flat root
#

ahh right

past lodge
#

without it being pushed to a maven or maven local

flat root
#

@cunning mulch energy is ready for review

flat root
#

regarding legacy migration, do we generally prefer to migrate all legacy calls in the NeoForge codebase, or only the ones that must be migrated?

#

to illustrate: FluidContentsTint calls FluidUtil.getFluidContained(stack) for which we are able to provide a reasonable implementation in the new system; do we want to leave that call as-is in the transfer rework, or migrate it to its reworked equivalent?

noble latch
#

I think Neo itself should be migrated fully, even if just to serve as an example for mods doing their own migration

flat root
#

makes sense to me

#

next up: we cannot use Capabilities.EnergyHandler as the container class for the energy capabilities since it would force us to use a FQN for EnergyHandler

#

do we want to do Capabilities.Energy.*?

waxen dawn
#

whats an FQN?

flat root
#

google it

serene igloo
#

fully qualified name

#

like, with the whole package

flat root
#

and do we prefer Capabilities.FluidHandler.* or Capabilities.Fluid.*? (either works since the actual handler type is ResourceHandler)

noble latch
#

I would just strip the handler suffix from all of them

waxen dawn
#

no one asked but, i concur

vernal dust
#

I still think the storage and handling should be two interfaces, but I got outvoted on that a long time ago..

cunning mulch
#

(I opened it at least and am poking at it a bit)

loud stirrup
#

where's the bool isValid() { return true; } 😅

flat root
loud stirrup
#

I am also looking at it, since it's so symmetric with ResourceHandler, yes, it's quick

flat root
#

I noticed that getCapacityAsInt actually returns long 😅

loud stirrup
#

Nothing looks terribly surprising about that PR, tbh

#

Which is a good thing

cunning mulch
#

getting close to being done reviewing

#

at like 10 comments so far

flat root
#

OK great. I started the legacy migration already

#

not sure what to do with StackCopySlot since it's the only thing that's not related to IItemHandler in the NNN.items package

#

we can move it to NNN.transfer.item if we want to, or move it somewhere else, or leave it there

#

other question: should we offer "permanent" helper methods to imitate the semantics of IItemHandler (i.e. taking a stack and returning a leftover stack), in a helper class like ItemUtil? how about we don't offer them for now, and potentially add them later?

#

since we are deprecating IItemHandler it seems a bit weird to keep some of its old semantics around, but people would maybe appreciate it? we will have IItemHandler.of(ResourceHandler<ItemResource>) to help with migration in any case

cunning mulch
#

there have your 20 review comments on the energy handler slice

cunning mulch
cunning mulch
#

or something

#

I don't remember which of the stacks split returns

flat root
#

ah yeah that would work well enough I suppose

#

since you can just insert ItemResource.of(stack), stack.getCount() and then split the stack based on the result

flat root
flat root
#

@cunning mulch ok went through your comments on the energy PR

#

a left a few comments; the main design question is whether we want to be making capacity, maxInsert and maxExtract mutable

#

I think it could be nice, to make the impl more flexible

#

the existing EnergyStorage that served as a reference also has them mutable

loud stirrup
#

Explain mutable?

#

In the spec or the provided impl

#

Wait it's the base impl anyway. I mean it's not that important of the outside API is unaffected

cunning mulch
cunning mulch
flat root
#

ok I made them non-final, I think we're good now

#

anything I need to do here?

image.png
#

ah nvm you already marked it as resolved

cunning mulch
#

if either min or max extract is negative then it will pass a negative to super and then the delegate and then crash

flat root
#

ah that one still has final fields but we should also make them non-final

cunning mulch
#

I think if someone sets it to negative we should treat it as zero or instead of protected have a setter

#

It isn't final

flat root
#

ohhh I already switched my branch 😅

cunning mulch
#

you changed it when adding the delegating handler

flat root
#

yeah

cunning mulch
#

Do I need to leave a comment, or are you dealing with it

flat root
# 
    @Override
    public int insert(int amount, TransactionContext transaction) {
        int toInsert = Math.min(amount, maxInsert);
        return toInsert <= 0 ? 0 : super.insert(toInsert, transaction);
    }

something like this?

cunning mulch
#

that's fine. Technically based on the EnergyHandler spec if amount is the thing that makes that negative then it should be throwing an IllegalArgumentException

#

but kaishrug

flat root
#

ok I'll add a TransferPreconditions.checkNonNegative(amount);

cunning mulch
#

(don't forget to do the same block of code for the extract method)

flat root
#

yeah

#

done

cunning mulch
#

copy paste go brr

#

cmon discord

#

it was trolling me and pasted the wrong image from my clipboard because I was complaining about your copy paste error

flat root
#

grrr

#

that's embarassing

#

and fixed

#

thanks

#

will merge tomorrow to give a bit of time, since this is not blocking further work on legacy migration

cunning mulch
#

Don't forget providing a class for the legacy migration of energy handlers

#

granted it should be pretty straightforward

#

once it does get merged assuming no one has any complaints about it in the next day, what slices are left? I know there is legacy, but do we also have a test slice, or did we end up moving all of the tests into earlier relevant slices? (Also are there any other slices I am forgetting)

silent root
#

There is still the matter of default resources

cunning mulch
#

ah right, thanks. I pulled open a tab on my browser for the transfer rework TODOs issue to make sure that those things at least get reviewed

flat root
#

I think we have enough tests as is especially with the work shartte did at some point

#

If someone else wants to look into the default resources todo that'd be appreciated, I'm happy to review it

cunning mulch
#

I just couldn't remember if he had brought over all the other ones soaryn wrote for the slices from before we decided to include the tests per relevant slice

flat root
#

I think they're mostly from scratch but likely better aligned with best practices

#

A lot of my own tests came from extensive regression tests I wrote for Fabric as issues did come up

cunning mulch
#

well someone should definitely check to make sure all of the test cases soaryn has already written tests for are included

flat root
#

Can you add that to the TODO list? Should be reasonable to look at those while the legacy migration is under review

loud stirrup
#

There can always be more tests, but ultimately those are more against regressions

flat root
#

Yeah. Still I think it would be nice to go through Soaryn's tests once the rework is under review

#

Even if they land a couple of days later it's fine, just to make sure we didn't miss any edge case

#

(i.e. not highest priority right now but still a good thing to do soon)

cunning mulch
cunning mulch
wind steppe
#

how we doin gang

cunning mulch
#

I think the thing tech has been working on most recently is the legacy compat stuff

flat root
#

Yeah that's the last missing piece

#

It's not yet ready for PRing. Should be either tonight or tomorrow evening

flat root
#

shouldn't we at least have ItemUtil.getStackInSlot(ResourceHandler, int)

flat root
silent root
flat root
#

close enough 🤷

flat root
#

there are still a few TODOs as noted in the PR description

#

especially regarding FluidUtil which is the most annoying

silent root
#

Any reason you gutted most of ItemUtil from what we had?

image.png
#

The resourceStack returns sort of made sense, not to much the rest

flat root
#

the other way around: I never added this

#

currently leaning more on the minimal side, as more can be added later

silent root
#

drop should be added at least

flat root
#

the itemstack ones should already be covered by vanilla

#

the itemresource ones either by calling vanilla with a stack, or by using PlayerInventoryWrapper#drop in a transaction

silent root
#

An example of the resource one?

#

As

ItemUtil.dropContents(level, pos, data.Buffer.asHandler());

is blindingly simple

flat root
#

if you have a NonNullList<ItemStack>, use Containers.dropContents

#

otherwise there is nothing as short currently

silent root
#

I don't have a list of ItemStacks at that point

#

This is what we had

image.png
#

We also had a simple insertStacking which did the ItemResource conversion for you

image.png
#

As wellas an index ordered variant

#

Basically this class was an interopt between ItemStacks and ResourceHandlerUtil

flat root
#

I don't think that duplicating all helper methods to deconstruct an ItemStack is a good idea

cunning mulch
#

I will try to start poking through the PR to see if there are any bugs tomorrow/Saturday. Especially in relation to the patches and making sure they aren't changing any behavior of things, as I know it took a couple passes to get the dropper's patch fixed properly in the original PR

flat root
#

sounds good, thanks

#

I'll try to get it out of draft (meaning all TODOs addressed) tomorrow

cunning mulch
#

that's not too bad. Went through and basically marked the package info files and any classes that the only change was class level deprecation addition as viewed

image.png
flat root
#

Yeah a lot is quite mechanical

flat root
#

OK I pushed a lot more; the two big remaining things are:

  • Actually fixing the IFluidHandlerItem bridge. I need to think a bit about what exactly the item access needs to do. Might be enough to just use a Container wrapper
  • The fluid pickup and placement helpers which I will simply defer since they need a bit of thinking. This also includes DispenseFluidContainer. The old methods actually continue to work so this can be fixed a little bit later
#

ok, all game tests pass

#

for the old tests I actually kept them as is such that they test both the legacy adapters, and that their modern counterpart still implements the same behavior

cunning mulch
cunning mulch
#

I will work on reviewing the PR tomorrow, might do a bit of misc review tonight. So far found one bug

flat root
flat root
#

@cunning mulch is this the bug you found? 😄

image.png
#

OK I read through the PR entirely. I pushed the fix for this bug as well as a round of small documentation polishing

#

it's really cool that this just works 😄

image.png
hidden geode
cunning mulch
cunning mulch
#

I will continue reviewing later today

#

But posted the comment for the bug

cunning mulch
#

I probably won't get a chance to review it really until this evening, but I will try to finish it tonight so that by (your, tech) tomorrow morning you have my comments and can work on addressing them

flat root
#

I already went through the easier comments and will look at the rest tonight 🙂

flat root
#

I reaaaaaalllllyyyy hate this

image.png
loud stirrup
#

I didn't even know we had that 🤔

#

I am assuming the part you hate is that it unconditionally voids whatever is already there?

flat root
#

no, what I hate is that it uses IFluidHandler just enough to make the helper work

#

and to port this to the new system I don't want to implement this wrapper in a transactional way so I have to "inline" this into the helper

cunning mulch
#

But yes it is cursed

cunning mulch
flat root
#

down the line we might need a generalized transactional level wrapper to set blocks

#

but not at this stage

cunning mulch
#

I believe I had initially told Soaryn that we might want to at some point move those private records to their own class instead of duplicating them

#

Anyway I can look at whatever changes you are making in a couple hours (going to take a nap because my cat insisted I wake up early this morning to give her pets)

flat root
#

stooopid cat 😄

flat root
#

pff only FluidUtil uses FluidType#canBePlacedInLevel...

#

OK I think we have reached a "good enough stage"

#

I think that playing the sound at the player's position instead of the block's position is really not a concern; we are inheriting very ugly helpers and already making a huge improvement to them. Further improvements can come later

#

hmm I have an idea though

#

nope, too messy

flat root
#

@cunning mulch I'm quite available today so let me know once you're done with the review

cunning mulch
#

I will try to take a look in like an hour

flat root
#

A TODO for the vaporization hook sounds good

cunning mulch
#

okay @flat root posted a review, if you really want I guess the game event stuff can be moved to a TODO, but especially if we do fix the sound position it should be pretty trivial to implement firing it, so then I don't see a reason not to just do so to better mirror buckets

#

oh also I forgot to comment but CustomFluidContainerTest, should we move the ones that still use the deprecated try pickup/place to using the no longer deprecated ones?

#

instead of mix and matching fluid utils

flat root
#

ok having a look rn

cunning mulch
#

(your test for placing water tries to place using an empty bucket)

#

is that intended

flat root
#

you mean the second call to tryPlaceFluid?

cunning mulch
#

yes

flat root
#

nice catch

# 
    Test testWaterloggedSlabPlacement:
        Failed - GameTest fail: second placement result is empty on tick 0
    Test testWaterBlockPlacement:
        Failed - GameTest fail: second placement result is empty on tick 0

hmmm

#

in fact the second placement does succeed here

#

because you can just place water a second time into the same block

cunning mulch
#

yeah that was the other thing I was going to ask

#

once you told me if it was intended that you were trying with an empty bucket or not

#

to see that it doesn't work if there is nothing to place

#

or if you just did the test wrong

flat root
#

I'll change it to test both

#

ok done

#

looks like only the sound position and game event are missing

#

I am adding a @Nullable BlockPos pos param to interactWithFluidHandler

cunning mulch
#

is the hand empty, or is it an empty bucket?

image.png
#

I thought bucket item access replaces in place if it is the only bucket

#

or did I read something wrong/am misremembering

flat root
#

yeah I meant empty bucket

#

when we play the sound do we want to just pass null as the player?

#

interactions with handlers retrieved from the block capability should not be performed on the client-side, so it would make sense to pass a null player

cunning mulch
#

sounds reasonable to me

#

keep in mind your try pickup fluid util does pass a player to play sound for bucket pickup

#

similar to vanilla's buckets, except those go on both sides

#

so may want to change that to match what you are doing

#

(as in the util, not how buckets behave)

flat root
#

yeah I'm changing it all to one function

cunning mulch
#

if the position is passed as null, should we skip the game event?

flat root
#

I am currently sending it at the player's position

#

there's always either a player or a blockpos

cunning mulch
#

yes I know

flat root
#

I don't even know what the implications of game events are 😅

#

is it just the sculk sensor? (and similar stuff like warden)

cunning mulch
#

sculk sensors/shriekers

#

and any modded cases

#

and yeah the warden

flat root
#

I guess if we're gonna play a sound we should also send a game event then?

#

I wonder if that's maybe how these game events came to be in the first place

#

i.e. Mojang checking every place where a sound was played to see if there should be a warden-noticeable event as well

cunning mulch
#

potentially

flat root
#

Do we want to add a TODO or open an issue to re-evaluate how that hook works?
what exactly should the TODO be about?

#

the fact that it uses this.content elsewhere maybe

cunning mulch
#

yes, like is it meant to be using the stack in full, or what is the point

#

it seems a little odd to require player/pos for the move helper, but given it is private and we always have a context, I suppose it makes sense

flat root
#

we can always make it handle both null args later if needed

cunning mulch
#

yeah

#

also where was the TODO about the game events and stuff for cauldrons? Either I am blind or I am looking in the wrong place (or we forgot to actually write it somewhere)

flat root
cunning mulch
#

Ah, I am blind, because of it not having a TODO at the front it wasn't colored so I just skipped over reading it

flat root
#

ok just added the TODO to BucketItem

cunning mulch
#

only partially used in the vaporization check

#

it assumes the type matches content

#

it just uses it for the stack of the content

flat root
#

oh yeah true that's even worse

cunning mulch
#

yeah, I don't know how you want to phrase that bit

#

but definitely worth pointing out as part of the TODO

flat root
#

// TODO: The stack is only used as a parameter in the vaporization check. What if the fluidType is different? And should the rest of the method be made stack-aware as well? maybe?

cunning mulch
#

that's fine

#

What is next after merging? Rebasing against 1.21.9-rc1, or working on some of the TODOs in https://github.com/neoforged/NeoForge/issues/2570? At least some of those TODOs are ones that definitely should be addressed before the overall rework is merged, though some seem reasonable to have as a followup afterwards

flat root
#

I think it's time to rebase and open the final transfer rework PR

#

I don't think there's any TODO that needs to be addressed before release?

#

caching the data components especially can come a little later but writing the blog post and making the actual release happen are more important

#

hmm actually Multiply by the current amount in ItemAccessRH is the only one I would consider critical

#

(and of course Inline deprecation version, and ensure it references the correct MC version)

#

is that what you meant?

noble latch
flat root
#

of course 😉

#

solid effort 😂

image.png
cunning mulch
#

but yes, the ones you listed of multiply, inline, and imo the caching is worth it

#

it isn't that hard, the hard part is deciding on the best way to do it to limit "API" scope

loud stirrup
#

I don't think 1) is strictly necessary since introducing that isn't breaking and ends up being only a perf improvement

cunning mulch
#

while yes it isn't strictly necessary

#

I do think it should be included

flat root
#

ok let's discuss the approaches while I do the rebasing

cunning mulch
#

it isn't like we are in a rush, and we can have people start reviewing the overall PR if they feel so inclined on the side

flat root
#

ideally this PR should land in about 24h; I can live with 48h but it also seems unnecessary

cunning mulch
#

approaches:

  1. Patch in a field on item/fluid, have a getter that lazily initializes and can return null in the case it is not done initializing yet so that ItemResource#of can differentiate that it actually needs to be creating a new instance to return so that it can be cached.
  2. Map (potentially lazily populated) for the default resource representations

The first would have better performance as it would allow skipping map lookups

#

for fluids it probably doesn't matter which is the case as the number of fluid types is likely small enough that a map won't have any performance implications. For items, who knows

flat root
#

on Fabric I used a mixin to patch an extra field into item

#

part of the annoyance is that we don't really want a method to be accessible from Item directly

cunning mulch
#

meh, just mark it as internal

flat root
#

there's also the question of invalidating the item default resource if the prototype changed

cunning mulch
#

and doccument as call via ItemResource#of

cunning mulch
#

as invalidating the cached resource

#

and then it can lazily reinitialize

#

in general I don't think anyone will have even queried resources by the time that stage is finalized, but if we are lazily initializing anyway, there is no harm in then just setting null to null, just to make sure

#

and if people modify default components via mixin instead of the event we provide (and decide to do so at a later stage), well that's on them to actually make use of our API

flat root
#

I also think it's quite unlikely but the failure potential is too high to not do anything about it 😄

cunning mulch
#

well especially given it is so trivial, as we already have a method sectioned off for handling updating states of things in the item when the components change

waxen dawn
#

assuming this was the last PR, thank you so much to tech, pup, adrian, soaryn, shartte and all the others for putting in the time writing and reviewing this ❤️

flat root
#

I have to make a little change to this test to remove the extra directions

image.png
#

will add fixing up this test as part of the corresponding todo

cunning mulch
flat root
#

it's in the rebase kek

#

i.e. inside the rebased legacy migration commit

cunning mulch
#

ah

#

when you said as part of the TODO I thought you meant as doing the TODOs

#

from the issue

flat root
#

ahh

cunning mulch
#

not the one that was on the highlighted line

flat root
#

ok yes; then that will come later since the composter is not urgent

cunning mulch
#

but yeah, the:

  • Multiply by the current amount in ItemAccessRH
  • Cache item and fluid resources with no data component patch, to limit allocations in common cases.
    should be as a separate PR imo that just targets the rebased branch. And I meant to ping me once you have that PR xlurk
flat root
#

ah ok now I understand what you meant

waxen dawn
#

has neo not updated to 21.9-rc1?

loud stirrup
#

it has

#

we didnt rename the branch

waxen dawn
#

something wong 🤔

loud stirrup
#

You can ignore that

copper fjord
#

was that weird mc version constant inlined?

loud stirrup
#

Hm?

#

Oh the deprectionversionthingy?

copper fjord
#

yeah

cunning mulch
#

yes

loud stirrup
#

Done

flat root
#

thoughts on this patched into Item (+ the corresponding field)?

    /** @deprecated Neo: do not use, use {@link net.neoforged.neoforge.transfer.item.ItemResource#of} instead. */
    @org.jetbrains.annotations.ApiStatus.Internal @Deprecated
    public @Nullable net.neoforged.neoforge.transfer.item.ItemResource getOrUpdateDefaultResource(@Nullable net.neoforged.neoforge.transfer.item.ItemResource newDefaultResource) {
        if (this.defaultResource == null && newDefaultResource != null) {
            this.defaultResource = newDefaultResource;
        }
        return this.defaultResource;
    }
#

usage would be

    public static ItemResource of(ItemLike item) {
        Item value = item.asItem();
        if (value == Items.AIR) return EMPTY;
        var resource = value.getOrUpdateDefaultResource(null);
        if (resource == null) {
            resource = new ItemResource(new ItemStack(value));
            value.getOrUpdateDefaultResource(resource);
        }
        return resource;
    }
cunning mulch
#

also mark it as internal

#

but yeah that seems fine. I would have done the logic more on the item's end. Where it doesn't take a parameter passed to it

#

but doesn't really matter

flat root
#

the thing is that I don't want to expose an ItemResource constructor or anything like that

echo saddleBOT
#
pupnewfster

[Reference to](#1183818213134446742 message) #1183818213134446742 [➤ ](#1183818213134446742 message)nah, it definitely would be a bit meh, but I more was thinking the following which doesn't need a setter:

private boolean cacheInitializationStarted;
@Nullable
private ItemResource defaultResource;

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

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

holy, my discord is lagging out from trying to go to that previous message

noble latch
#

You could pass a Function<Item, ItemResource> to getOrUpdateDefaultResource() instead, then you don't need to call it twice. You'd just pass in ItemResource::new

cunning mulch
#

but yeah, xfact's idea might be better yet

#

given I do prefer the getOrUpdate idea to having it be getDefault even with the internal annotations etc

flat root
#

yeah

#

I also like it

#

I'd maybe call it internalGetDefaultResource 😄

#

(it should look ugly in the autocomplete)

cunning mulch
#

kaishrug I don't care, feels redundant though with the @Internal annotation

flat root
#

computeDefaultResource

cunning mulch
#

yeah thats better

#

I will try to look at notifications, but failing that I will be back in like an hour or two and can review it then

#

And remember to invalidate in default component update (I think fluids can skip that as I don’t think fluids themselves have components?)

flat root
#

yes, I remembered 😄

#

just doing some tests now

#

psst @hidden geode look at how we made this cache super clean ^^ (IMO)

loud stirrup
#

Someone is going to call it stupidly, but I dont have an alternative to offer

flat root
#

the @Deprecated + the Function parameter makes it very annoying to call

cunning mulch
#

The case item access is empty, should it return zero for capacity or not? (Especially when queried with the empty resource)

flat root
#

what's the capacity of the lack of a bucket? imo 0

#

in the sense that unless something changes in the item access you cannot insert anything

cunning mulch
#

I just don’t have a good intuition for when item access might be empty

#

I am guessing it is fine, I just wanted to verify

flat root
#

the item access would be empty afterwards

cunning mulch
#

But it wouldn’t become not empty later, right?

flat root
#

it could be since it's a view into some mutable inventory

#

it could be that someone is adding stuff to e.g. the player's hand in the same transaction and suddenly the item access contains something again

cunning mulch
#

Meh, I am unsure if the impl provides a great return result when passed the empty resource. But I don’t think there is a good one, and zero does seem reasonable if backing item is not present

flat root
#

tsss @noble latch 😄

image.png
noble latch
#

I know that it's right above the method it's related to, that doesn't make it any less ugly though. I really think we should get away from this habit

flat root
#

I don't have a strong opinion of course, but having a single hunk that is less sensible to field addition seems slightly preferable to me

#

it could be worse... in the IC2 source code all fields are actually at the end of the whole file 😄

noble latch
#

If they add a method at the bottom, then that entire hunk blows up and you haven't won anything either

loud stirrup
#

Yup

#

I'd prefer just having them in sensible locations

flat root
#

wdym by sensible?

copper fjord
#

mixin it

cunning mulch
#

we already have other fields in that class we are patching in though

#

are you saying tech should move those up top as well?

#

(I don't really care one way or the other, but if we are adding the hunk at the top for the fields, we might as well move the other ones in the same classes to the top)

flat root
#

I wouldn't necessarily do that as part of this since it will be hidden in the middle of a huge squashed commit

cunning mulch
#

fair enough

noble latch
#

Moving existing fields can wait, I'd just prefer newly added ones to be added at the top. I've been moving fields to the top during porting whenever I encounter one that was rejected or needed to be changed anyway

cunning mulch
loud stirrup
#

There are two other options, none ideal

#
  1. Package-Private, add a new class to the same package to forward the call, which at least hides the method from auto-completion
  2. Use StackWalker to only allow calls from net.neoforged (slow)
cunning mulch
#

like technically because of neither item nor fluid having a super class, we could shoehorn it in as a super class that then stores the field and also is package private to the resource package for purposes of the method that sets it, but that feels even worse

loud stirrup
#
  1. could be combined with 2) to only pay the stackwalker cost once, I suppose
flat root
#

We also have the other method to change the default components and it seems to have been fine in the past

noble latch
#

I don't think it's that much of a problem that it would require outlandish workarounds to prevent calling it. Since a mod doesn't have access to the ItemResource ctor, the only thing they could pass in is a lambda using ItemResource.of(). If they do that before the default resource is initialized, then they would just get a stack overflow (unless they start adding stuff to the component patch). If they do it after the default resource was initialized through some other means, then nothing would happen because their function wouldn't be called at all. The worst that could happen is poisoning the default resource with non-default components.

hidden geode
#

Very nice

mental raven
#

So, we are at the whim of whoever creates the first resource for what data components are on it? that seems very dangerous

#

hmm, seems the calls are gated behind functions taking Fluid or ItemLike, however, if someone calls it manually, the cache is fubar

flat root
#

What would you pass as a lambda?

#

You'd have to go out of your way to mess it up imo

#

What we could do is monitor waifu for this method

loud stirrup
#

Well I outlined two other options, but honestly... eh

mental raven
#

Are the methods marked as ApiStatus.Internal?

loud stirrup
#

Should be, and/or deprecated

mental raven
#

as long as marked internal, idc :)

flat root
#

Both internal and deprecated ^^

mental raven
#

guess i missed that skimming

flat root
hidden geode
#

Done

#

Approved

flat root
#

thanks

serene igloo
#

quick merge!

hidden geode
#

You want to hit the button yourself (outside of normal rules?)

#

You deserve it here

#

You did all the heavy lifting

#

And getting it ready

flat root
#

normal rule = approval; doesn't matter who hits the button

#

so I did it 😛

#

I added Adrian as a co-author as well

hidden geode
#

Very nice

#

Did you add soaryn as well?

flat root
#

yes

hidden geode
#

Perfecto 😄

flat root
#

shadows and shartte are there as well heh

crude zephyr
#

Which is the lowest supported mc version for transfer rework, 1.21.7?

silent root
#

21.9 iirc

#

Well the code is suitable for all, but it is a breaking change

#

So 21.9 is the only target that makes sense to apply it to

meager nexus
#

what would be the total diff loc for the entire PR set ? 50k?

crude zephyr
#

Looks like I can't use the latest transfer api right away

flat root
#

blog post coming soon™

meager nexus
#

thats just the main PR no

flat root
#

which contains all the smaller PRs

meager nexus
#

oh ? awesome

cunning mulch
#

we did it

#

only probably took a thousand or two comments on the PR(s)

wind steppe
#

HELL YEAH

cunning mulch
#

definitely over two thousand actually. Given the pre split one was already at 1651

image.png
meager nexus
cunning mulch
#

transactions 2, electric fox spin

silent root
#

That is next week

wind steppe
#

So who's buying the champagne

cunning mulch
#

nah was today, fox version. Clothing version was a few years ago

silent root
#

Juice boxes!! soarynComfy

#

(Or dr pepper)

wind steppe
#

im down for dr pep

silent root
#

I'll set it aside then for you

flat root
#

oh yes some dr pepper would be good; not so easy to find in europe

#

anyway I hope nothing will break now 😄

silent root
#

Yeah the Dr Pepper in Finland was ... sweeter than here (due to real sugar vs corn syrup)

HOPEFULLY nothing will break, I'll try to test today

wind steppe
#

Gonna port Ad Astra to this this weekend 🙌

#

I’m so hyped

karmic sentinel
#

The rework not the legacy version right

wind steppe
#

the rework yeah. I ain’t touching the legacy version kek

silent root
#

@flat root the Nullability is still causing issues

image.png
#

This is caused by

image.png
#

And again, the ONLY one that should be null is Void. Not everything else

flat root
#

We might have to switch to jspecify

#

The capability context has the same issue

#

Didn't have that in older intellij versions

silent root
#

"older" how old are we talking here?

flat root
#

2024 something

#

I don't remember and I reset my pc since then

silent root
#

Why don't we have FluidUtil.getResource and the firstContainedEquivalent?

#

I shouldn't need to go to a stack at all

#

Can we make the moveWithSound public?

flat root
flat root
silent root
#

getResource arguably is less needed, given you can just do handler.getResource(i);

but getFirstResource is desired as it will give you the first non-empty resource or empty if none found rather than having to manually loop ourselves

flat root
#

I guess but what are you going to do with it?

#

(the first non empty resource)

silent root
#

Correct

#

Something like what is stored on this item

#

But if I don't need the stack, that is just wasteful computation

flat root
#

I'd probably not overthink it and just use the stack helper 😛

silent root
#

It isn't over thinking it

#

I am going from resource to resource

#

what the util does now is provide me a stack so I go resource -> stack -> resource

flat root
#

Part of the problem is that providing all the combinations is just too much

#

We could have a method to get the first contents in a ResourceHandler but it would likely return a ResourceStack (unless we also add a resource-only one)

#

But then you'd have to handle the cap query on your own

silent root
#

I had both in the past

flat root
#

Yes, you had too much which reduces the visibility of the more important methods

silent root
#

The alternate extreme you have was to axe all of them and have no utility for a "wait and see what is asked for". I'm asking for a getFirstResource now. Take that as you will

#

But as it stands right now, the utility class is pretty lacking

serene igloo
#

I can use ResourceHandlerUtil.insertStacking or ItemUtil.insertItemReturnRemaining but not both

silent root
#

Wouldn't you just do stack.shrink on the insertStacking result?

#

You are correct, there is no insertStackingReturnRemainder

serene igloo
#

would rather have a new itemstack

#

there we go

image.png
silent root
#

That looks correct

serene igloo
silent root
#

Looool

#

Transaction.open(null)

#

That is a copy paste error, as that method was decided on not being added

#

Opening a transaction should always require the user to know the "parent" and specify if there isn't one

#

In the case of a brand new transaction, you open with null

#

However, if you are in a transaction when you open a new one without specifying the correct parent, it will throw on not properly being chained

serene igloo
#

eurgh

mossy chasm
silent root
#

There was some problems that could arise otherwise

#

it is doable

#

but it definitely had a few back and forths and needed to be tested first

#

Transaction.open() could have been the route we took. but then you are guaranteed to be paying a lookup cost each time

#

vs just passing through

serene igloo
#

okay, so I need to pass in a TransactionContext to my helper above

silent root
#

Yeah if you know it won't be a root

serene igloo
#

it's more that I don't know whether it would be a root, I think

silent root
#

So a nullable TransactionContext

#

I don't advise this fully, but you CAN try using the getCurrentTransaction()

serene igloo
#

no no I think it's more like

silent root
#

I use it in places I don't have the ability to pass the context

serene igloo
#

the caller decides whether to open(null) or pass in an existing transaction?

#

now do I open a new transaction when I call ResourceHandlerUtil.isnertStacking?

    public static ItemStack insertItemStacked(ItemStacksResourceHandler handler, ItemStack stack, TransactionContext context)
    {
        int oldCount = stack.getCount();
        int inserted = ResourceHandlerUtil.insertStacking(handler, ItemResource.of(stack), oldCount, Transaction.open(context));
        return stack.copyWithCount(oldCount - inserted);
    }
silent root
#

If you pass null in, it will open a root

serene igloo
#

or do I just pass in the context I pass

    public static ItemStack insertItemStacked(ItemStacksResourceHandler handler, ItemStack stack, TransactionContext context)
    {
        int oldCount = stack.getCount();
        int inserted = ResourceHandlerUtil.insertStacking(handler, ItemResource.of(stack), oldCount, context);
        return stack.copyWithCount(oldCount - inserted);
    }
silent root
#

if you are in a chain, you pass in the context

#

If you pass in a context that you just opened, you are in complete control

#

so you can also choose to commit if you don't like what you got

serene igloo
#

right, so whatever is calling MY util method here should be deciding that

#

not my util method itself

silent root
#

For the most part yea

serene igloo
#

extract is a little confusing

#

why do I need to specify the resource as a param

#

I don't have the resource yet, that's why I'm extracting it

silent root
#

In the handler?

#

You need to know the resource there, which you can get from the getResource(index)

hollow maple
#

You should be able to pass null for that AFAIK - it's designed for "extract only these items" style consumers

silent root
#

Not in the ResourceHandler methods

#

nothing can be null

hollow maple
#

Oh, that must've changed at some point then

serene igloo
#

well, I'm overriding it anyway, so I can just slap a Nullable on it

silent root
#

It should not be null...

#

you pass null into it, it will fail

#

TransferPreconditions will throw

#

Must be a non-empty non negative input

hollow maple
silent root
#

Nope

#

That is for the filter in the Util

#

and then it is resource -> true

hollow maple
#

gotcha

wind steppe
silent root
#

But it won't tell you what moved

serene igloo
#

eurgh

silent root
#

Ah

#

Extract first is likely what you want

#

ResourceHandlerUtil.extractFirst

#

It'll give you a resource stack

#

THAT is nullable

serene igloo
#

extractFirst doesn't take a slot though

silent root
#

If you know the index, you get the resource

serene igloo
#

that seems like extra steps

wind steppe
#

You can make a simple utility that’s like, getAnyInSlot or something. Maybe that’s something we add to the utils and it just returns a resource stack

silent root
#

Couldn't be avoided without adding a method

#

We HAD a utility though iirc that did this

wind steppe
#

probably lol

silent root
#

May have been removed in the great purge of "This seems unecessary axe it because maintenance!"

#

LOL I did

#

It was removed

#

Along with the IStackFactory

image.png
#

so blame tech on that one

wind steppe
#

:)

silent root
#

I just copied the ones I used and made my own util

#

as the ones there seem mostly ... bleh now

wind steppe
#

I’m probably gonna make a small kotlin lib that has a bunch of utilities in it

serene igloo
#

ugh, okay like

#

I have a furnace which has a hidden "backstock" inventory which is a list of itemstacks

#

I don't have an itemhandler for that it's just a list

#

if I take something out of the output slot (which does have an itemhandler which I've overriding extract on) and there's anything in the backstock, it pops one and sticks it in the output slot

#

how the hell do I roll that back if the transaction fails

wind steppe
#

How do you store the current state of the furnace with everything in it?

#

Like, for serializing

serene igloo
#

previously it was a bunch of ItemStackHandlers for the slots + a list of itemstacks for the backstock

#

(the itemstackhandlers I just serialized as themselves)

wind steppe
#

Honestly if you wanna just be done with it, the simplest way is just to serialize to a tag as your snapshot and then read from the tag to roll back :P

#

Using your existing methods

serene igloo
#

I'll probably just have to implement the backstock as its own ResourceHandler

wind steppe
#

Not necessarily. The backstock is part of what exactly? Like, is it in the handler or part of the block entity?

serene igloo
#

there's a bunch of things going on inside this furnace that would actually benefit from transaction rollbacks (even if the external itemhandling doesn't)

serene igloo
#

it's basically a kludge for like

#

"okay I'm crafting m items into n other items and I'm pretty sure I have room for all of them but not 100% sure, so if I craft the items and they don't have room when they're done smelting then just stick them... somewhere"

#

up the furnace's butt

wind steppe
#

You just make changes to the backstock as if you were doing it for real, and then set the list with the old contents to rollback. Just don’t call setChanged on the block entity outside the onFinalCommit or whatever the method ended up being called

serene igloo
#

yeah but then I'd have to figure out what the old contents were

wind steppe
#

you can save it as just like, a cloned list

#

The snapshot object can be literally anything

#

You make the snapshots, you read the snapshots. The transfer api doesn’t care what it is

serene igloo
#

where do I handle snapshots

wind steppe
#

You extend snapshot journal in your handler, and then right before you make changes to the contents of your container that’d need to be rolled back, you call the updateSnapshot method, which makes a snapshot of the container as it is before changes are made

serene igloo
#

ah okay so I make a handler for the backstock

wind steppe
#

You don’t necessarily have to, you can handle the rollbacks in your main handler as long as you put the backstock contents in your snapshot

serene igloo
#

how do I get a snapshot

wind steppe
#

You probably shouldnt? What for?

serene igloo
#

I don't understand any of the things you're saying

hollow maple
#

a snapshot is just a copy of the state at a certain point in time

wind steppe
#

Yes. It’s basically taking a picture of the container as it is, and then if someone during their transaction doesn’t like the end result, they can put things back the way they were in the picture and everything is hunky dory

serene igloo
#

okay lemme rethink this over

hollow maple
#

so, the snapshot journal would basically have something like List.copyOf(backstock) in its "create snapshot" method

#

and then when you restore you'd do backstock = thatCopy

wind steppe
#

Exactly yeah

serene igloo
#

during the extract, I want to get a copy of my backstock list and modify it, and keep that copy... somewhere

#

then when the transaction commits, I want to set the modified list in my blockentity?

wind steppe
#

No no no, you don’t manage the copies yourself

silent root
#

During extract, you copy your current thing

#

Then just modify as you would

serene igloo
#

okay, so I have to handle the rollback, not the commit

silent root
#

BASICALLY anywhere you had a check for simulat / execute, you just replace that block with snapshot first then modify the contents

#

Rollback is handled in the snapshot journal

serene igloo
#

how do I make a snapshot journal

wind steppe
#

You just extend it

#

In your handler

serene igloo
#

wat

wind steppe
#

It’s a class

serene igloo
#

thonk are y'all doing instanceof on this

silent root
#

I do this

    private final List<IndexJournal> indexJournals = new ArrayList<>();
wind steppe
#

uhh idk that was soaryns implementation

silent root
#

sec

#

Trying to parse out the relevant parts while streaming soarynLOL

# 

private final List<IndexJournal> indexJournals = new ArrayList<>();

Constructor(){
 ...
 for (var index = 0; index < size; index++) {
  indexJournals.add(new IndexJournal(index));
 }
...
}


private class IndexJournal extends SnapshotJournal<Quantified<TResource>> {
        private final int index;

        private IndexJournal(int index) {
            this.index = index;
        }

        @Override
        protected Quantified<TResource> createSnapshot() {
            return Quantified.of(stacks.get(index));
        }

        @Override
        protected void revertToSnapshot(Quantified<TResource> snapshot) {
            stacks.set(index, snapshot);
        }

        @Override
        public void updateSnapshots(TransactionContext transaction) {
            super.updateSnapshots(transaction);
            changedJournal.updateSnapshots(transaction);
        }
    }
#

Now this is just the container I have

#

In extract...

# 
 private int extractBehaviour(int index, TResource resource, int amount, TransactionContext transaction) {
        if (!behavior.canExtract(index)) return 0;

        var currentStack = container.get(index);
        if (!resource.equals(currentStack.resource())) return 0;

        var currentAmount = currentStack.amount();
        int handledAmount = Math.min(amount, currentAmount);

        container.getIndexJournal(index).updateSnapshots(transaction);
        set(index, resource, currentAmount - handledAmount);
        return handledAmount;
    }
wind steppe
#

Oh weird. I guess that’s one way of doing it

silent root
#

So before your set, just update. BUT if you are a mutable thing, your snapshot must be a copy not just the thing itself

serene igloo
#

ahh SnapshotJournal has an onRootCommit

#

so I can override that to update my blockentity's backstock list on commit

silent root
#

Yeah I do this

public final class NotifyingSnapshotJournal extends SnapshotJournal<Void> {

    public static final NotifyingSnapshotJournal EMPTY = new NotifyingSnapshotJournal(null, null);

    @Nullable
    private final Runnable commitCallback;
    @Nullable
    private final Runnable revertCallback;

    /**
     * Creates a snapshot journal with custom commit and revert logic
     * in scenarios you don't need to allocate a new snapshot or have any state that would otherwise need to be recorded.
     * Only one runnable in a given transaction will run. It is either successful or not.
     *
     * @param commitCallback Action called when the transaction successfully commits its chain.
     * @param revertCallback Action called when the transaction reverts to a snapshot.
     * @return A Journal able to be take notes of when a value was changed, but doesn't allocate any value.
     */
    public static NotifyingSnapshotJournal of(@Nullable Runnable commitCallback, @Nullable Runnable revertCallback) {
        return new NotifyingSnapshotJournal(commitCallback, revertCallback);
    }

    /**
     * Creates a snapshot journal with custom commit logic
     * in scenarios you don't need to allocate a new snapshot or have any state that would otherwise need to be recorded.
     * Only runs the given runnable when the transaction is successful and committing.
     *
     * @param commitCallback Action called when the transaction successfully commits its chain.
     * @return A Journal able to be take notes of when a value was changed, but doesn't allocate any value.
     */
    public static NotifyingSnapshotJournal commitWith(@Nullable Runnable commitCallback) {
        return new NotifyingSnapshotJournal(commitCallback, null);
    }

    /**
     * Creates a snapshot journal with custom revert logic
     * in scenarios you don't need to allocate a new snapshot or have any state that would otherwise need to be recorded.
     * Only runs the given runnable when the transaction is aborted and reverting.
     *
     * @param revertCallback Action called when the transaction reverts to a snapshot.
     * @return A Journal able to be take notes of when a value was changed, but doesn't allocate any value.
     */
    public static NotifyingSnapshotJournal revertWith(@Nullable Runnable revertCallback) {
        return new NotifyingSnapshotJournal(null, revertCallback);
    }

    private NotifyingSnapshotJournal(@Nullable Runnable commitCallback, @Nullable Runnable revertCallback) {
        this.commitCallback = commitCallback;
        this.revertCallback = revertCallback;
    }


    @Override
    protected Void createSnapshot() {
        return null;
    }

    @Override
    protected void revertToSnapshot(@Nullable Void snapshot) {
        runRevertCallback();
    }

    public void runRevertCallback() {
        if (revertCallback != null) {
            revertCallback.run();
        }
    }

    @Override
    protected void onRootCommit(@Nullable Void original) {
        runCommitCallback();
    }

    public void runCommitCallback() {
        if (commitCallback != null)
            commitCallback.run();
    }

}
#

And just pass that into the other one

#

so each index keeps track of its items

#

but only one onChange is fired in the endd

wind steppe
#

I think?

#

That sounds right to me

#

Yeah

serene igloo
#

yeah no I'd rather just not update my blockentity until the commit happens

wind steppe
#

your handler’s state needs to reflect all the changes made to it as if you actually performed all the operations on it

serene igloo
#

nah this is just for internal stuff

wind steppe
#

I mean, wouldn’t it just be easier to move the backstock into the handler and just treat it as part of the handlers storage stuff

serene igloo
#

probably

wind steppe
#

If you did want to make the backstock separate still, you don’t have to make it a whole ass handler

#

You can make it just a snapshot journal

serene igloo
#

calling it a journal is throwing my brain off

#

I'm gonna call it a stack

wind steppe
#

Sure

serene igloo
#

but like... where do I use it

#

ah nvm I forgot soaryn had the thing up there

#

okay I'm gonna think more about this over the weekend

wind steppe
#

You can make a class that wraps the ways you manipulate your backstock and holds the backstock. This whole thing extends SnapshotJournal, it’d be the snapshot stack for the backstock. Every time it changes, you call createSnapshot which makes a copy of the list, then you change the list for real. The manipulation methods would take in the transaction context, and would be basically an inner transaction thing. Itd also be more performant this way I think, cuz you’re not always adding stuff to the backstock, so you only need to rollback when the backstock has changes to it

#

Shoot me a dm when you’re working on it if you want some help. I can also take a look at your handler if you want and help you out there

flat root
# serene igloo

Oops that's because I wrote a lot of the post back in June

flat root
#

You can just pass null to many helpers, they'll accept it

eager knoll
#

It would be nice to have supportsInsertion and supportsExtraction methods in the EnergyHandler interface and maybe in the ResourceHandler<T> interface too. The default implementation should return true.

My use case:
I would need this feature for my implementation of cables. I have two collections of IEnergyStorages (one for energy consumers of the whole network and one for directly connected energy producers). Every cable block in the network goes through every directly connected producer and checks how much energy could be extracted at most. Afterwards it checks how much energy can be consumed by consumers of the whole network at most and distributes energy evenly. Now the changes are commit to consumers and producers.

In my implementation the algorithm is run per cable block. I only check for producers if they are direct neighbors and consumers for the whole cable network. If no producers are directly connected to a cable block, the algorithm execution will be skipped.
(Implementation of the cable network on NeoForge 21.9.0-beta: https://github.com/JDDev0/EnergizedPower/blob/2079566589c1f2bff463be6d3e1856c03bd9561e/src/main/java/me/jddev0/ep/block/entity/CableBlockEntity.java)

Without does methods I have to assume that every connected block is an energy consumer and an energy producer. This could cause performance problems for large cable networks in my current implementation, because I would have to run the algorithm for every cable block where any block is connected. With does methods I would not have to run the algorithm for cable blocks if no energy producers are directly connected - this would improve performance.

I could change the implementation to run the algorithm once for the entire network, but if I don't have too, I'd rather not.

flat root
#

Yeah the main reason to have these methods would be this sort of optimization

#

It won't change your worst case behavior but could speedup the average case by a factor of 2

loud stirrup
#

Does it depend on the result of that method being constant for theifetime of the handler?

flat root
#

The result of these methods should be constant for the lifetime of the handler, yes

#

Personally I'm happy to add these methods back. I wanted them originally but we agreed to remove them for now

cunning mulch
#

Yeah, iirc we just decided to delay them so we can better decide how we wanted to add them back. Such as if a more broad system (such as the bit mask thing or whatever)

flat root
#

I hate this bit mask idea

cunning mulch
#

Yeah I don’t know how I feel about that either

flat root
#

I already said multiple times that it's a solution in search of a problem and I stand by it 😛

cunning mulch
#

I just mean that there might be other ways to do it than just the simple booleans

flat root
#

Did any of the other bit masks make any sense?

cunning mulch
#

Ehh knowing that something is voiding to reduce priority for sending to that destination pile be useful, but kaishrug . The bit mask thing happened after I had finished my initial review of the large PR, so I never really looked at what the various masks were

flat root
#

It's not the right mechanism to do it

#

Gameplay decisions shouldn't be affected by these flags that provide limited control

cunning mulch
#

I think @hidden geode and @vernal dust were the ones discussing things with @silent root that led to it. So maybe they have more specific scenarios

loud stirrup
#

Boolean getters please

cunning mulch
#

I don’t have any preferences, I just wanted to bring up the previous discussions on it

stone dragon
ivory mauve
#

as another example, maybe you have a recipe that accepts a large number of input options. That is basically "voiding" even if the machine isn't a voider

loud stirrup
flat root
#

Should it take a collection or list vs the sequenced collection?

cunning mulch
#

Sequenced seems fine. At most broaden to collection, I wouldn’t narrow it to list

#

It sort of depends on if you want the impl detail of how the things get indexed to be more obvious. If your collection isn’t ordered then you don’t necessarily know which indices to interact with. Granted now that we have index less insert and extract methods maybe it doesn’t matter

loud stirrup
#

it's not an impl detail for the combined handler that it maintains the order of the handlers given in the exposed slots

#

that's why I think sequenced is so nice here, since it can model that without requiring a list

cunning mulch
#

Yeah it seems fine to me, we will see what tech says. My main point is that I wouldn’t narrow it to list

loud stirrup
#

He's just not used yet to the new sequenced collection goodness

flat root
#

It's fine I just wanted to discuss it a bit

eager knoll
#

I noticed that the insert method in net.neoforged.neoforge.transfer.StacksResourceHandler uses the getCapacity method instead of the getCapacityAsInt or getCapacityAsLong method. If a modder would override the getCapacityAsLong method when using for example the ItemStacksResourceHandler to limit the capacity to 1 for every slot, the insert method could still insert more than 1 item if the user of the insert API did not check the capacity himself.

I can think of two possible solutions to prevent this problem:

  1. Use getCapacityAsInt instead of getCapacity
  2. Mark the getCapacityAsLong method in the StacksResourceHandler as final. This would introduce a breaking change in the API of the StacksResourceHandler class
flat root
#

why didn't you override getCapacity?

cunning mulch
#

isn't getCapacity abstract? (aka must have been overridden)

eager knoll
#

Because I did not notice that there was a getCapacity method.

eager knoll
cunning mulch
#

I think given we are still in beta, the optimal solution might be to make getCapacityAsLong final as realistically there are no cases for the given impl where you can use values greater than max int, as it all works on the getCapacity internally anyway, so if you wanted a higher value you would need a custom impl.

flat root
#

I generally don't want to lock down the base impls because who knows what it might make sense to override

#

but I guess that in this case it might be better to make the method final anyway

loud stirrup
flat root
flat root
#

todo list ^^

#

I started thinking about opening transactions from onRootCommit and opened a PR already, but for now I only wrote a test

#

the basic idea will be that if an onRootCommit callback is already scheduled when the "recursive"/"inner" transaction completes then we just keep it scheduled as-is

#

the absolute worst case is scheduling an onRootCommit callback from inside the very same onRootCommit

#

not sure what exactly we should be doing in that case 😅

#

to give a specific example, it is possible that the drop call here triggers a new transaction which in turns triggers more stuff to be dropped; so should we schedule another onRootCommit?

image.png
#

probably yes

flat root
#

grrr I have to rethink the entire transaction state management 😅

loud stirrup
#

for the "recursive" tx?

#

or rather final-commit queue?

flat root
#

I have the final-commit queue figured out: there is a global queue in the tx manager and stuff just gets added to it

#

the trick is indeed the "recursive" tx management

flat root
#

I love this test 😄

image.png
#

5 levels of recursive transactions, what could go wrong 😄

#

(in reality there's only ever 2 levels in this case hehehe)

#

still a few TODOs in the base implementations, but please do already have a look 😄

loud stirrup
#

Oh that test is nice

cunning mulch
flat root
#

funnily enough I'm gonna have to implement this PR in MI 1.21.1 to fix some issues I have 😄

#

(MI 1.21.1 ships with a subset of fabric's transfer API, used internally only)

flat root
#

hmmm, in case of an exception should we really go through the rootCommit callbacks?

#

I encountered a funny infinite loop where an exception in onRootCommit does not prevent more onRootCommit callbacks from running, and they keep failing but only after enqueuing more callbacks...

#

it's trying to drop 1M items 😅

ripe crest
#

why is an onRootCommit callback enqueing more callbacks thonk that sounds very very odd

flat root
#

here's the test

image.png
#

basically I drop 2 carrots, and also register a listener for ItemTossEvent that will drop an additional golden carrot for each carrot

#

the event listener opens its own transaction which changes some state, and thus enqueues another onRootCommit callback 😄

#

to be clear I designed this test specifically to catch the CME in PlayerInventoryWrapper.DroppedItems#onRootCommit

ripe crest
#

ah yeah, i could see a situation like that happening with enough mods

flat root
flat root
#

OK now I fixed the base implementations, except for one: the composter

#

basically we have to perform the changes in the level immediately

#

we'll probably need a transactional Random 😅 to remain deterministic

flat root
# 
public class TransactionalRandom extends SnapshotJournal<Long> {
    private long seed = RandomSupport.generateUniqueSeed();

    @Override
    protected Long createSnapshot() {
        return seed;
    }

    @Override
    protected void revertToSnapshot(Long snapshot) {
        seed = snapshot;
    }

    public double nextDouble(TransactionContext transaction) {
        updateSnapshots(transaction);
        var random = new SingleThreadedRandomSource(seed);
        double rand = random.nextDouble();
        seed = random.nextLong();
        return rand;
    }
}

there it is

#

not so bad in the end 😉

loud stirrup
#

Hehehehe

flat root
#

good enough

image.png
flat root
cunning mulch
stone dragon
cunning mulch
#

reviewed the transaction stuff, mostly just trusting your impl is fine as I don't exactly have intuition on it. I may try to review the tests later, but for now my eyes just keep glossing over them xlurk

flat root
#

Thanks, I'll have a look in a bit

flat root
#

you think that this check is just a sophisticated isRemoved?

image.png
flat root
#

@cunning mulch I guess this is the last comment that I haven't addressed

image.png
#

my take would be to not check, and then add it later if it becomes a problem

#

because it sounds quite annoying to be checking if the BE exists / block exists in every single method; and since we don't want to be doing it everywhere I don't think it makes sense to be doing it here

cunning mulch
#

kaishrug I don’t really have a preference. In theory the cap should get invalidated if the block is replaced anyway? Is there any cases when someone from a transactional context might still be interacting with the old one (rather than just the root commit callback)

flat root
#

well if you keep a handler lying around then yes

#

but that's a problem with any BE as well

#

indeed the cap will get invalidated

cunning mulch
flat root
#

indeed

ripe crest
#

have an IS_RUNNING_IN_IDE check and throw if it doesnt exist to catch devs who dont invalidate in dev?

stone dragon
#

But yeah I'd say throwing when in a nonsensical situation is fine, garbage in garbage out and all

flat root
frigid ruin
#

is this channel appropriate for user questions?

flat root
#

Yes

#

There's more chance I see them here

frigid ruin
#

is there a FilteringStorage insertOnlyOf/extractOnlyOf yet?

# 
    private final SimpleContainer inputs = new SimpleContainer(9);
    private final SimpleContainer outputs = new SimpleContainer(9);
    private final SimpleContainer quotes = new SimpleContainer(3);

    private final ResourceHandler<ItemResource> inputStorage = VanillaContainerWrapper.of(inputs);
    private final ResourceHandler<ItemResource> outputStorage = VanillaContainerWrapper.of(outputs);
    private final ResourceHandler<ItemResource> exposed = new CombinedResourceHandler<>(List.of(
            FilteringStorage.insertOnlyOf(inputStorage),
            FilteringStorage.extractOnlyOf(outputStorage)));
``` and is this still a fine way for a simple inventory
loud stirrup
#

should be, i think

flat root
#

There isn't yet

flat root
#

I'd probably recommend ItemStacksRH but that's fine

flat root
cunning mulch
#

don't think so

flat root
#

Thanks

cunning mulch
#

Do note though I didn’t really look at the tests

vernal dust
#

Is there a function for List<ItemStack> to Map<ItemResource, Integer> anywhere?

#

Deduping and converting several stacks to a resource-count map?

silent root
#

Would this do what you want? soarynHmm

 public static  Object2IntMap<ItemResource> resourceCounts(List<ItemStack> items) {
        var openHash = new Object2IntOpenHashMap<ItemResource>();
        for (var item : items) {
            openHash.merge(ItemResource.of(item), item.getCount(), Integer::sum );
        }
        return openHash;
    }
#

Haven't tested, but I don't think anything like that exists in the examples nor do I know what they would need it for per se

vernal dust
#

It's part of my preamble for a treecutter component

image.png
#

Gather drops, prep as resources, prepare to yeet into other places

#

Although I'm now modifying this to just use ItemStacksResourceHandler and ResourceHandlerUtil#move instead

silent root
#

Just remember move will not do stacking if that behaviour is desired

vernal dust
silent root
#

Those by default aren't stacking in the vanilla examples

flat root
#

I'm happy to get rid of this 😅

image.png
flat root
weary marsh
#

Why does the createSnapshot method in ItemStackResourceHandler replace the contained stack with a copy and return the original one? Couldn't it just return getStack().copy()? Seems like many unnecessary setStack calls to me

frigid ruin
#

probably something to do with itemstacks being mutable, which cannot be kept in a journal

#

hmm but then wouldn't you want to return a copy, right

ripe crest
#

is it to enable it to be run after an operation that may already have access to the currently stored stack?

weary marsh
# weary marsh Why does the createSnapshot method in ItemStackResourceHandler replace the conta...

to add more context, my goal is to react to changes in my ItemStackResourceHandler. Since it has nothing like onContentsChanged, I thought I could just place my logic in the setStack method, expecting it to only get called once when placing/removing an item. However, during debugging I noticed setStack gets called a whole 13 times for each side (server/client), which seems very wrong to me

#

Just saw onRootCommit now, maybe this is what I'm looking for?

serene igloo
#

would it be viable to use ItemStacksResourceHandler (which does have onContentsChanged) instead of ItemStackResourceHandler?

#

but yeah onRootCommit is invoked after transactions finish and finalize

weary marsh
boreal plover
weary marsh
serene igloo
#

what operations are being performed on your handler

#

block interactions? menu stuff?

weary marsh
#

it's for a block entity menu. Used in a ResourceHandlerSlot

serene igloo
serene igloo
#

you'd have to put overrides in the slot to do stuff on change maybe

boreal plover
weary marsh
#

I guess ItemStacksResourceHandler would be the best choice for now then. Would be awesome if someone who knows the purpose of ItemStackResourceHandler could explan it though 🙂

weary marsh
flat root
#

Ok but have you checked why it's being called?

flat root
#

In any case don't place any special logic in setStack (which will get called many times because of transactions). onRootCommit is the right place to put it

weary marsh
weary marsh
flat root
#

I'd also expect it to be called two times only, so if you have any info (stacktrace) I'd appreciate it

flat root
#

Its set function (likely in a super class) calls onContentsChanged manually

#

So you'd have to do the same when using the slot (i.e. make sure that the IndexModifier also calls onContentsChanged)

weary marsh
#

okay I've changed my SingleItemRH now to only run the update callback in the IndexModifier (set method). However, when for example removing an item from the slot the set method runs 8 times.

flat root
#

Ok so when placing an item there's a call from Slot.setByPlayer and a call from StackCopySlot.setChanged

#

On each side

#

That's actually quite reasonable given how StackCopySlot works

weary marsh
#

I guess when placing an item thats acceptable (although ideally I would like to have a method that gets called only once to not recalculate my be state more times than needed)

#

I guess removing the item currently is the bigger problem as that would cause the state to be updated 4 times

flat root
#

What's the problem with updating the state 4 times? Player slot interactions are quite infrequent

versed finch
#

Can you explain to me why the moved variable is equal to 0? What should I change?

var itemAccess = ItemAccess.forStack(Items.WATER_BUCKET.getDefaultInstance());
var cap = itemAccess.getCapability(Capabilities.Fluid.ITEM);
var tank = new FluidStacksResourceHandler(1, 10_000);
var moved = ResourceHandlerUtil.move(cap, tank, __ -> true, FluidType.BUCKET_VOLUME, null);
System.out.println("Moved " + moved + " mB from bucket to tank.");
weary marsh
#

one fix for it though could be to check if the stack is actually different before calling onContentsChanged. If this is something wanted I could create a pull request for it.

This is what I've ended up with now in my ItemStackRH

public void set(int index, ItemResource resource, int amount) {
        TransferPreconditions.checkNonNegative(amount);
        if (resource.isEmpty() && amount > 0) {
            throw new IllegalArgumentException("Resource is empty but the amount is positive: " + amount);
        }

        ItemStack stack = resource.toStack(amount);

        // only set if the stack is different
        if (!ItemStack.isSameItemSameComponents(this.getStack(), stack)) {
            this.setStack(stack);
            this.onChanged.accept(this.stack);
        }
    }
flat root
#

I guess we could do that at the slot level, provided there's a comment making it sufficiently clear

#

At least in setChanged

flat root
#

Where is the water bucket stored?

versed finch
#

In a slot of a blockentity

flat root
#

Where should empty buckets go? Remain in place?

versed finch
#

It should replace the full bucket with the empty bucket always in the same slot

flat root
versed finch
#

Ok, i'll try

#

Ok, it works. Thanks.

flat root
#

Ok I haven't touched this in a while, is there anything to do?

#

Potential todo list:

  • Make getCapacityAsLong final in the base implementations that override it?
  • Add supportsInsertion and supportsExtraction methods?
  • Implement support for the new bookshelf (or whatever it's called... Shelf?)
distant merlin
#

the Shelf, I believe you mean

cunning mulch
flat root
#

Oh yeah there's that too heh

sudden shoal
mental raven
#

they look very Container hardcoded

flat root
#

I suppose we should look into that as well

mental raven
#

good luck

silent root
#

Now that we have JSpecify in, @flat root you'll need to see about fixing this up a bit, as this is not providing the correct information to the user in regards to what it is needing. The default implementation should not be immediate warnings.

The ONLY time it was ever be null is when the type is Void, as according to our docs and the original design it was never intended to be null.

image.png
flat root
#

It is unfortunately possible that intellij is wrong

#

This is on 1.21.11, right?

#

I can have another look

silent root
#

Well, you are telling the Type binding that it is nullable, but the method has no idea what you are doing as the package info is saying it is null marked. That type binding doesn't entail the method params use per se. It isn't so much of Intellij being wrong, but rather it is being told 2 different things

flat root
#

Well my expectation is that since the type parameter is Integer, which is a non-null type, the other methods using it should also take a non-null argument

#

The Kotlin compiler might be more accurate than intellij here, but that's more annoying to check

cunning mulch
noble latch
# silent root Well, you are telling the Type binding that it is nullable, but the method has n...

IDEA's jspecify nullness checks are plain broken in various scenarios related to generics: #squirrels-🦊 message
The code surrounding that screenshot has a similar "problem" where a dummy type may be needed to represent not caring about the value, except that I used Unit because null has a different meaning in my use case. Switching from nullable with Void to Unit could be worth exploring but I'm not convinced it's gonna make IDEA shut up

clear tusk
#

looking at the new snapshot output and i saw TypedInstance<T> might make sense for RegisteredResource<T> to extend this as it provides everything that iface does

its also implemented onto ItemStack so we might also want to implement it onto FluidStack too

image.png image.png
loud stirrup
#

yeah saw that too

vernal dust
#

inb4 Moj taking notes again

vernal dust
#

stab everyone who didn't notice the param order swap, including myself

image.png
flat root
#

Wtf is this 😅

#

That's a complete misuse of the function

#

It's meant to check amounts not indices

vernal dust
#

resource isn't empty
slot index is non negative
NonEmptyNonNegative
all unit and integration tests pass, cap'n

#

doc just says "value", does not refer to count or resource amount at all

image.png
flat root
#

For the index it's preferable to use Objects.checkIndex so you also check against the upper bound

#

The parameter order was chosen because transfer method signatures usually have T resource followed by int amount

loud stirrup
#

The replacements for FluidUtil#tryFillContainer & tryEmptyContainer are IMHO insufficient. As in, there aren't any with the same features 😄

#

The new FluidUtil#moveWithSound is kinda what you want, but it's private and has a wonky interface

flat root
#

Yeah, it is private because it has a wonky interface 😄

loud stirrup
#

Yes I mean something like it not the method as is hehehe

loud stirrup
#

Our ResourceHandlerUtils.move is non-stacking, right?

silent root
#

Correct 🙁

#

I had to make my own

cunning mulch
#

I thought we had one of each? Or was that the original PR? And it got trimmed with being able to add back later if people found it useful to have

loud stirrup
#

I find it useful to have. Let's add it back 😄

#

Sth along the lines of moveStacking

cunning mulch
#

(Make a pr and link it and I will review it)

loud stirrup
#

You're too meticulous 😄

cunning mulch
#

Then do it right!

#

It isn’t like there is much to a single method

#

Especially when there is a similar existing one, and there is the existing one in the old large PR that I already reviewed if you are worried that I will have comments

loud stirrup
#

I'd not copy that, it should be a trivial change but will also require junit tests

cunning mulch
#

Ah right the tests. Granted I tend to be less meticulous about reviewing tests than api stuff

loud stirrup
#

I chose not to copy paste all the existing tests given I just moved the existing impl into a new "internal" method with a boolean stacking parameter

cunning mulch
#

I will look at it later when I get back to my pc later today. From a quick glance it looks like it is most likely fine but with formatting on mobile I want to make sure the logic is correct

loud stirrup
#

Yeah it's just replacing the single call to to.insert() with an if (stacking) insertStacking(...) else to.insert instead

#

I also check there's really only one call to insert in those methods, but double-checking doesnt hurt

#

Oh, another util I missed: interacting with fluid tanks in UIs

#

There is FluidUtil#interactWithFluidHandler (and overloads), but they only deal with in-world interactions.

#

I want to have it play the appropriate sounds when interacting with handlers from within a screen

cunning mulch
#

have your approval

loud stirrup
#

ty

flat root
#

Looks good yeah 😄

loud stirrup
#

Right now, FluidUtil.getFirstStackContained crashes for an empty stack. Shouldn't that just return FluidStack.EMPTY?

flat root
#

oh yeah

#

it should be FluidStack.EMPTY

loud stirrup
hidden geode
#

:lgtm:

flat root
#

@lavish marsh I finally reviewed your PR (https://github.com/neoforged/NeoForge/pull/2797), sorry for the delay. I asked for a few small changes in the bundle handler but it shouldn't be too bad hopefully. I like how you implemented slotted access to the bundle

lavish marsh
#

for the top one max stack size is the bundle limit, would that condition already be checked?

#

for the bottom one why would it be divided

flat root
#

If you're inserting 10 items into 2 bundles you can only insert 5 per bundle

flat root
lavish marsh
#

ah ok

#

I readded the backpack cuz I realized its there to test custom sizes

flat root
#

Ah ok. The default component is a bit weird but it will do 🤷

sinful tiger
#

why is newResource not empty

image.png
#

(though if this is the first and only bug that's attributable to my failure to understand the API rather than a stupid typo in my code, i'll count that as a win)

flat root
#

If you put a non-empty resource in a variable then it will not be empty?

#

Basically you just asked "why is this int set to 2?" Without additional context I really can't help you

sinful tiger
#

when emptying an ItemAccessResourceHandler, it calls update() with these values [i.e. the resource it previously contained and a quantity of zero, rather than the empty resource]

silent root
#

In the case of the fluid one, it gets an empty from the fluidStack when it turns the resource into a stack with 0

#

The empty aspect in this case is more driven by amount rather than resource