Further Fluid Tank Implementation #78
No reviewers
Labels
No labels
Area-Assets
Area-Backend
Area-Conduits
Area-Datapacks
Area-Lang
Area-Mod Compat
Area-Parity
Area-Rendering
Good first issue
MC-1.19.2
MC-1.20.1
MC-1.20.4
MC-1.20.6
MC-1.21
MC-1.21.1
Modtoberfest
P-0-High
P-1-Medium
P-2-Low
Status-Awaiting Response
Status-Behind-Flag
Status-Blocked
Status-Cannot Reproduce
Status-Duplicate
Status-Help Wanted
Status-Incomplete Report
Status-Invalid
Status-Needs LTS Backport
Status-Needs Updating
Status-Stale
Status-To Implement
Status-Triage
Status-Wontfix
Status-Wontmerge
Type-Backport
Type-Bug
Type-Documentation
Type-Enhancement
Type-Question
Type-RFC
Type-Suggestion
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: Team-EnderIO/EnderIO#78
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feature/fluid_tank"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Description
Added the following functionality to Fluid Tanks:
IFluidHandlerItemIFluidHandlerItemcapabilityPossible Breaking Changes:
MachineBlockEntitymean the IOConfig, Redstone Control, and Inventory data is only written to NBT if it needs to be (i.e. is non-default), so if something expects it to always be there, it now may not becopyNBTmethod ofMachinesLootTableto use the newImprovedContextNbtProvidermay affect other things, and may not be futureproof. TheImprovedContextNbtProvidercannot simply extend theContextNbtProviderclass because itprivates a lot of its underlying functionality (I implementedImprovedContextNbtProviderby just duplicating theContextNbtProviderclass and editing the parts I needed to) so if changes occur toContextNbtProviderin the future, they may not be reflected in this code. I don't know if this is something we could submit a PR to Forge for, to expose the class more?Checklist:
Just a quick skim through, a number of comments and changes. I'm not a fan of how some of the animation stuff is working in terms of tracking all the block entities in a map; might be worth looking into other ways of doing this (maybe storing the animation state on the BE instead somehow).
@ -59,0 +60,4 @@/*** Whether the IO config is unchanged from initial values.*/boolean isDefault();Personally not a fan of having this method, but I'm unsure of a way that would alleviate the need for it
@ -0,0 +6,4 @@/*** Class for animating between two values over an amount of time, using {@link Mth#lerp}.*/public class Animation {Javadocs please :)
Might be worth investigating if there's a vanilla constant for this, but I do like the idea of keeping all our tags together
Can you use the annotations instead of registering manually here please
I'm not a massive fan of tracking things like this, but again I understand why its here
@ -35,2 +42,2 @@if (tank.getFluidAmount() > 0) {FluidStack fluidStack = tank.getFluid();// Check to see if there is an existing animation for this block.AnimationInformation information = blockEntity.getAnimationInformation();Could I ask for more documentation around the animation implementation you've added?
@ -96,2 +107,4 @@&& outputItem.getCount() < outputItem.getMaxStackSize())) {if (fillTankFromBucket(filledBucket) > 0) {inputItem.shrink(1);getInventory().insertItem(1, Items.BUCKET.getDefaultInstance(), false);Maybe move this into a static utility class like the EnergyUtil?
Mark shouldDrain as nullable if you're allowing null
is it necessary to try calling remove? As far as my understanding goes, whenever save is called its a fresh tag, so existing keys shouldn't be present?
@ -417,3 +415,3 @@public void load(CompoundTag pTag) {// Load io config.ioConfig.deserializeNBT(pTag.getCompound("io_config"));ioConfig.deserializeNBT(pTag.getCompound(MachineNbtTags.IO_CONFIG_NBT_KEY));I don't think its necessary to do this check, for the very little storage it saves it'll add a bunch of time onto the saving process
@ -26,4 +58,28 @@ public class FluidTankItem extends BlockItem {}Why this check? We know this is a FluidTankItem
@ -0,0 +27,4 @@* entity's NBT data based on an {@link LootContext.EntityTarget} but without* the id and positional data.*/public class ImprovedContextNbtProvider implements NbtProvider {What is the motive behind needing this over the old system?
There is only a private one in BlockItem
@ -96,2 +107,4 @@&& outputItem.getCount() < outputItem.getMaxStackSize())) {if (fillTankFromBucket(filledBucket) > 0) {inputItem.shrink(1);getInventory().insertItem(1, Items.BUCKET.getDefaultInstance(), false);also fix naming. Methods start with a lowercase words
@ -417,3 +415,3 @@public void load(CompoundTag pTag) {// Load io config.ioConfig.deserializeNBT(pTag.getCompound("io_config"));ioConfig.deserializeNBT(pTag.getCompound(MachineNbtTags.IO_CONFIG_NBT_KEY));The reason for this check is that when the
BlockEntityis converted over into anItemafter being broken, it retains the extra information in the NBT data. So, for example, when a Fluid Tank is spawned/crafted into the inventory, its item NBT data is{id: "enderio:fluid_tank", Count: 1b}. When it is placed into the world, then immediately broken again, its item NBT data (without my changes) becomes:{id: "enderio:fluid_tank", Count: 1b, tag: {BlockEntityTag: {x: ..., y: ..., z: ..., io_config: ..., redstone: ..., inventory: ..., id: "enderio:fluid_tank"}}}. This has the annoying (to me at least), although admittedly not game-breaking, issue of the fact that that Fluid Tank item will not stack with another empty Fluid Tank, despite no logical reason not to.This was simply the first way I came across of (partly) solving the issue. So I could move the functionality across into the NBT copying? But would probably need a specific provider for the Machine blocks, in the event that other things need to use that functionality.
I'll have to defer to your better judgement about the about the time in the saving process, but I could try to simplify the checks to just simple boolean checks by updating whether each item is empty/default when they are changed? Also isn't it better to potentially not save & transmit data, even at the slight increased complexity/time cost?
@ -26,4 +58,28 @@ public class FluidTankItem extends BlockItem {}I just copied this from the
BucketIemwhere they do the same check. It does seem odd, so happy to remove.@ -35,2 +42,2 @@if (tank.getFluidAmount() > 0) {FluidStack fluidStack = tank.getFluid();// Check to see if there is an existing animation for this block.AnimationInformation information = blockEntity.getAnimationInformation();Yes of course, do you mean just comments and Javadocs, or actual documentation in like a wiki or something?
Fixed in
7de7cbaaa8and11535800ec@ -96,2 +107,4 @@&& outputItem.getCount() < outputItem.getMaxStackSize())) {if (fillTankFromBucket(filledBucket) > 0) {inputItem.shrink(1);getInventory().insertItem(1, Items.BUCKET.getDefaultInstance(), false);Fixed in
ccb0d52f86@ -96,2 +107,4 @@&& outputItem.getCount() < outputItem.getMaxStackSize())) {if (fillTankFromBucket(filledBucket) > 0) {inputItem.shrink(1);getInventory().insertItem(1, Items.BUCKET.getDefaultInstance(), false);Fixed in
06c076d25b@ -35,2 +42,2 @@if (tank.getFluidAmount() > 0) {FluidStack fluidStack = tank.getFluid();// Check to see if there is an existing animation for this block.AnimationInformation information = blockEntity.getAnimationInformation();Yeah just comments and javadocs please
@ -0,0 +6,4 @@/*** Class for animating between two values over an amount of time, using {@link Mth#lerp}.*/public class Animation {Fixed in
cb65ecffc8@ -35,2 +42,2 @@if (tank.getFluidAmount() > 0) {FluidStack fluidStack = tank.getFluid();// Check to see if there is an existing animation for this block.AnimationInformation information = blockEntity.getAnimationInformation();Fixed in
3010b205c2I'm not sure why I didn't store the animation information in the BE in the first place 😅 I will give that a go.
@ -0,0 +27,4 @@* entity's NBT data based on an {@link LootContext.EntityTarget} but without* the id and positional data.*/public class ImprovedContextNbtProvider implements NbtProvider {As with the other checks above the reason for this is that when the
BlockEntityis converted over into an Item after being broken, it retains extra unneededBlockEntityinformation in the NBT data. So, for example, when a Fluid Tank is spawned/crafted into the inventory, its item NBT data is{id: "enderio:fluid_tank", Count: 1b}. When it is placed into the world, then immediately broken again, its item NBT data (without this class) becomes:{id: "enderio:fluid_tank", Count: 1b, tag: {BlockEntityTag: {x: ..., y: ..., z: ..., id: "enderio:fluid_tank"}}}. Again, this has the annoying issue of the Fluid Tank item not stacking with another empty Fluid Tank.This issue comes from the
ContextNbtProvidercallingsaveWithFullMetadataon theBlockEntity, writing thex,y,z, andidto the NBT Tag. This provider is a carbon copy of theContextNbtProviderclass, except it callssaveWithoutMetadataon theBlockEntity, and returnsnullif that returnsnull. I would have just extended theContextNbtProviderclass, but the relevant methods to override areprivate.As I mentioned above, the other exclusion of tags being written could be performed here as well.
@ -0,0 +1,84 @@package com.enderio.core.client.rendering;Fix link using # and put in one line
make start, target and totalDurationTick final
@ -16,57 +19,152 @@ import net.minecraft.core.Direction;import net.minecraft.world.inventory.InventoryMenu;fluid will be assigned in the constructor, don't do it here
fluids shouldn't be nullable but rather Fluids.EMPTY
not Nullable
mark nullable
Don't use exceptions for controlflow, the exception will never be thrown here, as ANIMATION_TICKS is positive
mark as nullable
@ -0,0 +1,200 @@package com.enderio.machines.common.block;use fluidHandlerCap.isEmpty
can be simplified to blockIsEmpty, because if blockIsEmpty, item can't be empty, because of the return ealier
can be simplified to itemIsEmpty, because block can't be empty, because of previous returns
the fallthorugh sems to handle this correctly, but is hard to read, add
return result.getLeft();to the first casemove fluidHandler to a local variable in the constructor, as it's only used there
add Nullable annotation and add UseOnly(LogicalSide.CLIENT)
@ -115,0 +125,4 @@public int fillTankFromBucket(@Nullable BucketItem bucket) {return FluidUtil.fillTankFromBucket(fluidTank, bucket);}Add UseOnly(LogicalSide.CLIENT) Annotation
Add Nullable Annotation
undo change
undo change
undo change
@ -390,3 +381,4 @@neighbor.getCapability(ForgeCapabilities.FLUID_HANDLER, direction.getOpposite())));} else {itemHandlerCache.put(direction, LazyOptional.empty());fluidHandlerCache.put(direction, LazyOptional.empty());undo change
@ -412,1 +402,3 @@pTag.put("inventory", inventory.serializeNBT());if (supportsRedstoneControl()) {RedstoneControl redstoneControl = getRedstoneControl();if (redstoneControl != RedstoneControl.ALWAYS_ACTIVE) {add this to MachineInventory
so this can be replaced with !inventory.isEmpty()
undo change
@ -0,0 +1,136 @@package com.enderio.base.common.loot.providers.nbt;use Set.of
name parameter
use Set.of
This seems to be unused, if there is likely a future reason for it, keep it
put on one line, rename Serializer to JsonSeriailzer or something, so you can import it
name parameter
@ -16,57 +19,152 @@ import net.minecraft.core.Direction;import net.minecraft.world.inventory.InventoryMenu;So would you prefer that I don't throw the Exception in the
Animationconstructor? Is there a way for it to bypass the Exception with an Annotation or something given that we knowANIMATION_TICKSis greater than zero? Sorry, I'm not versed in the deeper nuances of JavaApologies, this is because I am coding with VSCode, and the
.editorconfigvalues don't seem to be being respected. I did download IntelliJ to check, and it put this on one line, which is different from what this was originally. I have just left it as one line.@ -390,3 +381,4 @@neighbor.getCapability(ForgeCapabilities.FLUID_HANDLER, direction.getOpposite())));} else {itemHandlerCache.put(direction, LazyOptional.empty());fluidHandlerCache.put(direction, LazyOptional.empty());Again, this is because I am coding with VSCode, my apologies. I did format it with IntelliJ which gave something different to both, so I have left that in.
@ -16,57 +19,152 @@ import net.minecraft.core.Direction;import net.minecraft.world.inventory.InventoryMenu;throw an illegalargumentexception, don't declare that the constructor throws something and remove the catch
@ -0,0 +1,136 @@package com.enderio.base.common.loot.providers.nbt;The
ContextNbtProviderthat I shamelessly copied this from uses this, so I'm reluctant to change it, but if you think it's okay, then I will.@ -0,0 +1,136 @@package com.enderio.base.common.loot.providers.nbt;The
ContextNbtProviderthat I shamelessly copied this from uses this, so I'm reluctant to change it, but if you think it's okay, then I will.@ -0,0 +1,136 @@package com.enderio.base.common.loot.providers.nbt;it's effectivly the same, and at that point it's personal preference. ImmutableSet could indicate, that someone wants implementationdetails for a specific reason, while the set just indicates, that you want a set
Pull request closed