-
Notifications
You must be signed in to change notification settings - Fork 31
Fix #216
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
Fix #216
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Improves Minecraft-version compatibility for variant item component registration and hardens PacketEvents title-animation handling with better thread/plugin-state checks.
Changes:
- Added
is1_11OrNewer()andis1_12OrNewer()toNmsVersionfor more granular version gating. - Refactored variant item component registration to be conditional by server version, and wrapped initialization with error handling/logging.
- Updated
PacketAnimationListenerto only run tasks immediately on the primary thread and to avoid scheduling tasks when the plugin is disabled.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/main/java/fr/maxlego08/menu/ZComponentsManager.java |
Adds guarded variant-component initialization with logging and refactors variant loader registration by version. |
Hooks/PacketEvents/src/main/java/fr/maxlego08/menu/hooks/packetevents/listener/PacketAnimationListener.java |
Adjusts task execution conditions to respect main-thread execution and plugin enabled state. |
Common/src/main/java/fr/maxlego08/menu/common/utils/nms/NmsVersion.java |
Adds new helper version-check methods for 1.11+ and 1.12+. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } catch (Exception e) { | ||
| if (Configuration.enableDebug) { | ||
| Logger.info("Failed to initialize variant item components:"); | ||
| e.printStackTrace(); | ||
| } |
Copilot
AI
Jan 28, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The exception handling here only logs when Configuration.enableDebug is true, so production failures during variant component initialization become silent and can leave the plugin partially initialized with no indication why. Consider always logging at least a WARNING/ERROR (and use the project Logger rather than printStackTrace()), and if the intent is to guard against version/linkage problems, catch Throwable (or at minimum LinkageError) so missing API symbols don’t still crash startup.
|
|
||
| if (currentVersion.isNewMaterial()){ // 1.13+ | ||
| this.registerComponent(loaderFactory.getLoaderFox()); | ||
| this.registerComponent(loaderFactory.getLoaderMushroomCow()); | ||
| } | ||
| if (currentVersion.isNewNMSVersion()){ // 1.17+ | ||
| this.registerComponent(loaderFactory.getLoaderAxolotl()); | ||
| } | ||
| if (currentVersion.isAttributItemStack()){ // 1.20.5+ | ||
| this.registerComponent(loaderFactory.getLoaderWolfCollar()); | ||
| this.registerComponent(loaderFactory.getLoaderWolfVariant()); | ||
| this.registerComponent(loaderFactory.getLoaderPainting()); | ||
| } | ||
| if (currentVersion.is1_21_5OrNewer()){ // 1.21.5+ | ||
| this.registerComponent(loaderFactory.getLoaderChicken()); | ||
| this.registerComponent(loaderFactory.getLoaderCow()); | ||
| this.registerComponent(loaderFactory.getLoaderPig()); | ||
| } | ||
| if (currentVersion.isNewNBTVersion()) { // 1.18+ | ||
| this.registerComponent(loaderFactory.getLoaderFrog()); | ||
| } | ||
| if (currentVersion.is1_11OrNewer()){ // 1.11+ | ||
| this.registerComponent(loaderFactory.getLoaderLlama()); | ||
| this.registerComponent(loaderFactory.getLoaderShulkerBox()); | ||
| } | ||
| if (currentVersion.is1_12OrNewer()) { // 1.12+ | ||
| this.registerComponent(loaderFactory.getLoaderParrot()); | ||
| } |
Copilot
AI
Jan 28, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
initializeVariantComponents() is only called from the isAttributItemStack() (1.20.5+) branch, so version gates like isNewMaterial() / isNewNMSVersion() are redundant as written and can mislead future maintenance. Either remove the redundant checks, or move version gating fully into this method and call it unconditionally (ensuring incompatible loaders are never instantiated on older versions).
| if (currentVersion.isNewMaterial()){ // 1.13+ | |
| this.registerComponent(loaderFactory.getLoaderFox()); | |
| this.registerComponent(loaderFactory.getLoaderMushroomCow()); | |
| } | |
| if (currentVersion.isNewNMSVersion()){ // 1.17+ | |
| this.registerComponent(loaderFactory.getLoaderAxolotl()); | |
| } | |
| if (currentVersion.isAttributItemStack()){ // 1.20.5+ | |
| this.registerComponent(loaderFactory.getLoaderWolfCollar()); | |
| this.registerComponent(loaderFactory.getLoaderWolfVariant()); | |
| this.registerComponent(loaderFactory.getLoaderPainting()); | |
| } | |
| if (currentVersion.is1_21_5OrNewer()){ // 1.21.5+ | |
| this.registerComponent(loaderFactory.getLoaderChicken()); | |
| this.registerComponent(loaderFactory.getLoaderCow()); | |
| this.registerComponent(loaderFactory.getLoaderPig()); | |
| } | |
| if (currentVersion.isNewNBTVersion()) { // 1.18+ | |
| this.registerComponent(loaderFactory.getLoaderFrog()); | |
| } | |
| if (currentVersion.is1_11OrNewer()){ // 1.11+ | |
| this.registerComponent(loaderFactory.getLoaderLlama()); | |
| this.registerComponent(loaderFactory.getLoaderShulkerBox()); | |
| } | |
| if (currentVersion.is1_12OrNewer()) { // 1.12+ | |
| this.registerComponent(loaderFactory.getLoaderParrot()); | |
| } | |
| this.registerComponent(loaderFactory.getLoaderFox()); | |
| this.registerComponent(loaderFactory.getLoaderMushroomCow()); | |
| this.registerComponent(loaderFactory.getLoaderAxolotl()); | |
| this.registerComponent(loaderFactory.getLoaderWolfCollar()); | |
| this.registerComponent(loaderFactory.getLoaderWolfVariant()); | |
| this.registerComponent(loaderFactory.getLoaderPainting()); | |
| this.registerComponent(loaderFactory.getLoaderFrog()); | |
| this.registerComponent(loaderFactory.getLoaderLlama()); | |
| this.registerComponent(loaderFactory.getLoaderShulkerBox()); | |
| this.registerComponent(loaderFactory.getLoaderParrot()); | |
| if (currentVersion.is1_21_5OrNewer()){ // 1.21.5+ | |
| this.registerComponent(loaderFactory.getLoaderChicken()); | |
| this.registerComponent(loaderFactory.getLoaderCow()); | |
| this.registerComponent(loaderFactory.getLoaderPig()); | |
| } |
| if (Bukkit.isPrimaryThread()) { | ||
| task.run(); | ||
| } else { | ||
| } else if (this.plugin.isEnabled()) { | ||
| this.plugin.getScheduler().runNextTick(w -> task.run()); | ||
| } |
Copilot
AI
Jan 28, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When onPacketSend is invoked off the primary thread, this branch schedules task but the method still reads/writes playerAnimationData (a plain HashMap) on the packet thread while task mutates it on the Bukkit thread. This is a data race. Use a ConcurrentHashMap (and ensure PlayerAnimationData updates are thread-safe) or move all access to playerAnimationData into the scheduled main-thread task so it’s single-threaded.
| if (Bukkit.isPrimaryThread()) { | ||
| task.run(); | ||
| } else { | ||
| } else if (this.plugin.isEnabled()) { | ||
| this.plugin.getScheduler().runNextTick(w -> task.run()); | ||
| } |
Copilot
AI
Jan 28, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the WINDOW_ITEMS case, event.setCancelled(true) happens inside task. If onPacketSend is called off-thread (which this code path handles), cancelling on the next tick is typically too late because the packet may already have been sent and the event object may no longer be valid. Cancel the event synchronously before scheduling (e.g., by storing a “shouldHideItems” flag when the title animation starts and checking it immediately here), and use the scheduled task only for Bukkit/main-thread state updates.
This pull request introduces several improvements to version handling and component registration, particularly enhancing compatibility with different Minecraft versions. The most significant changes include adding new version-checking methods, refactoring how variant item components are registered based on version, and improving error handling during initialization.
Version handling and compatibility improvements:
is1_11OrNewer()andis1_12OrNewer()to theNmsVersionclass to enable more granular version checks.initializeVariantComponentsinZComponentsManagerto register variant item components conditionally, based on the current Minecraft version. This ensures only supported components are registered for each version, improving compatibility and reducing errors.Error handling and logging:
initializeVariantComponentsin a try-catch block, logging errors when initialization fails if debug mode is enabled. This makes debugging easier and prevents crashes during startup.Loggerclass to support improved logging.Threading and plugin state checks:
PacketAnimationListenerto ensure tasks are only scheduled if the plugin is enabled, and only run immediately if on the primary thread. This prevents potential issues when the plugin is disabled or running asynchronously. [1] [2] [3]