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.

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

Changed FlowableFluid getVelocity method name to getFlowDirectionVect… #3284

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

Conversation

Levoment
Copy link

@Levoment Levoment commented Sep 3, 2022

Changed FlowableFluid getVelocity method name to getFlowDirectionVector. Changed the argument in the same method with name state to mainFluidState. Also added a documentation for the method. This method tells the game the direction a fluid should flow by evaluating the level of other fluid blocks around it. It returns a 3D vector with numbers -1 OR 0 OR 1 for each component of the 3D vector to indicate what direction the fluid should flow towards.

I thought about using just getFlowDirection, but given that Direction is already a class that indicates something else, I went with getFlowDirectionVector to indicate in the name it returns a vector and not a Direction object. It is still a vector that is used to know the direction of flow from a fluid, but it's not a Direction object.

state I changed to mainFluidState to make it more clear in the code from the method that the variable holds the FluidState whose direction is desired since the method creates 2 other FluidState objects inside of it to make the calculations of flow direction.

…or. Changed the argument in the same method with name 'state' to 'mainFluidState'. Also added a documentation for the method.
@enbrain enbrain added refactor A PR that renames existing names. release A PR that targets a release version of Minecraft javadoc A PR that adds or refactors javadoc. labels Sep 3, 2022
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

Comment on lines 101 to 110
METHOD method_15782 (Lnet/minecraft/class_1922;Lnet/minecraft/class_2338;Lnet/minecraft/class_3610;)Lnet/minecraft/class_243;
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

@@ -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

…or. Changed the argument in the same method with name 'state' to 'mainFluidState'. Also added a documentation for the method.
… the FluidState getVelocity to getFlowDirectionVector which will cascade to overriding classes. Changed the argument in the same method with name 'mainFluidState' back to 'state'. Also moved documentation for the method to the Fluid class. This allows the documentation to cascade to overriding methods.
mappings/net/minecraft/fluid/Fluid.mapping Outdated Show resolved Hide resolved
mappings/net/minecraft/fluid/Fluid.mapping Outdated Show resolved Hide resolved
mappings/net/minecraft/fluid/Fluid.mapping Outdated Show resolved Hide resolved
Levoment and others added 4 commits September 3, 2022 21:45
Co-authored-by: enbrain <69905075+enbrain@users.noreply.github.com>
Co-authored-by: enbrain <69905075+enbrain@users.noreply.github.com>
Co-authored-by: enbrain <69905075+enbrain@users.noreply.github.com>
Co-authored-by: enbrain <69905075+enbrain@users.noreply.github.com>
@Levoment
Copy link
Author

Levoment commented Sep 4, 2022

I just committed the suggestions.

@enbrain enbrain requested a review from a team September 4, 2022 04:52
@Juuxel
Copy link
Member

Juuxel commented Sep 14, 2022

It returns a 3D vector with numbers -1 OR 0 OR 1 for each component of the 3D vector to indicate what direction the fluid should flow towards.

This might be good to include in the javadoc

@apple502j apple502j added the update-base Add this label to a pull request to automatically change the target branch to the default branch. label Oct 20, 2022
@github-actions github-actions bot changed the base branch from 1.19.2 to 22w42a October 20, 2022 17:05
@github-actions
Copy link
Contributor

🚀 Target branch has been updated to 22w42a

@apple502j apple502j removed the update-base Add this label to a pull request to automatically change the target branch to the default branch. label Oct 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
javadoc A PR that adds or refactors javadoc. refactor A PR that renames existing names. release A PR that targets a release version of Minecraft
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants