Refactor SpigotEntityIdProvider ID supplier logic - #45
Conversation
Refactor entity ID supplier detection logic to improve maintainability and clarity.
WalkthroughRefactored SpigotEntityIdProvider to improve ID-generation robustness by replacing direct reflection logic with dedicated helper methods that handle multiple entity ID storage mechanisms across different Minecraft and Paper versions. Changes
Sequence DiagramsequenceDiagram
participant Client
participant detectIdSupplier
participant resolveAtomicSupplier
participant resolveLegacySupplier
participant getStaticFieldOfType
Client->>detectIdSupplier: Request ID supplier
alt Paper 1.16+
detectIdSupplier->>detectIdSupplier: Use UnsafeValues.nextEntityId
else Minecraft 1.14+
detectIdSupplier->>resolveAtomicSupplier: Resolve AtomicInteger field
resolveAtomicSupplier->>getStaticFieldOfType: Locate AtomicInteger field
getStaticFieldOfType-->>resolveAtomicSupplier: Field located
resolveAtomicSupplier-->>detectIdSupplier: Increment supplier ready
else Legacy versions
detectIdSupplier->>resolveLegacySupplier: Resolve int field
resolveLegacySupplier->>getStaticFieldOfType: Locate int field
getStaticFieldOfType-->>resolveLegacySupplier: Field located
resolveLegacySupplier-->>detectIdSupplier: Read/increment supplier ready
end
detectIdSupplier-->>Client: ID supplier
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
platforms/spigot/src/main/java/me/tofaa/entitylib/spigot/SpigotEntityIdProvider.java (2)
69-76: Version‑aware supplier selection and helper split look soundThe refactored
detectIdSupplier()flow (Paper fast‑path → 1.14+ atomic path → legacy path) plusresolveAtomicSupplier/resolveLegacySupplieris logically consistent and keeps behavior centralized while preserving a clear failure mode viaIllegalStateException. The atomic path cleanly falls back to legacy when no suitableAtomicIntegeris found.If you want easier diagnostics on odd future versions, consider (optionally) adding a debug log when:
resolveAtomicSupplierreturnsnulldespite a 1.14+ version, and/orresolveLegacySupplierthrows because no legacy counter field was found.This would make it much easier to understand reflection breakage on new Spigot/Paper releases without changing current behavior.
Also applies to: 78-112
153-169:getStaticFieldOfTypehelper improves robustness; watch first‑match fallback semanticsCentralizing the reflection into
getStaticFieldOfTypewith:
- type checks (
desiredType.isAssignableFrom(field.getType())), and- a static‑modifier check,
is a solid improvement and should catch most NMS name changes automatically.
One thing to be aware of: the second loop returns the first static field of the requested type. If a future
Entityclass ever gains multiple staticAtomicIntegerorintfields, this might pick the wrong one. That’s probably acceptable given current NMS layouts, but if this ever becomes an issue, you could tighten the heuristic (e.g., filter byfinal, by name prefix, or by requiring uniqueness before accepting a match).Also, the Javadoc above
detectIdSupplier()still mentions only"entityCount"/"d"/"c"and not the newer aliases ("counter","nextEntityId","b"); updating that comment would better reflect the actual lookup behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
platforms/spigot/src/main/java/me/tofaa/entitylib/spigot/SpigotEntityIdProvider.java(3 hunks)
🔇 Additional comments (1)
platforms/spigot/src/main/java/me/tofaa/entitylib/spigot/SpigotEntityIdProvider.java (1)
3-20: Imports for reflection and Bukkit/Paper APIs look correctThe added
Modifierimport and the Bukkit / Paper / Platform imports align with the new helper and usage; no issues here.
Tofaa2
commented
Nov 19, 2025
Thank you! |
TWME-TW
commented
Nov 19, 2025
Please make sure to review this PR carefully, because I don't really know what I'm doing. |
Uh oh!
There was an error while loading. Please reload this page.
Refactor entity ID supplier detection logic to improve maintainability and clarity.
This pull request was written by AI. It currently fixes an issue I encountered in Spigot 1.21.10.
However, since I am not very familiar with reflection, I am not entirely sure about the contents of this PR. Therefore, please make sure to review it carefully to ensure that this PR meets the requirements.
Summary by CodeRabbit