Repository navigation
Feat/npc module - #4
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThe pull request adds a complete NPC module with public APIs, JSON persistence, skin caching, packet-based rendering, holograms, interactions, commands, lifecycle integration, platform bridges, tests, and documentation. ChangesNPC module
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Server
participant NpcManager
participant NpcViewerService
participant NpcRenderService
participant SkinCache
participant NpcPacketSenderImpl
Server->>NpcManager: tick()
NpcManager->>NpcViewerService: synchronize player session
NpcManager->>NpcRenderService: spawn visible NPC
NpcRenderService->>SkinCache: resolve skin
SkinCache-->>NpcRenderService: return cached or resolved skin
NpcRenderService->>NpcPacketSenderImpl: send player and entity packets
NpcPacketSenderImpl-->>NpcViewerService: update viewer-visible entity state
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 51
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/engineering/npcs/02-protocol-and-rendering.md (1)
48-49: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse a byte array in the UUID example.
UUID.nameUUIDFromBytesacceptsbyte[], notString. Use the same UTF-8 encoding as the implementation so the documented example compiles and matches the documented behavior.Proposed documentation fix
-UUID.nameUUIDFromBytes("bigbang-npc:" + npcId) +UUID.nameUUIDFromBytes( + ("bigbang-npc:" + npcId).getBytes(StandardCharsets.UTF_8))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/engineering/npcs/02-protocol-and-rendering.md` around lines 48 - 49, Update the UUID derivation example to pass a UTF-8 encoded byte array to UUID.nameUUIDFromBytes instead of a String, matching the implementation’s encoding and ensuring the documented example compiles.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@common/src/main/java/com/pedrodalben/bigbangessentials/BigBangEssentials.java`:
- Around line 811-817: Move the NPC shutdown block identified by
NpcManager.getInstance().shutdown() above the holograms shutdown block in the
shutdown sequence. Preserve its active-module check, logging, and exception
handling, so dependents stop before the holograms module they rely on.
In
`@common/src/main/java/com/pedrodalben/bigbangessentials/npcs/api/NpcAction.java`:
- Around line 9-12: Update the public NpcAction constructor to normalize command
strings the same way as playerCommand and consoleCommand, including removing any
leading “/” after trimming. Preserve the existing null-to-empty behavior and
ensure constructor-created actions match factory output.
In
`@common/src/main/java/com/pedrodalben/bigbangessentials/npcs/api/NpcDefinition.java`:
- Around line 33-34: Update the constructor normalization in NpcDefinition so
despawnDistance is clamped against the normalized this.viewDistance rather than
the raw viewDistance parameter. Preserve the existing minimum viewDistance of
1.0 and ensure despawnDistance never falls below it.
- Around line 38-44: Update NpcDefinition.normalizeId to lowercase IDs with a
locale-independent mechanism, such as an explicitly specified locale, while
preserving the existing trimming, validation, exception behavior, and normalized
return value.
In
`@common/src/main/java/com/pedrodalben/bigbangessentials/npcs/api/NpcHologramConfig.java`:
- Around line 15-22: Update the NpcHologramConfig constructor to prevent an
enabled configuration when lines is null or empty: either normalize enabled to
false or reject the input, while preserving existing line copying and other
field normalization.
In
`@common/src/main/java/com/pedrodalben/bigbangessentials/npcs/command/NpcCommand.java`:
- Around line 244-249: Update the `setline` and `removeline` command definitions
to use `IntegerArgumentType` for the `line` argument instead of
`StringArgumentType.word()`. Retrieve the value with the corresponding integer
accessor and remove the `Integer.parseInt` calls, preserving the existing
one-based-to-zero-based index adjustment.
- Around line 54-69: Restrict each NPC subcommand to its matching permission
instead of relying on hasAnyAdminPermission: add requires checks to createCmd
for CREATE, removeCmd for REMOVE, the edit subcommands for EDIT, reloadCmd for
RELOAD, and statsCmd for STATS. Preserve the existing console/admin access
behavior while preventing permissions for one operation from authorizing
unrelated commands.
- Around line 81-98: Update the `/npc create` execution block to check whether
the requested id already exists before constructing or passing the definition to
`api().create(def)`. Reject duplicate ids with an appropriate failure message
and return without overwriting the existing NPC; retain the current creation and
success-message flow for new ids.
In
`@common/src/main/java/com/pedrodalben/bigbangessentials/npcs/config/NpcConfigStore.java`:
- Around line 41-43: Update the missing-primary-file handling in NpcConfigStore
so it checks for and restores npcs.json.bak before calling createDefault(file).
Ensure replacement writes preserve the existing primary file until successful
completion, and prevent loading or saving an empty default configuration when a
valid backup is available.
In
`@common/src/main/java/com/pedrodalben/bigbangessentials/npcs/hologram/NpcHologramService.java`:
- Line 88: Update the owner check in the NPC stats counting logic to call equals
from the non-null NPC_OWNER constant rather than def.ownerId(), while preserving
the existing enabled condition and count behavior.
- Line 36: Remove the unused existing local and its api.findDefinition(holId)
lookup from the surrounding NpcHologramService method, since the result is never
consumed and the call adds no behavior.
- Around line 72-81: Update cleanupOrphans to compare existing NPC-owned
hologram IDs with knownNpcs before deleting, and remove only holograms whose IDs
do not correspond to a known NPC; retain createOrUpdate for known definitions.
Eliminate the unused removed result or log the deletion count.
In
`@common/src/main/java/com/pedrodalben/bigbangessentials/npcs/interaction/NpcInteractionService.java`:
- Around line 97-105: Validate the player name in executeConsoleCommand before
substituting it into the privileged command, accepting only 1–16 characters
matching [A-Za-z0-9_]. Reject invalid names without invoking
server.getCommands().performPrefixedCommand, while preserving the existing
substitution and execution flow for valid names.
In
`@common/src/main/java/com/pedrodalben/bigbangessentials/npcs/render/NpcPacketSender.java`:
- Around line 9-12: Update the NpcPacketSender interface by removing
textureValue and textureSignature from removePlayerInfo, retaining only the
viewer, UUID, and displayName parameters, and rename removeEntities to
removeEntity. Propagate both signature changes to every implementation and
caller.
In
`@common/src/main/java/com/pedrodalben/bigbangessentials/npcs/render/NpcPacketSenderImpl.java`:
- Around line 58-77: Update NpcPacketSenderImpl.removePlayerInfo() so the
constructed player-info entry uses listed=false instead of true, while retaining
the UPDATE_LISTED action and existing entity visibility behavior. Do not add the
NPC back to the tab list during the post-spawn hide operation.
- Around line 84-98: Update NpcPacketSenderImpl.rotateHead to send
ClientboundRotateHeadPacket with the computed yawByte so only the NPC head
rotation changes. Replace the relative ClientboundMoveEntityPacket.PosRot usage
in teleportEntity with the 1.21.1 ClientboundTeleportEntityPacket, passing
entityId, x, y, z, yaw, pitch, and the appropriate on-ground flag so the actual
NPC position and rotation are synchronized.
- Around line 16-35: Update NpcPacketSenderImpl’s reflective initialization and
packet creation to match the 1.21.1 APIs: resolve the Entry constructor with the
showHat and listOrder parameters plus the correct RemoteChatSession.Data type,
and resolve ClientboundPlayerInfoUpdatePacket using its EnumSet<Action>,
Collection<Entry> constructor. Adjust the corresponding newInstance calls to
pass those exact arguments, or replace the reflection with the official public
packet constructor.
In
`@common/src/main/java/com/pedrodalben/bigbangessentials/npcs/render/NpcRenderService.java`:
- Line 22: Change the renderStates field in NpcRenderService to a thread-safe
concurrent map, preserving the existing key/value types. In the
definition-assignment logic used by register, replace the
computeIfAbsent-then-get double lookup with a single map operation that returns
the created or existing NpcRenderState and assign the definition through that
result.
- Around line 114-125: Update NpcRenderService.despawn to remove the NPC’s entry
from session.npcStates() using npc.id(), alongside the existing visibleNpcIds
and entityIdToNpc cleanup, so subsequent spawns create fresh view state.
- Around line 57-65: Re-check the viewer state inside the server.execute
callback immediately before calling sendSpawn in NpcRenderService: verify the
player is still connected/registered, remains in the same valid dimension, and
is still within the NPC despawn range. Skip sendSpawn when any condition is no
longer valid, preventing stale session visibility and writes to a closed
connection.
- Around line 87-91: In the entity-data packet construction within
NpcRenderService, replace the hardcoded accessor id 17 with
Player.DATA_PLAYER_MODE_CUSTOMISATION, preserving the existing byte serializer
and skinLayers value.
In
`@common/src/main/java/com/pedrodalben/bigbangessentials/npcs/service/NpcManager.java`:
- Line 39: Remove the unused lookRoundRobinIndex field from NpcManager, since
syncLookAtPlayers does not read or update it. Do not add round-robin behavior;
leave the existing player/NPC iteration and budget handling unchanged.
- Around line 312-322: Move viewerService.clear() in shutdown() to after the
server player loop so onPlayerLeave(player) can remove each active session, send
despawn packets, and clear interaction cooldowns before the remaining viewer
state is cleared.
- Around line 210-267: Add an initialization guard at the start of
NpcManager.reload, save, and delete that detects unavailable required services,
including skinCache and renderService, and returns safely before dereferencing
them. Ensure the guard consistently handles commands invoked after initialize
fails while preserving normal behavior when initialization succeeded.
- Around line 358-360: Update the radius calculation in the NPC candidate-query
flow to use the largest configured distance across registered NPCs, considering
both each NPC’s viewDistance() and despawnDistance(), while retaining the
existing default/floor behavior when appropriate. Ensure the maximum is updated
when NPCs are registered or removed, using the relevant NpcManager
registration/removal methods and avoiding stale values.
- Line 89: Ensure visibilityScanIntervalTicks is clamped to at least 1 before
the modulo operation in NpcManager.tick(), preferably when NpcConfigStore loads
the value; preserve valid configured intervals and prevent zero or negative
values from reaching the modulo expression.
- Around line 164-172: Replace the direct skin-resolution mutation inside the
asynchronous callback with a synchronized manager handler such as
applyResolvedSkin, passing the NPC id, skin name, and resolved entry. In that
handler, look up the current NPC, skip when it was deleted/reloaded or its skin
name no longer matches, then update npcs and perform viewer invalidation (and
render registration if required). Ensure the update and packet sends run on the
server thread via MinecraftServer.execute when those operations are not
thread-safe.
In
`@common/src/main/java/com/pedrodalben/bigbangessentials/npcs/skin/MojangSkinResolver.java`:
- Around line 41-56: Update the request logic in MojangSkinResolver around the
local HttpURLConnection conn so disconnect() always executes from a finally
block, including failures from getInputStream() or JSON parsing. For non-200
responses, consume conn.getErrorStream() before returning the negative
SkinCacheEntry, while preserving the existing status handling and logging.
- Around line 101-104: Make configured skin TTLs flow through MojangSkinResolver
instead of using hardcoded values: add freshTtlMillis and negativeTtlMillis to
its constructor or configure method, remove the hardcoded TTLs at all resolver
sites, and pass the configured values to SkinCacheEntry factories. In
common/src/main/java/com/pedrodalben/bigbangessentials/npcs/skin/SkinCache.java
lines 27-30, forward both TTLs to MojangSkinResolver from construction and
configure; either recreate the request pool when maxConcurrentRequests changes
or document that it only applies at construction.
- Around line 112-119: Update resolveUuid(String playerName) to validate the
name against the allowed Minecraft profile character set before constructing
UUID_API + playerName, rejecting invalid or non-ASCII input without issuing a
request. Encode the validated name for safe URL-path use before passing it to
URI.create, while preserving the existing lookup behavior for valid names.
In
`@common/src/main/java/com/pedrodalben/bigbangessentials/npcs/skin/SkinCache.java`:
- Around line 110-125: Update refreshInBackground to register the request with
inflightRequests via computeIfAbsent, combining the existence check and future
creation atomically. Ensure the completion callback removes the same key without
leaving completed futures behind, while preserving the existing resolver, cache,
persistence, and error-handling behavior.
- Around line 32-36: Update the metric counters in SkinCache to use
AtomicInteger or LongAdder, including hits, staleHits, and failures (and keep
all counters consistent). Replace direct increments and reads in the related
cache metric methods with the chosen atomic API so updates are thread-safe and
visible across caller and executor threads.
- Around line 184-191: Update SkinCache.persistAsync to coalesce writes using a
dirty flag and one scheduled flush instead of submitting a full persist for
every resolution, while preserving eventual persistence of all changes. In
shutdown(), stop accepting work, await executor termination so queued persist
tasks finish, then perform the final persist without concurrent file writes.
- Around line 127-130: Update SkinCache.persist() to use
ResourceUtil.getMigratedDataPath(FILE), matching loadPersistent(). Write the
serialized cache to a temporary file in the target directory, then atomically
replace the destination using the existing AtomicFile or
StandardCopyOption.ATOMIC_MOVE mechanism so interrupted writes cannot truncate
skin-cache.json.
- Around line 74-99: Update the in-flight request creation in the skin
resolution method to use inflightRequests.computeIfAbsent, ensuring future
registration occurs atomically before completion cleanup can remove it; preserve
the existing resolver, fallback, and whenComplete behavior. Remove the redundant
if (result.negative()) conditional and perform the single memoryCache.put
operation unconditionally.
In
`@common/src/test/java/com/pedrodalben/bigbangessentials/npcs/NpcModuleTest.java`:
- Around line 186-197: Update spatialIndexChunkBoundary to place NPC entries
immediately on opposite sides of the 16-block chunk boundary, then query across
that boundary and assert the exact expected result set rather than only checking
inclusion/exclusion. Keep the test focused on NpcSpatialIndex chunk conversion,
using assertEquals for the complete returned set.
- Around line 248-258: Extend NpcModuleTest beyond defaultConfig with tests for
NpcConfigStore.load() and save() using an injectable temporary config directory.
Cover JSON round-trip persistence, malformed-config recovery, backup restoration
behavior, and createExample() output, verifying both resulting configurations
and relevant backup/file effects without relying on a real shared config path.
- Around line 24-45: The normalizeId helper in NpcDefinition and the normalize
helper in SkinCache must use locale-independent lowercasing via Locale.ROOT
rather than the default locale. Import Locale as needed and extend the relevant
tests, including NpcModuleTest, to temporarily use Turkish locale and verify
uppercase-I inputs produce stable ASCII-normalized IDs and skin cache keys,
restoring the original locale afterward.
In `@docs/engineering/npcs/01-architecture-audit.md`:
- Around line 236-240: The Network section’s NPC spawn estimate should reflect
both paths: document 3–5 packets per spawn, with five as the resolved-texture
worst case, and update the related burst calculation to use the corrected
estimate.
- Around line 141-155: Update sections 4.1 and 4.2 of the architecture audit to
use the implemented packet classes: replace ClientboundAddPlayerPacket with
ClientboundAddEntityPacket, and replace ClientboundRotateHeadPacket and
ClientboundTeleportEntityPacket with ClientboundMoveEntityPacket.Rot and
ClientboundMoveEntityPacket.PosRot. Keep the documented spawn sequence and
rotation responsibilities unchanged.
- Line 16: Align the NPC dependency contract by either implementing optional
handling for the "holograms" dependency, including guards around hologram calls,
or revising the audit and manual test plan to state that holograms is required.
Update the NPC registration and the related graceful-descope documentation
consistently.
In `@docs/engineering/npcs/02-protocol-and-rendering.md`:
- Around line 11-15: Update removePlayerInfo() in NpcPacketSenderImpl so the
UPDATE_LISTED Entry is constructed with listed=false, matching the documented
hidden-tab behavior. Add a packet-level assertion verifying the emitted update
marks the NPC profile as unlisted.
- Around line 17-22: Update the “Look Update (Per Viewer, Per Tick)”
documentation to match syncLookAtPlayers(): describe yaw as Math.atan2(-dx, dz)
and pitch as the negated Math.atan2(dy, hDist), then add examples clarifying the
signs for left/right and up/down look directions.
In `@docs/engineering/npcs/03-performance-model.md`:
- Around line 7-9: Enforce the documented maxDespawnsPerTick budget in
NpcManager.syncViewer by counting despawns and stopping once maxDespawnsPerTick
is reached, while preserving the existing visibility synchronization behavior
for remaining NPCs.
- Around line 10-12: Update the workload model table to calculate look updates
from visible viewer/NPC pairs: 50 players with five visible NPCs yields up to
250 pairs, capped at 200 updates and approximately 20 KB per tick at 100 bytes
each, or 25 KB without the cap. Replace “No N×M loop” with “No player × all
registered NPCs loop” and document the player-plus-visible-NPC iteration.
In `@docs/engineering/npcs/04-manual-test-plan.md`:
- Around line 55-61: Update TC-07 to explicitly make the cached skin entry stale
before disconnecting the internet and restarting the server, using a supported
test TTL, cache backdating, or clock injection. Keep the final /npc stats
assertion focused on skinStaleHits > 0.
- Around line 26-32: Update test case TC-03 to use the documented
despawnDistance of 56 blocks rather than treating the 48-block viewDistance as
the disappearance threshold; optionally verify that an already-visible NPC
remains visible between 48 and 56 blocks before asserting it disappears beyond
56.
- Around line 110-115: Split TC-15 into separate missing-file and corrupted-file
scenarios. Keep the existing delete-and-restart steps for verifying automatic
default config creation, and add a corrupted JSON scenario that replaces
npcs.json with malformed content, includes a valid backup file, restarts the
server, and verifies recovery from the backup.
In `@docs/modules/npcs.md`:
- Around line 42-56: Update NpcInteractionService.handleClick to enforce
NpcPermissions.USE and the corresponding USE_PREFIX per-NPC permission before
executing an interaction action, including when npc.interaction().permission()
is empty; preserve any configured interaction-specific permission checks and
deny the action when the required global or specific permission is missing.
- Around line 16-40: Update the NPC command permission wiring in
hasAnyAdminPermission and each command’s argument definitions: include MOVE,
SAVE, and EDIT in the admin-permission aggregate, and add
requires(Predicate<...>) checks matching each command’s documented permission
node. Ensure parent permissions do not grant access to child actions that
require ADMIN, particularly teleport, list, and info, while preserving the
documented ADMIN, EDIT, and STATS distinctions.
In
`@fabric/src/main/java/com/pedrodalben/bigbangessentials/npcs/FabricNpcInteractionBridge.java`:
- Around line 12-23: The NPC interaction bridges must intercept client
ServerboundInteractPacket handling before vanilla entity resolution, since
packet-only NPCs have no server Entity. In
fabric/src/main/java/com/pedrodalben/bigbangessentials/npcs/FabricNpcInteractionBridge.java:12-23
and
neoforge/src/main/java/com/pedrodalben/bigbangessentials/npcs/NeoForgeNpcInteractionBridge.java:10-18,
replace the entity callback integration with a server-side interact-packet
handler that extracts the clicked id, checks the player’s session for that NPC,
calls NpcInteractionService.handleClick, and cancels the interaction when
handled.
---
Outside diff comments:
In `@docs/engineering/npcs/02-protocol-and-rendering.md`:
- Around line 48-49: Update the UUID derivation example to pass a UTF-8 encoded
byte array to UUID.nameUUIDFromBytes instead of a String, matching the
implementation’s encoding and ensuring the documented example compiles.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a9802572-5ba1-4aff-97c7-42b70dfc717d
📒 Files selected for processing (40)
common/src/main/java/com/pedrodalben/bigbangessentials/BigBangEssentials.javacommon/src/main/java/com/pedrodalben/bigbangessentials/npcs/api/BigBangNpcs.javacommon/src/main/java/com/pedrodalben/bigbangessentials/npcs/api/NpcAction.javacommon/src/main/java/com/pedrodalben/bigbangessentials/npcs/api/NpcActionType.javacommon/src/main/java/com/pedrodalben/bigbangessentials/npcs/api/NpcDefinition.javacommon/src/main/java/com/pedrodalben/bigbangessentials/npcs/api/NpcHologramConfig.javacommon/src/main/java/com/pedrodalben/bigbangessentials/npcs/api/NpcInteractionConfig.javacommon/src/main/java/com/pedrodalben/bigbangessentials/npcs/api/NpcLocation.javacommon/src/main/java/com/pedrodalben/bigbangessentials/npcs/api/NpcLookSettings.javacommon/src/main/java/com/pedrodalben/bigbangessentials/npcs/api/NpcService.javacommon/src/main/java/com/pedrodalben/bigbangessentials/npcs/api/NpcSkin.javacommon/src/main/java/com/pedrodalben/bigbangessentials/npcs/api/NpcStats.javacommon/src/main/java/com/pedrodalben/bigbangessentials/npcs/command/NpcCommand.javacommon/src/main/java/com/pedrodalben/bigbangessentials/npcs/command/NpcPermissions.javacommon/src/main/java/com/pedrodalben/bigbangessentials/npcs/config/NpcConfig.javacommon/src/main/java/com/pedrodalben/bigbangessentials/npcs/config/NpcConfigStore.javacommon/src/main/java/com/pedrodalben/bigbangessentials/npcs/hologram/NpcHologramService.javacommon/src/main/java/com/pedrodalben/bigbangessentials/npcs/interaction/NpcInteractionService.javacommon/src/main/java/com/pedrodalben/bigbangessentials/npcs/render/NpcPacketSender.javacommon/src/main/java/com/pedrodalben/bigbangessentials/npcs/render/NpcPacketSenderImpl.javacommon/src/main/java/com/pedrodalben/bigbangessentials/npcs/render/NpcRenderService.javacommon/src/main/java/com/pedrodalben/bigbangessentials/npcs/render/NpcRenderSnapshot.javacommon/src/main/java/com/pedrodalben/bigbangessentials/npcs/render/NpcViewerService.javacommon/src/main/java/com/pedrodalben/bigbangessentials/npcs/render/NpcViewerSession.javacommon/src/main/java/com/pedrodalben/bigbangessentials/npcs/service/NpcManager.javacommon/src/main/java/com/pedrodalben/bigbangessentials/npcs/skin/MojangSkinResolver.javacommon/src/main/java/com/pedrodalben/bigbangessentials/npcs/skin/SkinCache.javacommon/src/main/java/com/pedrodalben/bigbangessentials/npcs/skin/SkinCacheEntry.javacommon/src/main/java/com/pedrodalben/bigbangessentials/npcs/spatial/NpcSpatialIndex.javacommon/src/test/java/com/pedrodalben/bigbangessentials/npcs/NpcModuleTest.javadocs/engineering/npcs/01-architecture-audit.mddocs/engineering/npcs/02-protocol-and-rendering.mddocs/engineering/npcs/03-performance-model.mddocs/engineering/npcs/04-manual-test-plan.mddocs/modules/npcs.mdfabric/src/main/java/com/pedrodalben/bigbangessentials/fabric/listener/FabricEvents.javafabric/src/main/java/com/pedrodalben/bigbangessentials/npcs/FabricNpcInteractionBridge.javaneoforge/src/main/java/com/pedrodalben/bigbangessentials/neoforge/NeoForgeModEntrypoint.javaneoforge/src/main/java/com/pedrodalben/bigbangessentials/neoforge/listener/NeoForgeEvents.javaneoforge/src/main/java/com/pedrodalben/bigbangessentials/npcs/NeoForgeNpcInteractionBridge.java
Comment on lines
+811
to
+817
| if (ModuleManager.getInstance().isActive("npcs")) try { | ||
| LOGGER.info("Shutting down NPC Module..."); | ||
| com.pedrodalben.bigbangessentials.npcs.service.NpcManager.getInstance().shutdown(); | ||
| } catch (Exception e) { | ||
| LOGGER.error("Failed to shutdown NPC Module", e); | ||
| } | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Shut down the NPC module before the holograms module.
registerModules declares npcs as dependent on holograms (line 247). Shutdown runs in the opposite order: holograms stop at lines 804-809, then NPCs at lines 811-816. NpcManager.shutdown does not call the hologram API today, so no failure occurs now. Move the NPC block above the holograms block to keep shutdown order the reverse of dependency order and to prevent breakage if NPC shutdown later touches holograms.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@common/src/main/java/com/pedrodalben/bigbangessentials/BigBangEssentials.java`
around lines 811 - 817, Move the NPC shutdown block identified by
NpcManager.getInstance().shutdown() above the holograms shutdown block in the
shutdown sequence. Preserve its active-module check, logging, and exception
handling, so dependents stop before the holograms module they rely on.
Comment on lines
+9
to
+12
| public NpcAction(NpcActionType type, String command) { | ||
| this.type = type != null ? type : NpcActionType.NONE; | ||
| this.command = command != null ? command.trim() : ""; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Normalize commands in the public constructor.
The constructor preserves a leading /, but playerCommand and consoleCommand remove it. A persistence loader or external caller can create a command with a different format from the factory output.
Proposed fix
- this.command = command != null ? command.trim() : "";
+ this.command = stripSlash(command);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@common/src/main/java/com/pedrodalben/bigbangessentials/npcs/api/NpcAction.java`
around lines 9 - 12, Update the public NpcAction constructor to normalize
command strings the same way as playerCommand and consoleCommand, including
removing any leading “/” after trimming. Preserve the existing null-to-empty
behavior and ensure constructor-created actions match factory output.
Comment on lines
+33
to
+34
| this.viewDistance = Math.max(1.0, viewDistance); | ||
| this.despawnDistance = Math.max(viewDistance, despawnDistance); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep despawnDistance at or above the normalized view distance.
When viewDistance is less than 1.0, Line 34 compares against the raw value. For example, inputs of 0.0 and 0.0 create viewDistance == 1.0 and despawnDistance == 0.0.
Proposed fix
this.viewDistance = Math.max(1.0, viewDistance);
- this.despawnDistance = Math.max(viewDistance, despawnDistance);
+ this.despawnDistance = Math.max(this.viewDistance, despawnDistance);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| this.viewDistance = Math.max(1.0, viewDistance); | |
| this.despawnDistance = Math.max(viewDistance, despawnDistance); | |
| this.viewDistance = Math.max(1.0, viewDistance); | |
| this.despawnDistance = Math.max(this.viewDistance, despawnDistance); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@common/src/main/java/com/pedrodalben/bigbangessentials/npcs/api/NpcDefinition.java`
around lines 33 - 34, Update the constructor normalization in NpcDefinition so
despawnDistance is clamped against the normalized this.viewDistance rather than
the raw viewDistance parameter. Preserve the existing minimum viewDistance of
1.0 and ensure despawnDistance never falls below it.
Comment on lines
+38
to
+44
| public static String normalizeId(String id) { | ||
| if (id == null) throw new IllegalArgumentException("NPC id cannot be null"); | ||
| String normalized = id.trim().toLowerCase(); | ||
| if (!VALID_ID.matcher(normalized).matches()) { | ||
| throw new IllegalArgumentException("Invalid NPC id: '" + id + "'. Must match [a-z0-9_-]{1,64}"); | ||
| } | ||
| return normalized; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use locale-independent ID normalization.
String.toLowerCase() uses the server default locale. Under a Turkish locale, an input such as I becomes ı and fails the ASCII ID pattern.
Proposed fix
import java.util.Objects;
+import java.util.Locale;
import java.util.regex.Pattern;
- String normalized = id.trim().toLowerCase();
+ String normalized = id.trim().toLowerCase(Locale.ROOT);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@common/src/main/java/com/pedrodalben/bigbangessentials/npcs/api/NpcDefinition.java`
around lines 38 - 44, Update NpcDefinition.normalizeId to lowercase IDs with a
locale-independent mechanism, such as an explicitly specified locale, while
preserving the existing trimming, validation, exception behavior, and normalized
return value.
Comment on lines
+15
to
+22
| public NpcHologramConfig(boolean enabled, List<String> lines, double offsetY, double viewDistance, boolean shadow, boolean seeThrough) { | ||
| this.enabled = enabled; | ||
| this.lines = Collections.unmodifiableList(new ArrayList<>(lines != null ? lines : List.of())); | ||
| this.offsetY = Math.max(0.0, offsetY); | ||
| this.viewDistance = Math.max(1.0, viewDistance); | ||
| this.shadow = shadow; | ||
| this.seeThrough = seeThrough; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not allow an enabled hologram with no lines.
This constructor permits an enabled empty configuration. NpcHologramService.createOrUpdate in common/src/main/java/com/pedrodalben/bigbangessentials/npcs/hologram/NpcHologramService.java:22-65 returns without deleting an existing hologram for that state. Normalize this state to disabled, or reject it.
Proposed fix
- this.enabled = enabled;
this.lines = Collections.unmodifiableList(new ArrayList<>(lines != null ? lines : List.of()));
+ this.enabled = enabled && !this.lines.isEmpty();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public NpcHologramConfig(boolean enabled, List<String> lines, double offsetY, double viewDistance, boolean shadow, boolean seeThrough) { | |
| this.enabled = enabled; | |
| this.lines = Collections.unmodifiableList(new ArrayList<>(lines != null ? lines : List.of())); | |
| this.offsetY = Math.max(0.0, offsetY); | |
| this.viewDistance = Math.max(1.0, viewDistance); | |
| this.shadow = shadow; | |
| this.seeThrough = seeThrough; | |
| } | |
| public NpcHologramConfig(boolean enabled, List<String> lines, double offsetY, double viewDistance, boolean shadow, boolean seeThrough) { | |
| this.lines = Collections.unmodifiableList(new ArrayList<>(lines != null ? lines : List.of())); | |
| this.enabled = enabled && !this.lines.isEmpty(); | |
| this.offsetY = Math.max(0.0, offsetY); | |
| this.viewDistance = Math.max(1.0, viewDistance); | |
| this.shadow = shadow; | |
| this.seeThrough = seeThrough; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@common/src/main/java/com/pedrodalben/bigbangessentials/npcs/api/NpcHologramConfig.java`
around lines 15 - 22, Update the NpcHologramConfig constructor to prevent an
enabled configuration when lines is null or empty: either normalize enabled to
false or reject the input, while preserving existing line copying and other
field normalization.
Comment on lines
+26
to
+32
| ### TC-03: Visibility | ||
| ``` | ||
| 1. Walk away from NPC beyond view distance (48 blocks) | ||
| 2. Expected: NPC disappears | ||
| 3. Walk back within view distance | ||
| 4. Expected: NPC reappears | ||
| ``` |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use the despawn distance in TC-03.
The documented defaults use viewDistance = 48 and despawnDistance = 56. An already-visible NPC should remain visible between those distances. Change the test to walk beyond 56 blocks, or explicitly verify retention between 48 and 56 before asserting disappearance.
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 26-26: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 27-27: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 27-27: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/engineering/npcs/04-manual-test-plan.md` around lines 26 - 32, Update
test case TC-03 to use the documented despawnDistance of 56 blocks rather than
treating the 48-block viewDistance as the disappearance threshold; optionally
verify that an already-visible NPC remains visible between 48 and 56 blocks
before asserting it disappears beyond 56.
Comment on lines
+55
to
+61
| ### TC-07: Skin cache persistence | ||
| ``` | ||
| 1. Disconnect internet | ||
| 2. Restart server | ||
| 3. Expected: NPC still shows correct skin from cache | ||
| 4. /npc stats — skinStaleHits > 0 | ||
| ``` |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Force a stale cache entry before checking skinStaleHits.
Disconnecting the internet and restarting does not make a fresh 24-hour cache entry stale. After TC-01, the entry is likely still fresh, so skinStaleHits can remain zero. Backdate the cache entry, use a test TTL, or inject a clock before asserting stale-cache behavior. (raw.githubusercontent.com)
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 55-55: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 56-56: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 56-56: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/engineering/npcs/04-manual-test-plan.md` around lines 55 - 61, Update
TC-07 to explicitly make the cached skin entry stale before disconnecting the
internet and restarting the server, using a supported test TTL, cache
backdating, or clock injection. Keep the final /npc stats assertion focused on
skinStaleHits > 0.
Comment on lines
+110
to
+115
| ### TC-15: Corrupted config | ||
| ``` | ||
| 1. Delete npcs.json | ||
| 2. Restart server | ||
| 3. Expected: Default config created automatically | ||
| ``` |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
Separate missing-file and corrupted-file tests.
TC-15 deletes npcs.json, which tests default-file creation. It does not test malformed JSON or backup recovery. Add a corrupted JSON step and a valid-backup step, then keep missing-file creation as a separate scenario. (raw.githubusercontent.com)
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 110-110: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 111-111: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 111-111: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/engineering/npcs/04-manual-test-plan.md` around lines 110 - 115, Split
TC-15 into separate missing-file and corrupted-file scenarios. Keep the existing
delete-and-restart steps for verifying automatic default config creation, and
add a corrupted JSON scenario that replaces npcs.json with malformed content,
includes a valid backup file, restarts the server, and verifies recovery from
the backup.
Comment on lines
+16
to
+40
| | Command | Permission | Description | | ||
| |---------|-----------|-------------| | ||
| | `/npc create <id> <skin>` | `bigbangessentials.npcs.create` | Create NPC at your position | | ||
| | `/npc remove <id>` | `bigbangessentials.npcs.remove` | Delete an NPC | | ||
| | `/npc movehere <id>` | `bigbangessentials.npcs.move` | Move NPC to your position | | ||
| | `/npc skin <id> <name>` | `bigbangessentials.npcs.edit` | Change NPC skin | | ||
| | `/npc name <id> <text>` | `bigbangessentials.npcs.edit` | Set display name | | ||
| | `/npc command <id> <cmd>` | `bigbangessentials.npcs.edit` | Set click command (player) | | ||
| | `/npc consolecommand <id> <cmd>` | `bigbangessentials.npcs.consolecommand` | Set click command (console) | | ||
| | `/npc hologram <id> on` | `bigbangessentials.npcs.edit` | Enable hologram | | ||
| | `/npc hologram <id> off` | `bigbangessentials.npcs.edit` | Disable hologram | | ||
| | `/npc hologram <id> addline <text>` | `bigbangessentials.npcs.edit` | Add hologram line | | ||
| | `/npc hologram <id> setline <n> <text>` | `bigbangessentials.npcs.edit` | Set hologram line | | ||
| | `/npc hologram <id> removeline <n>` | `bigbangessentials.npcs.edit` | Remove hologram line | | ||
| | `/npc look <id> on` | `bigbangessentials.npcs.edit` | Enable look-at-player | | ||
| | `/npc look <id> off` | `bigbangessentials.npcs.edit` | Disable look-at-player | | ||
| | `/npc enable <id>` | `bigbangessentials.npcs.edit` | Enable NPC | | ||
| | `/npc disable <id>` | `bigbangessentials.npcs.edit` | Disable NPC | | ||
| | `/npc teleport <id>` | `bigbangessentials.npcs.admin` | Teleport to NPC | | ||
| | `/npc info <id>` | `bigbangessentials.npcs.admin` | Show NPC details | | ||
| | `/npc list` | `bigbangessentials.npcs.admin` | List all NPCs | | ||
| | `/npc reload` | `bigbangessentials.npcs.reload` | Reload from config | | ||
| | `/npc save` | `bigbangessentials.npcs.save` | Force save to disk | | ||
| | `/npc stats` | `bigbangessentials.npcs.stats` | Show metrics | | ||
| | `/npcs` | (same) | Alias for `/npc` | |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C4 'hasAnyAdminPermission|\.requires|Commands\.literal' \
common/src/main/java/com/pedrodalben/bigbangessentials/npcs/command/NpcCommand.javaRepository: pedro-dalben/BigBangEssentials
Length of output: 11847
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the command builders and permission constants/API without executing repository code.
wc -l common/src/main/java/com/pedrodalben/bigbangessentials/npcs/command/NpcCommand.java
sed -n '28,440p' common/src/main/java/com/pedrodalben/bigbangessentials/npcs/command/NpcCommand.java
printf '\n--- Permission definitions ---\n'
rg -n 'CONSOLE_COMMAND|ADMIN|CREATE|REMOVE|MOVE|SAVE|STATS|LOAD|PERMISSION' common/src/main/java/com/pedrodalben/bigbangessentials/npcs -SRepository: pedro-dalben/BigBangEssentials
Length of output: 28929
Enforce permissions for each NPC command.
Add the missing MOVE, SAVE, and EDIT nodes to hasAnyAdminPermission, and add per-argument .requires(Predicate<P... checks for every command. ADMIN, EDIT, and STATS permissions currently route into child commands like teleport, list, and info that require ADMIN, so lower-privileged users reach admin-only actions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/modules/npcs.md` around lines 16 - 40, Update the NPC command permission
wiring in hasAnyAdminPermission and each command’s argument definitions: include
MOVE, SAVE, and EDIT in the admin-permission aggregate, and add
requires(Predicate<...>) checks matching each command’s documented permission
node. Ensure parent permissions do not grant access to child actions that
require ADMIN, particularly teleport, list, and info, while preserving the
documented ADMIN, EDIT, and STATS distinctions.
Comment on lines
+42
to
+56
| ## Permissions | ||
|
|
||
| ``` | ||
| bigbangessentials.npcs.admin - All NPC permissions | ||
| bigbangessentials.npcs.create - Create NPCs | ||
| bigbangessentials.npcs.remove - Remove NPCs | ||
| bigbangessentials.npcs.edit - Edit NPC properties | ||
| bigbangessentials.npcs.move - Move NPCs | ||
| bigbangessentials.npcs.reload - Reload configuration | ||
| bigbangessentials.npcs.save - Force save | ||
| bigbangessentials.npcs.stats - View metrics | ||
| bigbangessentials.npcs.consolecommand - Set console commands | ||
| bigbangessentials.npcs.use - Interact with NPCs | ||
| bigbangessentials.npcs.use.<npc-id> - Interact with specific NPC | ||
| ``` |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Enforce the documented interaction permissions.
NpcPermissions.USE and USE_PREFIX are documented, but NpcInteractionService.handleClick checks only npc.interaction().permission() when it is non-empty. With the example configuration's empty permission, the global and per-NPC nodes are never consulted. Add the global/specific permission checks before executing the action, or remove these nodes from the documentation and permission API. (raw.githubusercontent.com)
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 44-44: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/modules/npcs.md` around lines 42 - 56, Update
NpcInteractionService.handleClick to enforce NpcPermissions.USE and the
corresponding USE_PREFIX per-NPC permission before executing an interaction
action, including when npc.interaction().permission() is empty; preserve any
configured interaction-specific permission checks and deny the action when the
required global or specific permission is missing.
Comment on lines
+54
to
+69
| private static boolean hasAnyAdminPermission(CommandSourceStack src) { | ||
| if (src.hasPermission(4)) return true; | ||
| try { | ||
| ServerPlayer player = src.getPlayer(); | ||
| if (player != null) { | ||
| UUID uuid = player.getUUID(); | ||
| if (PermissionAPI.hasPermission(uuid, NpcPermissions.ADMIN)) return true; | ||
| if (PermissionAPI.hasPermission(uuid, NpcPermissions.CREATE)) return true; | ||
| if (PermissionAPI.hasPermission(uuid, NpcPermissions.REMOVE)) return true; | ||
| if (PermissionAPI.hasPermission(uuid, NpcPermissions.EDIT)) return true; | ||
| if (PermissionAPI.hasPermission(uuid, NpcPermissions.RELOAD)) return true; | ||
| if (PermissionAPI.hasPermission(uuid, NpcPermissions.STATS)) return true; | ||
| } | ||
| } catch (Exception ignored) {} | ||
| return false; | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
The permission check grants the whole command tree to any single NPC permission.
hasAnyAdminPermission returns true if the player holds ADMIN, CREATE, REMOVE, EDIT, RELOAD, or STATS. Only consolecommand adds its own requires check at lines 201-207. A player who holds only NpcPermissions.STATS can therefore run /npc create, /npc remove, /npc reload, and every edit subcommand. Separate NpcPermissions values exist, so the intent is per-operation authorization.
Add a requires clause to each subcommand builder that checks the matching permission.
🛡️ Proposed structure
+ private static java.util.function.Predicate<CommandSourceStack> requirePermission(String node) {
+ return src -> {
+ if (src.hasPermission(4)) return true;
+ try {
+ ServerPlayer p = src.getPlayer();
+ return p != null && (PermissionAPI.hasPermission(p.getUUID(), NpcPermissions.ADMIN)
+ || PermissionAPI.hasPermission(p.getUUID(), node));
+ } catch (Exception e) { return false; }
+ };
+ }Then apply it, for example:
private static com.mojang.brigadier.builder.LiteralArgumentBuilder<CommandSourceStack> createCmd() {
return Commands.literal("create")
+ .requires(requirePermission(NpcPermissions.CREATE))
.then(Commands.argument("id", StringArgumentType.word())Apply the same change to removeCmd (REMOVE), the edit subcommands (EDIT), reloadCmd (RELOAD), and statsCmd (STATS).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@common/src/main/java/com/pedrodalben/bigbangessentials/npcs/command/NpcCommand.java`
around lines 54 - 69, Restrict each NPC subcommand to its matching permission
instead of relying on hasAnyAdminPermission: add requires checks to createCmd
for CREATE, removeCmd for REMOVE, the edit subcommands for EDIT, reloadCmd for
RELOAD, and statsCmd for STATS. Preserve the existing console/admin access
behavior while preventing permissions for one operation from authorizing
unrelated commands.
Comment on lines
+81
to
+98
| .executes(ctx -> { | ||
| ServerPlayer p = player(ctx); | ||
| String id = StringArgumentType.getString(ctx, "id"); | ||
| String skinName = StringArgumentType.getString(ctx, "skin"); | ||
|
|
||
| NpcLocation loc = new NpcLocation(p.level().dimension().location(), | ||
| p.getX(), p.getY(), p.getZ(), p.getYRot(), p.getXRot()); | ||
| NpcDefinition def = new NpcDefinition(id, true, id, loc, NpcSkin.unresolved(skinName), | ||
| NpcAction.none(), NpcHologramConfig.defaults(id), NpcLookSettings.defaults(), 48.0, 56.0, NpcInteractionConfig.defaults()); | ||
| api().create(def); | ||
|
|
||
| p.sendSystemMessage(Component.literal("§aNPC '" + id + "' criado com sucesso.")); | ||
| p.sendSystemMessage(Component.literal("§7Use:")); | ||
| p.sendSystemMessage(Component.literal("§7- /npc name " + id + " <nome>")); | ||
| p.sendSystemMessage(Component.literal("§7- /npc command " + id + " warp end")); | ||
| p.sendSystemMessage(Component.literal("§7- /npc hologram " + id + " setline 1 <texto>")); | ||
| return 1; | ||
| }))); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
/npc create overwrites an existing NPC without warning.
api().create(def) calls NpcManager.create, which calls registerNpc and replaces any entry with the same id. A second /npc create shop <skin> therefore discards the existing display name, hologram lines, and action, and reports success. Reject the command when the id already exists.
🐛 Proposed fix
String skinName = StringArgumentType.getString(ctx, "skin");
+ if (api().find(id).isPresent()) {
+ ctx.getSource().sendSystemMessage(Component.literal("§cJá existe um NPC com o ID '" + id + "'."));
+ return 0;
+ }
NpcLocation loc = new NpcLocation(p.level().dimension().location(),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| .executes(ctx -> { | |
| ServerPlayer p = player(ctx); | |
| String id = StringArgumentType.getString(ctx, "id"); | |
| String skinName = StringArgumentType.getString(ctx, "skin"); | |
| NpcLocation loc = new NpcLocation(p.level().dimension().location(), | |
| p.getX(), p.getY(), p.getZ(), p.getYRot(), p.getXRot()); | |
| NpcDefinition def = new NpcDefinition(id, true, id, loc, NpcSkin.unresolved(skinName), | |
| NpcAction.none(), NpcHologramConfig.defaults(id), NpcLookSettings.defaults(), 48.0, 56.0, NpcInteractionConfig.defaults()); | |
| api().create(def); | |
| p.sendSystemMessage(Component.literal("§aNPC '" + id + "' criado com sucesso.")); | |
| p.sendSystemMessage(Component.literal("§7Use:")); | |
| p.sendSystemMessage(Component.literal("§7- /npc name " + id + " <nome>")); | |
| p.sendSystemMessage(Component.literal("§7- /npc command " + id + " warp end")); | |
| p.sendSystemMessage(Component.literal("§7- /npc hologram " + id + " setline 1 <texto>")); | |
| return 1; | |
| }))); | |
| .executes(ctx -> { | |
| ServerPlayer p = player(ctx); | |
| String id = StringArgumentType.getString(ctx, "id"); | |
| String skinName = StringArgumentType.getString(ctx, "skin"); | |
| if (api().find(id).isPresent()) { | |
| ctx.getSource().sendSystemMessage(Component.literal("§cJá existe um NPC com o ID '" + id + "'.")); | |
| return 0; | |
| } | |
| NpcLocation loc = new NpcLocation(p.level().dimension().location(), | |
| p.getX(), p.getY(), p.getZ(), p.getYRot(), p.getXRot()); | |
| NpcDefinition def = new NpcDefinition(id, true, id, loc, NpcSkin.unresolved(skinName), | |
| NpcAction.none(), NpcHologramConfig.defaults(id), NpcLookSettings.defaults(), 48.0, 56.0, NpcInteractionConfig.defaults()); | |
| api().create(def); | |
| p.sendSystemMessage(Component.literal("§aNPC '" + id + "' criado com sucesso.")); | |
| p.sendSystemMessage(Component.literal("§7Use:")); | |
| p.sendSystemMessage(Component.literal("§7- /npc name " + id + " <nome>")); | |
| p.sendSystemMessage(Component.literal("§7- /npc command " + id + " warp end")); | |
| p.sendSystemMessage(Component.literal("§7- /npc hologram " + id + " setline 1 <texto>")); | |
| return 1; | |
| }))); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@common/src/main/java/com/pedrodalben/bigbangessentials/npcs/command/NpcCommand.java`
around lines 81 - 98, Update the `/npc create` execution block to check whether
the requested id already exists before constructing or passing the definition to
`api().create(def)`. Reject duplicate ids with an appropriate failure message
and return without overwriting the existing NPC; retain the current creation and
success-message flow for new ids.
|
|
||
| if (config.lines().isEmpty()) return; | ||
|
|
||
| var existing = api.findDefinition(holId); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Remove the unused local variable existing.
api.findDefinition(holId) result is never read. The call adds no behavior. Either use the existing definition to skip redundant writes, or delete the lookup.
♻️ Proposed cleanup
- var existing = api.findDefinition(holId);
HologramDefinition definition;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| var existing = api.findDefinition(holId); |
🧰 Tools
🪛 PMD (7.26.0)
[Medium] 36-36: UnusedLocalVariable (Best Practices): Avoid unused local variables such as 'existing'.
(UnusedLocalVariable (Best Practices))
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@common/src/main/java/com/pedrodalben/bigbangessentials/npcs/hologram/NpcHologramService.java`
at line 36, Remove the unused existing local and its api.findDefinition(holId)
lookup from the surrounding NpcHologramService method, since the result is never
consumed and the call adds no behavior.
Source: Linters/SAST tools
Comment on lines
+72
to
+81
| public void cleanupOrphans(Map<String, NpcDefinition> knownNpcs) { | ||
| HologramService api = BigBangHolograms.getApi(); | ||
| if (api == null) return; | ||
|
|
||
| int removed = api.deleteByOwner(NPC_OWNER); | ||
|
|
||
| for (NpcDefinition npc : knownNpcs.values()) { | ||
| createOrUpdate(npc); | ||
| } | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Delete only orphaned holograms instead of all NPC-owned holograms.
cleanupOrphans calls api.deleteByOwner(NPC_OWNER), which deletes every hologram owned by the NPC module, then recreates one for each known NPC. BigBangHologramsManager.deleteByOwner (see common/src/main/java/com/pedrodalben/bigbangessentials/holograms/service/BigBangHologramsManager.java lines 265-280) deletes each hologram individually, so every call performs a full delete plus recreate cycle. NpcManager.initialize (line 76) and NpcManager.reload (line 262) both call this method, so a reload destroys and rebuilds all NPC holograms. Players in range observe hologram removal and respawn, and the hologram store is rewritten for every NPC.
Compute the orphan set first, and delete only ids that do not map to a known NPC. The removed count is also discarded; log it or drop the variable.
♻️ Proposed fix
public void cleanupOrphans(Map<String, NpcDefinition> knownNpcs) {
HologramService api = BigBangHolograms.getApi();
if (api == null) return;
- int removed = api.deleteByOwner(NPC_OWNER);
-
+ Set<String> expected = new HashSet<>();
+ for (String npcId : knownNpcs.keySet()) {
+ expected.add(hologramId(npcId));
+ }
+ int removed = 0;
+ for (HologramDefinition def : api.getDefinitions()) {
+ if (NPC_OWNER.equals(def.ownerId()) && !expected.contains(def.id())) {
+ api.delete(def.id());
+ removed++;
+ }
+ }
+ if (removed > 0) {
+ LOGGER.info("Removed {} orphaned NPC hologram(s)", removed);
+ }
for (NpcDefinition npc : knownNpcs.values()) {
createOrUpdate(npc);
}
}🧰 Tools
🪛 PMD (7.26.0)
[Medium] 76-76: UnusedLocalVariable (Best Practices): Avoid unused local variables such as 'removed'.
(UnusedLocalVariable (Best Practices))
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@common/src/main/java/com/pedrodalben/bigbangessentials/npcs/hologram/NpcHologramService.java`
around lines 72 - 81, Update cleanupOrphans to compare existing NPC-owned
hologram IDs with knownNpcs before deleting, and remove only holograms whose IDs
do not correspond to a known NPC; retain createOrUpdate for known definitions.
Eliminate the unused removed result or log the deletion count.
Source: Linters/SAST tools
Comment on lines
+97
to
+105
| private void executeConsoleCommand(String command, ServerPlayer player) { | ||
| if (command.isEmpty()) return; | ||
| try { | ||
| MinecraftServer server = player.getServer(); | ||
| if (server != null) { | ||
| String resolved = command.replace("{player}", player.getGameProfile().getName()); | ||
| server.getCommands().performPrefixedCommand( | ||
| server.createCommandSourceStack(), resolved); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Validate the substituted player name before running an op-level command.
server.createCommandSourceStack() runs with permission level 4. Line 102 substitutes the raw profile name into that command string. If a name contains a space or a command separator, the extra text becomes additional arguments of an op-level command. Vanilla name validation blocks this, but offline-mode servers and proxy-supplied profiles can present other names. Reject or escape names outside [A-Za-z0-9_]{1,16}.
🛡️ Proposed hardening
+ private static final java.util.regex.Pattern SAFE_NAME =
+ java.util.regex.Pattern.compile("[A-Za-z0-9_]{1,16}");
+
private void executeConsoleCommand(String command, ServerPlayer player) {
if (command.isEmpty()) return;
try {
MinecraftServer server = player.getServer();
if (server != null) {
- String resolved = command.replace("{player}", player.getGameProfile().getName());
+ String name = player.getGameProfile().getName();
+ if (command.contains("{player}") && !SAFE_NAME.matcher(name).matches()) {
+ LOGGER.warn("Skipping NPC console command for unsafe player name '{}'", name);
+ return;
+ }
+ String resolved = command.replace("{player}", name);
server.getCommands().performPrefixedCommand(
server.createCommandSourceStack(), resolved);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private void executeConsoleCommand(String command, ServerPlayer player) { | |
| if (command.isEmpty()) return; | |
| try { | |
| MinecraftServer server = player.getServer(); | |
| if (server != null) { | |
| String resolved = command.replace("{player}", player.getGameProfile().getName()); | |
| server.getCommands().performPrefixedCommand( | |
| server.createCommandSourceStack(), resolved); | |
| } | |
| private static final java.util.regex.Pattern SAFE_NAME = | |
| java.util.regex.Pattern.compile("[A-Za-z0-9_]{1,16}"); | |
| private void executeConsoleCommand(String command, ServerPlayer player) { | |
| if (command.isEmpty()) return; | |
| try { | |
| MinecraftServer server = player.getServer(); | |
| if (server != null) { | |
| String name = player.getGameProfile().getName(); | |
| if (command.contains("{player}") && !SAFE_NAME.matcher(name).matches()) { | |
| LOGGER.warn("Skipping NPC console command for unsafe player name '{}'", name); | |
| return; | |
| } | |
| String resolved = command.replace("{player}", name); | |
| server.getCommands().performPrefixedCommand( | |
| server.createCommandSourceStack(), resolved); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@common/src/main/java/com/pedrodalben/bigbangessentials/npcs/interaction/NpcInteractionService.java`
around lines 97 - 105, Validate the player name in executeConsoleCommand before
substituting it into the privileged command, accepting only 1–16 characters
matching [A-Za-z0-9_]. Reject invalid names without invoking
server.getCommands().performPrefixedCommand, while preserving the existing
substitution and execution flow for valid names.
Comment on lines
+164
to
+172
| if (!old.skin().playerName().equals(definition.skin().playerName())) { | ||
| String skinName = definition.skin().playerName(); | ||
| skinCache.resolve(skinName).thenAccept(entry -> { | ||
| NpcDefinition withSkin = npcs.get(id).withSkin( | ||
| new NpcSkin(skinName, entry.uuid(), entry.textureValue(), entry.textureSignature(), entry.model(), entry.fetchedAt())); | ||
| npcs.put(id, withSkin); | ||
| invalidateViewersForNpc(id); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Guard the asynchronous skin callback: it mutates npcs off the server thread and can dereference null.
skinCache.resolve(...).thenAccept(...) runs on a cache/HTTP thread. The callback writes to npcs, which is a plain LinkedHashMap protected everywhere else by synchronized methods. Two defects follow:
- Unsynchronized mutation of
LinkedHashMapraces withtick,list,save, andreload, and can corrupt the map. npcs.get(id)returns null if the NPC was deleted or reloaded before the skin resolved, so.withSkin(...)throws aNullPointerExceptioninside the callback and the failure is swallowed by the future.
Also invalidateViewersForNpc sends packets from that thread. Route the callback back through a synchronized manager method, and skip the update when the NPC no longer exists.
🐛 Proposed fix
if (!old.skin().playerName().equals(definition.skin().playerName())) {
String skinName = definition.skin().playerName();
- skinCache.resolve(skinName).thenAccept(entry -> {
- NpcDefinition withSkin = npcs.get(id).withSkin(
- new NpcSkin(skinName, entry.uuid(), entry.textureValue(), entry.textureSignature(), entry.model(), entry.fetchedAt()));
- npcs.put(id, withSkin);
- invalidateViewersForNpc(id);
- });
+ skinCache.resolve(skinName).thenAccept(entry -> applyResolvedSkin(id, skinName, entry));
}Add the synchronized handler:
private synchronized void applyResolvedSkin(String id, String skinName, SkinCacheEntry entry) {
NpcDefinition current = npcs.get(id);
if (current == null || !skinName.equals(current.skin().playerName())) return;
npcs.put(id, current.withSkin(new NpcSkin(skinName, entry.uuid(), entry.textureValue(),
entry.textureSignature(), entry.model(), entry.fetchedAt())));
renderService.register(npcs.get(id));
invalidateViewersForNpc(id);
}Confirm that invalidateViewersForNpc and renderService packet sends are safe from a non-main thread. If they are not, schedule the body with MinecraftServer.execute.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@common/src/main/java/com/pedrodalben/bigbangessentials/npcs/service/NpcManager.java`
around lines 164 - 172, Replace the direct skin-resolution mutation inside the
asynchronous callback with a synchronized manager handler such as
applyResolvedSkin, passing the NPC id, skin name, and resolved entry. In that
handler, look up the current NPC, skip when it was deleted/reloaded or its skin
name no longer matches, then update npcs and perform viewer invalidation (and
render registration if required). Ensure the update and packet sends run on the
server thread via MinecraftServer.execute when those operations are not
thread-safe.
Comment on lines
+210
to
+267
| public synchronized void reload() { | ||
| long start = System.currentTimeMillis(); | ||
| NpcConfig newConfig = configStore.load(); | ||
|
|
||
| skinCache.configure(newConfig.freshTtlHours(), newConfig.staleTtlDays(), newConfig.negativeCacheMinutes(), | ||
| newConfig.maxConcurrentRequests(), newConfig.connectTimeoutMillis(), newConfig.requestTimeoutMillis()); | ||
|
|
||
| this.config = newConfig; | ||
|
|
||
| Map<String, NpcDefinition> newNpcs = newConfig.npcs(); | ||
| Set<String> added = new HashSet<>(newNpcs.keySet()); | ||
| added.removeAll(npcs.keySet()); | ||
| Set<String> removed = new HashSet<>(npcs.keySet()); | ||
| removed.removeAll(newNpcs.keySet()); | ||
| int updated = 0; | ||
| int unchanged = 0; | ||
|
|
||
| for (String id : removed) { | ||
| NpcDefinition def = npcs.remove(id); | ||
| renderService.unregister(id); | ||
| spatialIndex.remove(id); | ||
| hologramService.remove(id); | ||
| MinecraftServer server = Platform.getCurrentServer(); | ||
| if (server != null && def != null) { | ||
| for (ServerPlayer player : server.getPlayerList().getPlayers()) { | ||
| NpcViewerSession session = viewerService.getSession(player.getUUID()); | ||
| if (session != null && session.visibleNpcIds().remove(id)) { | ||
| renderService.despawn(player, def); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| for (var entry : newNpcs.entrySet()) { | ||
| if (!npcs.containsKey(entry.getKey())) { | ||
| registerNpc(entry.getValue()); | ||
| hologramService.createOrUpdate(entry.getValue()); | ||
| continue; | ||
| } | ||
| NpcDefinition old = npcs.get(entry.getKey()); | ||
| if (!entry.getValue().equals(old)) { | ||
| registerNpc(entry.getValue()); | ||
| hologramService.createOrUpdate(entry.getValue()); | ||
| if (skinChanged(old, entry.getValue()) || locChanged(old, entry.getValue())) { | ||
| invalidateViewersForNpc(entry.getKey()); | ||
| } | ||
| updated++; | ||
| } else { | ||
| unchanged++; | ||
| } | ||
| } | ||
|
|
||
| hologramService.cleanupOrphans(npcs); | ||
| lastReloadMillis = System.currentTimeMillis() - start; | ||
|
|
||
| LOGGER.info("NPC reload complete: {} added, {} removed, {} updated, {} unchanged, {} invalid in {}ms", | ||
| added.size(), removed.size(), updated, unchanged, 0, lastReloadMillis); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
reload dereferences skinCache and renderService without a null check.
initialize assigns skinCache, renderService, and interactionService. If initialize failed, BigBangEssentials.GameEvents.onServerStarting logs the failure and marks the module failed, but ModuleManager.isActive still reports the module as active for command registration, so /npc reload remains reachable. Line 214 then throws a NullPointerException. Add an initialization guard at the start of reload, save, and delete.
🛡️ Proposed guard
public synchronized void reload() {
+ if (!initialized) {
+ LOGGER.warn("NPC reload requested before initialization completed");
+ return;
+ }
long start = System.currentTimeMillis();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@common/src/main/java/com/pedrodalben/bigbangessentials/npcs/service/NpcManager.java`
around lines 210 - 267, Add an initialization guard at the start of
NpcManager.reload, save, and delete that detects unavailable required services,
including skinCache and renderService, and returns safely before dereferencing
them. Ensure the guard consistently handles commands invoked after initialize
fails while preserving normal behavior when initialization succeeded.
Comment on lines
+312
to
+322
| public synchronized void shutdown() { | ||
| shuttingDown = true; | ||
| if (skinCache != null) skinCache.shutdown(); | ||
| viewerService.clear(); | ||
|
|
||
| MinecraftServer server = Platform.getCurrentServer(); | ||
| if (server != null) { | ||
| for (ServerPlayer player : server.getPlayerList().getPlayers()) { | ||
| onPlayerLeave(player); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
viewerService.clear() runs before the per-player cleanup, so the cleanup does nothing.
Line 315 clears all sessions. onPlayerLeave then calls viewerService.removeSession(...), receives null, and returns at line 108. No despawn packet is sent, and interactionService.clearCooldowns never runs. Move viewerService.clear() after the player loop.
🐛 Proposed fix
shuttingDown = true;
if (skinCache != null) skinCache.shutdown();
- viewerService.clear();
MinecraftServer server = Platform.getCurrentServer();
if (server != null) {
for (ServerPlayer player : server.getPlayerList().getPlayers()) {
onPlayerLeave(player);
}
}
+ viewerService.clear();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public synchronized void shutdown() { | |
| shuttingDown = true; | |
| if (skinCache != null) skinCache.shutdown(); | |
| viewerService.clear(); | |
| MinecraftServer server = Platform.getCurrentServer(); | |
| if (server != null) { | |
| for (ServerPlayer player : server.getPlayerList().getPlayers()) { | |
| onPlayerLeave(player); | |
| } | |
| } | |
| public synchronized void shutdown() { | |
| shuttingDown = true; | |
| if (skinCache != null) skinCache.shutdown(); | |
| MinecraftServer server = Platform.getCurrentServer(); | |
| if (server != null) { | |
| for (ServerPlayer player : server.getPlayerList().getPlayers()) { | |
| onPlayerLeave(player); | |
| } | |
| } | |
| viewerService.clear(); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@common/src/main/java/com/pedrodalben/bigbangessentials/npcs/service/NpcManager.java`
around lines 312 - 322, Move viewerService.clear() in shutdown() to after the
server player loop so onPlayerLeave(player) can remove each active session, send
despawn packets, and clear interaction cooldowns before the remaining viewer
state is cleared.
Comment on lines
+358
to
+360
| int radius = (int) Math.ceil(Math.max(config.defaultViewDistance(), 64.0)); | ||
| Set<String> candidates = spatialIndex.query( | ||
| player.serverLevel().dimension().location(), player.getX(), player.getZ(), radius); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The candidate query radius ignores per-NPC view distance.
The radius derives only from config.defaultViewDistance() and a 64 block floor. Line 369 then compares against npc.viewDistance(), and line 372 against npc.despawnDistance(). create uses 48.0 and 56.0, but an operator can configure larger per-NPC values in the NPC config file. Any NPC whose viewDistance or despawnDistance exceeds the query radius never appears in candidates, so it never spawns, and it never despawns through this path once out of the radius.
Derive the radius from the largest configured NPC distance.
🐛 Proposed fix
- int radius = (int) Math.ceil(Math.max(config.defaultViewDistance(), 64.0));
+ double maxDistance = Math.max(config.defaultViewDistance(), 64.0);
+ for (NpcDefinition npc : npcs.values()) {
+ maxDistance = Math.max(maxDistance, Math.max(npc.viewDistance(), npc.despawnDistance()));
+ }
+ int radius = (int) Math.ceil(maxDistance);Cache the maximum on registration and removal if the NPC count is large.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| int radius = (int) Math.ceil(Math.max(config.defaultViewDistance(), 64.0)); | |
| Set<String> candidates = spatialIndex.query( | |
| player.serverLevel().dimension().location(), player.getX(), player.getZ(), radius); | |
| double maxDistance = Math.max(config.defaultViewDistance(), 64.0); | |
| for (NpcDefinition npc : npcs.values()) { | |
| maxDistance = Math.max(maxDistance, Math.max(npc.viewDistance(), npc.despawnDistance())); | |
| } | |
| int radius = (int) Math.ceil(maxDistance); | |
| Set<String> candidates = spatialIndex.query( | |
| player.serverLevel().dimension().location(), player.getX(), player.getZ(), radius); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@common/src/main/java/com/pedrodalben/bigbangessentials/npcs/service/NpcManager.java`
around lines 358 - 360, Update the radius calculation in the NPC candidate-query
flow to use the largest configured distance across registered NPCs, considering
both each NPC’s viewDistance() and despawnDistance(), while retaining the
existing default/floor behavior when appropriate. Ensure the maximum is updated
when NPCs are registered or removed, using the relevant NpcManager
registration/removal methods and avoiding stale values.
Comment on lines
+12
to
+23
| public static void register() { | ||
| UseEntityCallback.EVENT.register((player, world, hand, entity, hitResult) -> { | ||
| if (world.isClientSide() || !(player instanceof ServerPlayer sp) || hand != InteractionHand.MAIN_HAND) { | ||
| return InteractionResult.PASS; | ||
| } | ||
| var svc = NpcManager.getInstance().getInteractionService(); | ||
| if (svc != null && svc.handleClick(sp, entity.getId())) { | ||
| return InteractionResult.SUCCESS; | ||
| } | ||
| return InteractionResult.PASS; | ||
| }); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -t f 'NpcPacketSenderImpl.java' -x cat -n {}
rg -n 'ServerboundInteractPacket|addFreshEntity|ClientboundAddEntityPacket' --type=javaRepository: pedro-dalben/BigBangEssentials
Length of output: 5376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate NPC files =="
fd -t f 'Npc|npc|Npc.*\.java|npc.*\.java' . | sed -n '1,120p'
echo
echo "== NPC packet sender interface/"
fd -t f 'NpcPacketSender' . -x sh -c 'echo "--- $1"; ast-grep outline "$1" --view expanded || true' sh {}
echo
echo "== interaction callbacks/bridges =="
fd -t f 'FabricNpcInteractionBridge|NeoForgeNpcInteractionBridge|Npc.*Interaction.*\.java|.*Interaction.*\.java' . -x sh -c 'echo "--- $1"; ast-grep outline "$1" --view expanded || true; sed -n "1,120p" "$1"' sh {}
echo
echo "== ServerboundInteractPacket references =="
rg -n 'ServerboundInteractPacket|InteractEntity|EntityInteractSpecific|EntityInteract|handleInteract|setCanceled|IsClientSide|handle|register' --type=java -SRepository: pedro-dalben/BigBangEssentials
Length of output: 44765
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== NpcRenderService outline/contents =="
fd -t f 'NpcRenderService.java' . -x sh -c 'echo "--- $1"; wc -l "$1"; ast-grep outline "$1" --view expanded || true; cat -n "$1"' sh {}
echo
echo "== NpcViewerService outline/contents =="
fd -t f 'NpcViewerService.java' . -x sh -c 'echo "--- $1"; wc -l "$1"; ast-grep outline "$1" --view expanded || true; cat -n "$1"' sh {}
echo
echo "== NpcManager outline/contents =="
fd -t f 'NpcManager.java' . -x sh -c 'echo "--- $1"; wc -l "$1"; ast-grep outline "$1" --view expanded || true; cat -n "$1"' sh {}
echo
echo "== broad ClientboundPlayerInfoUpdatePacket / NpcRenderService references =="
rg -n 'ClientboundPlayerInfoUpdatePacket|NpcRenderService|npcIdToEntity|entityIdToNpc|handleClick|registerInteraction|addPlayerInfo|teleportEntity|rotateHead|removeEntities' --type=java -SRepository: pedro-dalben/BigBangEssentials
Length of output: 36123
Handle NPC clicks before vanilla entity resolution. NPCs are spawned from packet data only: ClientboundPlayerInfoUpdatePacket, ClientboundAddEntityPacket, ClientboundSetEntityDataPacket, and entity motion data. The server does not create Entity objects for these ids, so UseEntityCallback/PlayerInteractEvent.EntityInteractSpecific cannot receive the NPC entity. Intercept the client click on the server side with a ServerboundInteractPacket handler, then call NpcInteractionService.handleClick using the NPC id from the session and cancel the interaction if handled.
📍 Affects 2 files
fabric/src/main/java/com/pedrodalben/bigbangessentials/npcs/FabricNpcInteractionBridge.java#L12-L23(this comment)neoforge/src/main/java/com/pedrodalben/bigbangessentials/npcs/NeoForgeNpcInteractionBridge.java#L10-L18
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@fabric/src/main/java/com/pedrodalben/bigbangessentials/npcs/FabricNpcInteractionBridge.java`
around lines 12 - 23, The NPC interaction bridges must intercept client
ServerboundInteractPacket handling before vanilla entity resolution, since
packet-only NPCs have no server Entity. In
fabric/src/main/java/com/pedrodalben/bigbangessentials/npcs/FabricNpcInteractionBridge.java:12-23
and
neoforge/src/main/java/com/pedrodalben/bigbangessentials/npcs/NeoForgeNpcInteractionBridge.java:10-18,
replace the entity callback integration with a server-side interact-packet
handler that extracts the clicked id, checks the player’s session for that NPC,
calls NpcInteractionService.handleClick, and cancels the interaction when
handled.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
/npcand/npcsadministration commands for creating, editing, moving, enabling, teleporting, listing, reloading, saving, and viewing statistics.