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

Extend BukkitScheduler with new/simplified methods #11825

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

maxcom1
Copy link
Contributor

@maxcom1 maxcom1 commented Dec 25, 2024

For quite some time, I’ve noticed various plugins implementing strange calculations for converting real time into ticks, especially when scheduling tasks at real-world dates and times. These implementations often involve a lot of complexity around time zones.

In this PR, I’ve added several methods aimed at simplifying scheduler usage. I utilized Java’s modern time API, including Duration, LocalDateTime and LocalTime

Added API:

  • Standard: scheduling of tasks to run after a specific Duration
  • Standard: scheduling of repeating tasks at a specified Duration interval
  • New: scheduling single-run (non-repeated) tasks for a specific date and time, using LocalDateTime. Time zone handling and potential clock adjustments are managed automatically without user intervention
  • New: scheduling of repeated tasks, daily at the same time (hour, minute, second), using LocalTime. As with the above, time zone handling does not require user intervention. This required the creation of a new class extending CraftTask. In the current version of this PR, I implemented this only for synchronous tasks, as doing it for asynchronous ones would be more complex. I’d like to hear your opinion on this approach first. The existing class has been coded in such a way to share most of the code if the async version was created

This was quite challenging to implement, cause the CraftScheduler codebase is very inconsistent. I tried to model my methods on the existing ones in style and structure. I did not touch methods that were annotated as deprecated. I believe the scheduler’s overall design is something that should be addressed, but I refrained from making changes myself as I’m unsure if that’s something the team desires.

I’ve provided proper Javadocs for all the new methods, based on the existing ones and adding some additional notes for users to consider.

I’m not entirely sure how the team prefers these methods to be implemented or which parts of the API should be designed differently. However, I think that these methods will help simplify plugin code and improve its readability. If implemented and documented correctly, these methods shouldn’t cause any issues.

I will be grateful for your feedback and happy to adapt to it

@maxcom1 maxcom1 requested a review from a team as a code owner December 25, 2024 22:32
@maxcom1 maxcom1 changed the title Extend BukkitScheduler with simplified methods Extend BukkitScheduler with new/simplified methods Dec 25, 2024
@electronicboy
Copy link
Member

My concern with exposing stuff like Duration is that it sounds like the API is making a level of promise that it can actually fulfil a wall clock/"human" based delay when it blatantly cannot

@vadcx
Copy link

vadcx commented Dec 26, 2024

How does this interact with this command https://minecraft.wiki/w/Commands/tick? How does it deal with the low TPS drift?


Something along the lines of https://www.creativedeletion.com/2015/01/28/falsehoods-programmers-date-time-zones.html - the long standing problem is, as electronicboy said, the wall time scheduling that has plagued the game forever. If I want to schedule a restart at exactly 18:00 the next day - what do I do? It's obvious the server cannot be at exactly 20.00 TPS (or any 1.00x TPS speed) and will always undershoot the desired frequency. In a favourable scenario, the scheduled restart task will occur within 1 minute, but be greatly off with fluctuations in actual TPS.

Another example, suppose you have birthday today. Your next birthday isn't 365 days from now, it is exactly one calendar year from now. Otherwise you'd be surprised if your next birthday was offset by a day thanks to a February 29th.

Let's assume the function interfaces are fine: does Paper want to publish an inaccurate implementation? Won't the users assume and adopt to the "seconds are roughly ticks" implementation and be wrong if the impl changes to more accurately schedule wall time events?

If a plugin developer writes this seconds->ticks calculation algorithm, the pitfalls should be clear to them. If this is part of a core API, this understanding is not a given. Why not provide these separately as a util class to make this separation obvious?

As an end end-user (not just client code) I would love to finally have date-time aligned events/scheduling. However I think the scheduler would need to be accurate and resilient enough, like a cron daemon implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Awaiting review
Development

Successfully merging this pull request may close these issues.

3 participants