Fix/improved naked npc fixes for NPC and Players - #880
Open
absol89 wants to merge 26 commits into
Open
Conversation
Original naked NPC fix worked around the "fighting over NPC ownership" issues in STR by checking periodically to see if NPCs were naked and redressing them if they were. It didn't really address root cause, and introduced some new bugs. The changes here: - Are constrained by the lack of solid reproducers. Simulations only get you so far. - Address all the root-cause issues that could be found / inferred in reasonable time. Difficult to say if it gets all (or enough) of them without solid reproducers. - Fixes the new bugs introduced by the first version. - Keeps a last-resort timeout to redress NPCs in case this still doesn't fix all the root causes. Root cause: - Common wisdom was the Ownership code was fighting over NPCs. Leader and Member jump into a cell together and each try to set up the NPCs, broadcasting conflicting changes to the other. - This turned out to be true in many forms. First, when an Actor is constructed, shortly after creation the engine fires a bunch of EquipmentChangeEvents as the engine dresses the actor. Some to all of these these could be dropped because ServerIds for the actors or equipment aren't available yet. But, it is a real problem if some of them ARE broadcast once server IDs are allocated. Equipping an item the sender has but the receiver doesn't can end up unequipping. Even worse if inventory changes are sent. - The new code blocks sending updates until WaitingFor3D ends (according to CK, accessing the default outfit before the Actor is fully constructed is undefined). And, it blocks sending updates while WaitingForAssignment. This is the KEY fix, since at the end of WaitingForAssignment, we know who should win, and the winner has sends a full Inventory and an full Equipment list. Applying that (by the non-owners) should fully dress the NPC. At least as long as the Owner has dressed the NPC. - As a last resort/backstop, a "naked deadline" trips on the Actor. If this happens where the actor IsLocal(), and the actor is not dressed, then they will equip the default outfit (if there is one). These changes are synced. - That last is sort of what the original fix did, but improved. This redressing is non-destructive. Bugs fixed in Vanilla: - Blocks the sending of potentially destructive equipment and possibly inventory changes while debating Ownership. This may or may not be a complete fix. Bugs fixed in oriignal Naked fix: - The original code just tried to dress NPCs once every second. Note, that was not "after a second", but between 0 and 1 seconds after creation. I can't prove it, but this firing too soon, and in particular before WaitingFor3D leads to undefined behavior and is probably one of the reasons for lingering nakedness, and syncing broken equip states (and inventory?). This was replaced with a per-Actor timeout that works out to 2-3 seconds. It is reset at the end of WaitingFor3D and again at the end of Ownership change. - Replaces the forced-redress by fully resetting inventory with simply equipping the default outfit; no changes to inventory. This is critical for followers that may have accummulated a lot of loot, but applies to any NPC that has loot. - Does not try to recreate the default outfit inventory if you have removed it from inventory. This is probably what you wanted anyway. - Only checks for redressing once per ownership change of the Actor, rather than rechecking every second. So you can change what the actor is wearing and it will stay that way as long as you stay in the cell (and the Leader doesn't enter and take over)
Root cause: the NPC's authoritative inventory (InventoryComponent::Content) is wholesale-replaced from a client's GetActorInventory() snapshot on ownership transfer (ApplyActorData). During the transient undressed window after a cell reload (before the engine re-dresses the NPC), the owner's snapshot lacks the body piece, so Content becomes body-piece-less and propagates nakedness to every client via InitialInventory. The same overwrite also reset NPCs whose equipment was legitimately removed. Fix: detect a destructive fully-naked overwrite (incoming snapshot has no worn entries while the authoritative Content still has worn items, and the NPC is alive) and ignore it rather than applying it. A later real inventory interaction (server InventoryService incremental change for that entity) sends correct data, so there is no need to mix in the stale naked snapshot. The only legitimate fully-empty states are: a player steals all items from a living NPC (pickpocket - syncs incrementally and empties Content first) or loots all items from a dead NPC. A dead NPC's naked overwrite is therefore allowed through, as is any non-naked overwrite. - Inventory: add IsFullyNaked()/HasWornItems() helpers (no game-specific slot knowledge). - CharacterService::ApplyActorData: ignore the destructive alive-NPC overwrite instead of applying it. Pickpocket removals still flow through the incremental AddOrRemoveEntry/UpdateEquipment path and sync normally; only the racing naked overwrite on a living NPC is suppressed.
…hoques#860 Drops the per-Actor 'naked deadline' backstop machinery (RunNakedNPCBugChecks redress + SetNakedDeadline/nakedDeadline + the now-unused EquipOutfit, SetOutfit, IsWearingBodyPiece, ShouldWearBodyPiece helpers and BGSOutfit includes). This is the 'last resort' client band-aid that cannot hold against server authority: any client re-equip of the body piece is reconciled away by the server, whose authoritative inventory omits the body piece on NPC reload. The essential root-cause fix in PR tiltedphoques#860 - blocking equipment/inventory sends while WaitingForAssignment - is retained.
Root cause of the reload-naked-NPC bug: on ownership transfer (and player spawn) the client forwards its live GetActorInventory() snapshot to the server, which becomes the authoritative InventoryComponent::Content and is broadcast to every client. During the transient window right after a cell reload - before the engine re-dresses the NPC - that snapshot reports the actor as wearing nothing. The server stored the naked snapshot (both at entity emplace and at ApplyActorData overwrite) and propagated it permanently; resetinventory fixed it by forcing a correct re-sync. The server cannot derive a dressed inventory, so the owner client - which has engine access - must send correct data instead. GetActorInventory() now falls back to the NPC's default outfit (baseForm->outfits[0]->outfitItems) when the live snapshot is fully naked for a living, non-player actor. The fallback is only used when it actually yields worn items, so it never invents gear for a legitimately empty inventory. This is the real root-cause fix, replacing PR tiltedphoques#860's client-side polling/redress band-aid (which could not hold against server authority). The server-side guard in ApplyActorData remains as a safety net against later destructive naked overwrites of an already-dressed NPC.
- GetActorInventory fallback now resolves outfits[0] and outfits[1] (guards often define only outfits[1]), and iterates every entry of a leveled item list (count stored 8 bytes before pLeveledListA per TESLeveledList layout) instead of just the first. - SetActorInventory now bails out of SetInventory()/RemoveAllItems() when the incoming snapshot has no worn items but the actor is currently dressed: there is nothing to unequip, so forcing it through only strips a correctly-dressed NPC. Dead NPCs and legitimately empty (pickpocket/stripped) snapshots are unaffected. - Add [NakedFix] diagnostic logging to the fallback path so the next build can confirm which outfit slot / leveled entry resolves for each broken NPC.
…eq-class fix) The naked NPC each run is whichever actor receives a naked server snapshot during an ownership flip / leveled spawn - it's a race class, not tied to any NPC type. The old guard only skipped the strip when the actor was ALREADY dressed, so a leveled/temp-base spawn that was never dressed yet still got stripped/left naked. - Extract DeriveOutfitInventory() helper (outfits[0]/[1] + leveled list A), shared by GetActorInventory (owner-upload side) and SetActorInventory (apply side). - GetActorInventory: use the helper so a local owner never uploads a naked snapshot. - SetActorInventory: when an alive non-player NPC receives a fully-naked snapshot, substitute the locally-derived outfit (supplemented onto any non-worn inventory the server sent) instead of applying the empty one. Dead NPCs and incremental pickpocket/loot removals (which send non-empty worn entries) are unaffected. - Keep [NakedFix] diagnostics for the next repro.
…iningQuestItems Passing a temporary as the non-const Inventory& arg failed to compile. Capture it in a local currentInventory, matching the existing normal-path call.
PART 1+2 fixed the networked inventory (members see dressed) and PART 3 fixed remote clients receiving a snapshot, but the OWNER's locally-spawned NPC stayed naked: the owner never receives its own snapshot to trigger SetActorInventory, and the owner block in OnAssignCharacter never applied inventory at all. When a locally-owned NPC is assigned and currently renders with no worn items, derive its outfit from the base form and apply it to the owner's own actor. Reuses DeriveOutfitInventory() and the now-safe SetActorInventory (which applies normally when the incoming snapshot has worn items). Diagnosed via Aela the Huntress (1A697/1A696): leader rendered naked while the member (fed by PART 1+2's corrected upload) was dressed.
Apply the derived-outfit dress on the assignment frame, then queue a single deferred re-check on the next tick. If an engine hiccup overwrote the same-tick apply (actor 3D not yet settled at assignment), the re-check re-dresses once. Extracted into a file-local OwnerSelfHealDress() helper (idempotent: dressed NPCs are a no-op, so it never fights legit gameplay). No periodic scan — single re-check only.
…erwrite The single deferred re-check at assignment fires too early: the engine applies its own (naked) visual equipment to a locally-owned NPC several frames later, after the 3D loads (Olava the Feeble 1A699/13BAE reproved this - assigned dressed, no further events, yet rendered naked on the leader). Add a ~1s periodic re-check in RunLocalUpdates that re-dresses any locally-owned NPC still rendering with no worn items. Idempotent (dressed = no-op, never fights gameplay) and throttled so cost is negligible. Periodic path logs only actual dresses (aVerbose=false) to avoid spam for NPCs with no base outfit (e.g. Cow).
…ngine 3D-load undress) The naked-NPC race is the engine's async equip pass: when a locally-owned NPC's 3D finishes loading, Skyrim calls TESObjectREFR::SetInventory, which does RemoveAllItems() first. If the engine-side inventory is empty/incomplete at that moment, the NPC is stripped -- even though it was assigned dressed (Olava the Feeble 1A699/13BAE reproved this: assigned dressed, no network events, yet rendered naked). Guard TESObjectREFR::SetInventory: if the target is a living non-player actor and the incoming inventory has no worn items, skip the RemoveAllItems()+reapply. A fully-naked overwrite of a living NPC is never legitimate -- dead-NPC loot clears and incremental pickpocket both send non-empty worn entries, and the player is excluded. This stops the strip at the source instead of redressing after the fact. Keep the periodic owner self-heal as a backstop, throttled to ~3s (original band-aid periodicity).
The periodic re-dress scan was spinning every 3s on guards (temp and non-temp) because the engine strips living NPCs via EquipManager::UnequipAll (engine native 38899), NOT SetInventory -- so the SetInventory guard never fired (confirmed: 0 skips in log). The engine's 3D-load equip pass does UnequipAll then re-equips from the NPC's empty engine-side equipment, leaving guards naked/partial. - Guard EquipManager::UnequipAll: skip the engine full-unequip when the target is a living non-player NPC that currently has worn items. This blocks the strip at the source and kills the re-strip/re-dress loop. Dead (loot) and player unequips still pass through. - Demote the GetActorInventory fallback diagnostic logs (fallback path / outfit detail / no wearable outfit derived) from info to debug -- they flooded the log every 3s per NPC via movement-sync and the periodic scan. Actionable logs (substituting, SetInventory skip, owner self-heal dress) stay at info. Periodic owner self-heal retained as a backstop.
Owner self-heal left templated (FF-prefixed leveled-character) NPCs naked on the owner's own screen: DeriveOutfitInventory read outfits[] off the leaf base, which is null for template spawns. Fall back to GetTemplateBase() when the leaf defines no outfit. Owner-only; remote clients already dress from the network snapshot.
The owner-side naked-NPC render is a desync: the logical inventory container reports items as worn (ExtraWorn flag set) while the engine never equipped them onto the actor's biped, so the render is naked. The self-heal was gated on HasWornItems() (container flags) at both the call sites and inside the helper, so it saw 'dressed' and never ran -- no NakedFix log appeared for the affected actor at all. Fix: - Add Actor::ForceEquipWornArmor(): re-issues EquipManager::Equip(force=true) for every container item flagged worn, applying it to the biped. Reads raw GetArmor() (not GetActorInventory, which masks empty state by deriving). - OwnerSelfHealDress now uses the honest GetWornArmor() signal: if worn armor exists, force re-equip; only if none exists, derive+apply a base outfit. - Remove the faulty HasWornItems() pre-gate at the OnAssignCharacter call site. - Also run the self-heal on ownership change (TakeOwnership), per repro where the Wayfarer flips ownership repeatedly. - Drop the ~3s periodic RunLocalUpdates naked scan: assignment + ownership change now cover it, saving per-frame cost.
…load guard) The forced re-equip crashed on the deferred re-check for a dynamic FE actor (summon) whose 3D/process wasn't settled, and spammed OnEquipmentChangeEvent network sends because the Equip calls ran outside ScopedEquipOverride. - Wrap the loop in ScopedEquipOverride: matches every other internal engine equip in the codebase; suppresses the sync-event trigger and equip-hook re-entrancy that caused the crash. - Bail early unless GetNiNode() && currentProcess are present, so we never force-equip onto an actor that isn't fully loaded.
… crash) The previous ForceEquipWornArmor approach crashed: it called EquipManager::Equip for every owned NPC on connect, from the network thread at assignment AND again from a deferred main-thread re-check. Engine equip must run on the main thread and only the re-check reached a loaded actor -- crashing on the FIRST NPC's re-check (not the last). It also passed hand equip slots for body armor and spammed OnEquipmentChangeEvent. Replace all of it with the same path remotes already use safely: - OwnerSelfHealDress now just reapplies the actor's own inventory via SetActorInventory (GetActorInventory already derives a base outfit for empty templated/leveled spawns; SetActorInventory has its own naked-strip guard). - Runs synchronously in OnAssignCharacter (owner branch) and TakeOwnership, on the same thread the remote SetActorInventory runs on. No deferred cross-thread Queue, no bespoke engine equip. - When an owned NPC is genuinely naked with no derivable outfit, log it instead of forcing an equip. - Remove Actor::ForceEquipWornArmor entirely.
…ync) Perf analysis of build 0c9d20c (5-min playbench): leader ran 106 self-heal reapplies (69 unique actors), same NPC re-dressed up to 10x due to MP ownership thrash (CancelServerAssignment -> respawn -> reclaim). Each reapply called SetActorInventory (RemoveAllItems + full re-add + per-item Equip), and for a LOCAL actor each per-item Equip fired OnEquipmentChangeEvent to the server -- the leader broadcast ~1000 equipment-change packets in 5 min, almost entirely self-inflicted. That is the CPU stutter source. Fix 1 (network/broadcast storm): EquipHook already suppressed the sync for REMOTE actors under ScopedEquipOverride; extend the same gate to LOCAL actors so internal re-dress equips don't broadcast. Wrap the self-heal's SetActorInventory call in ScopedEquipOverride. The engine equip still runs, so the NPC is dressed locally; only the network sync is skipped (server already has the inventory). Player/AI-driven equips stay synced (not under the override). Vampire Lord cosmetic armor and debug-view equips are unaffected (not under the override). Fix 2 (CPU dedup): skip the heavy re-apply when the biped already reports worn armor (honest GetEquipment() signal), so already-dressed NPCs from an ownership re-claim no longer run RemoveAllItems + re-add + re-equip. Genuinely-naked NPCs (biped empty) still go through and get fixed. Net: self-heal now only re-dresses NPCs that are actually naked, and does so without flooding the server.
Skyrim's nude body skin is never synced (SaveSkinFar is disabled in Actor::Serialize), so a remote actor only receives a torso/legs/feet mesh if its inventory snapshot carries a body-slot (cuirass) ARMO. A player wearing helmet+boots but no torso armor - or a fully nude player - therefore renders with no body (and no feet, since feet are part of the body-slot model) on every other client, regardless of which client crosses a cell border. The bug is in how the remote representation is rebuilt purely from the synced inventory, which cannot carry the nude body. Fix: in TESObjectREFR::SetInventory, for remote actors, detect whether any worn entry is a body-piece ARMO (TESObjectARMO::IsBodyPiece, slotType & 0x4). If none, inject a worn body ARMO into the local working inventory so the remote client has a body model to render. The server-side inventory is untouched (only the local working copy is mutated), and the injection fires on every re-sync including cell transitions. Stand-in body form is the Vampire Lord armor (02011A85, Dawnguard mod index 0x02) whose body model renders the actor's nude body; to be replaced with a load-order-independent nude body once identified.
The body-slot fallback injected the stand-in body ARMO for ANY remote actor, but the Vampire Lord armor body does not fit NPC bodies and rendered them naked on the leader's screen during playtest. The fix is meant for players (the reported bug: a player wearing helmet+boots but no cuirass, or a nude player, shows no body/feet to others). Gate the injection on IsRemotePlayer() instead of IsRemote() so NPCs are excluded. NPCs normally carry a body via their base outfit or worn armor, and must not receive the fake worn item.
Contributor
Author
One-line summary for the mergeThis branch fixes the root cause of locally-owned NPCs rendering naked (server applies a transient naked snapshot during ownership/cell-reload; engine strips dressed NPCs on 3D-load) by guarding those two overwrite paths and substituting a derived outfit for garbage-naked snapshots, backed by a quiet, event-driven, network-silent, CPU-deduped owner self-heal. It deletes the old per-frame Reviewer checklist / things to verify
PR Review Details: NakedFix (improved naked-NPC fix)Branch: Problem statementIn Skyrim Together Reborn, NPCs that a player "owns" (nearest-client ownership) can render naked on the owner's own screen while looking dressed on every other player's screen. Root cause: when ownership transfers or the owner reloads a cell, the NPC's authoritative inventory ( Why the owner's screen specifically: remote clients dress NPCs from the spawn/assignment snapshot, but the owning client renders the NPC from its own (transiently naked) local state and relied on the ineffective periodic re-dress. The fix (commit by commit, in merge order)1.
|
The IsRemotePlayer() guard only wrapped the body-detection loop, not the injection. Every NPC (wayfarers, laborers, etc.) skipped the detection loop, leaving hasBody false, so the ungated 'if (!hasBody)' injected the Vampire Lord body ARMO onto them. Move the injection inside the IsRemotePlayer() guard so only remote players receive it.
Leveled/templated NPCs (road encounters like Knight/Dawnguard Sergeant) render naked on the OWNER's screen: at ownership-claim time the biped/3D is not yet loaded, so the immediate OwnerSelfHealDress re-equip no-ops. Remotes are unaffected (they dress from the snapshot after 3D is up). Add a deferred retry mirroring the existing weapon-draw mechanism: enqueue the local actor into m_selfHealRetries at claim/assign, then re-run OwnerSelfHealDress on the update tick in 2 passes (~0.5s, ~1.5s). OwnerSelfHealDress already self-dedups on HasWornItems, so retries on an already-dressed actor are cheap no-ops. Logs wornBefore/wornAfter per pass for verification.
New logs show static NPCs (Gerdur 13489, Frodnar 1348B, +others) render naked on the OWNER after an ownership TRANSFER, not just at spawn. The transfer strips the biped but leaves the container's worn flags set, so re-dressing is a diff no-op: SetInventory sees the items already flagged worn and equips nothing. Retry log confirmed wornBefore=false wornAfter=false on both gentle passes despite GetActorInventory().HasWornItems()==true. Extend the deferred retry into a 3-stage machine: two gentle re-dress passes (covers the biped-not- ready case), then if the biped is still naked while the container claims worn items, escalate to a DisableImpl()/EnableImpl() 3D rebuild - the same render-rebuild primitive the leveled-conform path uses. EnableImpl(false) keeps inventory intact and re-attaches worn armor to the fresh biped. Guarded on loadedState so distant/unloaded actors aren't touched.
New log: a Courier (19015C71, base FF00152D) rendered naked on the OWNER while the self-heal reported wornBefore=true wornAfter=true - so the guard short-circuited and never re-dressed or escalated. Cause: HasWornItems() returns true for ANY worn item (satchel/boots/gloves), but 'naked' means the BODY/torso slot is empty. An NPC wearing only accessories passes the generic check yet looks naked. Add HasWornBodyPiece() (worn ARMO with slotType & 0x4, mirroring the remote body-ARMO injection path) and use it for every self-heal guard: the OwnerSelfHealDress dedup, the retry wornBefore/After signals, the 3D-rebuild re-enable check, and the escalation container check. Now partial-equip NPCs are treated as naked and re-dressed / rebuilt.
Contributor
Author
|
Adding a log here from the latest commit test 41e3954, as previously couriers that only had a backpack slot could evade the checks. Checking for a body slot armor garment now seems to keep all npc from riverwood to whiterun dressed. |
…ave/anim OOB) The self-heal 3D-rebuild escalation calls EnableImpl() to rebuild the biped but left the actor's stale GraphDescriptorHash intact. Re-enabling rebuilds the animation graph, and the old hash can index the new graph's variable set out of bounds - the same OOB variable-index crash the leveled-conform re-enable (GraphDescriptorHash=0) and the werewolf/VL transforms already guard against. Symptom: freeze/hang when saving near dense static-NPC clusters (Riverwood Gerdur/Frodnar) where the escalation fires often. Zero GraphDescriptorHash right after EnableImpl() so the next sync tick recomputes it from the fresh graph (engine treats hash==0 as 'recompute', per TESObjectREFR). Mirrors the existing conform path.
Two fixes for NPCs (esp. horses/persistent owned actors) accumulating multiple copies of their outfit near the spawn area: 1. SetActorInventory supplement now merges derived worn entries by BaseId instead of blind push_back. The owner self-heal re-runs SetActorInventory on every ownership flip; without the merge each flip appended another full derived outfit, stacking items (observed 5x on horses). Replacing on BaseId makes a re-apply a no-op for already-present items. 2. TakeOwnership / OnAssignCharacter no longer re-arm a self-heal retry that is already tracked for the actor. Ownership flips fire repeatedly for persistent owned NPCs; each re-arm reset the retry timer and re-ran SetInventory, churning the inventory. One active self-heal per actor; a fresh TakeOwnership after the entry is removed starts a new one. Cross-log evidence: stacked actors (e.g. horse 9B7A9, 12x re-dress) appear only in the OWNER's log, never in other clients' logs, confirming the duplication is generated locally by the self-heal, not server-authoritative.
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
2 participants
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.
Long story short, rfortier discovered that the original RunNakedNPCBugChecks() didn't solve the issue. Checked every 3s.
What I discovered when testing the PR code, was that there were still cases where NPC were naked during ownership flip.
On some players, this ownership flip left local characters in a RemoveAllItems state, as inventory or equip calls sent empty.
What my commits since then have been trying to do is the following:
When comparing before and after absol89@ed7ed1e , the previous commit's code isRemote() revealed NPC with bad worn state. That gave them the vampire lord's armor as proof that this was what was going on. Now, I changed the final commit to only give isRemotePlayer() the vampire lord armor to make sure their other limbs and helmets would render when they travel shirtless. Hopefully this fixes 3 issues in one swoop, and I am submitting it as a separate PR than 860 so there's possibility to review separately.
Video of npc not being naked after fixes. You can watch it without sound, it is just a 11 minute stress test https://www.youtube.com/watch?v=mCgSCQRPPiQ
Video of pickpocket sync working (leader did it). Watch with sound https://www.youtube.com/watch?v=tFoMUSwnAZo
Picture of an NPC being fully equipless by player pickpocket. It was important that this didn't trigger outfit reset, seems ok.
Picture of how player's look when not wearing torso item, and wearing helmet, gloves and boots on vanilla STR 1.8.0 (live).
Video of fix to limb rendering when player is not wearing torso items (gets armor 02011a85). https://youtu.be/EfgXv9WK8fE