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

Jump to bottom

Changed FlowableFluid getVelocity method name to getFlowDirectionVect… #3284

Open
wants to merge 8 commits into
base: 22w42a
Choose a base branch
from
Open

Changed FlowableFluid getVelocity method name to getFlowDirectionVect… #3284

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
0f28a95
Changed FlowableFluid getVelocity method name to getFlowDirectionVect…
Levoment Sep 3, 2022
75da893
Changed FlowableFluid getVelocity method name to getFlowDirectionVect…
Levoment Sep 3, 2022
5076644
Changed FlowableFluid getVelocity method name back to normal. Changed…
Levoment Sep 3, 2022
9d33d69
Update mappings/net/minecraft/fluid/FlowableFluid.mapping
Levoment Sep 4, 2022
eb70174
Update mappings/net/minecraft/fluid/Fluid.mapping
Levoment Sep 4, 2022
280f95b
Update mappings/net/minecraft/fluid/Fluid.mapping
Levoment Sep 4, 2022
3f15943
Update mappings/net/minecraft/fluid/Fluid.mapping
Levoment Sep 4, 2022
f67ed68
Merge branch '22w42a' into 1.19.2
Levoment Mar 25, 2023
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
10 changes: 10 additions & 0 deletions mappings/net/minecraft/fluid/FlowableFluid.mapping
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,16 @@ CLASS net/minecraft/class_3609 net/minecraft/fluid/FlowableFluid
ARG 2 pos
ARG 3 state
ARG 4 fluid
METHOD method_15782 (Lnet/minecraft/class_1922;Lnet/minecraft/class_2338;Lnet/minecraft/class_3610;)Lnet/minecraft/class_243;
Levoment marked this conversation as resolved.
Show resolved Hide resolved
COMMENT Gets a 3D vector indicating the direction the given fluid state will flow towards
COMMENT
COMMENT Example: a return of (0, 0, -1) means the given {@link FluidState} will flow towards negative-Z in the game which is towards the North.
COMMENT Likewise (0, 0, 1) means the given {@link FluidState} will flow towards positive-Z in the game which is South.
COMMENT
COMMENT @param world the world that will be queried for the fluid states around the fluid state whose direction of flow is desired
COMMENT @param pos the {@link BlockPos} containing the position of the fluid state whose direction of flow is desired
COMMENT @param mainFluidState the {@link FluidState} whose direction of flow is desired to be known
COMMENT @return a {@link Vec3D} vector indicating the direction the given {@link FluidState} should flow
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 these docs should be in the parent Fluid#getFlowDirectionVector since they are not specific to this implementation.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I fixed it to be in the Fluid class instead on 5076644

METHOD method_17774 isFluidAboveEqual (Lnet/minecraft/class_3610;Lnet/minecraft/class_1922;Lnet/minecraft/class_2338;)Z
ARG 0 state
ARG 1 world
Expand Down
4 changes: 2 additions & 2 deletions mappings/net/minecraft/fluid/Fluid.mapping
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,10 @@ CLASS net/minecraft/class_3611 net/minecraft/fluid/Fluid
ARG 1 fluid
METHOD method_15781 setDefaultState (Lnet/minecraft/class_3610;)V
ARG 1 state
METHOD method_15782 getVelocity (Lnet/minecraft/class_1922;Lnet/minecraft/class_2338;Lnet/minecraft/class_3610;)Lnet/minecraft/class_243;
METHOD method_15782 getFlowDirectionVector (Lnet/minecraft/class_1922;Lnet/minecraft/class_2338;Lnet/minecraft/class_3610;)Lnet/minecraft/class_243;
Copy link
Contributor

Choose a reason for hiding this comment

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

FluidState#getVelocity should also be renamed for consistency.

Copy link
Author

Choose a reason for hiding this comment

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

Done. I have changed it there so that every method that overrides or implements will get the Javadocs. 5076644

ARG 1 world
ARG 2 pos
ARG 3 state
ARG 3 mainFluidState
Copy link
Contributor

Choose a reason for hiding this comment

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

Since there is only one fluid state from the API user perspective, state is less confusing imo. Ideally, local variables should get descriptive names but we can't rename them at the moment.

Copy link
Author

Choose a reason for hiding this comment

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

I kinda prefer mainFluidState because it tells me that from all the other states in the method, that is the main one related to the desired operation. But since in most of the code the name is just state I guess it makes sense to keep it the same. I have changed it back in 5076644

METHOD method_15783 getStateManager ()Lnet/minecraft/class_2689;
METHOD method_15784 getBlastResistance ()F
METHOD method_15785 getDefaultState ()Lnet/minecraft/class_3610;
Expand Down