Skip to content

Conversation

@OPmasterLEO
Copy link

Added new custom schedulers to add support for folia, canvas and archlight

Updated maven compiler and nms version
For 1.21.1 I had to replace spigot with paper because i was getting compile errors

Copilot AI review requested due to automatic review settings January 7, 2026 15:42
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adds comprehensive Folia support to SignGUI, enabling the library to work correctly on Folia's regionized multithreaded server architecture, as well as other platforms like CanvasMC and Archlight. The implementation uses a scheduler abstraction layer with platform detection to automatically select the appropriate scheduler implementation.

Key changes:

  • Introduces a new scheduler adapter architecture with platform-aware implementations (Folia, Bukkit)
  • Adds platform detection utilities to identify Folia, CanvasMC, Archlight, and standard Bukkit servers
  • Updates Maven compiler and NMS plugin versions across all modules
  • Switches 1.21.1 module from Spigot to Paper API to resolve compilation errors

Reviewed changes

Copilot reviewed 32 out of 32 changed files in this pull request and generated 11 comments.

Show a summary per file
File Description
pom.xml Adds PaperMC repository and updates maven-compiler-plugin to 3.14.1
api/pom.xml Adds Folia API dependency for compile-time reflection access
api/src/main/java/de/rapha149/signgui/util/scheduler/SchedulerFactory.java Factory class for creating appropriate scheduler adapters based on platform
api/src/main/java/de/rapha149/signgui/util/scheduler/FoliaSchedulerAdapter.java Folia-compatible scheduler using reflection to access region-based scheduling
api/src/main/java/de/rapha149/signgui/util/scheduler/BukkitSchedulerAdapter.java Standard Bukkit/Spigot/Paper scheduler adapter
api/src/main/java/de/rapha149/signgui/util/SchedulerAdapter.java Interface defining cross-platform scheduler operations
api/src/main/java/de/rapha149/signgui/util/PlatformType.java Enum representing different server platform types
api/src/main/java/de/rapha149/signgui/util/PlatformDetector.java Utility for detecting the current server platform
api/src/main/java/de/rapha149/signgui/SignGUIBuilder.java Updates documentation for callHandlerSynchronously method
api/src/main/java/de/rapha149/signgui/SignGUI.java Integrates SchedulerFactory to use platform-aware scheduling
README.md Adds platform support documentation and usage recommendations
Mojang1_21_R5/pom.xml Updates maven-compiler-plugin to 3.14.1 and paper-nms-maven-plugin to 1.4.10
Mojang1_21_R4/pom.xml Updates maven-compiler-plugin to 3.14.1 and paper-nms-maven-plugin to 1.4.10
Mojang1_21_R3/pom.xml Updates maven-compiler-plugin to 3.14.1 and paper-nms-maven-plugin to 1.4.10
Mojang1_21_R2/pom.xml Updates maven-compiler-plugin to 3.14.1 and paper-nms-maven-plugin to 1.4.10
Mojang1_21_R1/pom.xml Updates maven-compiler-plugin to 3.14.1 and paper-nms-maven-plugin to 1.4.10
Mojang1_20_R4/pom.xml Updates paper-nms-maven-plugin to 1.4.10
1_21_R5/pom.xml Updates maven-compiler-plugin to 3.14.1
1_21_R4/pom.xml Updates maven-compiler-plugin to 3.14.1
1_21_R3/pom.xml Updates maven-compiler-plugin to 3.14.1
1_21_R2/pom.xml Updates maven-compiler-plugin to 3.14.1
1_21_R1/pom.xml Switches from Spigot to Paper API, adds PaperMC repository, updates maven-compiler-plugin to 3.14.1
1_20_R4/pom.xml Updates maven-compiler-plugin to 3.14.1
1_20_R3/pom.xml Updates maven-compiler-plugin to 3.14.1
1_20_R2/pom.xml Updates maven-compiler-plugin to 3.14.1
1_20_R1/pom.xml Updates maven-compiler-plugin to 3.14.1
1_19_R3/pom.xml Updates maven-compiler-plugin to 3.14.1
1_19_R2/pom.xml Updates maven-compiler-plugin to 3.14.1
1_19_R1/pom.xml Updates maven-compiler-plugin to 3.14.1
1_18_R2/pom.xml Updates maven-compiler-plugin to 3.14.1
1_18_R1/pom.xml Updates maven-compiler-plugin to 3.14.1
1_17_R1/pom.xml Updates maven-compiler-plugin to 3.14.1

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +91 to +92
public static boolean hasRegionScheduler() {
return hasRegionScheduler;
Copy link

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

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

The method hasRegionScheduler() returns the static field hasRegionScheduler, but this creates a naming collision and confusion. The method and field have the same name, making the code harder to read. Consider renaming the field to something like hasRegionSchedulerClass or regionSchedulerAvailable to distinguish it from the getter method.

Copilot uses AI. Check for mistakes.
Comment on lines +35 to +40
public void runTaskAsynchronously(JavaPlugin plugin, Runnable task) {
if (Bukkit.isPrimaryThread()) {
Bukkit.getScheduler().runTaskAsynchronously(plugin, task);
} else {
task.run();
}
Copy link

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

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

The runTaskAsynchronously method has inverted logic that could lead to unexpected behavior. When already on an async thread (isPrimaryThread() returns false), it runs the task synchronously in the current thread. However, if the current thread is already a custom async thread (not primary thread), running task.run() directly means it stays async, which is correct. But when on the primary thread, it correctly schedules async. The logic works but the comment or implementation could be clearer about this intentional behavior to avoid confusion. Consider adding a comment explaining why this optimization is safe.

Copilot uses AI. Check for mistakes.
README.md Outdated
})

// RECOMMENDED: Call handler synchronously for thread safety
// REQUIRED for Folia, CanvasMC, and Archlight
Copy link

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

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

The statement "REQUIRED for Folia, CanvasMC, and Archlight" is somewhat misleading. Based on the PlatformDetector implementation, CanvasMC and Archlight are detected but both use the BukkitSchedulerAdapter, not a special scheduler. So while using callHandlerSynchronously is good practice for thread safety, it's technically not "REQUIRED" specifically for CanvasMC and Archlight in the same way it is for Folia. Consider revising to "REQUIRED for Folia; RECOMMENDED for CanvasMC, Archlight, and other platforms".

Suggested change
// REQUIRED for Folia, CanvasMC, and Archlight
// REQUIRED for Folia; RECOMMENDED for CanvasMC, Archlight, and other platforms

Copilot uses AI. Check for mistakes.
try {
boolean isOwned = (boolean) IS_OWNED_BY_CURRENT_REGION_METHOD.invoke(null, location);
if (isOwned) {
task.run();
Copy link

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

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

When isOwned is true, the task runs directly on the current thread without any error handling. If the task throws an exception, it will propagate up and potentially break the calling code. In contrast, when scheduled through the region scheduler, Folia would handle exceptions internally. Consider wrapping the direct task.run() call in a try-catch block to maintain consistent error handling behavior between direct execution and scheduled execution.

Suggested change
task.run();
try {
task.run();
} catch (Exception ex) {
ex.printStackTrace();
}

Copilot uses AI. Check for mistakes.
* If called the handler will be called synchronously by calling the method {@link org.bukkit.scheduler.BukkitScheduler#runTask(Plugin, Runnable)}
* If called the handler will be called synchronously by using the appropriate scheduler.
* This is required for Folia, CanvasMC, and Archlight support and ensures thread-safe execution.
* On Folia, tasks are scheduled on the player's region thread.
Copy link

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

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

The documentation states that on Folia "tasks are scheduled on the player's region thread," but the actual implementation in SignGUI.java line 113 calls runTask(plugin, player, runnable). This correctly schedules on the player's entity scheduler. However, the SignGUIBuilder documentation should be consistent and mention "entity scheduler" to align with Folia's terminology, or verify that "region thread" accurately describes where entity-scheduled tasks execute.

Suggested change
* On Folia, tasks are scheduled on the player's region thread.
* On Folia, tasks are scheduled via the player's entity scheduler (on the thread owning the player's region).

Copilot uses AI. Check for mistakes.
Comment on lines 248 to 250
// On Folia, there is no single main thread - we're always on a valid region thread
// if we're executing game logic
return true;
Copy link

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

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

The isOnMainThread() method always returns true on Folia, which is misleading. While Folia doesn't have a single main thread, there are still invalid threads (like async threads) where this should return false. Consider checking if the current thread is a region thread or global region thread by attempting to invoke a Folia-specific method that identifies the thread type, or clarify in documentation that this method's semantics change on Folia.

Suggested change
// On Folia, there is no single main thread - we're always on a valid region thread
// if we're executing game logic
return true;
// On Folia, there is no single main thread; use Bukkit's primary thread check,
// which returns true on region/global region threads and false on async threads.
return Bukkit.isPrimaryThread();

Copilot uses AI. Check for mistakes.
Comment on lines +258 to +260
private SchedulerAdapter fallbackScheduler() {
return new BukkitSchedulerAdapter();
}
Copy link

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

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

Creating a new BukkitSchedulerAdapter instance on every fallback call is inefficient. Each time a Folia method fails and falls back, or when FOLIA_AVAILABLE is false, a new adapter is instantiated. Consider caching the fallback scheduler as a static final field or reusing the same instance to avoid unnecessary object creation.

Copilot uses AI. Check for mistakes.
Comment on lines +18 to +29
public static SchedulerAdapter getScheduler() {
if (cachedAdapter != null) {
return cachedAdapter;
}

if (PlatformDetector.isFolia()) {
cachedAdapter = new FoliaSchedulerAdapter();
} else {
cachedAdapter = new BukkitSchedulerAdapter();
}

return cachedAdapter;
Copy link

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

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

The cachedAdapter field is not thread-safe. Multiple threads calling getScheduler() concurrently during initialization could create multiple adapter instances due to a race condition in the check-then-set pattern (lines 19-20 and 23-27). Consider using double-checked locking with volatile, or initialize the adapter in a static block to ensure thread safety.

Copilot uses AI. Check for mistakes.
README.md Outdated

You don't have to call all methods. Only `setHandler` is mandatory.

**Important for Folia/CanvasMC/Archlight:** Always use `callHandlerSynchronously(plugin)` to ensure thread-safe operation on all platforms. On Folia, this ensures tasks run on the correct region thread. On other platforms, it ensures tasks run on the main thread.
Copy link

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

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

The statement "REQUIRED for Folia, CanvasMC, and Archlight" is somewhat misleading. Based on the PlatformDetector implementation, CanvasMC and Archlight use the BukkitSchedulerAdapter, not a Folia-specific scheduler. While using callHandlerSynchronously is good practice for thread safety, it's not technically "REQUIRED" for CanvasMC and Archlight in the same way it is for Folia. Consider revising to "REQUIRED for Folia; RECOMMENDED for all other platforms for thread safety".

Suggested change
**Important for Folia/CanvasMC/Archlight:** Always use `callHandlerSynchronously(plugin)` to ensure thread-safe operation on all platforms. On Folia, this ensures tasks run on the correct region thread. On other platforms, it ensures tasks run on the main thread.
**Important:** `callHandlerSynchronously(plugin)` is **REQUIRED for Folia** and **RECOMMENDED for all other platforms for thread safety**. On Folia, this ensures tasks run on the correct region thread. On other platforms, it ensures tasks run on the main thread.

Copilot uses AI. Check for mistakes.
Comment on lines 83 to 85
} catch (Exception e) {
System.err.println("Warning: Folia classes not found, but FoliaSchedulerAdapter was attempted to be used. " +
"This should not happen. Falling back to Bukkit scheduler.");
Copy link

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

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

The static initializer catches all exceptions broadly with "catch (Exception e)" and prints to System.err, but doesn't provide any details about which specific class or method failed to load. This makes debugging difficult if there are issues with Folia API compatibility. Consider catching ClassNotFoundException and NoSuchMethodException separately and logging which specific class or method caused the failure to help diagnose version incompatibilities.

Suggested change
} catch (Exception e) {
System.err.println("Warning: Folia classes not found, but FoliaSchedulerAdapter was attempted to be used. " +
"This should not happen. Falling back to Bukkit scheduler.");
} catch (ClassNotFoundException e) {
System.err.println("Warning: Required Folia/Paper scheduler class not found while initializing FoliaSchedulerAdapter: "
+ e.getMessage());
System.err.println("Falling back to Bukkit scheduler. This usually indicates a Folia/Paper version incompatibility or that Folia is not present.");
e.printStackTrace(System.err);
} catch (NoSuchMethodException e) {
System.err.println("Warning: Required Folia/Paper scheduler method not found while initializing FoliaSchedulerAdapter: "
+ e.getMessage());
System.err.println("Falling back to Bukkit scheduler. This usually indicates a Folia/Paper or Bukkit API version incompatibility.");
e.printStackTrace(System.err);
} catch (Exception e) {
System.err.println("Warning: Unexpected error while initializing FoliaSchedulerAdapter. Falling back to Bukkit scheduler.");
e.printStackTrace(System.err);

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 32 out of 32 changed files in this pull request and generated 5 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +18 to +30
public static SchedulerAdapter getScheduler() {
if (cachedAdapter != null) {
return cachedAdapter;
}

if (PlatformDetector.isFolia()) {
cachedAdapter = new FoliaSchedulerAdapter();
} else {
cachedAdapter = new BukkitSchedulerAdapter();
}

return cachedAdapter;
}
Copy link

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

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

The getScheduler method has a race condition. Multiple threads could simultaneously check that cachedAdapter is null (line 19), causing multiple scheduler instances to be created (lines 24 and 26). While this may not cause correctness issues since schedulers are stateless, it violates the intent of caching and could lead to inconsistent behavior.

Consider making this method synchronized or using double-checked locking with volatile to ensure thread safety. Alternatively, initialize cachedAdapter in a static initializer block.

Copilot uses AI. Check for mistakes.
import org.bukkit.plugin.java.JavaPlugin;

import java.lang.reflect.Method;
import java.util.concurrent.TimeUnit;
Copy link

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

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

The TimeUnit import is not used anywhere in this class. Consider removing this unused import to keep the code clean.

Suggested change
import java.util.concurrent.TimeUnit;

Copilot uses AI. Check for mistakes.
Comment on lines +209 to +213
Method globalRunDelayed = globalScheduler.getClass().getMethod("runDelayed",
org.bukkit.plugin.Plugin.class, java.util.function.Consumer.class, long.class);
globalRunDelayed.invoke(globalScheduler, plugin,
(java.util.function.Consumer<Object>) scheduledTask -> task.run(),
delayTicks);
Copy link

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

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

The runTaskLater method resolves the globalRunDelayed method at runtime using reflection (lines 209-210), while other methods cache the Method references in static final fields during class initialization. This is inconsistent with the pattern used elsewhere in the class and adds unnecessary overhead on each invocation.

Consider resolving this method during static initialization and storing it in a static final field like GLOBAL_RUN_DELAYED_METHOD to maintain consistency and improve performance.

Copilot uses AI. Check for mistakes.
Comment on lines +269 to +271
private SchedulerAdapter fallbackScheduler() {
return new BukkitSchedulerAdapter();
}
Copy link

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

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

The fallbackScheduler method creates a new BukkitSchedulerAdapter instance on every call. Since BukkitSchedulerAdapter is stateless, this is inefficient and creates unnecessary object allocations.

Consider caching the fallback scheduler instance in a static final field (e.g., FALLBACK_SCHEDULER) to improve performance and reduce garbage collection pressure.

Copilot uses AI. Check for mistakes.
Comment on lines +150 to 152
**Important:** `callHandlerSynchronously(plugin)` is **REQUIRED for Folia** and **RECOMMENDED for all other platforms for thread safety**. On Folia, this ensures tasks run on the correct region thread. On other platforms, it ensures tasks run on the main thread.

By default, the handler is called by an asynchronous thread. You can change that behaviour by calling the method `callHandlerSynchronously` of the builder.
Copy link

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

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

The documentation on line 150 states that callHandlerSynchronously is "REQUIRED for Folia", but line 152 says "By default, the handler is called by an asynchronous thread." This creates a contradiction: if it's required for Folia, then Folia users won't have working code by default.

Consider clarifying this by either: 1) Making callHandlerSynchronously the default behavior (to avoid breaking Folia), 2) Adding a note that without calling this method, the plugin will not work correctly on Folia, or 3) Automatically detecting Folia and forcing synchronous execution internally.

Suggested change
**Important:** `callHandlerSynchronously(plugin)` is **REQUIRED for Folia** and **RECOMMENDED for all other platforms for thread safety**. On Folia, this ensures tasks run on the correct region thread. On other platforms, it ensures tasks run on the main thread.
By default, the handler is called by an asynchronous thread. You can change that behaviour by calling the method `callHandlerSynchronously` of the builder.
**Important:** On **Folia**, SignGUI automatically runs the handler synchronously on the correct region thread. On other platforms, calling `callHandlerSynchronously(plugin)` is **RECOMMENDED** for thread safety, as it ensures tasks run on the main server thread.
By default on **non-Folia** platforms, the handler is called by an asynchronous thread. You can change that behaviour by calling the method `callHandlerSynchronously` of the builder.

Copilot uses AI. Check for mistakes.
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.

1 participant