#Transfer rework (IItemHandler, IFluidHandler, IEnergyStorage)

9055 messages · Page 10 of 10 (latest)

silent root
#

Though, it MIGHT be beneficial to look into doing a check pre-emptively in the existing Access handlers to (where currentAmountPerItem == extractedPerItem) pass in Empty to the update method

#

I don't have many cases where I interact with items myself directly, so not sure if ideal or not

#

The biggest reason we don't have that in now that I can guess, is that there would need to be a specification of what "empty" is in each handler, though that wouldn't really be much of an issue I'd argue and could just be driven by the constructor

sinful tiger
#

lmao FluidUtil/ResourceHandlerutil blindly iterates through all the slots in the handler without checking if the number of slots [or the identity of the handler] has changed, so i get an index error because my full bottle has one tank per possible result [i thought i was blocking "potion of water" fluid from coming up, which is what's happening in this case, but it's legitimate for tagged fluids]

#

i get why it tries to transfer from multiple tanks of the same fluid, but it seems too clever

silent root
#

Any changes to the slot count should be really deferred when reducing (and possibly adding) but it isn't expecting changes. That would take a rather large portion of time to constantly reask the question "Should I start over?"

sinful tiger
#

i think in my case i'm just going to suppress the exceptions in the handler

silent root
#

Part of the contract in the ResourceHandler interface 🙂

#

Then you will crash

#

Don't just try catch them and hope. soarynLOL

sinful tiger
#

i mean, i'm just going to "be lenient to accomodate for callers holding onto a previously returned size"

#

really arguably it shouldn't be calling my handler at all, after the item's changed, the resulting item may have its own handler that's different.

hidden geode
sinful tiger
#

i mean i'm suppressing it in the handler [i.e. try super catch return 0], not outside it

#

i could check the size there instead i guess

#

either way this is a bit of a special case, most item-swapping fluid handlers will only have one tank

#

i'm doing like "xp bottle contains every fluid in the xp tag"

#

how do you defer things? it's not what i went with in this case but, like, is there a way to schedule something to run after a transaction involving my container is committed?

silent root
#

The journal

#

For an item... may be not as valid

#

SnapshotJournals provide the ability to snapshot but also run callback actions on success

#

(or failures)

#

success being commit

sinful tiger
#

hmm ok

flat root
lavish marsh
#

working on it schedule is a bit full but I can finish it before uni starts tomorrow I think

flat root
#

There's no rush, I was just reminding you

#

I forgot it's exam season

silent root
#

@flat root seems that in BundleItemHandler the index check is not using an index check but instead non-negative.... soarynLOL that should be looked at as well as any others that were added at the time #1183818213134446742 message

silent root
#

Another more general issue, is in exchange I think in ItemAccess. Let's say you have an item that can store 250mB of fluid, and they can stack to 16 filled or unfilled. It seems the exchange REQUIRES the exact same number for insert & extract. Which is a problem when a partial stack exists where the sum of the new filling would be over that.

So 10 unfilled containers being filled into a slot with 8 filled containers just nopes out instead of stacking 6 there.

#

Also means that if you try to fill a stack of buckets it can't even fill one

#

as the insert would be 1, the extract would be 16

#

Though that one is mostly due to the ItemAccessItemHandler incorrectly assuming it should be an all or nothing ordeal. This is something we had in the original PR, so you may need to go back and revise that

#

To be clear on a more general use:
Something like a tank has 3 buckets worth of water, you put 16, it should fill 3 buckets. Right now... the basic implementations don't seem to let you do that

#

This is at least a fix for the exchange problem when it HAS the fluid to work with:

@Override
public int exchange(ItemResource newResource, int amount, TransactionContext transaction) {
    TransferPreconditions.checkNonEmptyNonNegative(newResource, amount);
    ItemResource currentResource = getResource();
    TransferPreconditions.checkNonEmpty(currentResource);
    int inserted = 0;
    try (Transaction subTransaction = Transaction.open(transaction)) {
        int extracted = extract(currentResource, amount, subTransaction);
        if (extracted > 0) {
            inserted = insert(newResource, extracted, subTransaction);
            if (inserted == extracted) {
                subTransaction.commit();
                return extracted;
            }
        }
    }
    if (inserted == 0) return 0;

    try (Transaction fallback = Transaction.open(transaction)) {
        int extracted = extract(currentResource, inserted, fallback);
        if (extracted > 0 && extracted == insert(newResource, extracted, fallback)) {
            fallback.commit();
            return extracted;
        }
    }
    return 0;
}
#

But we still need to have a fix to the scenario of say the tank has only 1 bucket worth of water, and trying to fill a stack of buckets. The current ItemAccessResourceHandler with combination of the BucketResourceHandler doesn't allow the partial fill, as it wants ALL buckets to be filled, but we only have enough for 1. So instead of handling that, it just says no fluid shall move.

#

Lol just noticed this as well....

image.png
#

A "simple" but possibly not best solution is to just do a fallback in the ItemAccessResourceHandler as well and try amount/capacity and use that as the mutation number after trying amount/numberOfContainers, but this will cause problems in cases where it could do partials, so we need a way to specify we are a "stepped handler" or "all or nothing"

silent root
#

Ideal solution would be I think, in ItemAccessResourceHandler, have both a fallback in the IO methods, and a flag that can instruct if the container supports partial filling so we can skip in those cases entirely when necessary. On top of fixing the exchange like above #1183818213134446742 message

silent root
#

A possible implementation (roughly) of ItemAccessResourceHandler that supports partial or nonPartial based on desire

📎 recommendedChanges.java
flat root
#

I don't currently have time to look deeply into this. Containers that cannot be partially filled were indeed left as a TODO. The exchange problem also makes sense but is separate, and there should be a relatively simple fix. Generally if you use oneByOne you avoid both issues

silent root
#

Which is what I have been doing, but that is a bandaid that badically circumvents one of the major points of having ItemAccess soarynLOL

#

But both provided code points together fixed the problem for me

#

It is simple enough for non-partial supporting items to just have that flag and only do the amount/capacity as the amount moved around

cunning mulch
#

Or at least get a better sense of what you are proposing

silent root
silent root
#

Partials don't fully work either mind you. as they stand now

#

If I have a stack of say 8 each with 100, and want to drain 10, it can't seemingly divide that up nicely. It needs more math than just split across

#

Or even using a stack of say 8, and drain 1

#

Greatest common divisor would be my usual go to

wind steppe
#

Can we get a util function in ItemCapability to encourage more people to use ItemAccess? like ItemCapability.createAccessible or something that create a capability that defaults to using ItemAccess as the context there

cunning mulch
#

You mean similar to the createSided/createVoid helpers we have? If so sounds reasonable

wind steppe
#

Yeah