Fabric Transfer API: "fluid only" edition

[Technici4n]

Fabric Transfer API: "fluid only" edition

Building on #1234, this PR adds a new fabric-transfer-api-v1 for transfer of generic game resources, and uses it to define a "Fluid API" for blocks.

This PR replaces #1278 that I deemed too big to be implemented and reviewed properly. Let's go for quality above quantity.

I am also hosting builds of this module on my "maven" so that it's easier to try it out: https://github.com/Technici4n/Technici4n-maven. Please give it a try, any feedback is appreciated!

Problem scope

This PR is centered around fluid transfer between blocks: Reading fluids stored in a block, extracting fluids from a block and inserting fluids into a block.

Contrarily to #1278, this PR does NOT cover:

• item transfer between blocks,
• fluid-containing items such as buckets or portable tanks.

I will talk about these things a bit later.

API overview

Transactions

This PR introduces the transaction concept. Essentially, a transaction allows setting checkpoints like in a video game: if something doesn't go as expected, the state of the game reverts to what it was at the previous checkpoint. Game objects that take part in a transaction are commonly referred to as "participants" or "endpoints".

Here is an example that hopefully makes this clearer:

try (Transaction outerTransaction = Transaction.openOuter()) {
    /* (A) some transaction operations */
    try (Transaction nestedTransaction = outerTransaction.openNested()) {
        /* (B) more operations */
        nestedTransaction.commit(); // Validate the changes that happened in this transaction.
                                    // This is a nested transaction, so changes will only be applied if the outer
                                    // transaction is committed too.
    }
    /* (C) even more operations */
    outerTransaction.commit(); // This is an outer transaction: changes (A), (B) and (C) are applied.
}
// If we hadn't committed the outerTransaction, all changes (A), (B) or (C) would have been reverted.

Transaction aborting or committing is done by registering a callback with Transaction#addCloseCallback. This gives full freedom to participants to manage their state the way they want to. In practice, this is very low-level and participants that wish to save their state before modification so that changes can be reverted when a transaction is aborted can subclass SnapshotParticipant and call updateSnapshots right before mutating their internal state. That way, modifications will be cancelled by applying a previously saved state if necessary. Uncancellable modifications such as markDirty() or neighbor block updates should be deferred until onFinalCommit.

More details can be found in the Transaction javadoc, and I will be happy to answer any question.

Storage

Building on the transaction concept, this PR introduces Storage<T>: an interface that containers of game resources of type T can implement.
The rest of the storage package provides related base implementations and helpers.

As an example of how this API works, have a look at the generic StorageUtil#move function that moves game resources between two storages.

Storage<FluidVariant>

This API then defines an API lookup for Storage<FluidVariant> so that mods can start using this API for fluid transfer right away.
Cauldron compatibility for water is builtin. That and fluid constant definitions and a few preconditons can be found in the fluid package.

It is expected that most modders will use subclasses of SingleFluidStorage to implement their containers in most cases. These subclasses can be combined using CombinedStorage for fluid inventories that have multiple "tanks"/"slots".

What exactly is a FluidVariant? It is the immutable combination of a Fluid and an optional NBT tag, the methods should be quite straightforward. Client-side fluid variant rendering is also provided by this PR.

Cauldron support

Empty, water and lava cauldrons are supported. Mods can register mappings for other vanilla cauldrons, or for their own cauldrons. Mappings can be inspected. For example, a mod adding a powder snow fluid would first check for an existing mapping for the powder snow cauldron, before trying to register its own fluid. More details in CauldronFluidContent.

The fluid unit

Fluid transfer happens in discrete amounts: 1/81000s of a bucket. There have been attempts to allow per-call denominators (a denominator for every function call) or fractions. Unfortunately, none of those have proven to be practical.

• Per-call denominators introduce a massive API complexity, and since this API is generic, modders that wish to use it for item, essentia or transfer need to suffer from this special case that only pertains to fluids.
• It is simply impossible to prevent overflows with fractions. On top of the very not-so-ergonomic API this would imply, literally any operation (+, -, *, /) can cause overflows. Anyone who says otherwise is lying to you!

Essentially we are left with the choice of a denominator. 81000 is divisible by all the relevant numbers and can be easily expressed as millibuckets with a slight rounding error (possibly displayed only when shifting) - how the fluid amounts are displayed to the end user is not specified by this API. This API however provides a name for 1/81000 of a bucket: a droplet.

