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

Expand all runAtLocation methods to support chunk coordinate arguments #20

Closed
wants to merge 1 commit into from

Conversation

xGinko
Copy link

@xGinko xGinko commented Jun 23, 2024

Closes #11

Adds support for passing in world and chunk coordinates for all runAtLocation methods.

@CJCrafter
Copy link
Contributor

Hey xGinko, typically this developer likes to merge PRs into dev, not main. TechnicallyCoded handles all merges from dev into main then... Can you change the branch to dev?

Copy link
Contributor

@CJCrafter CJCrafter left a comment

Choose a reason for hiding this comment

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

These changes will be needed to match up with the dev branch.

* @param consumer Task to run
* @param delay Delay before execution in ticks
*/
void runAtLocationLater(World world, int chunkX, int chunkZ, @NotNull Consumer<WrappedTask> consumer, long delay);
Copy link
Contributor

Choose a reason for hiding this comment

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

This should return a CompleteableFuture<Void>, matching the other methods from #16

Copy link
Owner

Choose a reason for hiding this comment

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

Yep

* @param delay Delay before execution
* @param unit Time unit
*/
void runAtLocationLater(World world, int chunkX, int chunkZ, @NotNull Consumer<WrappedTask> consumer, long delay, TimeUnit unit);
Copy link
Contributor

Choose a reason for hiding this comment

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

This should return a CompleteableFuture<Void>, matching the other methods from #16

@xGinko
Copy link
Author

xGinko commented Jun 25, 2024

yo @CJCrafter thanks for pointing me in the right direction

@TechnicallyCoded TechnicallyCoded changed the base branch from main to dev July 3, 2024 09:47
* @param consumer Task to run
* @return Future when the task is completed
*/
CompletableFuture<Void> runAtLocation(World world, int chunkX, int chunkZ, @NotNull Consumer<WrappedTask> consumer);
Copy link
Owner

Choose a reason for hiding this comment

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

Could we possibly do runAtChunk please? It makes it a little more obvious what int & int parameters will be. (Same for all other runAtChunk changes)

* @param consumer Task to run
* @param delay Delay before execution in ticks
*/
void runAtLocationLater(World world, int chunkX, int chunkZ, @NotNull Consumer<WrappedTask> consumer, long delay);
Copy link
Owner

Choose a reason for hiding this comment

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

Yep

@TechnicallyCoded
Copy link
Owner

Hey @xGinko , thank you for contributing!

As you noticed, we requested a few changes, so there 2 main things that need changing before accepting this:

  • If you can address these pointers we gave you directly in the code review
  • Please move all the runAtChunk methods together instead of intertwined with the runAtLocation methods, the same way all the runAsync and runAtEntity are grouped together

@TechnicallyCoded
Copy link
Owner

@xGinko Should we keep this open?

@xGinko
Copy link
Author

xGinko commented Jul 23, 2024

@TechnicallyCoded I'll make a new one

@xGinko xGinko closed this Jul 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Method for runAtLocation() missing support for passing in chunk.
3 participants