MultiPaper / Folia / ShreddedPaper support. #111
Conversation
On platforms such as MultiPaper, where world(s) are split over multiple server instances, we should only store instances of an obelisk block entity if the location of the entity belongs to the server instance the plugin is running on. We make this simple call (`MultiLib#isChunkExternal(Entity)`) and return if it is true, meaning the entity does not belong to the current server instance. This is still backwards compatible with all other platforms thanks to the devs of MultiLib.
Unsure how MultiLib will handle getNearbyEntities so.... fun!
|
Warning Rate limit exceeded@Skullians has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 17 minutes and 6 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe pull request introduces integration with MultiLib, a library for managing cross-server interactions in Minecraft. The changes span multiple files, primarily focusing on replacing Bukkit's scheduler with MultiLib's scheduling mechanisms. Key modifications include updating task scheduling in various API classes, adding a new Maven repository, and incorporating MultiLib's methods for handling online players, chunk management, and asynchronous operations. The changes aim to enhance the plugin's cross-server compatibility and potentially improve performance. Changes
Sequence DiagramsequenceDiagram
participant Plugin
participant MultiLib
participant Bukkit
Plugin->>MultiLib: Request Async Task
MultiLib->>Bukkit: Schedule Task
Bukkit-->>MultiLib: Task Executed
MultiLib-->>Plugin: Callback/Result
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
paper/src/main/java/net/skullian/skyfactions/paper/api/SpigotObeliskAPI.java (1)
Line range hint
33-57: Consider refactoring spawn methods to reduce duplicationThe conversion to MultiLib's GlobalRegionScheduler is correct and aligns with the PR objectives. However, both
spawnPlayerObeliskandspawnFactionObeliskcontain nearly identical code. Consider extracting the common logic into a private helper method.Here's a suggested refactor:
+ private void spawnObelisk(String identifier, SkyIsland island, String worldSetting, String type) { + MultiLib.getGlobalRegionScheduler().run(SkyFactionsReborn.getInstance(), (consumer) -> { + World world = Bukkit.getWorld(worldSetting); + if (world != null) { + SkyLocation center = island.getCenter(world.getName()); + List<Integer> offset = ObeliskConfig.OBELISK_SPAWN_OFFSET.getIntegerList(); + SkyLocation offsetLocation = center.add(offset.get(0), offset.get(1), offset.get(2)); + + if (ObeliskConfig.OBELISK_CUSTOM_MODEL_DATA.getInt() != -1) { + SpigotObeliskBlockEntity bEntity = new SpigotObeliskBlockEntity(); + bEntity.placeBlock(offsetLocation, new ObeliskItem(SkyItemStack.builder() + .material(ObeliskConfig.OBELISK_MATERIAL.getString()).build())); + blockEntities.put(offsetLocation, bEntity); + } else { + Material obeliskMaterial = Material.getMaterial(ObeliskConfig.OBELISK_MATERIAL.getString()); + if (obeliskMaterial == null) return; + SpigotAdapter.adapt(offsetLocation).getBlock().setType(obeliskMaterial); + } + + applyPersistentData(identifier, offsetLocation, type); + } + }); + } @Override public void spawnPlayerObelisk(SkyUser player, SkyIsland island) { - MultiLib.getGlobalRegionScheduler().run(SkyFactionsReborn.getInstance(), (consumer) -> { - // ... existing code ... - }); + spawnObelisk(player.getUniqueId().toString(), island, + Settings.ISLAND_PLAYER_WORLD.getString(), "player"); } @Override public void spawnFactionObelisk(String faction, SkyIsland island) { - MultiLib.getGlobalRegionScheduler().run(SkyFactionsReborn.getInstance(), (consumer) -> { - // ... existing code ... - }); + spawnObelisk(faction, island, + Settings.ISLAND_FACTION_WORLD.getString(), "faction"); }Also applies to: 59-83
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
common/src/main/java/net/skullian/skyfactions/common/api/FactionAPI.java(2 hunks)gradle.properties(1 hunks)paper/build.gradle(2 hunks)paper/src/main/java/net/skullian/skyfactions/paper/SkyLoader.java(1 hunks)paper/src/main/java/net/skullian/skyfactions/paper/api/SpigotFactionAPI.java(2 hunks)paper/src/main/java/net/skullian/skyfactions/paper/api/SpigotObeliskAPI.java(4 hunks)paper/src/main/java/net/skullian/skyfactions/paper/api/SpigotPlayerAPI.java(2 hunks)paper/src/main/java/net/skullian/skyfactions/paper/api/SpigotRegionAPI.java(2 hunks)paper/src/main/java/net/skullian/skyfactions/paper/api/SpigotSkyAPI.java(1 hunks)paper/src/main/java/net/skullian/skyfactions/paper/defence/SpigotDefence.java(3 hunks)paper/src/main/java/net/skullian/skyfactions/paper/defence/defences/ArrowDefence.java(2 hunks)paper/src/main/java/net/skullian/skyfactions/paper/event/armor/ArmorListener.java(4 hunks)paper/src/main/java/net/skullian/skyfactions/paper/gui/items/impl/SpigotAsyncSkyItem.java(3 hunks)paper/src/main/java/net/skullian/skyfactions/paper/gui/screens/SpigotScreen.java(2 hunks)paper/src/main/java/net/skullian/skyfactions/paper/user/SpigotSkyUser.java(2 hunks)v1_21_R3/src/main/java/net/skullian/skyfactions/nms/v1_21_R3/NMSHandlerImpl.java(1 hunks)v1_21_R4/src/main/java/net/skullian/skyfactions/nms/v1_21_R4/NMSHandlerImpl.java(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- gradle.properties
- v1_21_R4/src/main/java/net/skullian/skyfactions/nms/v1_21_R4/NMSHandlerImpl.java
- v1_21_R3/src/main/java/net/skullian/skyfactions/nms/v1_21_R3/NMSHandlerImpl.java
🔇 Additional comments (31)
paper/src/main/java/net/skullian/skyfactions/paper/defence/SpigotDefence.java (4)
5-5: New import for MultiLib.
This import is essential for the chunk external check further down. Ensure that MultiLib is correctly declared as a dependency so that MultiLib.isChunkExternal is always available at runtime.
110-110: Directly returning the result simplifies the control flow.
Replacing an intermediate variable with a direct return eliminates redundancy and improves readability without harming maintainability.
126-126: Verify the chunk-external check.
Skipping external-chunk entities can improve performance and avoid cross-server entities, but confirm this is the intended behavior. In certain scenarios, ignoring these entities might cause unexpected gameplay interactions.
127-127: No functional changes detected.
This line appears to be a blank or spacing line. No action required.
paper/src/main/java/net/skullian/skyfactions/paper/api/SpigotPlayerAPI.java (2)
3-3: Check import usage and clarity
The import statement looks correct, aligning with the PR objectives of incorporating MultiLib for increased compatibility. Ensure the project’s build files declare the dependency properly (e.g., compile-only) to maintain minimal overhead on servers not using MultiLib.
70-70: Confirm cross-server compatibility assumption
Replacing Bukkit.getOnlinePlayers() with MultiLib.getAllOnlinePlayers() is consistent with the PR goal of supporting multiple server implementations. Ensure downstream code that processes these players correctly handles cross-server scenarios.
paper/src/main/java/net/skullian/skyfactions/paper/gui/screens/SpigotScreen.java (2)
3-3: Validate import alignment
This aligns with adopting MultiLib for scheduling. Confirm that the code no longer relies on Bukkit.getScheduler().
30-31: Ensure proper asynchronous scheduling usage
Switching from Bukkit.getScheduler().runTask(...) to MultiLib.getGlobalRegionScheduler().run(...) ensures cross-server compatibility but verify that the scheduling context remains the same (sync vs async). Confirm that any code reliant on synchronous behavior is unaffected by these changes.
paper/src/main/java/net/skullian/skyfactions/paper/defence/defences/ArrowDefence.java (2)
3-3: Import usage
Importing MultiLib is consistent with the PR’s goal to remove direct Bukkit scheduler usage. This import is valid as long as the multilib dependency is included in your build configuration.
30-30: Confirm asynchronous constraints for projectile logic
Replacing the Bukkit synchronous task with MultiLib.getAsyncScheduler().runNow(...) might change the thread context. Verify that projectile creation, spawning, and PDC application do not require synchronous server thread operations.
paper/src/main/java/net/skullian/skyfactions/paper/gui/items/impl/SpigotAsyncSkyItem.java (3)
3-3: Ensure build settings
Adding the MultiLib import for asynchronous scheduling is in line with the PR objectives. Double-check that your build tool references the correct MultiLib version.
34-35: Finalize item loading state asynchronously
Running setLoading(false) and notifyWindows() inside MultiLib.getAsyncScheduler().runNow(...) is appropriate for finalizing asynchronous item loading. Confirm UI updates do not require server-sync calls.
75-76: Immediate event cancellation vs async processing
Cancelling the click event immediately, then running asynchronous logic ensures minimal blocking. Good approach. Just confirm that any subsequent click-related logic does not rely on server-sync actions.
paper/src/main/java/net/skullian/skyfactions/paper/user/SpigotSkyUser.java (2)
3-3: MultiLib import aligns with PR objectives
Importing com.github.puregero.multilib.MultiLib is fully in line with the move to MultiLib schedulers for cross-server compatibility.
40-40: Use of asynchronous teleport
Replacing Bukkit's synchronous teleport with MultiLib.teleportAsync is consistent with the new scheduling approach. Verify that teleportation sequences (e.g., player state, chunk loading) remain stable when run asynchronously.
paper/src/main/java/net/skullian/skyfactions/paper/SkyLoader.java (2)
52-56: Addition of MultiLib Maven resolver
Defining multiLibResolver and directing it to the "multipaper" repository for com.github.puregero:multilib:1.2.4 ensures proper library resolution. This is necessary for the cross-server scheduling functionality.
60-60: Appending MultiLib resolver to plugin classpath
Adding the MultiLib resolver ensures that MultiLib is packaged correctly at runtime, supporting the newly introduced asynchronous tasks.
paper/src/main/java/net/skullian/skyfactions/paper/api/SpigotFactionAPI.java (2)
3-3: MultiLib import for global region scheduling
The import reflects the transition away from Bukkit's scheduler and is part of the wider move to MultiLib-based scheduling.
37-37: Global region scheduling usage
Using MultiLib.getGlobalRegionScheduler().run(...) shifts tasks to MultiLib’s concurrency model. Confirm concurrency safety in related code, particularly around WorldGuard or any shared data that might be affected by asynchronous operations.
paper/src/main/java/net/skullian/skyfactions/paper/api/SpigotRegionAPI.java (2)
3-3: MultiLib import for region scheduling
Introducing the MultiLib import here is consistent with the project-wide adoption of MultiLib.
105-105: Asynchronous world border modification
The switch to MultiLib.getGlobalRegionScheduler().run(...) avoids using Bukkit’s scheduler. Validate that queued border adjustments won’t conflict with other tasks (e.g., region modifications) also running asynchronously.
common/src/main/java/net/skullian/skyfactions/common/api/FactionAPI.java (2)
36-36: Consider concurrency implications of whenCompleteAsync.
By switching from whenComplete to whenCompleteAsync, you delegate completion handling to a separate thread. Verify that all accessed fields (e.g., factionCache, factionUserCache) are thread-safe or properly synchronized.
If concurrency issues are suspected, consider eliminating shared mutable state or using additional synchronization.
45-45: 🛠️ Refactor suggestion
Exercise caution with .join().
Calling .join() blocks the current thread until the future completes, possibly impacting responsiveness. If blocking is undesirable, consider chaining further CompletionStages or using appropriate concurrency utilities.
paper/src/main/java/net/skullian/skyfactions/paper/api/SpigotSkyAPI.java (1)
137-139: Check the ordering of preload calls.
Preloading obelisks before other modules is good for ensuring their availability. However, confirm that no other modules depend on obelisks being fully loaded beforehand.
paper/src/main/java/net/skullian/skyfactions/paper/event/armor/ArmorListener.java (4)
3-3: Good practice adding the MultiLib import.
The import ensures usage of the MultiLib schedulers as intended in subsequent lines.
101-101: Validate concurrency with global region scheduler.
Replacing Bukkit.getScheduler().runTask with MultiLib.getGlobalRegionScheduler().run is valid. Ensure any accesses to shared states or references in this runnable are thread-safe, given the possibility of multi-threaded execution.
124-124: Avoid blocking calls in asynchronous tasks.
Using MultiLib.getAsyncScheduler().runNow is a good approach. Just ensure you aren’t calling blocking operations (e.g., .join()) within the async environment.
216-216: Synchronize inventory checks carefully.
Running inventory checks and modifications via MultiLib.getGlobalRegionScheduler().run can help avoid main-thread blocking. However, watch for race conditions if the player’s inventory changes between scheduling and execution.
paper/build.gradle (2)
99-103: Repository addition looks good.
Adding the MultiPaper (MultiLib) repository expands the available artifacts. Ensure the repository URL is correct and stable.
114-114: Validate version alignment for multilib.
Including com.github.puregero:multilib is correct for custom schedulers. Confirm that the version in gradle.properties matches the requirement of other dependencies to avoid conflicts.
paper/src/main/java/net/skullian/skyfactions/paper/api/SpigotObeliskAPI.java (1)
3-3: LGTM: MultiLib import added correctly
The addition of MultiLib import aligns with the PR objective of converting to MultiLib schedulers.
paper/src/main/java/net/skullian/skyfactions/paper/api/SpigotObeliskAPI.java
Outdated
Show resolved
Hide resolved
Qodana Community for JVM1 new problem were found
💡 Qodana analysis was run in the pull request mode: only the changed files were checked Contact Qodana teamContact us at [email protected]
|
This PR converts all BukkitScheduler to MultiLib schedulers.
This adds support for MultiPaper, Folia, ShreddedPaper and similar while maintaining compatibility with Paper / forks.
Summary by CodeRabbit
New Features
multiliblibrary to improve task scheduling and player management.Bug Fixes
Documentation
Refactor