What about future-proofing or other denominator choices? Well, thanks to fallback providers that can be registered with API Lookups, we can provide an easy migration path if this choice of denominator proves to be wrong at some point in the future. In general, anyone can define their Storage<FluidVariant> lookup and register the suitable fallback providers at FluidTransfer.SIDED. The only place where the unit is encoded is with the cauldron compatibility, and the various constants in FluidConstants.

Unsided fluid transfer

Not all fluid transfer is sided. However, most of it is. That's why the API uses a non-null Direction context. Another lookup that would be unsided may be introduced in the future if it turns out to be useful. Note that 3rd-party mods can all refer to a BlockApiLookup<Storage<FluidVariant>, Void> with a common agreed-upon ID, and BlockApiLookup#get will return the same instance for every query with that ID.

The future

Item Transfer

In a subsequent PR, immutable count-less item stacks called ItemVariant are to be introduced, similar to FluidVariants. They are not included in this PR because providing a Storage<ItemVariant> implementation that wraps Inventory, SidedInventory and PlayerInventory adds a lot of implementation, and I would like reviews to focus on the generic API for this PR.

Do however keep in mind that Storage<T> can only work with immutable resource types T. This means that Storage<ItemStack> is not an option, and that must be considered before this PR is merged! I believe that using ItemVariants makes the API much less error-prone: inventory contents cannot be mutated, and there is no risk that someone forgets to copy a stack somewhere, leading to duplication or voiding bugs.

Fluid-containing items

Once #1352 is merged, we can look into providing an ItemApiLookup for access to Storage<FluidVariant> instances. There will be two challenges here: designing a context type C that is suitable for all kinds of inventories that may store fluid-containing items, and providing ad-hoc helpers to allow multiple mods to register fluid empty/fill actions for the same item (so that for example mod A could add a honey fluid to fill honey bottles and mod B could add a milk fluid and milk bottles - while allowing bottles to be filled with either fluid). I do however not expect that to be an issue.

[Devan-Kerman]

keys gooder and best, other than that I approve

[Technici4n]

Ok, I have added a base fluid storage that will be usable by most implementors. Multiple "slots" can be combined into a single Storage<Fluid> with CombinedStorage.

I am also hosting builds of this module on my "maven" so that it's easier to try it out: https://github.com/Technici4n/Technici4n-maven.

Again, any feedback is appreciated. Cheers!

[CheaterCodes]

All in all, I like it.
Most of my comments are smaller things I noticed, except the thread-safety one.
I only inspected the code visually, so I didn't do any testing or check the documentation, but I think I got a decent understanding of it and can approve of the general structure and implementation.

[Earthcomputer]

Remove the locking, please. It's not sensible. Anyone experienced in Minecraft modding will back me up here that it's up to the modder to not concurrently access data from the client or server thread, and if they do, it's up to the modder, not Fabric API, to use locks.

[Technici4n]

Remove the locking, please. It's not sensible. Anyone experienced in Minecraft modding will back me up here that it's up to the modder to not concurrently access data from the client or server thread, and if they do, it's up to the modder, not Fabric API, to use locks.

Done. 😉

I also added an OuterCloseCallback to defer markDirty, updateNeighbors and similar operations after the transaction is properly closed.

[Technici4n]

Did another documentation pass - package-info.java and the README.md are still missing, but that will be for another time. 😉

[Technici4n]

Ok so I think this should target 1.17 and be experimental because it's a very complicated API addition.
We then allow ourselves to make breaking changes when updating to 1.18, and so on until the API can be stabilized. The idea is that modders could use this API safely in 1.17, and we would wait until the 1.17->1.18 transition to correct any design mistake.

[Devan-Kerman]

a few minor changes, however I would still like to see

• Iterator -> insert
• Snapshot -> Key (not that big a priority though)

[Devan-Kerman]

a useful debug feature is to record the stacktrace from where the Transaction was created, if someone forgets to close their transaction, the exception can help narrow down who's responsible.
https://github.com/Astrarre/Astrarre/blob/4913a4cb9fa6e88183f31e5f2e5f77fb4f485bd4/astrarre-transfer-v0/src/main/java/io/github/astrarre/transfer/v0/api/transaction/Transaction.java#L38

[Technici4n]

Hmmm acquiring the stacktrace is quite an expensive operation, I certainly don't want this to happen by default. The risk that someone forgets to close the transaction is abysmal assuming they are always used within a try-with-resources block?

[Devan-Kerman]

