Skip to content

Add Scheduler#143

Closed
Nacioszeczek wants to merge 19 commits into
PaperMC:mainfrom
Nacioszeczek:feature/scheduler
Closed

Add Scheduler#143
Nacioszeczek wants to merge 19 commits into
PaperMC:mainfrom
Nacioszeczek:feature/scheduler

Conversation

@Nacioszeczek
Copy link
Copy Markdown
Contributor

Resolves #102

Copy link
Copy Markdown
Member

@olijeffers0n olijeffers0n 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 assuming this is still a WIP due to it ending abruptly and it being a draft PR, but here are a couple comments.

Comment thread docs/paper/dev/api/scheduler.md Outdated
Comment thread docs/paper/dev/api/scheduler.md Outdated
Comment thread docs/paper/dev/api/scheduler.md Outdated
Comment thread docs/paper/dev/api/scheduler.md
Comment thread docs/paper/dev/api/scheduler.md
Comment thread docs/paper/dev/api/scheduler.md Outdated
@Machine-Maker
Copy link
Copy Markdown
Member

A large part of the scheduler is being able to do things off the main thread. But this is barely explained in the PR. Is that intentional? Cause I feel like talking about "async" here makes a lot of sense.

@Nacioszeczek
Copy link
Copy Markdown
Contributor Author

@Machine-Maker do you mean sth like explain the definition of sync and async in the scheduler's case? atm the plan would be to add quite a few different sections with different examples for delayed, repeating, both sync and async, stuff like cancelling a repeating task some time later

and the PR is WIP like Ollie said

@Machine-Maker
Copy link
Copy Markdown
Member

Machine-Maker commented Mar 17, 2023

Yeah, I think the concept of "async" isn't very well understood, along with its limitations when interacting with "world state". But sounds like you have plans, I just think in addition to example sections, there should be a blurb on what "async" means in this context. You have async to the main thread, but also async in a general context, means smth like on a different thread than where this code is currently running.

@olijeffers0n
Copy link
Copy Markdown
Member

Yep, as MM says explaining the concept behind the event loop and the pros/cons async is vital.

I think the page name could be better in the sidebar. Even if it is just “The Scheduler” it would be better as just “Scheduler” looks weird IMO.

@Nacioszeczek
Copy link
Copy Markdown
Contributor Author

Nacioszeczek commented Mar 19, 2023

@Machine-Maker I assume the BukkitScheduler executes the async tasks at the start/end of a tick (on separate threads) therefore is still somewhat affected if mspt exceeds 50ms, and your own ScheduledExService wouldn't care at all?
I just think ScheduledExecutorService is worth mentioning somewhere in this doc as an alternative

@Nacioszeczek
Copy link
Copy Markdown
Contributor Author

@olijeffers0n wanna elaborate? Not sure I understood what you mean

the concept behind the event loop

Comment thread docs/paper/dev/api/scheduler.md Outdated
Comment thread docs/paper/dev/api/scheduler.md Outdated
@Nacioszeczek
Copy link
Copy Markdown
Contributor Author

Waiting for any suggestions, some criticism, etc. since I'm somewhat out of ideas for this;

also making ready to review since IMHO asynchronous tasks examples are not really needed since that would be just repeating the synchronous examples with different method names

@Nacioszeczek Nacioszeczek marked this pull request as ready for review May 13, 2023 10:57
@olijeffers0n
Copy link
Copy Markdown
Member

Looks good, is it worth mentioning bukkit runnables?

@Nacioszeczek
Copy link
Copy Markdown
Contributor Author

Nacioszeczek commented May 13, 2023

All scheduler methods that take BukkitRunnable are deprecated and the methods on BukkitRunnable itself I'm not a fan of because calling static stuff

Maybe could add it to the part about actually implementing Runnable in a separate class? So that people don't make anonymous classes which I'm also not a fan of

Comment thread docs/paper/dev/api/scheduler.md Outdated
Comment thread docs/paper/dev/api/scheduler.md Outdated
server.broadcast(Component.text("Hello, World!"));
}, 10 * 20, 5 * 20);

scheduler.runTaskLater(plugin, () -> task.cancel(), TimeUnit.MINUTES.toSeconds(10) * 20);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is kind of dank, it should refer back to using the consumer or extending bukkitrunnable and deciding in the task when to cancel itself

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not sure that's what you meant but changed

Comment thread docs/paper/dev/api/scheduler.md Outdated
Comment thread docs/paper/dev/api/scheduler.md Outdated
Comment thread docs/paper/dev/api/scheduler.md Outdated
@Nacioszeczek
Copy link
Copy Markdown
Contributor Author

Gonna get to it soon

Copy link
Copy Markdown
Member

@olijeffers0n olijeffers0n left a comment

Choose a reason for hiding this comment

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

Looks like its pretty much there, just a few suggestions really.

slug: /dev/scheduler
---

# Task Scheduler
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why is the page called task scheduler if you immediately refer to it with the BukkitScheduler name. Maybe just name the page "The Bukkit Scheduler" or something along those lines?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'd prefer something like Scheduling tasks over including Bukkit in the name


# Task Scheduler

The `BukkitScheduler` can be used to schedule your code to be run later or run it repeatedly.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
The `BukkitScheduler` can be used to schedule your code to be run later or run it repeatedly.
The `BukkitScheduler` can be used to schedule your code to be run later or repeatedly.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Will change

Comment on lines +46 to +49
- your plugin's instance,
- the code to run, either with a `Runnable` or `Consumer<BukkitTask>`,
- the delay in ticks before the task should run,
- if you're scheduling a repeating task - the period in ticks between each execution of the task.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
- your plugin's instance,
- the code to run, either with a `Runnable` or `Consumer<BukkitTask>`,
- the delay in ticks before the task should run,
- if you're scheduling a repeating task - the period in ticks between each execution of the task.
- Your plugin's instance,
- The code to run, either with a `Runnable` or `Consumer<BukkitTask>`,
- The delay in ticks before the task should run for the first time,
- If you're scheduling a repeating task - the period in ticks between each execution of the task.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Think of it as a long sentence, hence the commas and the period at the end,
will add for the first time

Comment on lines +117 to +123
plugin,
// Lambda:
() -> {
this.plugin.getServer().broadcast(Component.text("Hello, World!"));
},
// End of the lambda
20);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
plugin,
// Lambda:
() -> {
this.plugin.getServer().broadcast(Component.text("Hello, World!"));
},
// End of the lambda
20);
plugin, /* Lambda: */ () -> {
this.plugin.getServer().broadcast(Component.text("Hello, World!"));
}, /* End of the lambda */ 20);

Keeps it styled the same as the next example and looks cleaner

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Will change


```java
scheduler.runTaskTimer(plugin, /* Lambda: */ task -> {
if(this.entity.isDead()) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Missing a space between if and (

@olijeffers0n olijeffers0n mentioned this pull request Jul 24, 2023
@Nacioszeczek
Copy link
Copy Markdown
Contributor Author

succeeded by #204

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.

How to use the scheduler, what is a "tick"

5 participants