Enhance the scheduling system API for future additions #5960

TheLimeGlass · 2023-08-31T22:47:59Z

Description

Updates the entire Task scheduling system in Skript. This API change is required to support versioning reflection of future additions like Folia https://github.com/PaperMC/Folia

I have a branch which enables Folia support for Skript and completes the Task scheduling system to adapt to Folia properly https://github.com/SkriptLang/Skript/tree/feature/folia Do note this is in experimental phase and still throws exceptions due to the following;

The main changes that are needed to that branch is replacing Bukkit.getScheduler with Skript's new Task luckily old parts of Skript accomplish this, but ever since bensku additions, the Skript project has had the habit of using Bukkit.getScheduler directly, rather than Skript's own Task. So many parts of Skript are going to need changes in that branch.

Once this is merged, i'll directly work on the Folia branch for the FoliaScheduler class.

Target Minecraft Versions: none
Requirements: none
Related Issues: #5645

TheLimeGlass · 2023-08-31T22:53:38Z

Placing under draft until I get opinions from the Core SkriptLang team and I also have all the JavaDocs to do.

bluelhf · 2023-09-01T07:18:30Z

Not core team, but Folia support would definitely be nice and fits the goals of Skript :)

Would it be possible to convert all usages of BukkitScheduler to Skript's Task on the main branch and then rebase the folia branch? We probably should've stuck to Skript's Task anyway...

Moderocky

This is an admirable attempt and definitely something that needs doing, however it has brought to my attention how much of the existing task timing system is [dangerous/bizarre/inefficient] and I think this would be an excellent opportunity to clean up some of the main offenders.

We could fix the problems in a separate PR (in which case I'll dismiss my change request and approve this) but we should first discuss how we want to approach this.

Moderocky · 2023-09-01T08:57:01Z

src/main/java/org/skriptlang/skript/scheduler/TaskManager.java

+/**
+ * Class to handle the abstract versioning and to be loaded from the Skript main class.
+ */
+public class TaskManager {


This is a lot of static abuse for something that probably ought not be.

It's package only access. The Task class should not store which PlatformScheduler to use.

I'm going to agree with Kenzie here. I don't think there's a good reason for why we can't support multiple instances for this class.

I think it should be fine to accept any plugin instance. Additionally, if the static is needed for the Task class(es), have this class be used to create/submit new tasks. Though, the methods of this class seem to just wrap the Scheduler. Why not just have users submit tasks to the scheduler directly? e.g. Skript.getScheduler()

src/main/java/org/skriptlang/skript/scheduler/platforms/SpigotScheduler.java

sovdeeth · 2024-07-16T06:43:29Z

Closing due to inactivity.

TheLimeGlass · 2024-07-16T21:53:30Z

This is a very important future change requiring attention.

sovdeeth · 2024-07-16T21:58:47Z

This is a very important future change requiring attention.

There are requested changes still waiting from September 2023. These haven't been addressed since, hence closed for inactivity. Do you intend to address these requests?

KIttenPixel · 2024-08-13T02:09:37Z

I would like this.

TheLimeGlass · 2024-08-31T16:45:24Z

I didn't realize how well I engineered this API.
I had to re-understand all of what was being done.

bluelhf

looks like a good step toward folia support :)

src/main/java/org/skriptlang/skript/scheduler/AsyncTask.java

Co-authored-by: Ilari Suhonen <ilari.suhonen@gmail.com>

Moderocky

Good start.

sovdeeth

looks really good

src/main/java/org/skriptlang/skript/scheduler/AsyncTask.java

sovdeeth · 2024-10-12T23:29:10Z

src/main/java/org/skriptlang/skript/scheduler/AsyncTask.java

+	}
+
+	/**
+	 * Re-schedules the task to run next after the given delay. If this task was repeating it will continue so using the same period as before.


Does this line mean that this method has no effect on repeating tasks? I think it should be rewritten to be a little clearer

src/main/java/org/skriptlang/skript/scheduler/AsyncTask.java

src/main/java/org/skriptlang/skript/scheduler/PlatformScheduler.java

src/main/java/org/skriptlang/skript/scheduler/Task.java

sovdeeth · 2024-10-12T23:35:48Z

src/main/java/org/skriptlang/skript/scheduler/TaskManager.java

+import ch.njol.skript.Skript;
+
+/**
+ * Class to handle the abstract versioning and to be loaded from the Skript main class.


The comment doesn't make much sense. I would suggest instead maybe Handles the scheduling of tasks in a platform-agnostic manner. Loaded by the Skript main class.

src/main/java/org/skriptlang/skript/scheduler/package-info.java

src/main/java/org/skriptlang/skript/scheduler/platforms/FoliaScheduler.java

src/main/java/org/skriptlang/skript/scheduler/platforms/SpigotScheduler.java

sovdeeth · 2024-10-12T23:39:38Z

src/main/java/org/skriptlang/skript/scheduler/platforms/SpigotScheduler.java

+			long delayTicks = (task.getTimeUnit().toMillis(delay) / 1000) * 20;
+			long periodTicks = (task.getTimeUnit().toMillis(task.getPeriod()) / 1000) * 20;


again, why not / 50? Is it magic number concern?

Co-authored-by: sovdee <10354869+sovdeeth@users.noreply.github.com>

APickledWalrus

I would like to see some changes to the design so that we can move away from all of the static usage.

APickledWalrus · 2024-11-22T15:54:06Z

src/main/java/org/skriptlang/skript/scheduler/package-info.java

+/**
+ *   This file is part of Skript.
+ *
+ *  Skript is free software: you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation, either version 3 of the License, or
+ *  (at your option) any later version.
+ *
+ *  Skript is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with Skript.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Copyright Peter Güttinger, SkriptLang team and contributors
+ */