Hmmm acquiring the stacktrace is quite an expensive operation, I certainly don't want this to happen by default.

ye atm I only do it in dev or with a system variable

[Technici4n]

Updated with fluid key rendering. I will test all the recent changes in production with Modern Industrialization today or tomorrow.

1.17 is coming in about a week, and I hope this PR can be in last call by then. If you have some time, I would appreciate a review.

[Technici4n]

They are in case the API user needs to edit them directly for some reason, for example as part of machine processing. They can only be accessed through the API if people cast Storage to SingleFluidStorage, which is API misuse.

If they were protected, then it can be subclassed for that purpose.
But why wouldn't they just use the extract/insert api anyway?

Starting them out as public means they can't be encapsulated by people that don't want them public (except by wrapping the whole class and creating a delegate).

Typical use case would be something like:

public class MachineBE extends BlockEntity {
    private final SingleFluidStorage inputTank = new SingleFluidStorage() {
        protected getCapacity() { ... }
        protected canExtract() { return false; }
    };
    private final SingleFluidStorage outputTank = new SingleFluidStorage() {
        protected getCapacity() { ... }
        protected canInsert() { return false; }
    };
    private final Storage<FluidVariant> storage = new CombinedStorage<>(List.of(inputTank, outputTank));
}

Need to access amount directly, and can't use insert or extract cause they are designed for pipes, and not for a machine's own use.

[warjort]

However, this doesn't compound at all. Fabric "transactions" are basically an extension of this simulation concept, to allow simulating an operation across multiple participants.

And the same particpant multple times. This also doesn't work with something like simulate mode.
It still doesn't solve the "side effects" problem.

They should probably not be compared too much to database transactions.

I am not, I am comparing them to JTA transactions, a different thing and closer to what you have.

Actually, if you look at TransactionImpl#close(Result), you can see that it's not stored in the transaction. 😉

Which again is my critique.
I keep saying it should, you keep saying it doesn't, but not why it doesn't.
It's not that important anyway, its just something that will introduce annoying refactoring if somebody needs it in a context where you haven't thought to pass it as a parameter but they do have the transaction.

Why not do? You have to override the class anyway to do the other policy changes.
It makes reasoning about how the class state changes doable, something not possible with a public field.

public SingleFluidStorage() {
- public long amount;
+ protected long amount;
}

public class MachineBE extends BlockEntity {
    private final SingleFluidStorage inputTank = new SingleFluidStorage() {
        protected getCapacity() { ... }
        protected canExtract() { return false; }
+        // Only MachineBE can bypass the api
+        protected internalExtract(long amount) { this.amount -= amount; }
    };
}

The alternative for the paranoid like me is:

public class MachineBE extends BlockEntity {
   // public fields accessible here
    private final SingleFluidStorage inputTank = new SingleFluidStorage() {
        protected getCapacity() { ... }
    };
   // public fields not accessible here
    private final FluidStorage inputTankForExternalUse = new FluidStorageWrapper(inputTank) {
         protected boolean canExtract() {return false;}
          // where FluidStorageWrapper is lots of delegate methods
         someMethod() -> { inputTank.someMethod() } 
    }
}

[Technici4n]

And the same particpant multple times. This also doesn't work with something like simulate mode.
It still doesn't solve the "side effects" problem.

Hmmm? What exactly do you mean by simulate mode and side effects here?.

They should probably not be compared too much to database transactions.

I am not, I am comparing them to JTA transactions, a different thing and closer to what you have.

Ah ok, no clue what JTA is at all then, looks quite different though, but please keep suggesting stuff and we can go over it part by part.

Actually, if you look at TransactionImpl#close(Result), you can see that it's not stored in the transaction. 😉

Which again is my critique.
I keep saying it should, you keep saying it doesn't, but not why it doesn't.
It's not that important anyway, its just something that will introduce annoying refactoring if somebody needs it in a context where you haven't thought to pass it as a parameter but they do have the transaction.

Participants should not abort or commit a transaction. Only the user of the API who created the transaction is allowed to decide whether it is committed or aborted. That's why this information only makes sense in the close callbacks, to properly handle state updates.

Why not do? You have to override the class anyway to do the other policy changes.
It makes reasoning about how the class state changes doable, something not possible with a public field.

Personally I dislike such boilerplate. I think your IDE can highlight uses of the public fields if you wish.

The alternative for the paranoid like me is:

What exactly are you afraid of?

[warjort]

