Fluid API v1 #1136
Fluid API v1 #1136
Conversation
|
|
||
| /** | ||
| * If a fluid is still, its state index is zero. | ||
| * If a fluid is flowing, its state index is non-zero and represents its level and height. |
There was a problem hiding this comment.
Could you elaborate further on what part of the state index is level and which part is height?
There was a problem hiding this comment.
In the newer commits I removed the "and height" part from the javadoc, since that is confusing. The height of the fluid is level/(maxLevel+1), as seen in FlowableFluid#getHeight.
...ensions-v1/src/main/java/net/fabricmc/fabric/api/fluid/extension/v1/FabricFlowableFluid.java
Outdated
Show resolved
Hide resolved
...ensions-v1/src/main/java/net/fabricmc/fabric/api/fluid/extension/v1/FabricFlowableFluid.java
Outdated
Show resolved
Hide resolved
...ensions-v1/src/main/java/net/fabricmc/fabric/api/fluid/extension/v1/FabricFlowableFluid.java
Outdated
Show resolved
Hide resolved
| public FabricFlowableFluidBlock(Settings settings) { | ||
| super(settings); | ||
| this.fluid = getFluid(); | ||
| stateIndexProperty = this.fluid.getStateIndexProperty(); |
There was a problem hiding this comment.
The getStateIndexProperty should be wrapped with an Objects.requireNonNull
There was a problem hiding this comment.
This value cannot be null unless mixins/asm are used. Should I still wrap it?
...xtensions-v1/src/main/java/net/fabricmc/fabric/mixin/fluid/extension/FlowableFluidMixin.java
Outdated
Show resolved
Hide resolved
...xtensions-v1/src/main/java/net/fabricmc/fabric/mixin/fluid/extension/FlowableFluidMixin.java
Outdated
Show resolved
Hide resolved
...xtensions-v1/src/main/java/net/fabricmc/fabric/mixin/fluid/extension/FlowableFluidMixin.java
Outdated
Show resolved
Hide resolved
...xtensions-v1/src/main/java/net/fabricmc/fabric/mixin/fluid/extension/FlowableFluidMixin.java
Outdated
Show resolved
Hide resolved
...xtensions-v1/src/main/java/net/fabricmc/fabric/mixin/fluid/extension/FlowableFluidMixin.java
Outdated
Show resolved
Hide resolved
|
|
Imo, this module should just be called A fluid volume api would be |
Add Licenses Fix some formatting
Rename to Fluid Extension API Fix license and style Add suggestions from review
e317e1d to
4f06f67
Compare
Renamed the entire module and any instance of "fabric-fluid-extension-api" to "fabric-fluid-api" Removed a comma in the fabric.mod.json of the test mod
|
I have renamed the module as per Prospector's suggestion. |
fabric-fluid-api-v1/src/main/java/net/fabricmc/fabric/api/fluid/v1/FabricFlowableFluid.java
Outdated
Show resolved
Hide resolved
|
I am unsure about this PR because my original goal with it was to essentially replace vanilla's fluid system (which IMO isn't very good), and this PR accomplishes this quite well. I am now realizing though, that forcing users to use a single fluid and the custom fluid block class takes away control, and that this PR might not wholly fit into Fabric API. In the Discord API channel, it was discussed that a unified fluid class does not fit into Fabric API and that the FluidBlock class should be mixed into instead of replaced, but I am really not sure how to improve the system only partially. |
This module adds a new FabricFlowableFluid class and FabricFluidBlock class. The new fluid class lets the user register one fluid instead of both a still fluid and a flowing fluid the vanilla way. It also lets the user set the maximum level of the fluid via an abstract method. Both classes provide some optimizations over vanilla's counterpart classes and contain some new methods that can be overriden for more control.