Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The Council Of Bikeshed (Transfer API v1) #1278

Closed
wants to merge 53 commits into from

Conversation

Technici4n
Copy link
Member

@Technici4n Technici4n commented Jan 23, 2021

Disclaimer: Yes, this is a fluid api PR. Please keep the discussion constructive, or I will make sure that this issue is locked to contributors.

This PR is very big, and I expect many questions, so please open a code review thread to discuss a specific part of it.

This PR is also a draft. Please review the API as much as possible, as I think it is close to its final state. I am opening this PR already because the implementation details are taking a while, but there is no need to delay the review of the API for that... So please, I need your comments on the API already while I polish the implementation details and the javadoc. If you want to comment on that too, go ahead, I appreciate any feedback.

I am using [WIP] blocks in this message to indicate what still needs to be done, the rest can be considered ready for review.

Fabric Transfer API v1

Introduction

Building on #1234, this pull requests introduces fabric-transfer-api-v1: a new module for generic resource transfer. That module contains generic resource transfer classes and helpers, and a few helpers to make working with fluids and items easier. Generic means that the API can be used to transfer fluids, items, energy, gasses, enchanted potions, Thaumcraft essentia, or any other quantifiable game resource that can be imagined. (Note: the diff contains #1234 as well, I will rebase when that is merged.)

This pull requests also introduces an item package to fabric-api-lookup-api-v1 which contains ApiLookups for item-provided Apis, and ItemKey which is the equivalent of Fluid for item transfer.

Many thanks to @grondag who wrote Fluidity, of which this PR is very much inspired.

Closes #1166.

Problem scope

The problems this API aims to solve is:

  • Item transfer between blocks. Example use case: AE2, item pipes, machines, ...
  • Fluid transfer between blocks. Example use case: AE2, fluid pipes, machines, ...
  • Fluid transfer with items. Example use case: emptying and filling buckets or modded containers, from the player inventory or from a machine inventory.

More specifically, when we talk about transfer, we mean:

  • Insertion of resources.
  • Extraction of resources.
  • Reading of available resources (think ME Storage Bus).

The API

The core transfer API: Storage and StorageView

Storage<T> is a container for resources of type T. It allows inserting, extracting, and reading the contents. getVersion is optional, and is really meant for AE2-like mods to skip checking inventories that haven't change since the last scan.

StorageView<T> represents some stored resource, and optionally allows extracting it as well.

Note that there is an asymmetry between insertion and extraction.
That is because when we are inserting we know what we want to insert,
but in some cases we don't know what exactly to extract and we have to read the inventory's storage.
That's why it's also possible to extract directly from a StorageView.

[WIP] Base implementations (combined, insert only, extract only).

Transactions

Note that multiple functions have a transaction parameter. A transaction is a global operation where participants guarantee atomicity: either the whole operation succeeds, or it is completely cancelled. In that case, we say that it is aborted. Nested transactions are also supported.

See the Transaction class for more details. Important detail: participants are automatically enrolled in all the transactions of the transaction stack.

Fluid API

The fluid api is simply sided access to Storage<Fluid> instances.
Unsided access may be considered for a later PR.

[WIP] Cauldron compatibility is NYI, and will be added soon for regular 1.16 water cauldrons. For 1.17, I have a plan to allow registration of modded cauldrons.
[WIP] I want to provide a SimpleFluidStorage implementation, and will get to it eventually.

Item API

The item api is sided access to Storage<ItemKey> instances. ItemKeys are an item, and an immutable associated NBT tag. See the class for more details. It is planned that no mods will be using Inventory and SidedInventory directly after this API is merged. Thanks to a fallback provider in the BlockApiLookup, vanilla containers and modded containers relying on the vanilla interfaces are still supported.

Inventory, SidedInventory and PlayerInventory wrappers are also available: InventoryWrappers. Note the existence of an offerOrDrop function with transaction support.

Again, unsided access may be considered for a later PR.

[WIP] I will provide a base implementation for "simple" item storages.
[WIP] I will provide a Slot implementation to allow interacting with ItemKey-based storages. This is quite tricky as all the vanilla ScreenHandler methods mutate the returned item stacks directly.

Item-provided APIs