Hmmm? What exactly do you mean by simulate mode and side effects here?.

Simulate mode is what forge does.

tank.insert(1000);
tank.drain(1000, simulate);
tank.drain(1000, simulate); // works when it shouldn't because simulate mode doesn't keep track of previous simulations

or

inventory.remove(item1, simulate);
inventory.add(item2, simulate); // will fail if the inventory started full because item1 was not actually removed

Side effects are when things happen outside the transaction.

var amount = tank.drain();
if (amount != 0) {
    doSomethingElse();
}

The problem comes if you want to do multiple things including the above in a transaction but one fails.
You can't necessarily rollback the doSomethingElse() because it is not transactional and its not reversable.

And before you say add a commit callback so it only gets done when the transaction commits.
There will be cases where the other things in the transaction need to see the change to do things properly
(like the simulate mode problem above).

[warjort]

I think your IDE can highlight uses of the public fields if you wish.

The old "a tool will solve the problem" excuse of api designers. :-)

[Technici4n]

Ah, well simulate mode can be emulated by opening a new transaction and not committing it.

Side effects are when things happen outside the transaction.
The problem comes if you want to do multiple things including the above in a transaction but one fails.
You can't necessarily rollback the doSomethingElse() because it is not transactional and its not reversable.

And before you say add a commit callback so it only gets done when the transaction commits.
There will be cases where the other things in the transaction need to see the change to do things properly
(like the simulate mode problem above).

Well, that's precisely what transactions are intended to solve. You can't just have "things that transactions are there to fix, but please do it without transactions". At least that's how I see it. 😆

The old "a tool will solve the problem" excuse of api designers. :-)

Fair. :-P
Let's just agree to disagree here then, I want to keep the fields public, imo both options are valid for different reasons, so I think it's fair for me to decide to keep them!

[Technici4n]

Updated with a separation between Transaction and TransactionContext to make sure participants can't ever close a transaction they are passed. (Unless they rely on bogus casting)

[Technici4n]

@warjort I haven't received an update from you, not sure if I managed to address your concerns? Or maybe you are just busy with something else. 😄

[warjort]

@warjort I haven't received an update from you, not sure if I managed to address your concerns? Or maybe you are just busy with something else. 😄

I think you largely ignored my concerns :-)

• Users have to do book keeping of transactions - ignored because you want users to do this explicitly.
• Implementation using in place modification (but with no locking) and with optimistic style copying (but without the final did somebody else beat me to the change check on update). Ignored because concurrent use is not supported.
• Public implementation details - ignored because its your preference.
• BUCKET/fractional calc will have problems with 16 and 64 - ignored because I was late to the discussion.
• Participants not allowed to abort transactions - ignored because you don't want to support this.
• Transaction result not preserved - ignored because its your preference.

Incidently, I think the lack of concurrency will likely show up in "recursive" rather than real concurrent usage.
e.g. block 1 pushes fluid to block 2 which triggers a neighbour update to block 3 which does something with fluids that therefore wants its own transaction and might want to recurse back into block 1 or 2
I believe this scenario will NOT currently lead to concurrency problems simply because it will just fail when block 3 tries to start a new outer transaction (there is one already present on the thread).

[frqnny]

just weird stuff i guess, allowed me to look at the pr and I can't say enough how impressed I am with the javadocs, they are pretty big. amazing job

[Technici4n]

I think you largely ignored my concerns :-)

I guess that's fair... I agree with your summary though. ^^

Incidently, I think the lack of concurrency will likely show up in "recursive" rather than real concurrent usage.
e.g. block 1 pushes fluid to block 2 which triggers a neighbour update to block 3 which does something with fluids that therefore wants its own transaction and might want to recurse back into block 1 or 2
I believe this scenario will NOT currently lead to concurrency problems simply because it will just fail when block 3 tries to start a new outer transaction (there is one already present on the thread).

Good point, in that case a workaround would be to schedule an update for the next tick. I think that would be reasonable. :D

[Juuxel]

This was an old review comment, I forgot to add this probably weeks ago. Ignore it 😄

[Technici4n]

Hmmm ok. 😄
I chose toPacket to match fromPacket, perhaps both could be changed to writeToPacket and readFromPacket?

[Technici4n]

Well I can change it but it won't match toNbt and fromNbt. In doubt, probably better to leave it as-is.

[Juuxel]

This doesn't match up with Yarn conventions about serialisation, might be worth changing to match (though definitely not a blocking issue for this PR)