[1.21] Handler Rework #1115
[1.21] Handler Rework #1115
Conversation
|
| * | ||
| * <p>Examples include item resource with air as an item. | ||
| */ | ||
| boolean isBlank(); // TODO: potentially use a different name |
There was a problem hiding this comment.
isAbsent() perhaps to match an existing precedent set by Optional.
There was a problem hiding this comment.
isBlank better than isAbsent
There was a problem hiding this comment.
isEmpty is better than both but Tech vetoed it
There was a problem hiding this comment.
I disagree isBlank is better, but at this point I say isEmpty is the more correct choice. Looking at every data structure I can find isEmpty is exactly what the intent is. (I was incorrect on isAbsent being an optional method).
I don't think Tech should be given reign to "veto" a more correct term really.
There was a problem hiding this comment.
The reasoning is that a resource being blank doesn't necessarily correspond to a slot or stack being empty.
There was a problem hiding this comment.
It isn't a matter of if the stack is empty, is if the resource is empty. Which in the case that it matches air, it is
There was a problem hiding this comment.
Optional<ItemStack>.isEmpty() isn't asking if the itemstack is empty for example, it is asking if the optional contains an itemstack
| return innerStack.getFluidType().isVaporizedOnPlacement(level, pos, innerStack); | ||
| } | ||
|
|
||
| public void onVaporize(Player player, Level level, BlockPos pos) { |
There was a problem hiding this comment.
So I understand these are to not bypass immutability of fluidstack, but I feel the use cases for each call is going to be a nightmare to support. getTemperature or viscosity for example would be things I'd want, but then need the API to expose.
There was a problem hiding this comment.
yeah thats a valid concern. i added those there because they were methods that came up while i was in the middle of implementing some of the other default handlers in the rewrite. They may not be the best approach tho. Its not that expensive to just make a new fluid stack, so i guess that would be the solution, just do .toStack() and call the methods on that. in my ideal world the fluid resource would have all the same methods for info that the fluid stack did, but that might be too much for this PR
src/main/java/net/neoforged/neoforge/transfer/energy/IEnergyHandler.java
Outdated
Show resolved
Hide resolved
src/main/java/net/neoforged/neoforge/transfer/items/templates/ItemStorage.java
Outdated
Show resolved
Hide resolved
src/main/java/net/neoforged/neoforge/transfer/fluids/templates/AttachmentFluidStorage.java
Outdated
Show resolved
Hide resolved
| * A handler for placing and picking up fluid blocks in the world. | ||
| */ | ||
| public class BlockFluidHandler implements ISingleResourceHandler<FluidResource> { | ||
| private static final Logger LOGGER = LogManager.getLogger(); |
There was a problem hiding this comment.
Is this the right place for this? I would assume NeoForgeMod.LOGGER would be the correct one to use, but if the standard is to have a unique one here, then disregard :)
| * | ||
| * @param <T> The type of resource | ||
| */ | ||
| public class EmptyHandler<T extends IResource> implements ISingleResourceHandler<T> { |
There was a problem hiding this comment.
Is there a reason we don't want to make this final?
| @Override | ||
| public T getResource(int index) { | ||
| for (IResourceHandler<T> storage : getHandlers()) { | ||
| if (index < storage.size()) { |
There was a problem hiding this comment.
We probably only want to calculate storage.size() once here rather than doing it both here and when adjusting the index. Just in case, similar to this class' size implementation it isn't just a simple getter. On top of that we may want a couple private helper methods to simplify iterating over this and finding the correct index.
| protected final T emptyResource; | ||
| protected final ResourceStack<T> emptyStack; | ||
|
|
||
| protected Predicate<T> validator = r -> true; |
There was a problem hiding this comment.
I would prefer if this was final, and we just had a constructor overload that accepted the validator
|
|
||
| @Override | ||
| public int getCapacity() { | ||
| return singleItemLimit * context.getAmount(); |
There was a problem hiding this comment.
Is there a worry that this can overflow?
| public int extract(int index, T resource, int amount, TransferAction action) { | ||
| for (IResourceHandler<T> storage : getHandlers()) { | ||
| if (index < storage.size()) { | ||
| if (storage.allowsExtraction()) { |
There was a problem hiding this comment.
Do we need to check this? I feel like we should be able to just assume that the inner storage will short circuit if they aren't allowing extraction. That way if their check is for some reason not trivial, then it only has to be done once.
There was a problem hiding this comment.
Yes, it is never necessary to check allowsXxx before doing xxx (which is supposed to check it anyway). Perhaps that should be added to the javadoc.
| public int insert(int index, T resource, int amount, TransferAction action) { | ||
| for (IResourceHandler<T> storage : getHandlers()) { | ||
| if (index < storage.size()) { | ||
| if (storage.allowsInsertion()) { |
There was a problem hiding this comment.
Do we need to check this? I feel like we should be able to just assume that the inner storage will short circuit if they aren't allowing insertion. That way if their check is for some reason not trivial, then it only has to be done once.
| import javax.annotation.Nullable; | ||
| import java.util.function.Predicate; | ||
|
|
||
| public class HandlerUtil { |
There was a problem hiding this comment.
Helper classes should be final and have a private constructor.
| * while {@link TransferAction#EXECUTE} will actually perform the action. | ||
| * @return The amount of the resource that was (or would have been, if simulated) inserted. | ||
| */ | ||
| int insert(T resource, int amount, TransferAction action); |
There was a problem hiding this comment.
We probably want to give this function a default implementation.
| * while {@link TransferAction#EXECUTE} will actually perform the action. | ||
| * @return The amount of the resource that was (or would have been, if simulated) extracted. | ||
| */ | ||
| int extract(T resource, int amount, TransferAction action); |
There was a problem hiding this comment.
We probably want to give this function a default implementation.
| int getCapacity(int index, T resource); | ||
|
|
||
| /** | ||
| * Gets the theoretical maximum amount that the given index can hold of a resource. | ||
| * | ||
| * @param index The index to get the limit from. | ||
| * @return The limit of the resource at the given index. | ||
| */ | ||
| int getCapacity(int index); |
There was a problem hiding this comment.
Having both was not done before. To be discussed.
| boolean allowsInsertion(int index); | ||
|
|
||
| /** | ||
| * Checks if the given index allows extraction of a resource, regardless of the state of the handler. | ||
| * <p> | ||
| * As long as the handler could, under the right conditions, allow a resource to be extracted from the given index, | ||
| * this should return true. | ||
| * | ||
| * @param index The index to check. | ||
| * @return True if the resource can be extracted, false otherwise. | ||
| */ | ||
| boolean allowsExtraction(int index); |
There was a problem hiding this comment.
It might be worth discussing whether these methods should exist at all, since they didn't exist before?
| * | ||
| * @param <T> The type of resource | ||
| */ | ||
| public class EmptyHandler<T extends IResource> implements ISingleResourceHandler<T> { |
There was a problem hiding this comment.
Shouldn't the empty handler have 0 slots?
| @Override | ||
| public int insert(int index, T resource, int amount, TransferAction action) { | ||
| if (amount <= 0 || resource.isEmpty()) return 0; | ||
| var contents = getContents().builder(); |
There was a problem hiding this comment.
This allocation is not great I think. Transfer should be allocation-free in most cases.
| * | ||
| * @param <T> The type of resource that this storage can accept. | ||
| */ | ||
| public class VoidHandler<T extends IResource> implements ISingleResourceHandler<T> { |
There was a problem hiding this comment.
I'd maybe call this Voiding.
| import net.neoforged.neoforge.transfer.handlers.IResourceHandler; | ||
| import net.neoforged.neoforge.transfer.handlers.ResourceStorageContents; | ||
|
|
||
| public abstract class ResourceStorage<T extends IResource> implements IResourceHandler<T> { |
There was a problem hiding this comment.
I am not sure that this abstraction is worth it. I'll have to look further into it.
| return delegate.get(); | ||
| } | ||
|
|
||
| public static class Modifiable<T extends IResource> extends DelegatingHandlerWrapper<T> implements IResourceHandlerModifiable<T> { |
There was a problem hiding this comment.
Do we want to have this pattern for many helper classes? It might get a bit unwieldy.
|
I just ran into this PR. Without having looked into it in detail, feel free to take inspiration from the Ingredient Components API that I've been working with for several years now: https://github.com/CyclopsMC/CommonCapabilitiesAPI/wiki/Ingredient-Components |
|
@CodexAdrian, this pull request has conflicts, please resolve them for this PR to move forward. |
|
Adrian O.V seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
|
@CodexAdrian, this pull request has conflicts, please resolve them for this PR to move forward. |
Handler Rework
This PR is just a continuation of #944 which had some issues
Motivation
NeoForge's storage situation is not great. ItemHandler and FluidHandler are both extremely helpful to devs trying to create a unified standard for storage of resources, but aren't consistent with each other and rely on the usage of mutable resources with amounts attached for transfer. This creates situations where devs have to return copies of their resources in order to safely ensure a dev isnt trying to manipulate their storage directly, or copy the stack passed into the insert method before its inserted in order to ensure the implementor of insert method doesn't attempt to modify the stack when its inserted. Not to mention the inconvenient issues that arise when the resource (item + components) are joined in the same object where so much copying and mutation needs to occur to the stack when most of the time the developer just wants to change the amount the stack contains or insert the same stack into a container with a different amount.
The ideal solution to these problems would be creating a single unified storage class (for consistency) that uses immutable resources (for the mutation problems) whose count is separated from the resource (for the inconvenience problems).
Implementation
Much of the implementation was inspired by FabricMC/fabric#1356 and how it was subsequently built out in the later years, and is a continuation of Tech's initial PR to introduce immutable resources in #715
IResourceHandlerIResourceHandleris a new interface that will eventually fully replace usages ofIItemHandlerandIFluidHandler. It is a slotted container that stores an integer amount of a resource of typeT. This type doesn't necessarily have to be anIResourceclass, but utilities will be provided for storage classes that do. In this implementation,Tshould be immutable and should be countless, as the amount is retrieved in a separategetAmount(slot)method.IResourceandFluidResource/ItemResourceIResourceis the basis for the Neo provided storage capabilities. Its an immutable data component holder with a utility methods that return a new resource with modified components without mutating the resource to make it easy to interact with. Lastly, it contains anisBlankmethod to indicate whether or not the resource should be considered blank or empty, like if the resource is ofItems.AIRorFluids.EMPTY. The default resources provided by Neo areFluidResourceandItemResourceand represent immutable types of a fluid/item and data components.ResourceStackResourceStackis a utility class that contains an instance ofIResourceand an integer amount. It provides the same utility methodsIResourcemethods provides for copying with modifications while also providing similar utilities for amount based copying.ResourceStackshould largely replace the usage ofItemStackorFluidStackin the usage of code.IItemContextItemContextis a new interface for general usage when interacting with inventories. Its centered around a main slot, whose contentsIItemContextcan be used to get its current state (resource and amount). This context also allows devs to manipulate theItemResourceand amount in the main slot, by extracting, inserting or exchanging items into it. Extracting will extract items of a givenItemResourceand amount from the slot, returning how much was extracted from the main slot. Inserting into the context will not only insert into the main slot, but is also intended to insert whatever does not fit into the main slot into some kind of outer container. This can be an outer container like a buffer or a context could choose to drop the excess onto the ground, its up to the implementor ofIItemContextto say where items go. Exchanging allows devs to extract however many items that are requested to be exchanged and then inserting a new resource in its place up to the amount extracted. Devs should also dump excess items that were not successfully inserted into the main slot into some kind of overflow as well (if you choose to support having an overflow for your items).This new class largely replaces the need for any item specific capabilities, like
IFluidHandlerItem, while also providing devs with much needed utility to insert excess into a slot other than itself. This should make it possible to, for example, have a stacked fluid container where only 1 of 16 items are drained. The fluid storage can simply callexchangewith 1 empty storage container, without needing to worry about whether or not the stack is of stack size 1.ItemContextalso contains a helpful utility which you can use to retrieve the capability found on item in the main slot without needing to pass in an ItemStackThe Fallen
This PR also sets
IItemHandlerandIFluidHandlerfor deprecation and marked them for removal in 1.22. Currently these classes have been retrofit to extendIResourceHandler<ItemResource>andIResourceHandler<FluidResource>respectively, and will eventually be phased out entirely for just using the base storage classes. While consideration to keeping some of the old API around has been made, this PR does still make a breaking change in the removal ofIFluidHandlerItem, and the modification of existing Item capabilities to have the aforementionedIItemContextas its new context class (which also replaces the need of having thegetContainermethod inIFluidHandlerItem).Interaction changes
With immutable resources and
IResourceHandler<ItemResource>andIItemContext, its safe to say that modifying the ItemStack in which a capability was retrieved from will not always achieve the desired effect. As such, developers should now take care to useItemContextto make any modifications needed to the main stack using the methods provided inItemContext, even if those changes are just component or size related changes. Developers should treat that ItemStack given to them in the capability provider as read only, and should also take care to note it is only accurate at the time of creation of the cap class, and may not be updated to reflect changes made withIItemContext. Devs should use the resource and amount provided inIItemContextas an accurate image of what the item in its slot currently looks likeNotes
This was a doozy to write, and might not be a complete overlook of this PR when it is time to merge it. I'll do my best to update the description as needed. This PR is (as of May 8th, 2024) not complete, but was opened so others could see what the changes I was making were and could comment on them while in dev.