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.

Sign up for GitHub

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
Closed

The Council Of Bikeshed (Transfer API v1) #1278

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
53 commits
Select commit Hold shift + click to select a range
c38d075
Implement the item and block providers for provider api.
i509VCB Oct 25, 2020
7a1ab86
NotNull is implicit; so remove instances of it
i509VCB Oct 25, 2020
e1f0ed2
Some javadoc for context key
i509VCB Oct 25, 2020
a9ba2a4
Inline some api lookup return values
i509VCB Oct 25, 2020
3bd4cde
Add first iteration of provider api test mod
Technici4n Oct 26, 2020
85e56eb
Reorder no context field so static block is not needed.
i509VCB Oct 26, 2020
91980b6
Fix a bug related to testmod block entity not checking if it has a world
i509VCB Oct 26, 2020
8b48098
Add block entity caches
Technici4n Nov 21, 2020
77d8821
Add fallback providers
Technici4n Nov 24, 2020
10881ca
Merge branch '1.16' into api-provider
Technici4n Nov 25, 2020
0aa755e
Inline ContextKey, remove ApiLookup, add Class<T> and Class<C> parame…
Technici4n Nov 26, 2020
fa81038
Add item fallback providers
Technici4n Nov 26, 2020
fd63147
Add javadoc for BlockApiLookup
Technici4n Nov 26, 2020
64c61d8
Javadoc fixes, build.gradle update and version bump
Technici4n Nov 26, 2020
58a1cb0
Add a BlockState parameter to BlockApiProvider
Technici4n Dec 2, 2020
4f149eb
Bye items 🦀
Technici4n Dec 13, 2020
8d20007
Rename to fabric-api-lookup-api-v1 and add package-info.java
Technici4n Dec 17, 2020
d3b050d
Document all api classes, and fix the memory leak in the cache
Technici4n Dec 20, 2020
ed5f626
Make docs a bit less insane
Technici4n Dec 21, 2020
0cd4e36
Revert build.gradle changes
Technici4n Dec 21, 2020
da3072b
Add example code to javadoc
Technici4n Dec 21, 2020
3c3b8bb
Write README, document what BlockApiCache does, fix checkstyle
Technici4n Dec 22, 2020
30d45b6
Add @Nullable BlockState and BlockEntity parameters where applicable,…
Technici4n Dec 22, 2020
5477c43
Fix checkstyle
Technici4n Dec 22, 2020
dee0ad2
Some javadoc cleanup and slight tweaks in impl
i509VCB Dec 23, 2020
cb61f6d
Fix ApiLookupMapImpl bug and add a relevant unit test in the testmod,…
Technici4n Dec 25, 2020
047fe88
Javadoc formatting.
PepperCode1 Jan 11, 2021
eaba1d7
Module name in logger, remove inconsistent <br> tag
Technici4n Jan 13, 2021
8f91dfb
Add module lifecyle
Technici4n Jan 19, 2021
0f76220
Add ItemKeys already
Technici4n Jan 20, 2021
12d2832
Port most of transfer api stuff from FTL
Technici4n Jan 21, 2021
cc57649
Add Movement
Technici4n Jan 21, 2021
0373108
More Preconditions, identity function, TransactionResult instead of b…
Technici4n Jan 22, 2021
f13dd79
Set server thread
Technici4n Jan 22, 2021
90c3fdf
The item-provided fluid api nightmare continues
Technici4n Jan 23, 2021
5533049
Add denominator to Movement#move
Technici4n Jan 23, 2021
5de61bb
Allow transaction multithreading and make nesting explicit
Technici4n Jan 23, 2021
e940ac9
Transaction bug
Technici4n Jan 23, 2021
3516324
Forgot to set isOpen to false
Technici4n Jan 23, 2021
74fc1dc
Remove functions without a denominator parameter
Technici4n Jan 23, 2021
904a804
Typos
Technici4n Jan 23, 2021
e979f75
Apply first reviews
Technici4n Jan 23, 2021
3193cfd
Remove fractions 🦀
Technici4n Jan 24, 2021
4bd5c77
Add javadoc to the core transfer interfaces
Technici4n Jan 24, 2021
5ab7ccd
Inline StorageFunction into Storage, add tx parameter to forEach
Technici4n Jan 24, 2021
74a1014
Apply i5's review
Technici4n Jan 24, 2021
44d1379
Add missing `final`s
Technici4n Jan 24, 2021
91e3be8
Clean up and document inventory wrappers
Technici4n Jan 24, 2021
e08708c
Bucket filling and emptying for all fluids
Technici4n Jan 25, 2021
6648541
Oops
Technici4n Jan 25, 2021
6b963c0
Add cauldron wrapper
Technici4n Jan 26, 2021
5216ed5
Fix cauldron wrapper
Technici4n Jan 26, 2021
1412856
Add key mapping functions to the item fluid api
Technici4n Jan 27, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ...up-api-v1/src/main/java/net/fabricmc/fabric/api/lookup/v1/item/ItemApiLookupRegistry.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@

import net.fabricmc.fabric.impl.lookup.item.ItemApiLookupRegistryImpl;

