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

Create a Fluid Flow API #74

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

Create a Fluid Flow API #74

wants to merge 22 commits into from

Conversation

OroArmor
Copy link
Member

This copies from https://github.com/FabricMC/fabric/pulls/1402, however, as I am the authors of said PRs, I allow my code to be added to QSL.

The testmod is a good example of how to add a new fluid flow event.

Questions to be asked:

  • Should this API be part of a larger Fluid API?
  • Should the event holding be restructured to something else?

Before this is merged, I would like to merge in a game test api to ensure vanilla behavior remains and to have a more effective test than currently having to go into a world to view the change.

@OroArmor OroArmor added new: library A pull request which adds a new library. new: module A pull request which adds a new module question Further information is requested labels Jan 30, 2022
@OroArmor OroArmor marked this pull request as ready for review February 2, 2022 01:12
@LambdAurora LambdAurora added the t: new api This adds a new API. label Feb 18, 2022
LambdAurora
LambdAurora suggested changes Feb 24, 2022
View reviewed changes
Copy link
Contributor

@LambdAurora LambdAurora left a comment

Choose a reason for hiding this comment

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

I'm not sure if it's useful to get an event depending on the direction?
Especially if you iterate through every directions anyway.

library/fluid/fluid_flow/src/main/java/org/quiltmc/qsl/fluid/flow/api/FluidFlowEvents.java Outdated Show resolved Hide resolved
library/fluid/fluid_flow/src/main/java/org/quiltmc/qsl/fluid/flow/api/FluidFlowEvents.java Outdated Show resolved Hide resolved
library/fluid/fluid_flow/src/main/java/org/quiltmc/qsl/fluid/flow/api/FluidFlowEvents.java Outdated Show resolved Hide resolved
library/fluid/fluid_flow/src/main/java/org/quiltmc/qsl/fluid/flow/api/FluidFlowEvents.java Outdated Show resolved Hide resolved
@OroArmor OroArmor requested a review from a team as a code owner June 20, 2022 19:53
@OroArmor OroArmor changed the base branch from 1.18 to 1.19 June 20, 2022 19:54
@OroArmor
Copy link
Member Author

Thats a good point, ill have the direction be a parameter

@OroArmor OroArmor requested a review from a team June 20, 2022 20:31
EnnuiL
EnnuiL suggested changes Jun 20, 2022
View reviewed changes
Copy link
Contributor

@EnnuiL EnnuiL left a comment

Choose a reason for hiding this comment

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

So far, the PR looks good; Just apply these tiny changes and all will be fine

library/fluid/fluid_flow/src/main/resources/quilt_fluid_flow.mixins.json Outdated Show resolved Hide resolved
library/fluid/fluid_flow/src/main/java/org/quiltmc/qsl/fluid/flow/mixin/FluidBlockMixin.java Outdated Show resolved Hide resolved
library/fluid/fluid_flow/src/main/java/org/quiltmc/qsl/fluid/flow/api/FluidFlowEvents.java Outdated Show resolved Hide resolved
library/fluid/fluid_flow/src/main/java/org/quiltmc/qsl/fluid/flow/api/FluidFlowEvents.java Outdated Show resolved Hide resolved
library/worldgen/src/testmod/resources/quilt.mod.json Outdated Show resolved Hide resolved
EnnuiL
EnnuiL approved these changes Jun 23, 2022
View reviewed changes
Copy link
Contributor

@EnnuiL EnnuiL left a comment

Choose a reason for hiding this comment

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

I just realized that the onFlow event's parameter names might be a bit too verbose? but yeah, I can't think of any alternative shorter names that didn't sacrifice readability; Oh well

Here's the Ennui's Stamp of Approval

@oliviathevampire
Copy link

This needs to be updated to newest code + maybe can be made into an REA?

@OroArmor
Copy link
Member Author

OroArmor commented Aug 6, 2022

This can't easily be made into a REA, but yeah I can update it.

@OroArmor
Copy link
Member Author

OroArmor commented Aug 6, 2022

And by easily I mean it shouldn't. It's an event, not a property because there are too many variables

EnnuiL
EnnuiL approved these changes Aug 6, 2022
View reviewed changes
Copy link
Contributor

@EnnuiL EnnuiL left a comment

Choose a reason for hiding this comment

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

I still have no objections, however, here's a few nitpicks; Consider the general PR approved though

library/worldgen/src/testmod/resources/quilt.mod.json Outdated Show resolved Hide resolved
library/fluid/fluid_flow/src/main/java/org/quiltmc/qsl/fluid/flow/api/FluidFlowEvents.java Outdated Show resolved Hide resolved
@OroArmor @EnnuiL
Apply suggestions from code review
9ddf88c 
Co-authored-by: Ennui Langeweile <[email protected]>
@OroArmor OroArmor added the s: tested This pull request has been tested and confirmed as working. label Sep 7, 2022
Platymemo
Platymemo reviewed Oct 19, 2022
View reviewed changes
library/fluid/fluid_flow/src/main/java/org/quiltmc/qsl/fluid/flow/api/FluidFlowEvents.java
* @param interactionBlock the block it interacts with
* @return an event if the conditions are met, otherwise {@code null}
*/
public static @Nullable Event<FluidFlowInteractionCallback> getEvent(Block flowingBlock, Block interactionBlock) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have a standardization on whether @Nullable goes here or on the whole method declaration?

library/fluid/fluid_flow/src/main/java/org/quiltmc/qsl/fluid/flow/api/FluidFlowEvents.java
return null;
}

public interface FluidFlowInteractionCallback {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public interface FluidFlowInteractionCallback {
@FunctionalInterface
public interface FluidFlowInteractionCallback {

library/fluid/fluid_flow/src/main/java/org/quiltmc/qsl/fluid/flow/api/FluidFlowEvents.java Show resolved Hide resolved
@EnnuiL EnnuiL added s: outdated This pull request is outdated. and removed s: tested This pull request has been tested and confirmed as working. labels May 13, 2023
@OroArmor OroArmor changed the base branch from 1.19 to 1.20 June 8, 2023 23:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Platymemo Platymemo Platymemo left review comments

@EnnuiL EnnuiL EnnuiL approved these changes

@oliviathevampire oliviathevampire oliviathevampire approved these changes

@LambdAurora LambdAurora Awaiting requested review from LambdAurora

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
new: library A pull request which adds a new library. new: module A pull request which adds a new module question Further information is requested s: outdated This pull request is outdated. t: new api This adds a new API.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@OroArmor @oliviathevampire @LambdAurora @Platymemo @EnnuiL