Re-implement FluidTankList & OverlayedFluidHandler #1695

Tictim · 2023-04-11T05:40:42Z

What

This PR rewrites FluidTankList and OverlayedFluidHandler for a much cleaner implementation with new algorithm fully covered by unit tests.

Implementation Details

Although previous iteration of FluidTankList allowed creation of nested tank lists (FluidTankList inside FluidTankList), it didn't handle it correctly - specifically, differing allowSameFluidFill flag between parent and children. This led to creation of an abomination with a name of NotifiableFluidTankFromList, which was an extremely hacky solution to retain the parent IMultipleTankHandler and handle allowSameFluidFill setting manually during insertion calls. This questionable design decision then propagated to OverlayedFluidHandler which gave birth to per-fluid tank cache of distinct fluids.

In this PR, IMultipleTankHandler received a major redesign to accommodate behavior of nested FluidTankLists, and provide better API for usage. The insertion algorithm of FluidTankList was changed to handle nested tanks.

OverlayedFluidHandler, or rather a simulator for consecutive fill operations on IMultipleTankHandler, also received a major refactor. One notable change in API perspective is that the insertStackedFluidKey no longer receives FluidKeys. Since the algorithm can handle consecutive fill of same fluids now, it is possible to fill FluidStacks without worrying about redundant entry breaking recipe checks. As a result, usage of GTHashMaps#fromFluidCollection got completely phased out.

Since handling of nested tanks is no longer needed, implementation of MetaTileEntityMultiFluidHatch was changed to use standard NotifiableFluidTanks. I didn't test it yet. I just assumed it works flawlessly. 😃

NotifiableFluidTankFromList was marked as deprecated.

Test cases cover all insertion/extraction logic in both FluidTankList and OverlayedFluidHandler.

Additional Information

The distinct insertion logic of FluidTankList and OverlayedFluidHandler has a behavior change:

Previously, during the first iteration (inserting fluid to tanks with same type of fluid), only one tank could receive the fluid.
In the new algo, multiple tanks can receive the fluid if they have same type of fluid.

Distinct insertion on second iteration is unchanged.

Serialization result of FluidTankLists have a minor change of TankAmount field being removed. As the information was redundant in reading the contents of list tag, I just removed the field.

Potential Compatibility Issues

As all of the classes except multi fluid hatches are API, this PR is API breaking change. The actual severity of compat break depends on its usage.

Plain usage of FluidTankList, like instantiation or calling IFluidHandler methods, should remain unharmed.
```
FluidTankList fluidTanks = new FluidTankList(false, tanks);
fluidTanks.insert(fluid, true);
```

Instantiation of OverlayedFluidHandler should also remain.

OverlayedFluidHandler ofh = new OverlayedFluidHandler(fluidTanks);

Iterating over FluidTankList#getFluidTanks() still works I think. At least on code it works. Not sure about binary compat.
```
for(IFluidTank tank : fluidTanks){
  doSomething(tank);
}
```
Operations on fluid tanks returned by FluidTankList#getTankAt() should retain their functions; but any instance-specific operations, like casting the object or instance checking with refeq will need updates.
```
IFluidTank tank = fluidTanks.getTankAt(0);
if(tank instanceof FluidTank){ // compat break
```
Storing FluidTankList#getFluidTanks() in a field with type of List<IFluidTank> will produce compilation error on code, due to how generics work. I think it's fine on binary.
```
List<IFluidTank> tanks = fluidTanks.getFluidTanks(); // compat break
```
Calling OverlayedFluidHandler#insertStackedFluidKey will break after update.
```
ofh.insertStackedFluidKey(fluidKey, 1000); // compat break
```
FluidTankList#validateTankIndex() no longer exists; although I don't think anyone actually uses the method, it is technically an API break.

…tityMultiFluidHatch to use regular tank lists

src/main/java/gregtech/api/capability/IMultipleTankHandler.java

src/main/java/gregtech/api/capability/impl/FluidTankList.java

src/main/java/gregtech/api/capability/IFilter.java

src/main/java/gregtech/api/capability/IPropertyFluidFilter.java

src/main/java/gregtech/api/capability/IThermalFluidHandlerItemStack.java

src/main/java/gregtech/api/capability/impl/CommonFluidFilters.java

src/main/java/gregtech/api/capability/impl/FilteredFluidHandler.java

src/main/java/gregtech/api/capability/impl/ThermalFluidHandlerItemStack.java

src/main/java/gregtech/api/items/metaitem/FilteredFluidStats.java

src/main/java/gregtech/api/items/metaitem/ThermalFluidStats.java

src/main/java/gregtech/api/recipes/ModHandler.java

ALongStringOfNumbers · 2023-05-24T00:59:10Z

Getting this JEI integration error when launching the game, https://pastebin.com/VjaGBEf3

… for pipe code idk why

Tictim · 2023-05-24T21:55:57Z

In addition to the changes in FluidTankList and OverlayedFluidHandler, new changes in API were introduced.

A generic filter interface, IFilter, defines match function for a specific type of object, as well as numeric priority to minimize issue of order-dependent behavior changes during insertion. This is a required feature for Allow filtering output hatches #1752.
The interface is designed to encompass other types of instances as well, not just FluidStacks. Item filter is an obvious target, but the migration hasn't been done yet.
All instances of filtered fluid handler implementations in the mod are migrated to IFilter-based impl. Most of them are internal changes, but couple of breaking changes are present as well.
- ModHandler#isLava and ModHandler#getLava were removed. ModHandler#getFuelValue was also removed, despite the fact that this method has nothing whatsoever to do with any fluid logic. 🇹🇷
- ModHandler#isWater, ModHandler#isSteam, and ModHandler#getSteam were marked as deprecated.
- IThermalFluidHandlerItemStack, ThermalFluidHandlerItemStack, and SimpleThermalFluidHandlerItemStack were marked as deprecated. Code referencing these class such as item tooltip may cease to function.
- FilteredFluidStats#<init>(int, boolean, Function<FluidStack, Boolean>) were marked as deprecated.
- ThermalFluidStats were marked as deprecated.
- Other preexisting fluid container implementations such as FilteredFluidHandler, GTFluidHandlerItemStack, and GTSimpleFluidHandlerItemStack received a refactor. FilteredFluidHandler#setFillPredicate was marked as deprecated.

Tictim · 2023-05-24T22:20:53Z

JEI integration error should be resolved now.

ALongStringOfNumbers

In progress review

ALongStringOfNumbers · 2023-05-29T04:06:25Z

src/main/java/gregtech/api/capability/IPropertyFluidFilter.java

