fancynpcs-model: fix custom model desync on Folia respawn and /bettermodel reload - #308
Open
imBuzz wants to merge 8 commits into
Conversation
…eload On Folia, npcs are despawned/respawned per player based on distance (with an extra respawn cycle to work around a known Folia visibility bug in FancyNpcs core), but fancynpcs-model only ever attached the BetterModel tracker to online players once, at the moment the custom_model attribute was set. Players joining later, or walking back into range, never got the model re-spawned for them. - Add NpcSpawnEvent/NpcDespawnEvent listeners in fancynpcs-model to keep BetterModel's per-player tracker state in sync with FancyNpcs' own visibility tracking, dispatched through the player's own region scheduler (required by BetterModel's Folia-aware API). - Add NpcDespawnEvent to fancynpcs-v2 core, fired from Npc#checkAndUpdateVisibility, mirroring the existing NpcSpawnEvent. - Also fix hitbox click listeners (right/left click actions) being lost after /bettermodel reload recreates EntityTracker instances, by re-registering them via BetterModel's CreateEntityTrackerEvent.
reloadNpcs() clears and recreates every npc from scratch, but never notified addons that the old npc instances were being discarded - only NpcRemoveEvent (permanent deletion via /npc remove) did. This left any per-npc external state (e.g. fancynpcs-model's BetterModel trackers) orphaned on every /fancynpcs reload, which could then interfere with a subsequent /bettermodel reload iterating those dangling entries. fancynpcs-model now closes its trackers on NpcUnloadEvent the same way it already does for NpcRemoveEvent.
setModel() already re-runs on every single spawn()/update() cycle via NpcData#applyAllAttributes (called unconditionally from Npc#update, which every spawn() call ends with) - it was just missing the Folia-safe scheduler wrapping, which is what actually needed fixing and was already done directly in setModel()'s own spawn loop. The NpcSpawnEvent/NpcDespawnEvent listeners added on top of that were therefore redundant, and actively harmful: they captured a reference to the current tracker and deferred a spawn() call to the player's next scheduler tick, but setModel() closes and replaces that same tracker synchronously, moments later, in the same update() call. The deferred call then either landed on an already-closed tracker (silently doing nothing - models missing after a world change until a manual /bettermodel reload) or raced setModel()'s own tracker recreation before the npc's correct yaw was applied via move() (models stuck facing the tracker's creation-time default rotation). Removing the redundant listeners removes the race; setModel()'s own (now thread-safe) resync on every respawn is sufficient on its own.
Author
|
Update: removed the `NpcSpawnEvent`/`NpcDespawnEvent` tracker-sync listeners added earlier in this branch - they turned out to be redundant with (and racy against) `NpcData#applyAllAttributes`, which already re-runs `setModel()` on every `spawn()`/`update()` cycle. The Folia thread-safety fix inside `setModel()`'s own spawn loop was the actual fix needed; the extra listeners caused a race (deferred spawn call landing on an already-replaced tracker) that showed up as models occasionally missing after a world change, and models stuck facing the tracker's creation-time default rotation. `NpcUnloadEvent` (for the `/fancynpcs reload` orphaned-tracker leak) and the `CreateEntityTrackerEvent` hook (for `/bettermodel reload`) are unaffected and still included. |
setModel() re-applies the model attribute on every respawn (via NpcData#applyAllAttributes, called from Npc#update on every spawn()), including the scale line: tracker.scaler(tracker.scaler().multiply(scale)). Since this multiplies the tracker's *current* scaler instead of setting an absolute value, each re-application compounds on top of the last - npcs with a non-default scale grow exponentially the longer the server runs and the more respawn cycles occur (world changes, players joining, Folia's visibility-fix respawn), eventually blowing up BetterModel's hitbox size enough to make its per-tick collision check (BlockGetter#forEachBlockIntersectedBetween) hang the main thread for 10+ seconds and crash the server. Use ModelScaler.value(scale) instead, which is idempotent - safe to re-apply on every respawn.
The model has occasionally been observed not showing up for a player on the first respawn attempt (server restart, world change) with nothing logged - so the failure is happening somewhere inside BetterModel itself, not in a path we control or can fix directly. A second respawn cycle (rejoin, change world again) always recovers it. Schedule a second registry.spawn() call ~2s after the first as a cheap, safe mitigation: spawn() is idempotent, and registry is the stable per-entity object (not tied to a specific tracker instance), so this naturally picks up whatever the current tracker is even if it changed in the meantime.
Npc#update() calls NpcData#applyAllAttributes (which re-runs setModel(), creating a fresh BetterModel tracker) before it calls Npc#move() (which sets the npc's configured yaw/pitch on its underlying fake entity). Since BetterModel reads the entity's live rotation when a tracker is created, the model would spawn facing whatever stale/default rotation the entity still had, only fixed by something later forcing a fresh tracker after move() had already run (e.g. /bettermodel reload). Set the entity's rotation from the npc's configured location ourselves, right before creating the tracker, instead of relying on core to have already done it by this point.
…rors The delayed second registry.spawn() call, added as a speculative fix for models occasionally not appearing, is the likely cause of a client-side crash: "Invalid entity data item type for field 9 on entity Item Display ... old=Integer, new=Float". Two spawn() calls close together for the same viewer appear to send inconsistent entity metadata for the model's display-entity bones, desyncing the client's cached field types for that entity id. The mitigation was speculative and unproven to begin with; a client crash is worse than the rare cosmetic issue it was meant to paper over.
setModel() re-runs on every respawn with (almost always) the same model name, but unconditionally closed and recreated the BetterModel tracker every single time regardless. That tears down and immediately rebuilds the model's display-entity bones, racing BetterModel's own tracker close - observed causing a client-side crash (IllegalStateException: "Invalid entity data item type ... old=Integer, new=Float" on an Item Display entity) when a respawn happened shortly after another. This was not fixed by removing the earlier delayed-retry mitigation, since setModel() already re-runs frequently on its own via NpcData#applyAllAttributes. getOrCreate() already returns the existing tracker for a model if one exists, so only close (and only re-register hitbox listeners, which have no de-duplication) when actually switching to a different model.
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
Two related fixes for
fancynpcs-model:fancynpcs-modelonly ever attached the BetterModel tracker to online players once, at the moment thecustom_modelattribute was set. On Folia, npcs are despawned/respawned per player based on distance (with an extra respawn 100ms later to work around a known Folia visibility bug in FancyNpcs core), but nothing re-attached the model for a player who joined later or walked back into range - so the base npc reappeared but its custom model didn't. AddedNpcSpawnEvent/NpcDespawnEventlisteners to keep BetterModel's per-player tracker state in sync, dispatched through the player's own region scheduler (calling BetterModel's tracker API off the correct region thread throws on Folia)./bettermodel reload: hitbox click listeners (right/left click npc actions) were only ever registered once on theEntityTrackercreated insetModel()./bettermodel reloaddiscards and recreatesEntityTrackerinstances, silently dropping those listeners. Now re-registered via BetterModel'sCreateEntityTrackerEvent.Changes
fancynpcs-v2: addNpcDespawnEvent, fired fromNpc#checkAndUpdateVisibility(mirrors the existingNpcSpawnEvent), since core had no way to notify listeners when a npc is despawned for a specific player.fancynpcs-model:NpcSpawnListener/NpcDespawnListener: keep BetterModel's tracker spawn state synced with FancyNpcs' own per-player visibility.CustomModelAttribute: extracted tracker configuration (scale + hitbox listeners) into a shared method, now also invoked from aCreateEntityTrackerEventsubscription so it survives/bettermodel reload. AddedrunOnPlayerSchedulerhelper mirroringNpc#runOnPlayerSchedulerin core, since BetterModel's tracker API is region-thread sensitive on Folia the same way FancyNpcs' own packet sending is.Test plan
/bettermodel reloadfollowed by right/left-clicking the npc - actions now still fire (previously stopped working after reload).runOnPlayerSchedulerjust runs synchronously there).