Suggested change

/**

* This file is part of Skript.

*

* Skript is free software: you can redistribute it and/or modify

* it under the terms of the GNU General Public License as published by

* the Free Software Foundation, either version 3 of the License, or

* (at your option) any later version.

*

* Skript is distributed in the hope that it will be useful,

* but WITHOUT ANY WARRANTY; without even the implied warranty of

* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the

* GNU General Public License for more details.

*

* You should have received a copy of the GNU General Public License

* along with Skript. If not, see <http://www.gnu.org/licenses/>.

*

* Copyright Peter Güttinger, SkriptLang team and contributors

*/

Fine to remove now

APickledWalrus · 2024-11-22T15:56:34Z

src/main/java/org/skriptlang/skript/scheduler/Task.java

+	private long periodInTicks = -1;
+
+	public Task(long delayInTicks, long periodInTicks) {
+		this(Skript.getInstance(), delayInTicks, periodInTicks);


I would document for these methods that the Skript instance is being used

APickledWalrus · 2024-11-22T16:01:32Z

src/main/java/org/skriptlang/skript/scheduler/TaskManager.java

+/**
+ * Class to handle the abstract versioning and to be loaded from the Skript main class.
+ */
+public class TaskManager {


I'm going to agree with Kenzie here. I don't think there's a good reason for why we can't support multiple instances for this class.

I think it should be fine to accept any plugin instance. Additionally, if the static is needed for the Task class(es), have this class be used to create/submit new tasks. Though, the methods of this class seem to just wrap the Scheduler. Why not just have users submit tasks to the scheduler directly? e.g. Skript.getScheduler()

Enhance the scheduling system API for future additions

e5d4bad

TheLimeGlass added enhancement Feature request, an issue about something that could be improved, or a PR improving something. 2.8 Targeting a 2.8.X version release labels Aug 31, 2023

License

94729f4

TheLimeGlass requested a review from APickledWalrus August 31, 2023 22:51

TheLimeGlass marked this pull request as draft August 31, 2023 22:52

Add cancelAll

72e3ea1

Moderocky requested changes Sep 1, 2023

View reviewed changes

Moderocky force-pushed the master branch 3 times, most recently from bd134d0 to 3f08853 Compare September 16, 2023 16:59

TheLimeGlass changed the base branch from master to dev/feature November 23, 2023 20:30

sovdeeth removed the 2.8 Targeting a 2.8.X version release label Dec 30, 2023

sovdeeth closed this Jul 16, 2024

TheLimeGlass reopened this Jul 16, 2024

Conflicts

dbd7d56

Import conflicts

743c87e

Apply changes

bf891c5

TheLimeGlass marked this pull request as ready for review August 31, 2024 16:43

TheLimeGlass requested a review from Moderocky August 31, 2024 16:43

Apply changes

3905e18

TheLimeGlass and others added 2 commits August 31, 2024 10:49

Apply changes

14944f9

Merge branch 'dev/feature' into enhancement/schedulers

f48dc9f

bluelhf reviewed Sep 1, 2024

View reviewed changes

src/main/java/org/skriptlang/skript/scheduler/AsyncTask.java Outdated Show resolved Hide resolved

Update src/main/java/org/skriptlang/skript/scheduler/AsyncTask.java

7b0095e

Co-authored-by: Ilari Suhonen <ilari.suhonen@gmail.com>

TheLimeGlass requested a review from bluelhf September 1, 2024 20:20

Merge branch 'dev/feature' into enhancement/schedulers

d9ce8c0

Moderocky approved these changes Sep 6, 2024

View reviewed changes

sovdeeth requested changes Oct 12, 2024

View reviewed changes

sovdeeth and others added 2 commits October 12, 2024 19:40

Merge branch 'dev/feature' into enhancement/schedulers

dde972f

Apply suggestions from code review

594bec9

Co-authored-by: sovdee <10354869+sovdeeth@users.noreply.github.com>

APickledWalrus requested changes Nov 22, 2024

View reviewed changes

Efnilite added 2 commits December 11, 2024 12:16

Merge branch 'dev/feature' into enhancement/schedulers

08d831a

Merge branch 'dev/feature' into enhancement/schedulers

b481d97

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhance the scheduling system API for future additions #5960

Enhance the scheduling system API for future additions #5960

TheLimeGlass commented Aug 31, 2023 •

edited

Loading

TheLimeGlass commented Aug 31, 2023 •

edited

Loading

bluelhf commented Sep 1, 2023

Moderocky left a comment

Moderocky Sep 1, 2023

TheLimeGlass Sep 1, 2023

APickledWalrus Nov 22, 2024

sovdeeth commented Jul 16, 2024

TheLimeGlass commented Jul 16, 2024

sovdeeth commented Jul 16, 2024

KIttenPixel commented Aug 13, 2024

TheLimeGlass commented Aug 31, 2024

bluelhf left a comment

Moderocky left a comment

sovdeeth left a comment

sovdeeth Oct 12, 2024

sovdeeth Oct 12, 2024

sovdeeth Oct 12, 2024

APickledWalrus left a comment

APickledWalrus Nov 22, 2024

APickledWalrus Nov 22, 2024

APickledWalrus Nov 22, 2024

		long delayTicks = (task.getTimeUnit().toMillis(delay) / 1000) * 20;
		long periodTicks = (task.getTimeUnit().toMillis(task.getPeriod()) / 1000) * 20;

Enhance the scheduling system API for future additions #5960

Are you sure you want to change the base?

Enhance the scheduling system API for future additions #5960

Conversation

TheLimeGlass commented Aug 31, 2023 • edited Loading

Description

TheLimeGlass commented Aug 31, 2023 • edited Loading

bluelhf commented Sep 1, 2023

Moderocky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sovdeeth commented Jul 16, 2024

TheLimeGlass commented Jul 16, 2024

sovdeeth commented Jul 16, 2024

KIttenPixel commented Aug 13, 2024

TheLimeGlass commented Aug 31, 2024

bluelhf left a comment

Choose a reason for hiding this comment

Moderocky left a comment

Choose a reason for hiding this comment

sovdeeth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

APickledWalrus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheLimeGlass commented Aug 31, 2023 •

edited

Loading

TheLimeGlass commented Aug 31, 2023 •

edited

Loading