public class ItemApiLookupRegistry {
public final class ItemApiLookupRegistry {
public static <T, C> ItemApiLookup<T, C> getLookup(Identifier lookupId, Class<T> apiClass, Class<C> contextClass) {
Objects.requireNonNull(apiClass, "Id of API cannot be null");
Objects.requireNonNull(contextClass, "Context key cannot be null");
Expand Down
4 changes: 4 additions & 0 deletions fabric-api-lookup-api-v1/src/main/java/net/fabricmc/fabric/api/lookup/v1/item/ItemKey.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

import java.util.Objects;

import org.jetbrains.annotations.ApiStatus;
import org.jetbrains.annotations.Nullable;

import net.minecraft.item.Item;
Expand All @@ -31,7 +32,10 @@

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

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.

*
* <p>Do not implement.
*/
@ApiStatus.NonExtendable
public interface ItemKey {
Technici4n marked this conversation as resolved.
Show resolved Hide resolved
ItemKey EMPTY = ItemKeyImpl.of(Items.AIR, null);
Copy link
Contributor

@liach liach Jan 24, 2021 •
edited
Loading

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 •
edited
Loading

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.


Expand Down
3 changes: 3 additions & 0 deletions ...er-api-v1/src/main/java/net/fabricmc/fabric/api/transfer/v1/fluid/FluidPreconditions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@
import net.minecraft.fluid.Fluid;
import net.minecraft.fluid.Fluids;

/**
* Preconditions for fluid transfer.
*/
public class FluidPreconditions {
Technici4n marked this conversation as resolved.
Show resolved Hide resolved
public static void notEmpty(Fluid fluid) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Add the throws declaration on the method.

Copy link
Member Author

Choose a reason for hiding this comment

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

We probably shouldn't. Preconditions doesn't use it, and I just found this reference: https://stackoverflow.com/questions/824217/should-methods-that-throw-runtimeexception-indicate-it-in-method-signature.

if (fluid == Fluids.EMPTY) {
Expand Down
7 changes: 6 additions & 1 deletion ...sfer-api-v1/src/main/java/net/fabricmc/fabric/api/transfer/v1/item/InventoryWrappers.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@

import org.jetbrains.annotations.Nullable;

import net.minecraft.entity.player.PlayerEntity;
import net.minecraft.entity.player.PlayerInventory;
import net.minecraft.inventory.Inventory;
import net.minecraft.util.math.Direction;
Expand All @@ -30,7 +31,7 @@
import net.fabricmc.fabric.api.transfer.v1.storage.Storage;
import net.fabricmc.fabric.impl.transfer.item.InventoryWrappersImpl;

public class InventoryWrappers {
public final class InventoryWrappers {
// List<Storage<ItemKey>> has 7 values.
Technici4n marked this conversation as resolved.
Show resolved Hide resolved
// The 6 first for the various directions, and the last element for a null direction.
private static final WeakHashMap<Inventory, List<Storage<ItemKey>>> WRAPPERS = new WeakHashMap<>();
Expand All @@ -42,6 +43,10 @@ public static Storage<ItemKey> of(Inventory inventory, @Nullable Direction direc
return direction != null ? storages.get(direction.ordinal()) : storages.get(6);
}

public static PlayerInventoryWrapper ofPlayer(PlayerEntity player) {
return ofPlayerInventory(player.inventory);
}

public static PlayerInventoryWrapper ofPlayerInventory(PlayerInventory playerInventory) {
Technici4n marked this conversation as resolved.
Show resolved Hide resolved
return (PlayerInventoryWrapper) of(playerInventory, null);
}
Expand Down
8 changes: 7 additions & 1 deletion ...sfer-api-v1/src/main/java/net/fabricmc/fabric/api/transfer/v1/item/ItemPreconditions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,10 @@

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

public class ItemPreconditions {
/**
* Preconditions for item transfer.
*/
public final class ItemPreconditions {
public static void notEmpty(ItemKey key) {
if (key == null || key.isEmpty()) {
throw new IllegalArgumentException("ItemKey may not be empty or null.");
Expand All @@ -40,4 +43,7 @@ public static void notEmptyNotNegative(ItemKey key, long amount) {
ItemPreconditions.notEmpty(key);
Preconditions.checkArgument(amount >= 0);
}

private ItemPreconditions() {
}
}
6 changes: 4 additions & 2 deletions ...r-api-v1/src/main/java/net/fabricmc/fabric/impl/transfer/transaction/TransactionImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ public class TransactionImpl implements Transaction {

public static void setServerThread(Thread serverThread) {
if (innerLock.isLocked()) {
throw new AssertionError("Something is terribly wrong.");
throw new AssertionError("Trying to change server thread while a transaction is open.");
}

TransactionImpl.serverThread = serverThread;
Expand All @@ -57,7 +57,9 @@ public static void setServerThread(Thread serverThread) {
// 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.");
throw new IllegalStateException(String.format(
"Transaction operations are already ongoing on another thread.\nCurrent thread: %s.\n",
Thread.currentThread().getName()));
}

if (!allowAccess) {
Expand Down