#Transfer rework (IItemHandler, IFluidHandler, IEnergyStorage)

1 messages · Page 4 of 1

opaque vector
#

I would encourage people to DM me their feelings and thoughts about this. Hopefully a resolution will be reached by the end of today

opaque vector
#

Ok hopefully all the issues lately will become water under the bridge now. i have talked to everyone. And let's try and start on a blank slate here on how we feel about each other.

I am also half-tempted to reopen Adrian's PR as we have no rule about having two concurrent PRs trying to solve the same issue with different implementations. I think it would be good too so people can compare the two and see which one they prefer. Having both PRs does not invalidate either one so please, no one should feel defensive about their PR and try to take suggestions and ideas

distant merlin
#

the co-existence of NeoGradle and ModDevGradle serves as a large example of that kekw

loud stirrup
#

So ... you want two transfer APIs to co-exist in Neoforge? 🤔

distant merlin
#

nah, more talking about how two things that solve the same issue (how to build NeoForge) with different implementations can co-exist

loud stirrup
#

yeah but it doesn't really work that way with APIs we have in Neo

#

tools are just not comparable to that

distant merlin
#

though the expectation here is that either the two PRs converge to one implementation, or we pick one over the other (however that's done)

silent root
#

May the Survival of the fittest win!

wind steppe
#

After speaking with TG, I think it’d be good to reopen our PR. I think our PR is most of the way there, and if there are deal breaker things in our PR, then I don’t see why we can’t discuss them or bring over whatever preferred implementations exist in Tech’s PR to get this over the finish line. We’ve come this far, let’s get something done.

flat root
#

I will continue to work on my PR, as you can imagine

wind steppe
#

I would like to hear what you would change given the current state of our PR. Many of the comments have been addressed.

flat root
#

Most of it would be undoing changes

wind steppe
#

Well if we could lay out what those were, that’d be great

flat root
#

Quite literally: everything that I have done differently in my PR

hidden geode
#

That seems quite harsh.

#

Now I am going to have to re-read the prs myself.....

flat root
#

Sure, can always use more reviews

wind steppe
hidden geode
#

We already pointed out to Tech that better communication is needed from his end, and statements like "Quite literally: everything" are not furthering his cause

flat root
#

Just the transaction stuff that was brought over had various name changes applied to it that just don't make sense to me

hidden geode
#

So we are aware of how unhelpfull his answer was

#

Does not mean that he might be wrong

#

Or might not be wrong

#

So lets just keep it to the facts

wind steppe
#

Alright

hidden geode
#

Github is being amazing again and only loading half the shit....

wind steppe
#

Could we start with reopening my pr? I believe it’s still closed

hidden geode
#

Yeah I will

#

Reopened with the SC decision and statement regarding it attached

flat root
# flat root Quite literally: everything that I have done differently in my PR

And to be clear, I don't mean that in an aggressive way even though it might look that way, so apologies for that. I have years of experience with this stuff, and I know precisely what worked and did not work well in the previous iterations of this. My changes are specifically trying to fix mistakes made in previous versions of this API

hidden geode
#

Instead of redoing it?

flat root
#

(and, equally importantly, not changing things that have shown to be working)

hidden geode
#

Because from my reading, and I have been around the block also a couple of times on this topic, there is not really anything majorly wrong with their current implemention.

Regardless of your experience, it would be nice if you could articulate the reason why your PR should be chosen of theirs, which seems to be in good shape, in a consise matter on GitHub

flat root
hidden geode
#

Which either means you have local changes we can't fairly compare, or your changes seem to be covered by theirs to at least a certain degree

flat root
#

Yes, all of my changes were copied over overnight, which lead to this whole drama

hidden geode
#

And in an open source project that is completely okey

#

It might make their PR better

#

And your PR simply redundant

#

It would be nice if they gave credit

#

Which yeah I have to give tech right

#

The transaction stuff looks too similar not to be a straight copy with a rename

#

So some credit is for sure needed

flat root
#

What I mean is that it is significantly harder to write new code than to copy over what exists, which is why my PR might look "behind" right now

wind steppe
#

I can update it to give tech credit, Soaryn didn’t tell me how the transaction stuff was added but it makes sense if it came from their PR

#

I do have to say that if it was that simple to use your changes then we could have used our PR from the get go

hollow maple
#

My understanding WRT the transaction stuff was that it was seen as a requirement for the PR to go anywhere if it were to be reopened, and yes, I do believe most of the implementation was lifted partly to demonstrate that it was a trivial change

hidden geode
#

Agreed the transaction stuff is pretty trivial

#

I had a pseudocopy of this implementation in my white paper of nearly 8 years ago

#

It also looks similar to Fabrics

#

@flat root Are there any major differences that I need to look out for here? Or is this a plain copy-on-write snapshot mechanic?

wind steppe
#

Despite all of the drama that surrounded this, I am really happy that we finally got to transactions in Neo. It does raise the question again about touching the energy handler? Initially we weren’t changing it enough to justify touching it, but now with the addition of transactions it might feel appropriate

hidden geode
#

Personal opinion: transactions are massive niche usecase

#

And should be available to power users

#

But should be wrapped behind much simpler interfaces with a similar API to what we have today for the day2day users

hollow maple
#

I'm gonna be honest, the energy handler should be a wrapper over something like ResourceHandler<EnergyUnit>, which I believe is one of the sample snippets Soaryn wrote in Adrian's PR

wind steppe
#

I think their usage is fairly easy to understand

hidden geode
#

But we do nothing else then heavy coding every day

#

Day in day out

#

For newcomers

#

And casual modders

#

The existing system is much easier to understand

#

Put item in

#

Extract item

#

Done

#

Nothing more easy then that

wind steppe
hidden geode
#

It is also alligned with existing mojang code

#

Which makes finding similar examples a breeze

#

Transactions are modding only constructs

hollow maple
cunning mulch
wind steppe
#

Nah Sara totally endorsed it

hollow maple
#

Hey, ultimately I'm just one person, I'm just thinking "we already wrote this once, the infrastructure is already there, why don't we just reuse it?"

hidden geode
#

Sooooo

opaque vector
#

Side question, was this poll’s result respected in the PRs?

flat root
flat root
hidden geode
#

@flat root Is there anything in your PR that is not in theirs? (I see changes to bookshelfs etc, which I seem to be missing from theirs, but which I consider to be trivial needed adaptations to the new system)

hidden geode
flat root
wind steppe
#

oh right chiseled bookshelf

hidden geode
#

And that means priori art exists for modders to draw experience from

hollow maple
#

...Complexity?

hidden geode
cunning mulch
wind steppe
#

im fairly certain i handled most of the places where handlers were in the code before

#

like hoppers

hollow maple
#

To me, complexity is the code that's duplicated for no reason because if there's a bug in one there's almost certainly a bug in the other

hidden geode
hollow maple
#

And it's just easier to make a mistake

hidden geode
#

Just missed the patch

wind steppe
#

oh

flat root
hidden geode
#

@wind steppe There are some open questions with regards to memory leaks on the caches.

Rightfully you make most of the caches have weakvalues.

But you also use containers as Keys............

hidden geode
wind steppe
#

if you could be more specific thatd be really helpful

flat root
#

These caches are copied over from what I did in fabric and have been shown to work for 4 years

hidden geode
#

Am I missing something?

#

That is not immediatly obvious?

opaque vector
#

Also for extra context that I think is important to bring up is Technician did do Fabric’s Transfer API so if his PR is a near copy of it, then it does mean it’s general implementation was battle-tested a bit in fabric ecosystem. Not to say Adrian’s PR has no shot but does mean it has to consider what was tried and true. Just keep that in back of mind

hollow maple
#

I understand the whole YAGNI argument, but it isn't really YAGNI when IEnergyHandler is just an IResourceHandler<EnergyUnit> with a bunch of helpers

hidden geode
wind steppe
#

I did also base alot of what I did based on Fabric's Transfer API, which i am also quite familiar with through the process of writing my transfer library abstraction

flat root
wind steppe
hidden geode
wind steppe
#

i am more than happy to defer to you about performance claims

#

i am not an expert there

hidden geode
#

It is more of a memory issue

#

I am okey with the maps existing

#

But I though weak keys were needed for stuff like that to work

#

Especially since you also hold levels in some places as keys

#

And you know......

#

We have cut ourselves on those more then once

flat root
#

It has to use weak values because the values often refer to the key

hidden geode
#

So it is stuff I look for when reviewing this

hidden geode
#

But I was more thinking of both

#

WeakKeys and WeakValues

hollow maple
#

Personally, I'm not sure the caching is necessary. It should generally be cached by the capability cache anyway

flat root
#

Technically it doesn't make a difference if the value has a strong reference to the key, which is why I never bothered

hidden geode
#

Meh

#

We can always add it

#

Regardless of which way we go

#

Iterate and move on

flat root
hollow maple
#

wdym?

hidden geode
#

If you have two wrappers

#

that means that there could be two distinct states which can colide

hollow maple
#

Ah, is this for the snapshot restoration when a transaction is closed?

flat root
#

Yes it is related to that. You don't want competing snapshot stacks

hidden geode
#

Yep

#

That is one example

hollow maple
#

Right, that makes more sense now

hidden geode
#

There are many other combinations that you can get to

#

There is technically two ways to solve that

#

One is a single state approach (taken here)

#

In the past I also researched the multi-state with reject if mismatched approach

hollow maple
#

If the various container implementations implemented the transactions natively (say, through a patch) I assume this caching wouldn't be necessary

hidden geode
#

The later is simpler to implement

#

But has concequences for the API consumer that they need to handle the case that their snapshot might have become invalid in the mean time

#

And that is annoying to deal with

past lodge
# hidden geode For us yeah

transactions being explained like how say, postgres does it makes it incredibly easy for most people to understand how they work

loud stirrup
#

What I did as more of an experiment in Techs PR is implement IItemHandler and IFluidHandler as-is on top of Storage<T>

#

In essentially an auto-commit kinda way

#

Not sure if that ends up being useful or not 🤔

wind steppe
#

i believe our PR did that as well

hidden geode
wind steppe
#

its been a while, i think that was one of the first things i tried

loud stirrup
#

I am not 100% certain that those implementations are not just transitional helpers, really

flat root
#

yes, I have also been thinking about a clean-ish migration path

hidden geode
past lodge
#

this might sound mean but, if someone disregards reading docs or actually learning the basics they are willing jumping into a hole

hidden geode
past lodge
#

nothing you do can save them

hidden geode
#

Sometimes we have to keep the dumbest possible modder into account

#

forgot the nice german term for this

#

Ah tehere we go

past lodge
#

also i would like to mention that if neoforge doesn't correctly implement transactions for jukeboxes you forfeit your spleen, create fabric gotten far too many issues regarding that and i don't want the same for create forge stabolb /s

wind steppe
#

not for nothing but there are many a people who are confused about the transaction system. That was me at some point

hidden geode
#

"Der dümbste anzunehmende benutzer"

hollow maple
#

You can design your APIs to be as foolproof as possible, but the universe will always create a bigger idiot

hidden geode
#

Or in this case

#

Which ever modder uses MCCreator

#

/jk

flat root
hollow maple
#

Speaking of MCCreator, I got added to a random mccreator project on curseforge which supposedly intentionally makes your game run slower

#

lmao

hidden geode
#

FUCK

#

I forgot to check of which files I viewed

#

Back to top

#

Forgots sake

#

These PRs are both to big

wind steppe
#

well

hidden geode
#

I know i wrote For Gods wrong

#

But you know

wind steppe
#

handlers have been around for almost a decade now, swapping them out wasnt going to be a small task xd

past lodge
hidden geode
#

That makes me feel old

#

Jesus

vernal dust
wind steppe
flat root
#

what I like about this implementation compared to Fabric's is that we can simply add the few tweaks needed to the Container interface

#

instead of having to write 10 mixins for it 😅

vernal dust
#

I remember experiencing EE1/2 power flowers when they were a new thing... in 1.2.5...

hidden geode
#

Yeah well

#

That is NF Source patching for you Tech

#

It has its moments

wind steppe
#

ez, just add patches to fabric

vernal dust
#

Neo and Fabric do a trade: Fabric gets Neo patching, and Neo gets Fabric loader working on multiple versions

hidden geode
#

@wind steppe Are you trying to make people mad at you? Ints for energy exchange...... puh

vernal dust
#

can we NOT use longs for energy, please

#

we don't need MORE power creep

hidden geode
#

hehehehehhehehehhehehhehe

wind steppe
#

Im not gonna speak on behalf of Soaryn, but he raised several good points about why we shouldnt use longs for transfer

hidden geode
#

Ints are perfectly a okey

wind steppe
#

i will ping him again to join the discord

flat root
#

ah, another fun topic. My PR is currently using longs 😛

hidden geode
#

Then always have been

vernal dust
#

"You want to smelt my raw ore? Really? 50 thousand RF please..."

wind steppe
flat root
#

nothing is decided really

vernal dust
#

istg if Neo goes long energy CM7+ tunnels won't have energy by default

wind steppe
#

well except the things we agreed on!

vernal dust
#

someone can addon mod that in

flat root
#

I think it's hard to say without actually trying it in practice

flat root
#

all we can do is make an educated guess for now

hidden geode
#

@wind steppe You have the legacy -> modern conversion methods commented out, is that on purpose? Are you planning to reactive them again?

wind steppe
#

please be more specific 😭

hidden geode
#

When can I expect those?

flat root
#

my estimate is that my PR will be ready for review in about a week

#

but I am being a bit conservative on purpose 😛

#

might get to a good enough state for PR publishing in a couple of hours for all I know

hidden geode
wind steppe
#

oh

flat root
#

most of the work was getting the transaction scaffolding reimplemented, and that is now mostly done

wind steppe
#

i uh

void thicket
wind steppe
#

dont know what that is. that must have been a soaryn addition

hidden geode
#

Don't worry @flat root I will be reviewing it all weekend

hidden geode
#

Additionally where has: ItemHandlerHelper#calcRedstoneFromInventory gone?

#

Am I blind or is there simply no alternative in your PR?

wind steppe
#

i believe its in a more generic version now?

past lodge
#

longs are annoying for stuff like items and fluids because of the int count thing

wind steppe
past lodge
#

however energy should probably be a long because you don't have that issue

#

you likely won't be casting to int anywhere

#

or well, be forced to

hidden geode
#

Okey, people, I am of to bed and hand of to @opaque vector to sheppard you guys and keep you guys on track.

I got through 2270 pretty well, but it is a smallish PR, so that is understandable.
I am 53/188 through 1115 so I have some ways to go tomorrow.

Have a good night, and be nice to each other!

flat root
#

good night!

vale birch
fossil cave
#

What about complex amounts of items?

#

I encounter 3.5-7i items all the time

flat root
#

int or long complex?

#

if it's an int just pack them as a long

flat root
#

ok, I think we'll manage to provide modders a reasonable migration strategy

#

(except maybe for IFluidHandler, TBD)

#

at least on the user side, it should be relatively easy to get the code working again, such that modders can first take care of fixing the vanilla-induced changes, before they have to make major changes to their transfer code

loud stirrup
#

I'll try to port the fabric tests

#

Maybe feasible, maybe not

flat root
#

I think they are gametests unfortunately

#

hopefully many can be turned into unit tests

#

would be good to see if anything broke

loud stirrup
#

Hm, I am looking at a set of unit tests

flat root
#

ah, right

#

it is in fact split between unit tests and game tests

loud stirrup
#

There are also gametests for the vanilla game object wrappers

#

Which is sensible

flat root
#

looks like I did this "correctly" 😄

loud stirrup
#

But yes, JUnit is preferrable for the class-based ones

#

So far, yes 😄

#

The tests could be better 😛

#

(structured)

flat root
#

pff

loud stirrup
#

Hehehehehe. That's just a matter of style, I know

#

I oscillate between having many assertions in one test method, and doing @nested + many methods with a single assert each

flat root
#

I don't use @nested, always the former 😄

#

ok, I need a way to apply the component patch of one item stack exactly to another item stack 🤔

#

ideally without allocations 😄

loud stirrup
#

Hm

#

Funnily enough what I did implement is the ability to merge a patch into variants

#

which is not what you want, I guess

flat root
#

no, it's very specific

loud stirrup
#

However, when you say "apply", you mean while keeping the ItemStack reference?

flat root
#

yes exactly

#

it's a big "hack" inside the Container wrapper

#

essentially it will try to apply to change to the original stack reference in onFinalCommit, or clear out the original stack if it can't

#

not sure it's necessary to have it there though; it might be enough to move it to a context implementation 🤔

loud stirrup
#

Mhmmm

flat root
#

it works at least (second run is after hot-reloading 😄 )

image.png
loud stirrup
#

Oh, now I think I get it

#

Interesting concept, really

#

Fabric accessor mixins its way to victory, or what?

flat root
#

wdym?

#

BTW look at this. This is all that is needed to do to migrate from the IItemHandler to the Storage capability. (At least for an initial port)

image.png
#

and the test passes 😄

loud stirrup
#

yes that is very nice

flat root
#

(the special sauce I mentioned above is needed to make StackStorageContext correctly mutate the stack reference it received)

vale birch
#

Btw does this make it easier to create a damned item that can hold a specific type of fluid with either 0 or 100mb in it?
Last time I tried to do it I got a headache xd

flat root
#

yes it does, although that's just a side effect from trying to design new "template" implementations that are more flexible than the previous ones

vale birch
#

Now if we only got the containers and screens easier as well... I might actually finish making my mod...

flat root
#

ok, bringing over EntityEquipmentInvWrapper to transactions is annoying

#

it calls LivingEntity#setItemSlot which of course performs a ton of side effects 😄

flat root
#

@stone dragon I am running into an interesting error with immaculate:

image.png
#

immaculateApply does not seem to fix it

#

okay so I changed that file, reran immaculateApply, and now javaImmaculateCheck is happy again? 😄

stone dragon
#

WTF

#

Lemme know if you can reproduce it, but... uhh... WTF

flat root
#

could have been some caching issue maybe? it's very weird

#

how does immaculate even end up with an empty error message 🤔

stone dragon
#

That's what I'm wondering

stone dragon
#

Hmm... that error message is supposed to contain the original and final contents of the file

#

Could be some sort of wacky race or whatever where something else wasn't finished saving the file when it ran? IDK

#

...But then it wouldn't have happened when you ran apply

#

What it displays is effectively a diff

#

So it looks like it ended up with a diff that was empty from two things that were the different somehow still? ...which is weird

flat root
#

maybe some trailing whitespace or CRLF

stone dragon
#

But idk

flat root
#

I guess we'll see if it happens again 🤷

loud stirrup
#

Thing not testable in unit test: filling a bucket with milk, but ok.

flat root
#

alright, the capabilities for IFluidHandler and IItemHandler are now gone

#

for all 6 of them, there is a reasonably easy way to wrap the new capability in the old interface

silent root
silent root
wind steppe
silent root
#

I'm not doing that, but showing examples of what is and is not possible is something the api (especially in tests) SHOULd do

void thicket
#

huh?

silent root
#

It isn't in main api, it is in the Debug tests game test stuff

wind steppe
#

I don’t think shadows was referring to anything you wrote specifically, just not liking the idea in general

void thicket
#

Yeah I have no idea what you're referring to, lol

wind steppe
#

We were talking about energy resources earlier in the channel

silent root
#

Ah lol I see (then I am taking things out of context, my apologies)

void thicket
#

I just don't want energy units. Or energy slots, (or god forbid, energy data components)

silent root
#

Energy data components are... already a thing though?

void thicket
#

No I mean like "data components on the energy unit"

wind steppe
#

I don’t think slotted energy is a bad idea, I think we had a fruitful discussion about them in the separate thread about energy handler changes

silent root
#

Oh oh oh

void thicket
#

That would be haunted™

wind steppe
silent root
#

Slotted energy does have quite a few benefits, but that is separate

void thicket
# wind steppe God no

Granted, I'm sure a system like GT energy would love to throw voltage/amperage information onto an energy "stack" lul

silent root
#

So, I keep seeing "Suboptimal" thrown around for our pr, but I have yet to see a concrete example. Anyone care to share?

#

As I'm all for slicing and dicing the implementations to improve them, but the backing interfaces and such are incredibly solid

wind steppe
#

I think Tech had some concerns about how the PR brings over the transaction changes? I don’t remember if anything specific was brought up

silent root
#

"Has concerns" is definitely not concrete to me. That just sounds more like "I didn't write it so it can't possibly have been done correctly" And there is a chance, but I need exact examples

#

Obviously not expecting you to know 😛

#

But no one can improve if you don't tell them how they are doing it wrong soarynLurk

wind steppe
#

We have definitely had some headbutts, let’s try to stay positive xd. I think Orion did mention some things about the specific transaction implementation, I’m sure he’ll clarify in his review over the weekend

silent root
#

The Handlers that we originally had that did item components I know feel scuffed

#

But that is because of the last minute pivot to transactions

#

AH

#

I did just now see ours is reopened

#

(Just ended stream so catching waaay up)

#

Thank the nintendo switch 2 controllers for dying early on me soarynLOL

wind steppe
#

Lmao. I forgot that came out today. I feel like time has flown by since they announced the release date

wind steppe
#

yeah i agree @silent root, i think they are good to have but I don't love the way they are nested there. I ended up doing it this way because I saw this patten in other neo classes and wanted to be consistent, but it'd be better to just extract them out imo

image.png
silent root
#

So far it has been nice to just type SomeHandlerType.Modifiable

#

without having a namespace of ModifableSomeHandlerType or
SomeHandlerTypeModifiable

#

but that is again, going to be all over the place to who prefers what

#

THAT at least is easy enough to change with f6, if it really is deemed necessary

#

I've definitely leaned more towards the nested variation of it in the end after use

wind steppe
#

yeah it was fine initially but towards the end, especially with how many there ended up being, it started to feel less good

noble latch
#

I skimmed your PR earlier and two things stood out to me:

  • At a glance the storage container system made zero sense to me. Why do we need yet another layer on top of the resource handlers?
  • Removing the basic EnergyStorage implementation is not gonna fly. I know Soaryn likes (ab)using data attachments for everything but simple use cases with an energy storage held by a BE shouldn't be forced to either use an attachment or re-implement the basic EnergyStorage
silent root
#

Wait the energystorage is removed? Didn't you revert all energy changes back last year Adrian? soarynLOL

wind steppe
#

I thought i did?

silent root
#

Seems like no

wind steppe
#

Maybe i changed the item one

silent root
wind steppe
#

whoops

silent root
#

LOoool

wind steppe
#

yeah uh, lets revert those changes

noble latch
#

👍

silent root
#

I'll let you handle that one, the energy changes WERE originally part of this but it was moved to a different PR that Im holding off on until after this one

#

as it keeps the names the same standard as these

wind steppe
silent root
#

I don't want to force anyone into attachments, but those are very much a good example use case to give to people

#

If you are talking about the resource containers then that is an example template I have in XyCraft where the data and the handling are separate (which is true for capabilities anyway)

noble latch
silent root
#

Of course

wind steppe
#

you want like, block entity specific implementations with simple toNbt/fromNbt methods?

silent root
#

The container one is my more polished example, but Adrian has several in there for both

#

ItemStorageHandler as an example

#

has both attachment and component sub classes

#

The container is just too mutable for component data really

noble latch
wind steppe
#

makes sense, seems simple enough

silent root
#

Then yeah there are several item versions available in the template,

ItemStorageHandler.Component
FluidStorageHandler.Component
Then there is a different one for storing just a single resource stack in teh tests

# 
   DeferredHolder<DataComponentType<?>, DataComponentType<ResourceStack<ItemResource>>> SINGLE_ITEM_CONTENT = Registry.COMPONENTS.register(
                "simple_item_content", () -> DataComponentType.<ResourceStack<ItemResource>>builder()
                        .persistent(ResourceStack.codec(ItemResource.OPTIONAL_CODEC))
                        .networkSynchronized(ResourceStack.streamCodec(ItemResource.STREAM_CODEC))
                        .build());
#

That looks... scary in discord

#

ResourceHandlerTestSetup

#

Has all of the component setups

#

ComponentResourceTests should be testing those

#

Or a couple at least

noble latch
silent root
#

So you could bind it all up to the same thing, but realistically providing something like this

  itemContainer = SimpleItemResourceContainer
                .from(items)
                .onChange(this::markBlockEntityAsDirty)
                .build();

And maintain the container as one component, then pass the handler off to something else with it

itemContainer.asHandler(IHandleIOBehaviour.EXTRACT_ONLY);

so they only had a low view of it. That is, just really a difference of implementation, either way works. Could make it all one monolithic class, but it is how they were structured in XyCraft from personal use that made this.

#

I personally don't mind if we don't ship the container templates, but I left it in to ensure they were able to be made the way they were

#

As while they don't necessarily need to be in, the ability to make them, I feel, does

#

They HAVE helped isolate improvements to be made when designing at the very least soarynLOL

#

Arguably in the past there were reasons to go from our container to say a vanilla container, but I don't see that as being as useful generically nor as much anymore

#

As an example of how I actually use it in xy for items / fluids TestResourceContainerAttachment has some building methods if you haven't seen them yet

wind steppe
#

I uh, did not see that you had added this. Im gonna be honest, I dont think this needs to be in neo proper?

noble latch
#

I personally think the simplicity of just having the handlers and wrapping one handler in another one for sided IO and similar filtering tasks is preferable, has worked perfectly with the existing API for years and seems like it should continue to work just fine with the proposed resource handler API

silent root
noble latch
wind steppe
#

._.

silent root
#

LOOOoool so the reason for the builder pattern:

wind steppe
#

I totally get what the usecase for it is, but i think the PR already sufficiently covers the basic templates a user might need to get started without writing their own

silent root
#

Trying to find the class with like 5 constructors

#

I just knew for certain those worked exactly as expected and were decently performant, but yea they can be rather overwhelming at first glance and most users would likely ask "Ok... but how item?"

#

AH infact energystorage soarynLOL

#

has incrementive constructors where I really only maybe need to change 1 of those, but I need to pull the entire chain in to do the one I want

#

where as a builder I can pick and choose

#

BUT I think I left the constructor open too on the containers

#

so the user gets to pick which they were more comfortable with

#

(Again, these don't need to stay, just pointing out a benefit of the builder pattern with this chain)

#

Oh Adrian, I did change the name of IItemContext, not sure if you saw

wind steppe
#

what

silent root
#

Wanted to try to clarify it is a capability context

#

IItemCapabilityContext

wind steppe
#

I leave you alone for 5 minutes

silent root
#

LOL

#

Well... several months

wind steppe
#

shhhh

silent root
#

I changed that a few days ago though soarynSip

#

It seemed like peopel were getting confused on IItemContext not being a handler

#

so I tried to help clear up the nature of it

wind steppe
#

Personally i liked IItemContext. It can be a handler, but it isnt always a handler

silent root
#

True

#

And the name wasn't bad for sure

wind steppe
#

actually i need to check which default implementations I made for that, I think fabric has a few that I missed

noble latch
silent root
#

There are a few cases, but a builder shouldn't be feared. It only wishes to help you... it can't... won't hurt you this time

noble latch
#

Looks at the button widget builder that's often more annoying than helpful kek

silent root
#

my ui buidler soarynFine

#

Regardless, backing up to the base line handler, is there anything there you see in your reviewing?

#

allowsInsertion has been noted by @clear tusk that it may lend itself to the wrong understanding, but I'm having troubles rationalizing other names for it that help inform the user that this doesn't control the logic, just helps identify which indices/handlers are insertable/extractable

#

The doc is definitely.... wordy

wind steppe
#

We have a similar builder style structure for our UI mod

silent root
#

Like I wrote almost a TOS ignore me level of wordiness

image.png
noble latch
wind steppe
#

anyways, its clear I need to refamiliarize myself with the PR. With all due respect, I am gonna change the context name back to IItemContext as there is no universe where Tech isn't gonna argue against that name change and I wouldnt disagree with him

silent root
#

If you do, can you change the PR notes md as well

#

I don't mind either name.

#

Though we should consider calling it something like ISamantha or ITim

#

Can't lose people's minds then!

wind steppe
#

i also think that its extremely unlikely this container system gets past the review process, i think itd be much easier if we just.. remove it for the sake of making it more digestable for the maintainers

noble latch
silent root
#

Supports does sound decent enough to me

silent root
#

Again, when making edits it is best to ensure it compiles rather than make a mistake midway through and be like Woooopsiees doopsie

#

But will do so

#

Really the only files we ACTUALLY need:

  • IResourceHandler<T:IResource>
  • IResource
  • Transaction stuff
  • IItemContext
  • Vanilla implementations

templates are needed sure, but what we have for them can be divied out however

#

Everything else is purely implementation that CAN be done on a modder's side. ResourceStack is HANDY but not necessary until you add in ResourceHandlerUtil (which we should have)

#

ItemResource and Fluid Resource is assumed in IResource btw

noble latch
#

Oh, another thing that seemed weird to me: what's the thought behind the registered resource thingy, especially if basically nobody can rely on it being implemented?

silent root
#

Originally I was thinking it was going to make some of the resources nice to ensure implementation, but.... that one does feel unnecessary

#

I had plans of making a couple utilities for it, that would work cross say fluid or items or anything else that has a holder.

#

But I dont think that really feels great atm

#

So we may just "inline it" and just have IResource only

#

I'll go ahead and remove that as those ideas don't really seem great after thinking about it more

noble latch
#

👍

silent root
#

Oh intellij thank you

#

Inline removed the class and pushed it all down soarynChamp

#

Anything else we should remove? 🙂

#

I've moved the container stuffs

#

And inlined the proxy resource interface

wind steppe
#

How are we handling migration of older users? In my initial branch I had simply made the existing IItemHandler extend IResourceHandler<ItemResource>. But it doesnt seem to have that, and this adapter class is commented out

#

I dont remember adding this adapter

silent root
#

Adapter class I had troubles determining how snapshots should look

#

I added it when it was without transactions

noble latch
silent root
#

Okeydokie!

#

Sleep well buddy!

#

I'd recommend focus be on these classes more than anything #1183818213134446742 message

#

Or anything else in the resources package as those are likely all that need to be super hashed out since they are the structural pillars of the PR

#

Doesn't matter how much you stack on top, those few are the thing that people need to agree on

silent root
wind steppe
#

well thats a problem

silent root
#

Indeed

#

So the question: do we make a adapter for a legacy -> handler? Iassume yes

#

so the old cap tokens are just not viable

#

but something like asResourceHandler() on each of them for a temp solution

wind steppe
#

ive layered simulations over transactions, but doing the other way around seems extremely impractical. You fundamentally need rollbacks, simulation based systems dont have those

silent root
#

right

wind steppe
#

idk how youd do that tbh

silent root
#

which is why I had the legacy adapters commented out as it was SIMPLE without transactions soarynLOL

wind steppe
#

yeah it was just a matter of conversion

#

Techs PR does not seem to have Legacy -> Handler, just the other way around

silent root
#

If we could guarantee they were modifiable, it'd be ok to roughly do, but I'm not seeing a clear Legacy -> New

#

New -> old doesn't solve anything really from what I can think of if we remove the cap tokens

wind steppe
#

yeah i dont see a use for that short of people just wanting to keep their old code or if they want to do simple stuff without txns

#

which is fair i guess

silent root
#

While I can't think of a programatic solution yet, I DO think a link to a primer explaining how it is expected would at least be beneficial to the end.

#

BUT that.... still doesn't solve their needs 1:1

#

I'm open to suggestions though soarynSip

stone dragon
#

General question (my apologies if this is answered somewhere above that I've missed): will transactions work for energy stuff too or just for item/fluid/etc. stuff? (aka will I be able to incude energy transfer as part of a transaction moving items/whatever else around)

silent root
#

I have a sibling pr that I was working on before but was waiting on this one to finish first

wind steppe
#

It will eventually, i mentioned it earlier but idk if energy is on the table again

silent root
#

I'd like it to, but depends on how we do this

#

The pr I have on that one is also pretty much done, but this PR needs to be first as its goal was to match the names and implementations just with a primitive backing type instead

#

I don't know if there is a "reserve to make PR" or not at all I can go do

stone dragon
#

Tossing my thoughts in, I would very much like to be able to do energy stuff as part of transactions. But obviously that could totally make sense as a separate PR, so don't let me pull stuff off the rails

silent root
#

Oh yeah yeah I agree

stone dragon
#

Now that transactions are finally on the table this has gotten me excited to revive a mod I haven't touched in a good while... I'll be watching how everything falls out with interest!

silent root
#

Again, I have no idea if we can "reserve the ability to make a PR or not" as I need this one (in some form) to base it on

#

but it should basically be a 1:1 to IResourceHandler just squashing out the resource parameter

#

@wind steppe To confirm, containers out; and the IRegisteredResource thingy out. Nothing else?

wind steppe
#

the heck is a registered resource

#

:(

silent root
#

It was a middleman to resources that were expected to had data components. I ORIGINALLY wanted to make some utils for it. Those just didn't pan out

#

Simple is better

wind steppe
#

Orion mentioned there were a few random comments in there, he asked we clean up the PR

silent root
#

I'll check

wind steppe
#

i think it should be in a state where we arent making last minute removals from the PR if it got approved. Just remove them altogether instead of moving them into a cleanup package

silent root
#

Fair

#

You know item components reasonably well yes?

wind steppe
#

yeah, whatsup?

silent root
#

Can you check on the implementations of the components we have ItemContainerContents we added (probably a me thing, and possibly fine) just wanted to be sure that and the container components were to your standard

wind steppe
#

Oh right, i think we had spoken about that on stream at some point

silent root
#

Admittedly the vanilla impl prob need a review as well

#

Yeaaaa, one of the reasons I use data attachments so much, is most of my usecases are block bound. So I usually don't need nor have experience with data components fully

#

In tests they functioned as I'd expect, but wasn't sure if they were necessarily optimal

past lodge
#

energy stacks and data components for energy stacks would be quite funny

#

and silly

silent root
#

We don't need energy stacks soarynLOL

#

Funny! but no

past lodge
#

what we really need is a energy fluid /j

silent root
#

Energy is a fluid, what do you mean?

#

Typeless fluid

wind steppe
cunning mulch
silent root
#

LOL

cunning mulch
#

Granted that is just the texture we use for joules but still

silent root
loud stirrup
ripe crest
#

that would actually be hell to calculate given how many mods add random ways to make things renewable lol

loud stirrup
#

That'd be a nice april fools though 😄

hidden geode
#

420%

#

always

flat root
flat root
past lodge
#

but that's still quite funny

wind steppe
# hidden geode But should be wrapped behind much simpler interfaces with a similar API to what ...

Following up on this, how did you envision this existing in Neo? Keeping around the old interfaces? I feel itd probably be better to just create some kind of record that acts as a wrapper for the existing API that has something along the lines of the old structure. I see the utility of having something like the old stuff, but dont think we should keep the old stuff around long term since ultimately one of the aims was to unify how people interact with item/fluid transfer.

flat root
#
  • slight reactor
  • 300 lines diff
    kek
flat root
#

@wind steppe @silent root I have closed my PR in favor of yours, and I will write a thorough review in the coming days. Let's make this rework happen.

I would like you to add me as a co-author to the commits that have been derived from my work. This will require force-pushing so I get that it's a bit annoying, but I think it's a fair ask.

Finally, it would be good if your PR were retargeted to 1.21 temporarily such that it can be tested in as many mods as possible.

silent root
# flat root <@218557488142876683> <@95734205660528640> I have closed my PR in favor of yours...

First I wanna say thank you! I do want this to work out and I know how much you work you have put into what you have worked on. This wasnt an easy decision I imagine so thank you.

I do want to come to an agreement with this before we do so, (just to try to make this smooth) as I think you will be a positive influence when it comes to performance.

In terms of function on the vanilla implementations, I'll defer to you for most of those as I'm fairly confident you have more experience in that domain. Go ham! I'm very interested in what we can do about that map in the container wrapper.

In terms of naming: I feel it may be best to Let Adrian and I handle that and bridge the gap with what we intend to what the community feels. There are going to be points we disagree, but I will say Adrian and I have a pretty good repertoire when it comes to hashing out what sounds like a reasonable and (hopefully) intuitive name to start with and finding a reasonable compromise with what feedback we receive. These will change pretty quickly to the "solidified name" rather than waiting as it helps reviewing in the long term.

Longs: no. There is a large (large) conversation to be had on those, and I know how much it would benefit mods like MI and afew others, but for now, we are going to focus on ints. Something like the energy handler atm in the pr, has a util that returns a long as an example, but that is because of what it is doing not the interface level.

And the last point I can think of (was going to bed) any changes that you would think the base interfaces need (the handler interface, IResource), I would like you to bring those ideas to me and Adrian first before going crazysoarynLurk . I trust you and I dont imagine one day waking up to see all of thr interfaces changed for a new paradigm soarynLOL but I want to be sure that the reasons match the intent. I feel pretty confident on what we have on the api foundational layer though @hidden geode @vernal dust and I were hashing out a interesting notion you may be able to help with in terms of simplistic composable utils. Transaction interfaces are your code so and you definitely know functionally what is best I feel, but I will follow Orion's advise pretty closely if he has an idea.

#

That all being said it also depends on adrian

wind steppe
#

Aaaaaa Soaryn it’s too early to be reading a wall of text

silent root
#

Lol I wanted to be verbose!

#

I think what we can create is a pretty solid thing, and I think foundationally we are pretty good, implementations/templates could always use polish or even reworks

#

Importantly Tech, I dont want you to think I mean any ill will with any of these requests, it is purely to play to our strengths and alleviate confusions.

silent root
vernal dust
#

WALL indeed

silent root
#

SorrrryyyysoarynSad

#

"Join discord join discord! Aaaaah dont use discord to chat you heathen!"

vernal dust
#

Read quickly though, agreed on several good points

wind steppe
#

yeah agreed. But since Soaryn was the one who did the transaction stuff I’m putting that on you, Soaryn, to add tech as a coauthor. Git is my sworn enemy

vernal dust
#

To summarize the one point, the idea was to try and make the actual operations functional interfaces, and leave implementation to the different types - so you gain both low level power via direct operation control and high level simplicity via helper functions/classes

silent root
#

Can I? As not pr author? (Googling or git wizards later) sleep is soon (like minutes) but can look into that tonight)

vernal dust
#

We hit the Java type erasure wall though, so it might not be possible. Will copy some pseudo code in here in a bit

silent root
wind steppe
#

You should be able to force push I think… ?

silent root
vernal dust
#

Sleep good. I just want to get the idea over here so it's visible and considered before bumping to GH/MVP status

silent root
#

But I can look tonight

vernal dust
#

One of the functional examples:

image.png
silent root
#

The problem we had with that one was the capturing as well

vernal dust
#

Our main proof of concept was a cobble to lava gen - consume some amount of cobblestone from an item handler, and generate some lava into a fluid handler, in one transaction

silent root
vernal dust
#

You basically need to edit the commit messages and author info

silent root
#

As I have never needed to do that before and I want to ensure we do it right 🙂 just not when Im sleep deprived

vernal dust
#

Which is why you need to force push

silent root
#

Noted

#

I did miss a note, 1.21.1 targeting may need to have a little bit longer of a discussion when Im awake. As at the moment, it wouldnt be in a necessarily testable state smoothly like that since it is designed as a "migrate now" sort of operation with classes and methods of old linking to their new counter parts, but that jerry rigging to get working may not be smooth necessarily just by targeting 1.21.

I would need to check, but my initial estimation is that it wouldnt be a viable ROI on those testing it as they'd need to strip out their handlers

untold eagle
vale birch
#

Or just interactive rebase to the same branch and set the commit to edit....

silent root
#

Referencing every pr and making it part of this one? You got it coach! soarynChamp ... I might have done that beforesoarynLurk

copper fjord
#

NO

#

DON'T EVER DO THAT AGAIN

silent root
#

Loool I DIDNT MEAN TO!

copper fjord
#

I'LL PERSONALLY MURDER YOU ||for legal reasons this is a joke||

silent root
#

I remember looking at a different pr and wondering "huh... why is mine here?", then you pinged me

wind steppe
silent root
#

You can ask @hollow maple how many times I ping her to just help me do a simple git task now....

#

"Hey, I want to pull this.... is it just this button?"

hollow maple
#

I think so DokiDerp

silent root
hollow maple
vernal dust
silent root
#

However I sleep now

hollow maple
#

I mostly deal with git from the command line so figuring out how jetbrains has labeled half of it has certainly been interesting

flat root
flat root
# silent root First I wanna say thank you! I do want this to work out and I know how much you ...

I would prefer not to push code directly, so I will make suggestions regarding the vanilla implementations 😛
The biggest missing thing is the entity equipment wrapper... it probably needs another set of patches 😦

Re names I will open review threads and we'll see what we do. I would already suggest dropping the I prefix for interfaces, we don't have that requirement anymore.

I can live with ints for fluids and items. For energy I would prefer long, but I already have a workaround: GrandPower, which will simply be updated to the new API. We should definitely change the energy API to be transactional in any case.

Re last point, I don't plan to push stuff directly anyway so don't worry about waking up with random changes. I do think shartte was interested in bringing over some more unit tests from Fabric though

flat root
hidden geode
hidden geode
hidden geode
loud stirrup
#

Yeah but keep in mind: if you don't make it long, no mod can use long. If you make it long, it doesn't really mean other mods internally have to use long (at least not necessarily).

#

Doesn't really matter that much to me, I think. It was more important on Fabric due to Fluids using 81200 or whatever it was as the base unit for bucket volume

#

But for fluids on NF, it means 2 million buckets is the upper cap for a tank

#

Since the API is now generic, that also means no other modded type of resource can exceed MAX_INT for storage (at least not exposed)

#

Don't take that as a strong endorsement either way, it's just something that has to be taken into consideration

hidden geode
#

We could just go to long and deal with it

#

I don't really mind

past lodge
#

for fabric unity using long would be a good move

loud stirrup
#

It's not really a design goal I think, the strongest argument for it is not constraining the design for mods custom resource types, ig

hidden geode
void thicket
#

I think we should be keeping int for everything

hidden geode
#

I mean you are right in the core, but it is for sure a debate we could have

wind steppe
#

I think a pretty valid issue is the issue of conversion from long to the stack classes we have. Both FluidStack and ItemStack use int internally, so it couldn’t truly use the max long value for a single fluid stack

opaque vector
#

Long for less constraints makes sense but on the other hand, mods probably should balance themselves around ints to prevent runaway powercreep

wind steppe
#

I support ints, but to play devils advocate; mod resources might not necessarily be 1 usable thing = 1 of x. Fluids it’s 1000 of something for a usable amount, other things might scale even larger. There could be a gas mod that chooses to use the 81k standard like fabric.

void thicket
#

using long also means no use of base DataSlot in menus

opaque vector
dapper dust
void thicket
#

We patch dataslot from short to varint

opaque vector
void thicket
#

What thonk

flat root
#

Personally I don't particularly care. The people who want to use longs will use them anyway, e.g. with https://github.com/Technici4n/GrandPower. It's just nicer if this sort of external API doesn't need to exist

void thicket
#

It checks the connection type

dapper dust
opaque vector
void thicket
#

The only thing long really gets you is more than max-int-per-operation

Storage can always be handled internally

#

And a bunch of (int) casts when working with things

dapper dust
dapper dust
flat root
loud stirrup
void thicket
#

You don't really need to know that jeff's magical storage has 17 trillion items in it™️

loud stirrup
#

It's not a terrrrrrrible problem overall

flat root
loud stirrup
#

Operations I don't particularly care about

#

getAmount is more annoying

#

For casting you can do Ints.saturatedCast

#

Which takes care of proper clamping

opaque vector
#

Let’s just go with long to reduce wrappers and workarounds in mods.

dapper dust
loud stirrup
#

I have yet to go deep on reviewing Adrians PR, depending on how the template implementations are structured

#

if they are specialized for items/fluids, they can implement the methods int-based

#

Depends a bit on the hierarchy

void thicket
wind steppe
#

I guess the question is: how much consideration do we give for devs using non neo resources. I think it makes a lot of sense to use int for the vanilla resources that the transfer API deals with, but things other than that might have valuable reasons to want or even need the use of longs for their projects. But those other things are other things, they could very well just copy the Neo classes and switch them to longs, they don’t necessarily have to use ours.

loud stirrup
#

I mean we can probably think a bit of what kind of cross-mod transfers exist today

#

I know of Chemicals for Mek

#

Mana for Botania

#

Uh.... Ars Source?

void thicket
#

Ars source yeah

loud stirrup
#

And I am already kinda running out of ideas 😄

wind steppe
#

Every magic mod pretty much uses their own thing

loud stirrup
#

I am pretty much just iterating AE2 addons that deal with other mods resources

hidden geode
wind steppe
#

That are akin to energy more than they are fluids or items tho

hidden geode
#

A whole bunch of stuff

loud stirrup
#

It's hard to predict what some mod will come up with

void thicket
#

The only thing really close to items/fluids is mek chemicals/gasses

loud stirrup
#

But OTOH the only real case for it that I remember is Fabrics modeling of fluids

#

with a high base unit to model fractions

#

That can in principle appear for other types as well

wind steppe
#

A mod we’re working on uses ink types as a resource. Buuz has that replicator mod that has the matter types as unique resources

loud stirrup
#

The offer to mods is that if they use the base IResourceHandler interface for their own resource type, they instantly benefit from a lot of nice template implementations + utilities around it

#

We probably just have to judge whether constraining to int for the sake of itemstack+fluidstack constrains those use cases too much (or not)

#

No idea, honestly. I worked with long forever in AE2 in the context of ItemStack and FluidStack

#

So I might just be used to it 😄

wind steppe
opaque vector
wind steppe
loud stirrup
#

It's a tradeoff of usability. I don't think the balancing plays a large role, people are not going to consider int on the API a problem for making Long.MAX_VALUE barrels

#

But the required clamping to make ItemStacks out of it is, OTOH I don't think the clamping will actually go away even if you use int

#

Don't you always have to clamp for ItemStack due to max-stack-size constraints?

#

regardless of int vs long? The more annoying part is the cast inbetween

opaque vector
wind steppe
loud stirrup
#

Well you always have it on the edges with vanilla, but the more important part is that you can extract/insert/query without having to account for the max stacksize limits

wind steppe
#

yeah

loud stirrup
#

That's why I'd care more about being able to query for long while not necessarily having the ability to extract/insert with long, although some people are going to be hella annoyed by the asymmetry if you'd not use the same type there lol

hidden geode
#

There is also the issue with stuff generating values much larger then int max

wind steppe
#

I… wouldn’t be opposed to that

hidden geode
#

And now the insertion needs to repeatedly call the int insert

#

Which can take a long time

dapper dust
wind steppe
#

So we lock rogue in a broom closet for the duration of the review and move forward? /j

hidden geode
#

We hand them a couple new graphics cards

#

Send her out to test

#

And they will be gone for a couple of days

dapper dust
#

what GPUs you offering?

hidden geode
#

Unsure

#

What GPUs are you still missing?

wind steppe
#

We sneak into China and steal some of their new nvidia competitors

dapper dust
#

Tesla my card might be broken (GTX 285), Turing (2080ti), Ada (4090), Blackwell (5090)
GCN1 ill take a better card (have a 7850, would like 7970/280x), GCN2 i think my card is broken (R9 390x), RDNA2 (RX 6950 XT)

silent root
opaque vector
#

We already got a few mod examples tho right now without deep digging

silent root
#

There are a few mechancial problems with doong longs on the consumer side both dev and player, but the point about 3rd party resources is a good one to note

opaque vector
#

Mek, ae2, etc unless I misunderstood

wind steppe
#

MI as well

dapper dust
silent root
#

Internally I do longs, but between mods I prefer ints for that implementation which I can doc/explain later, trying to get ready for stream soarynLOL

opaque vector
#

Geeze. If we can just fill a hand of mod examples bypassing ints in just a 5 second brainstorming, ints doesn’t seem like it is doing anything at this point

dapper dust
silent root
#

There is a mechanical burden which will trip up newer or less practiced devs. I currently have now 2 examples of why (not who) it would be beneficial, can we add to that?

#

That mechanical burden also can entail data loss or accidental saturation

#

Those are

  • "We want to be able to send more in one operation" not sure it makes sense fully for items but some scenarios can get tricky.
  • "IResource is generic thus those would be what data struct we set to some degree" which is fair, there are cases where someone would possibly need longs for theirs. Though Id argue part of that is a design scope they need to investigate if they actually need 9quintillion as their upper bound rather than 2 billion
#

But so far, those are the only two pros that I think I have been presented

loud stirrup
#

There's also being able to query for storages that do support more than storing Integer.MAX_VALUE, even if the per-operation only supports Integer.MAX_VALUE

dapper dust
#

for custom things, theres also the argument for doing fixed point with it, where long is helpful

silent root
#

That would be partly the first point but fair

#

Isnt int a fixed point? Or am I misunderstanding what you mean?

dapper dust
#

using the lower bits for fractions of a thing

loud stirrup
#

i.e. Fabric style fluids

#

Before I misquote their bucket amount I'll look it up

wind steppe
#

81k

dapper dust
#

ie: splitting it into 21 and 10 bits (for being able to have 1/1024th of a unit) would limit the upper bound to ~2million rather than ~2billion

loud stirrup
#

Yeah so the user-facing amount is not exceedingly large, necessarily, even if the technical internal amount is

wind steppe
# wind steppe 81k

Tho no example I can think of has ever stored their unique resource storage in increments that large

#

Even mods on Fabric, like Spectrum, store their unique resources, like InkStorage, as 1k increments

loud stirrup
#

Well, Fabric Fluids do 😄 Someone might get the idea to do a similar approach to fractional amounts

#

Although to be fair, they only did this to support the 1/3 bottle increments + other weird fractions present in the vanilla universe

#

At least IIRC

dapper dust
# wind steppe 81k

if this was the divisor, thats limited to 26KB in an int, which is a pretty damn low value IMO

wind steppe
flat root
#

81k-based fluids are indeed why Fabric went for longs for the general transfer API

wind steppe
#

I have never seen someone who uses the fabric Storage<> class to store their own unique resource with 81k as a divisor

loud stirrup
#

Well, there are enough mods that store items and fluids that internally store them as longs

#

👀

flat root
#

I guess you can add Powah to the list

loud stirrup
#

Sure but since energy isn't using the generic resource API (or is it?) that argument doesn't apply, right?

silent root
#

Correct that is a completely different can of worms

loud stirrup
#

Soaryn brought up the point of being able to fuck it up for noobs which is valid but I think I need to look at it again in code

#

funnily enough it's probably easier to fuck up for FluidStack where you don't already have to account for max stack sizes

silent root
#

Focusing on IResourceHandler if I were to run tests for example We have assertValue

#

If I do int, long

loud stirrup
#

while for ItemStack you must account for max stack sizes already

silent root
#

It fails

#

As an example

#

And I am fairly practived and that still tripped me up

#

Do note, I thought about this for a while. The best form of what we have right now:

hollow maple
#

My stance on ints vs longs is that the backing implementation can use longs, sure, but the API should still be ints

#

I can't really see any reason why you'd want to transfer more than 2 billion of anything in a single step, and if you do I feel at that point you probably want a different API anyway

loud stirrup
#

Not being able to at least pass the information that there's more available along is annoying though

#

(Btw. just checked, AE2 already clamps operation size to int anyway when interacting with IIH)

#

And at least we'd lose the need to iterate extraction until we fullfil even that

wind steppe
#

seems more and more like we should just make getAmount return a long

loud stirrup
#

If you want something really nasty

# 
int getAmount(int slot)
default long getAmountLong(int slot) { return getAmount(slot); }
wind steppe
#

ew

hollow maple
hollow maple
silent root
#

Ok sorry, had to help move a refrigerator...

hollow maple
#

AE/RS is kind of the only exception in that it can display "greater than 2 billion" amounts in an interface

wind steppe
#

Okay real question: what reason besides “it’s ugly” is there for not making the getAmount return a long and the transfer use and return an int?

hollow maple
#

I'd be okay with that

#

.NET does something very similar, actually

wind steppe
#

I love precedent

silent root
#

What we have right now, is an ability to inquire on all interfaces right now and reasonably assume we can have 1 long sustain that inquiry. Since everything is an int, we'd need a pull of 4 billion times to saturate a long. This is useful to keep the logic low without worrying if someone is unfamiliar with how primitives can have different sizes. (And more accurately why they do)

However, the get amounts I can see the inquiry problem for viewers to an extent that part is debatable I feel, the transfer has more implications than I personally desire both dev &player related

hollow maple
#

specifically, in the case of .NET, there's a type called ReadOnlySequence which is effectively a view over one or more arrays which represent some logical stream of data - its length is typed as a long, but the only decent way to get the actual data out of it is to use a Span (effectively, a non-owning view of an array) which deals in ints

silent root
#

Arguably my focus right now is to ensure what we provide has minimal entry level pitfalls as possible. Advanced users who need longs exist absolutely, but I'm not sure the mechanical burden is necessarily out weighed by those advanced uses that can technically be solved today as is with ints. I want to discuss this further, but I need to figure out what I'm streaming today (which may actually be working on code for this PR soarynLOL)

loud stirrup
#

I mean other than it looking like shit, I have yet to find an actual "danger" in having getLongAmount in addition for those who actually need it. Since any implementor wouldn't use the template classes anyway and would then have to make the int getAmount use Ints.saturedCast

silent root
#

When I say as is I mean with extensions between those mods, not just inquirying a lot

wind steppe
silent root
#

Ok, though having for inquiries such as getAmount I do like the idea of getAsLong() as a secondary... and may explore that a bit\

hollow maple
#

(I thought I pressed shift when replying there soarynGrump)

silent root
#

I WISH java could do final in interfaces

#

(not exactly related now that I think about it)

#

so if we omit the transfer operations, is it just getAmount and capacity?

loud stirrup
#

Well no wait, the reason int getAmount is there is so that any client of the interface who doesn't want to deal with longs

#

gets an automatically safe amount to use

wind steppe
#

Fair

loud stirrup
#

ah yes, forgot about capacity, probably same idea applies

hollow maple
#

I kinda understand the premise, but it still feels "meh"

wind steppe
#

Was just about to say

#

2 new methods, it’s not too bad, especially if it’s defaulted

hollow maple
#

I think amount/capacity could safely be longs, but the transfer APIs should still for sure be ints

loud stirrup
#

yeah doesnt feel nice, but that's the mess of trade-offs 😄

silent root
#

If we are just focusing on the inquiry aspect, then the gets could have a defaulted implementation

#

And that way it is more opt in as well on the consumer side

wind steppe
#

I say we go forward with the 2 new methods and pray it passes review

silent root
#

I'll stream this today to start and start with adding those two

#

getXAsLong sound good for a naming?

wind steppe
#

Nice, then we can clip this and say “it was soaryns fault”

silent root
#

We could do that without the clips to be honest

#

regardless if it actually was

wind steppe
#

Lmao

cunning mulch
#

Imo getX and getXClamped might make the naming clearer (assuming this is for things like capacity)

silent root
#

yes, but the default usage is intended to be int

cunning mulch
#

Fair

wind steppe
#

You mean make the default getter “getAmountClamped”?

silent root
#

so clamped would likely be odd for some newer (despite the name sounding good)

loud stirrup
#

yeah I'd keep the easy ones named simple

cunning mulch
#

Yeah I forgot about that part

silent root
#

simple()
simple1()

#

But I do like those names in future cases

loud stirrup
#

you could invert it to getAmount + getUnclampedAmount

silent root
#

Loool

#

getUnhingedAmount

wind steppe
#

I don’t mind unclamped

cunning mulch
#

not that bad

silent root
#

I think unclamped sounds good, but looking at it as a fresh pair of eyes, when you are looking for long return which would you immediately recognize? As unclamp COULD imply that it was clamped to begin with besides the data struct

cunning mulch
#

I will try to start reviewing the PR tomorrow though likely won't make it through the entire thing tomorrow so will end up continuing throughout the week at looking at stuff

silent root
#

That is fine

loud stirrup
#

I'd be very concerned if you could get through 17k+ lines in a day

#

you robot

silent root
#

SAME

loud stirrup
#

pupGPT

silent root
#

I wrote a lot of it with Adrian and I STILL wouldn't necessarily be able to

dapper dust
silent root
#

BUT if you need a starting point, the interfaces would be best still

loud stirrup
cunning mulch
loud stirrup
#

And you have my deepest respect for it

dapper dust
cunning mulch
#

(and then consequently at times I end up ignoring the tiny PRs as I am not in the mood to checkout branch after branch after branch to test and check them xlurk)

wind steppe
#

And you have my condolences

cunning mulch
#

I mean

#

there is no way it is worse than having had to review solo the two fluid API PRs

dapper dust
#

that kind of thing does happen at work, but its usually a i checked what you are wanting to do, and agree, but didnt check the details of how because it trust you to have not fucked it up rather than i didnt really look, but sure

past lodge
#

re long/int, the base ones definitely should use long if using int means other people aren't able to use longs

cunning mulch
#

the first one and then the one that got cleaned up and finally merged on a higher MC version

past lodge
#

just because a idea seems stupid doesn't mean there's not an actual use for it

cunning mulch
#

tbh longs feel meh because of ItemStack being int. (Same with FluidStack, though given we control that we could make that also use longs)...

silent root
#

If we solve the inquiry part I think that really resolves a lot of issues in some of the more practical cases. Moving a quintillion of something to one destination may not be idealistic even with the mechanical burdens aside

past lodge
#

yeah... however having the upperbounds being int means anyone who wants to use longs would have to resort to dirty hacks

silent root
#

Not necessarily dirty hacks, they just need to have an extension

#

It would be a com layer between the users then

#

I need concrete reasons still for arguments for/against

#

And the "why"s would be useful as well

cunning mulch
#

unless we similar to grandpower just had common libs for them, and (especially if we referenced/"official" endorsed certain ones via docs) then those could use wrappers in general, plus mods that actually care about having such a high throughput could request or PR implementation and support into the other mods for them. Given realistically if you take a mod that adds a block that is like vanilla's chest... there is no reason it needs to be specifically supporting longs

mossy chasm
silent root
#

Common or communication

#

interchangeable in this case

cunning mulch
#

but for something like a bin or barrel that intentionally is massive amounts of a single item, it may make sense to allow interacting with in larger batches

mossy chasm
#

Okay, I was scared it meant COM lol

silent root
#

as ... they already have a problem with int

past lodge
#

five hundred trillion items spill out

silent root
#

keep going soarynSip

#

quitillion is a scary number when used haphazardly soarynFine

cunning mulch
#

that is wy I am saying a separate API similar to grand power might make more sense. Because the only real usecase imo would be for things like AE or RS pulling directly from a barrel or stuff

silent root
#

Yeah yeah

cunning mulch
#

pipes (unless they are teleport based similar to EnderIO) really shouldn't for items

silent root
#

AH I misunderstood initially

cunning mulch
#

that doesn't mean people won't use it for those

#

but

#

imo they shouldn't

silent root
#

The solution for the pipe really is have it drop like what ee2 did

#

a thing that stores the contents in a blob thingy

#

so that the data is preserved without killing the client/server, but that is implementation still and not one I'd like to necessarily require/encourage in use practically

loud stirrup
#

But showing the actual amount in UI would be nice

#

assuming storage bus plonked against it

cunning mulch
#

basically what I was suggesting is: ints in Neo, and a common API that we try to promote that if people really do need higher rates, then they can have for compat purposes with the other mods

loud stirrup
#

At least we'd be rid of the need to iterate 2^31/maxStackSize times to actually get the item out, like we do now

past lodge
#

i guess we can always go with int for now and change it if there's ever a need later

cunning mulch
loud stirrup
#

Yup the super most pointless simulate ever

past lodge
#

just slap @apistatus.experimental on everything like it's been on fabric /hj

loud stirrup
#

if we do go with int, I'd still ask to consider the getUnclampedAmount getter which also allows anyone querying it to know the real amount. At least right now, I don't see a real downside other than the ugliness hehe

#

Not an immediate concern, actually 🤔

#

At least for implementing the PR it's just an added method, the PR itself wouldn't make use of it internally I think

past lodge
#

yeah that would be a good idea

cunning mulch
#

I agree, having a builtin helper to get the "raw" amount will mainly be useful for things like jade and storage busses etc

silent root
#

the getter as long is what I'll do in a few mins

#

I had one for the energy as a util somewhat but I can add that.

#

Question, do we expect that if we do getAsLong(index) and we get say more than a long we just cap at long?

#

or do we throw like a nightmare goblin?

#

I assume cap

#

In terms of iterating

loud stirrup
#

You mean for javadoc purposes?

#

Or where is that coming into play? 🤔

silent root
#

Like the util we have for getAmount(handler) would return the getAmount(index) iteration sum

past lodge
#

if your going above long you should re-think what your doing

loud stirrup
#

I mean what's the semantic for the int version?

#

That would also have to saturate?

silent root
#

so getAmount(handler) would do the int path

#

getAmountAsLong(handler) would do long path

#

the int one we could return long, as it would be the actual total

#

the long one... we CAN hit the limit immediately so I assume just clamp to that if we were to hit Long.Max

cunning mulch
#

max long imo

#

given it is a best "guess" in general

#

and it does contain that much of it

silent root
#

That is what I was thinking, just wanted to be sure that was the common gut feeling

cunning mulch
#

it contains more, but meh

onyx basalt
#

so I haven't really looked into the code, but whatever comes out of this PR, I hope that I can find the time to add proper documentation for this to the docs - I will probably annoy you all to explain stuff and proofread what I write up

silent root
#

Lol throw new YourDoneGoOutSideNow()

silent root
onyx basalt
#

all that said, uni is currently eating up most of my time so this will have to wait until July harold

silent root
#

A LOT of of it is javadoced

#

so you should be able to hopefully cut out needing to in a lot of simpler cases

onyx basalt
#

yeah, the docs will be more of a user guide for this

silent root
#

But if you have an inquiry don't feel scared to reach out to me 🙂

#

The primers we have prob will help get that started a bit (Albeit they are primers written for reviewers more than users atm)

onyx basalt
#

afaict the PR description is more a description of what changed compared to before, but I'll need to mostly write this for first-time users

#

but yeah, I'll poke away once I have time for that

dapper dust
silent root
#

No no, you have so much you actually own DoneGoOutsideNOw amount (but yes you are right... soarynLurk)

silent root
wind steppe
#

I did say at some point I wouldn’t mind helping write the docs

vernal dust
#

...how gross would a TransferAmount interface be, with Short/Int/Long implementations under it?

wind steppe
#

._.

hollow maple
vernal dust
#

Where backing storage/capacity is always a long and all the transfer amounts can just be deducted directly, numeric math/types be damned

hollow maple
vernal dust
#

TransferAmount.INFINITE can exist if we want, then

hollow maple
#

Since we're using signed integers, we could reserve -1 for infinite

vernal dust
#

TransferAmount.JUST_FILL_THE_WHOLE_THING_ALREADY

wind steppe
#

Lmao

vernal dust
#

But, more seriously, TransferAmount.of( short / int / long / ????? )

hollow maple
dapper dust
wind steppe
#

That is one issue, there isn’t a way to fill a container with a long capacity that has a long amount in one go without multiple pushes

hollow maple
dapper dust
#

im aware

hollow maple
#

Like in .NET, timeouts are usually signed, where -1 indicates infinity, and other negative values are invalid

dapper dust
#

-1 also happens to be the maximum unsigned value (~0ULL) too, which is nice there

vernal dust
# 
interface TransferAmount {
  static final TransferAmount INFINITE = (stored, capacity) => capacity;
  long amount(long stored, long capacity);

  static TransferAmount of(short amount) => new ShortTransferAmount(amount);
  static TransferAmount of(int amount) => new IntegerTransferAmount(amount);
  static TransferAmount of(long amount) => new LongTransferAmount(amount);
}

record ShortTransferAmount(short amt) implements TransferAmount { amount(stored, capacity) => (long) amt; }
record IntegerTransferAmount(int amt) implements TransferAmount { amount(stored, capacity) => (long) amt; }
record LongTransferAmount(long amt) implements TransferAmount { amount(stored, capacity) => amt; }
#

Maybe glue a Math.clamp in there to prevent negative and going beyond stored

void thicket
stone dragon
#

Not sure what the point is to the different size transfer amounts if it's all just treated as a long internally?

#

I feel like I'm missing something here

vernal dust
#

You're missing the ability to adapt to current values/have INFINITE

#

I could REQUEST that 10,000 FE gets extracted, and this can adapt to clamp that to the 5k actually stored

stone dragon
# vernal dust You're missing the ability to adapt to current values/have INFINITE

What do you mean, "adapt to current values"? Like, have transfer amounts whose amount transfered depends on the current stored in the storage? Sure. But still, why the 3 short/integer/long transfer amount records? Seems like it could be one record, with a long, and ctors that adapt to that, since they all just cast to a long anyways

hollow maple
stone dragon
#

Also not clear on why you need to "adapt to current values" -- wouldn't you just, like, check the capacity/stored of the thing before you figure out how much to transfer to it?

#

And INFINITE isn't really infinite in that implementation, it's just "smaller of capacity and long-max"

hollow maple
stone dragon
#

It feels very much like over-engineering to me

vernal dust
#

Eh, it's a psuedocode example. I just wanted to express how one could express INFINITE in the API, instead of assuming long.MAX or something

stone dragon
hollow maple
vernal dust
#

I'd want, in the API, a way to express that I want everything a storage currently can provide without checking first

#

Because values can desync between checking storage versus extract/insert

hollow maple
#

In the scope of a transaction, that's a lot harder at the very least

#

(It still happens of course, but y'know)

stone dragon
silent root
#

Out of curiosity who would I need to talk to about immaculate formatting potential changes?

hollow maple
#

It SHOULDN'T desync currently because we live in a world where everything's effectively single-threaded

vernal dust
#

It should not! ...but we're dealing with modders of every experience level, who knows what kind of weird ||*!()%%&|| people will do?

silent root
#

THIS validates the applyformatting but breaks the immaculate check

image.png
#

The last ) is the issue

trail fable
silent root
#

it needs to be on the same line as the last param which makes termination of the method tricky to spot at times

stone dragon
hollow maple
noble latch
# silent root

Welcome to the Google formatting standard, yes it looks ass, but it will almost certainly not be changed

stone dragon
# silent root

Yeah that's not immaculate, that's the eclipse formatter standard immacualte is given

silent root
#

Aaaaaah very well then!

#

Was making sure it wasn't something that we could just go easily change. Fair enough!

hollow maple
silent root
#

It WOULD be nice if we could try to clean up the error presented with it

stone dragon
silent root
#

as it took me a bit to isolate

hollow maple
#

(I think they're PROBABLY thread safe but, that's not important for this scenario really)

stone dragon
hollow maple
#

🤷‍♀️

stone dragon
#

Which, you're right, is a reasonable assumption to have from the start

hollow maple
#

On the micro scale where Nano's suggested API would be used, I don't THINK there'd be any potential for a desync of the nature that said kind of API would hope to protect against

stone dragon
#

So I guess short version no need to worry about the thing getting "out of sync" from when you last read from it

stone dragon
#

But you're right, with ticking being single-threaded probably not something huge to worry about

hollow maple
#

There MAY be a problem if we have some cursed semi-recursive capability - think two pipe mods trying to fill/empty each other

#

but I'm still a little too ill to be able to think through the consequences (and eventual stack trace) of such a thing

noble latch
vernal dust
#

I wanna set up a perfect loop of resource handlers now

loud stirrup
#

as in they're global

stone dragon
#

In that case it's stronger even

#

You'd want to have one root-level transaction at a time, period

loud stirrup
#

correct

#

that is also enforced, as far as I know

ivory mauve
#

oh, this was scrolled up

wind steppe
ivory mauve
#

I don't think I've ever had a billion of anything in a survival world

hollow maple
ivory mauve
#

who has 2 billion cobblestone?

#

I've never had that

#

if your amount of cobblestone is over 9000 you honestly can just say "its over 9000" and call it a day

hollow maple
#

It would be NICE to know that an inventory like a storage drawer contains more than 2bn, though it's certainly not necessary

#

The main issue is primarily in "aggregate" handlers

stone dragon
# ivory mauve who has 2 billion cobblestone?

You know those various compressed block mods? I swear I've seen modpacks make the highest-tier one required for a quest or something before. And you might have to ask "do you have this many present?" for, IDK, an auto-crafter request or something

ivory mauve
#

I think the small benefit of "the weird players can see their 3 billion cobblestone" is not worth the added API incompatabilities with everything else using int

#

sounds like dumb modpack design

#

we shouldn't encourage that

stone dragon
#

Sure

#

I mean, eh

#

Not really neo's place to pass judgement on modpack design

ivory mauve
#

pros of long:

  • dumb modpack design works better
    cons of long
  • developers lives are more difficult
#

I think its an easy choice

hollow maple
stone dragon
#

How are developers' lives more difficult

ivory mauve
#

because nothing uses long