+    int getMaxFluidTemperature();
+
+    /**
+     * This is always checked, regardless of the contained fluid being a {@link MaterialFluid} or not


Could we get a bit more clarification on this and the above javadoc, This is always checked, as it is implemented by vanilla, regardless...

Technically implemented by forge, since the whole fluid thing is mod-introduced system, but I'll expand the docs a bit.

ALongStringOfNumbers · 2023-05-29T04:14:40Z

src/main/java/gregtech/api/recipes/ModHandler.java

-    @Nonnull
-    public static FluidStack getLava(int amount) {
-        return new FluidStack(FluidRegistry.LAVA, amount);
+        return stack == null || CommonFluidFilters.BOILER_FLUID.test(stack);


I know you have this deprecated, but this is a logic change, in case people were still calling this. Before if the stack was null, this returned false. Now it will return true if the stack is null

Another stupid mistake by me 😃😎

ALongStringOfNumbers · 2023-05-29T22:47:45Z

src/main/java/gregtech/api/capability/impl/CommonFluidFilters.java

+     * @param fluid      fluid
+     * @return whether the fluid in fluid stack and fluid parameter are equal
+     */
+    public static boolean matchesFluid(@Nonnull FluidStack fluidStack, @Nonnull Fluid fluid) {


Would there be a case where we want a FluidStack and FluidStack matches method? Say for example the fluid regulator or fluid voiding cover where you can specify exact amounts of fluid in the filter to be operated upon.

FluidStack to FluidStack match is handled by FluidStack#isFluidEqual. Those two CommonFluidFilters#matchesFluid are just methods that simulates FluidStack#isFluidEqual without needing to create new FluidStack.

Also FluidStack#isFluidEqual doesn't check the amount of fluid, so if you want to match the amount of fluid you would need to create custom implementation of IFilter that calls a different match function.

ALongStringOfNumbers · 2023-05-29T23:04:35Z

src/main/java/gregtech/api/capability/IMultipleTankHandler.java

+     * @param fluidStack Fluid stack to search index
+     * @return Index corresponding to tank at {@link #getFluidTanks()} with matching
+     */
+    default int getIndexOfFluid(@Nullable FluidStack fluidStack) {


This would also have been an issue with the old implementation, but if allowSameFluidFill is true, couldn't the passed in fluid be in multiple slots?

This method was a part of NotifiableFluidTankFromList jank and can be removed, but I chose to keep it in. Still, this method is never called outside of that specific class. Also the contract of the method is not particularily associated to insertion logic so I don't think it's a problem.

ALongStringOfNumbers · 2023-05-29T23:07:03Z

src/main/java/gregtech/api/capability/IMultipleTankHandler.java

+    /**
+     * @return {@code false} if insertion to this fluid handler enforces input to be
+     * filled in one slot at max. {@code true} if it bypasses the rule.
+     */
    boolean allowSameFluidFill();


Could we rename this? This is one of the method names from CE that I really don't like, and doesn't really match well with the javadoc of its implementation.

b...but compatbilty 🥺

I guess we could just make a new method and link this method to the new one, but I don't have any strong idea for a new name so feel free to suggest some.

ALongStringOfNumbers · 2023-05-30T00:47:19Z

Could you add a @since tag in the javadoc for the deprecated classes and methods, so that way it is easier to know when things were deprecated? I want to allow at least 1 major version between deprecation and removal, so knowing when things were deprecated would help a lot for that. It would be nicer if we could use since in the deprecation annotation, but unfortunately that was from java 9

Tictim · 2023-05-30T15:44:31Z

The javadoc tag @since is for the version the stuff was added. Using it to communicate deprecated version instead might just lead to confusion.

# Conflicts: # src/main/java/gregtech/api/unification/material/properties/FluidPipeProperties.java # src/main/java/gregtech/common/metatileentities/storage/MetaTileEntityDrum.java

brachy84 · 2023-05-30T16:48:30Z

The javadoc tag @since is for the version the stuff was added. Using it to communicate deprecated version instead might just lead to confusion.

Using jetbrains annotations when 😔

Tictim added 3 commits April 11, 2023 02:13

Re-implement FluidTankList & OverlayedFluidHandler, change MetaTileEn…

d739232

…tityMultiFluidHatch to use regular tank lists

Remove redundant copying

ca12d52

Tests + fix insertion algo bypassing fully filled slots

d4c1016

Tictim added the type: refactor Suggestion to refactor a section of code label Apr 11, 2023

ALongStringOfNumbers added this to the 2.7 milestone Apr 30, 2023

Tictim added 2 commits May 8, 2023 12:06

Merge branch 'master' into fluid_tank_list

28b13bd

Merge branch 'master' into fluid_tank_list

e8ffff7

TechLord22 requested a review from PrototypeTrousers May 12, 2023 00:04

TechLord22 reviewed May 12, 2023

View reviewed changes

src/main/java/gregtech/api/capability/IMultipleTankHandler.java Outdated Show resolved Hide resolved

src/main/java/gregtech/api/capability/IMultipleTankHandler.java Outdated Show resolved Hide resolved

src/main/java/gregtech/api/capability/impl/FluidTankList.java Show resolved Hide resolved

Tictim added 5 commits May 23, 2023 03:52

Merge branch 'master' into fluid_tank_list

cc664c3

filter API, fluidtanklist insertion order

2461c07

expand steam 'lava' boiler 'api' slightly

fb1e7f9

tests, some renames, additional api migration, foam sprayer cleanup

35dab21

Merge branch 'master' into fluid_tank_list

55d3045

Tictim requested a review from a team as a code owner May 23, 2023 06:09

TechLord22 reviewed May 23, 2023

View reviewed changes

update docs, replace magic number with consts, also fix some oopsie

08a3ce6

Tictim added 3 commits May 25, 2023 05:10

Merge branch 'master' into fluid_tank_list

db8d06b

Implement IFluidHandler for MultiFluidTankEntry

27727c3

bypass max temperature check if the fluid is plasma, also did cleanup…

51706a3

… for pipe code idk why

lol

ebbecf3

ALongStringOfNumbers reviewed May 30, 2023

View reviewed changes

Tictim added 2 commits May 31, 2023 00:47

Merge branch 'master' into fluid_tank_list

8af90f2

# Conflicts: # src/main/java/gregtech/api/unification/material/properties/FluidPipeProperties.java # src/main/java/gregtech/common/metatileentities/storage/MetaTileEntityDrum.java

fix another wrong null check

f8124ad

ALongStringOfNumbers approved these changes Jun 1, 2023

View reviewed changes

TechLord22 merged commit dc82278 into GregTechCEu:master Jun 1, 2023

serenibyss added a commit that referenced this pull request Jun 25, 2023

update for #1695, reviews

37b9d4c

serenibyss mentioned this pull request Jul 8, 2023

Fix Multi Input Hatches leaving FluidStacks of 0L #1913

Merged

Tictim deleted the fluid_tank_list branch September 8, 2023 14:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Re-implement FluidTankList & OverlayedFluidHandler #1695

Re-implement FluidTankList & OverlayedFluidHandler #1695

Tictim commented Apr 11, 2023

ALongStringOfNumbers commented May 24, 2023

Tictim commented May 24, 2023

Tictim commented May 24, 2023

ALongStringOfNumbers left a comment

ALongStringOfNumbers May 29, 2023

Tictim May 30, 2023

ALongStringOfNumbers May 29, 2023

Tictim May 30, 2023

ALongStringOfNumbers May 29, 2023

Tictim May 30, 2023

ALongStringOfNumbers May 29, 2023

Tictim May 30, 2023

ALongStringOfNumbers May 29, 2023

Tictim May 30, 2023

ALongStringOfNumbers commented May 30, 2023

Tictim commented May 30, 2023

brachy84 commented May 30, 2023 •

edited

Loading

Re-implement FluidTankList & OverlayedFluidHandler #1695

Re-implement FluidTankList & OverlayedFluidHandler #1695

Conversation

Tictim commented Apr 11, 2023

What

Implementation Details

Additional Information

Potential Compatibility Issues

ALongStringOfNumbers commented May 24, 2023

Tictim commented May 24, 2023

Tictim commented May 24, 2023

ALongStringOfNumbers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ALongStringOfNumbers commented May 30, 2023

Tictim commented May 30, 2023

brachy84 commented May 30, 2023 • edited Loading

brachy84 commented May 30, 2023 •

edited

Loading