This PR adds ItemApiLookup and ItemApiLookupRegistry to the fabric-api-lookup-api-v1 module (see #1234).

This PR then defines ContainerItemContext to allow items to interact with the underlying container.

It uses these two building blocks to define the fluid api for container items. Item-containing items (think shulker boxes or backpacks) are out of scope for this PR.

[WIP] I will explain in detail how it works.
[WIP] Vanilla compat is not done yet, for now only water and lava bucket emptying is supported.

Other things

Unsided APIs

Unsided APIs are out of scope because they are not essential for mod compatibility, and the requirements are not that clear to me. The community can use the provided transfer APIs and the fabric-api-lookup-api-v1 module to experiment with that.

Change notification

I also believe that storage change notification is way too complicated to be added to the API. It requires listener management for inventories, and it introduces the need to identify the slot where the change happened, which is non-trivial for complicated inventories such as ME Chests. It would also be optional (we can't support it for Inventory wrappers anyway), and so it would still require special casing by the api consumer. I hope that Storage#getVersion will be good enough for small and mid size storages. Very large storages probably require a specialized thrid party API anyway, for which this PR can probably provide a lot of helpful building blocks.

[WIP] An extensive test mod will be added, for now I am porting my own mod Modern Industrialization to this API.
[WIP] A README will be added too, and a lot of javadoc, including package-info.java.

i509VCB and others added 30 commits October 25, 2020 16:45
Also includes stuff related to the util classes
… pass BlockState and BlockEntity to fallback providers, and reduce insanity overall
@CoolMineman
Copy link
Contributor

Any mod is free to define any denominator who suits them best

this is a con as now users will be stuck with different fluid systems that will not work well together

It is possible for some small amounts of fluid to be stuck here and there

this will be common as mods that treat bottles as 1/3 and mods that use 1/1000 will be common

Containers which cannot accept all amounts of fluid (for example cauldrons can only accept thirds) are much easier to implement.

Implementing cauldrons with the old FTL (which used fixed 1/81000) was trivial

The API is future-proof: if vanilla adds cauldrons with 7 levels, we can support it with no problem. If another denominator proves to be preferable in the future, we can support it.

You will wind up with mods using 1/1000 and not working with existing cauldrons. Additionally, if you think there is a real chance of MC adding cauldrons with a denominator of 7 just make the fixed denom support that.

In theory mod authors can agree on a denominator to prevent this.

Lets just do this now rather than later and reduce complexity in the API while increasing compat.

I don't think it's Fabric's role to pick a denominator for the mods

A goal of Fabric API is to increase compat between mods and this would fit within said goal.

Technici4n added a commit to AztechMC/Modern-Industrialization that referenced this pull request Jan 23, 2021
@Technici4n
Copy link
Member Author

Ok, thanks a lot people for your input on this. After some consideration, I have decided to move this PR to 81000 for fluids. The description will be updated shortly.

I had indeed completely forgotten that API API can be used for seamless conversion between fluid systems if that is required, which solves my main concern of such a choice, i.e. future-proofness.
Combined with the fact that fabric-transfer-api-v1 is kind of a "transfer API factory", anyone is free to define their own system with a different denominator and register the fallbacks if they really want to.

Additionally, we don't have to worry about denominators nor GCD conversions in this already complex API, which allowed me to remove fixed-denominator and integer base implementations.

Finally, I don't think fluids getting stuck in pipes is that big of an issue because the main pipes right now are Modern Industrialization's, for which I was always planning to use 81000 anyway. However, if some people start using 144 mb and others 111 1/9 mb for ingots, we will be in big trouble. That's one more reason to enforce 81000 as the standard.

I am now investigating if StorageFunction is still useful, or if it should be removed.

Copy link

@LemmaEOF LemmaEOF left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking really good! I've got a couple of things, but nothing I'd really consider merge-blocking. My biggest issue is with the name "api lookup api" - see the comment on it below

fabric-api-lookup-api-v1/README.md Show resolved Hide resolved
import net.fabricmc.fabric.impl.lookup.item.ItemKeyImpl;

/**
* The immutable combination of an item and additional NBT data. Compare using {@link ItemKey#equals}.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note in javadocs that it differs from an item stack in that it doesn't have count? Also possibly state that it's for filtering

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely needs more javadoc, I agree. This is both for filtering and direct storage in the item transfer api.

* </ul></p>
* @param <T> The type of the stored resources.
*/
public interface Storage<T> {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem to have any way to get individual entries in a Storage for purposes of testing. That's because this is only meant as an exposure for inter-object interactions yeah?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure exactly what you mean by testing in this context.

@LemmaEOF LemmaEOF mentioned this pull request Jan 24, 2021
Copy link
Contributor

@CoolMineman CoolMineman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@Azzyypaaras
Copy link

WAIT HOLY SHIT, IT IS FINALLY HAPPENING? By fuck I mean, this was coming eventually but it just feels... surreal for us to be this close... If this gets through it could mark a new era of fabric modding, we are on verge of greatness. I have been harsh on my views of the fabric community and the nature of the fapi before but this... it brings me hope, there is something just... simply... there is something that makes me genuinely happy about the fact that, in spite of our differences, of how long we've been in strife over this, in spite of all the issues that plague the fabric community at times... we've still come this far.
Cheers people, a new, woven dawn approaches!

import net.fabricmc.fabric.api.lookup.v1.item.ItemKey;

class ItemKeyCache {
static ItemKey get(Item item, @Nullable CompoundTag tag) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm how do you plan to cache this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As long as we can ensure the tag is not modified elsewhere (much like handling java arrays) it won't be too hard to cache by hashcode. Or if we don't want to cache tag we can just cache if the tag is null, which would make the logic much easier.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I plan on storing tag-less keys directly in a field in the Item, and using a LoadingCache for keys with a tag (à la https://github.com/grondag/fluidity/blob/1.16/src/main/java/grondag/fluidity/impl/article/ArticleCache.java).

ABORTED,
COMMITTED;

public boolean wasAborted() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was -> is

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's a result, was makes sense

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Past tense isn't really going to make sense. The transaction is canceled.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Imo we should just move it to a boolean. If this enum will have more entries this defeats the point of using an enum vs a pseudoenum that allows extension

Copy link
Member Author

@Technici4n Technici4n Jan 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When it is used, the transaction is already closed. Therefore I think it makes more sense to use a past tense.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Past tense for transaction doesn't mean past tense for the result. The result is always there since it has been or was derived.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think should be a boolean. It prevents errors (e.g. wrongly naming the boolean successful instead of failed is very hard to troubleshoot) by making the API so much more explicit, and it's really a trivial class. I am against result booleans in general for those reasons.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We usually use true for success and false for failure/voided stuff. So much that

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is based on my experience, when I was implementing the API I didn't remember if the boolean was for success or failure. A simple result.wasAborted() is so much more expressive. We are talking about a 30 LOC helper enum... I really think booleans should only be used for conditions, and not as flags...

// validate that Transaction instance methods are valid to call.
private void validateCurrent() {
if (!innerLock.isHeldByCurrentThread()) {
throw new IllegalStateException("Transaction operations are not allowed on this thread.");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe print the name of the thread you should be dispatching on?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that information is available without subclassing ReentrantLock: there is a getOwner method, but it's protected.

@Technici4n
Copy link
Member Author

I have decided to remove StorageFunctions entirely. They are a bit surprising to use (why store a private final function and return it in a method) and they are not strictly necessary anymore now that we are using integers only. isEmpty is replaced by supportsInsertion and supportsExtraction in Storage. I will soon add InsertOnlyStorage and ExtractOnlyStorage interfaces to provide default implementations for extraction and insertion respectively.

PS: I know the description desperately needs an update, I will do it eventually.

*/
@ApiStatus.NonExtendable
public interface ItemKey {
ItemKey EMPTY = ItemKeyImpl.of(Items.AIR, null);
Copy link
Contributor

@liach liach Jan 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason of keeping a separate ItemKeyImpl? Imo you can just have this as a pojo in api than writing a separate impl

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd agree here with a final class pojo for this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should be an interface, to allow people to make copy-on-write ItemKeys and whatnot

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Item keys are supposed to be immutable and thus thread safe, what the heck are you suggesting @Devan-Kerman
also if this is an interface, it is hard to implement equals across interfaces

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What? the ItemKey itself can be immutable, the underlying nbt tag is copied when the ItemKey is initialized, which isn't always necessary, people should be allowed to make their own ItemKey implementations, because they can optimize, like that case. Another one would be if some tool's ItemKey only cares about the durability, instead of storing a CompoundTag, it can store an int (if it doesn't have additional data of course)

Copy link
Contributor

@liach liach Jan 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just compare the compound tag and item in the equals method

@Devan-Kerman you realize this just defeats your whole point of tool not having to keep a full stack for performance purposes?
In the end the nbt isn't too big a hurdle. Your "save a tag" is pretty worthless compared to the problems and risks it introduces. if tags are that expensive why does mojang add a nbt to every tool item?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this should be a pojo

Being a pojo doesn't prevent it from being cached. You will just hide the constructor to be package private and users can only call a static method to obtain instances.

Also since there is mutability concern, I suggest making ItemKey a public final class to ensure safety.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure I understand the value of making this a pojo? I fail to see a direct benefit, and I would very much prefer the ugly implementation details to be kept outside of public API classes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you realize this just defeats your whole point of tool not having to keep a full stack for performance purposes?

no? copying 2 tags != copying 1 tag, and u could also short cut in some cases too.

if tags are that expensive why does mojang add a nbt to every tool item?

they also made TACS, item performance can actually be a pretty big problem

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think Mojang's decisions are made with modded item transfer in mind...

Anyway, I don't think there is a viable alternative to the ItemKey concept as it currently stands. You can't reasonably have immutable item + NBT, and allow mutating the NBT at the same time. Do however note that a lot of modded containers or pipes never touch the NBT directly. In some cases this may cause a tiny loss of performance, but I expect chest performance to be much better with ItemKeys because the stacks don't have to be copied... Splitting an expensive NBT stack of 2 items into 2 chests does not involve any allocation with item keys.

@Technici4n
Copy link
Member Author

I have updated the description of the PR with the changes that happened today.

We really need #1234 merged ASAP now that this PR needs reviews.
Speaking of reviews, although it is far from mergeable, I am pretty happy with how this PR was received, and how fast it's converging to an excellent transfer API imo. By all means let's keep discussing and improving it!

@Technici4n Technici4n requested a review from a team January 25, 2021 00:08
*
* <p>For transfer functions, the returned amount must be non-negative, and smaller than the passed maximum amount.
* Consumers of these functions are encourage to throw an exception if these postconditions are violated.
* @param <T> The type of the stored resources.
Copy link

@OroArmor OroArmor Jan 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would give some examples of what the stored resources can be. should I store Items or ItemStacks?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ItemKey

Copy link
Member Author

@Technici4n Technici4n Jan 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be obvious when you see how it's defined in ItemApi, although I agree that this needs more example code.

Copy link
Contributor

@williambl williambl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feels like an elegant API. I'd be happy to use this. (Not going to mark this as an approval just because the PR's not complete yet).

I used to be in favour of dynamic-denominator fractions (mostly just because LBA uses it and LBA's really nice), but honestly I think 81000-denom works fine for pretty much all purposes. (As long as nobody exposes it to the player of course)

EmptyItemsRegistry.registerEmptyItem(emptyItem, fullItem, fluid, amount);
}

// TODO: potion handling
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this imply it's in-scope to treat Potions similarly to Fluids, like LBA does?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not exactly. The plan is to allow any mod to define a mapping Potion -> (Fluid, long) so that the Storage<Fluid> for a full potion can correctly empty potions with a registered fluid mapping. In particular, this would only be used by the API for water bottles which are actually Potions.Water, but other mods may use it to register a Fluid per Potion if they want to.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see. Sounds useful.

*/
public interface Storage<T> {
/**
* Return whether this storage supports insertion, meaning that {@link #insert} will not always return 0.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be clearly defined as to when you should return false? Should it depend on state (does this support insertion right now?) or not (does this ever support insertion?)? For example, if a tank is at its capacity and cannot currently accept any insertions, should this be false? What is this intended to be used for? Would a pipe choose not to connect to a Storage that returns false here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It never depends on the state, that certainly needs better javadoc I agree.

@Technici4n
Copy link
Member Author

Replaced by #1352, #1356 and subsequent PRs.

@Technici4n Technici4n closed this Mar 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed in progress This issue has PR(s) open to resolve it (or a PR that is WIP) new module Pull requests that introduce new modules priority:low Low priority PRs that don't immediately need to be resolved reviews needed This PR needs more reviews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The Fluid API discussion issue