diff --git a/Code/client/Games/Skyrim/Actor.cpp b/Code/client/Games/Skyrim/Actor.cpp index 9e1933e862..5b48b683b6 100644 --- a/Code/client/Games/Skyrim/Actor.cpp +++ b/Code/client/Games/Skyrim/Actor.cpp @@ -234,7 +234,16 @@ GamePtr Actor::Create(TESNPC* apBaseForm) noexcept if (pCell && !(pCell->cellFlags & 1)) pCell = nullptr; - ModManager::Get()->Spawn(position, rotation, pCell, pWorldSpace, pActor); + // If the game doesn't place the reference, the actor has no form ID or handle and the returned GamePtr + // holds its only reference, so callers that keep a raw Actor* use it after it is freed. Moving it queues + // engine work that can then run on the freed actor (crash at SkyrimSE 13641 under MoveRefToNewSpace, + // with a form ID 0 actor). Don't hand out an actor that was never placed. + const uint32_t cRefHandle = ModManager::Get()->Spawn(position, rotation, pCell, pWorldSpace, pActor); + if (cRefHandle == 0 || pActor->formID == 0) + { + spdlog::error("Actor::Create: failed to place actor with base form {:X}", apBaseForm ? apBaseForm->formID : 0); + return {}; + } pActor->ForcePosition(position); @@ -439,12 +448,51 @@ bool TP_MAKE_THISCALL(HookRemoveSpell, Actor, MagicItem* apSpell) return result; } + +namespace +{ +// Diagnostic for SkyrimSE+1AF607: Characters that end up with form ID 0 later crash on a stale parent cell. +// Logs the caller chain as module+offset so the code that creates or loads them can be found. +void LogFormIdZeroActor(const char* acpWhere, const Actor* apActor) noexcept +{ + void* frames[24]{}; + const USHORT cCount = RtlCaptureStackBackTrace(1, static_cast(std::size(frames)), frames, nullptr); + + std::string stack; + for (USHORT i = 0; i < cCount; ++i) + { + HMODULE hModule = nullptr; + char moduleName[MAX_PATH] = "?"; + if (GetModuleHandleExA(GET_MODULE_HANDLE_EX_FLAG_FROM_ADDRESS | GET_MODULE_HANDLE_EX_FLAG_UNCHANGED_REFCOUNT, static_cast(frames[i]), &hModule) && hModule) + { + char path[MAX_PATH]{}; + if (GetModuleFileNameA(hModule, path, MAX_PATH)) + { + const char* cpName = std::strrchr(path, '\\'); + strncpy_s(moduleName, cpName ? cpName + 1 : path, _TRUNCATE); + } + } + + const auto cOffset = reinterpret_cast(frames[i]) - reinterpret_cast(hModule); + stack += fmt::format(" {}+{:X}", moduleName, cOffset); + } + + const auto* pNpc = Cast(apActor->baseForm); + spdlog::warn("[FormIdZeroActor] {}: actor {}, base {:X} ({}), parent cell {}, flags {:X}, thread {}, stack:{}", acpWhere, fmt::ptr(apActor), + apActor->baseForm ? apActor->baseForm->formID : 0, pNpc && pNpc->fullName.value.AsAscii() ? pNpc->fullName.value.AsAscii() : "", fmt::ptr(apActor->parentCell), + apActor->flags, GetCurrentThreadId(), stack); +} +} // namespace + Actor* TP_MAKE_THISCALL(HookCharacterConstructor, Actor) { TP_EMPTY_HOOK_PLACEHOLDER; TiltedPhoques::ThisCall(RealCharacterConstructor, apThis); + if (apThis->formID == 0) + LogFormIdZeroActor("Character constructor", apThis); + return apThis; } @@ -454,6 +502,9 @@ Actor* TP_MAKE_THISCALL(HookCharacterConstructor2, Actor, uint8_t aUnk) TiltedPhoques::ThisCall(RealCharacterConstructor2, apThis, aUnk); + if (apThis->formID == 0) + LogFormIdZeroActor("Character constructor 2", apThis); + return apThis; } @@ -954,6 +1005,9 @@ bool TP_MAKE_THISCALL(HookSpawnActorInWorld, Actor) spdlog::info("Spawn Actor: {:X}, and NPC {}", apThis->formID, pNpc->fullName.value); } + if (apThis->formID == 0) + LogFormIdZeroActor("UpdateReference3D", apThis); + return TiltedPhoques::ThisCall(RealSpawnActorInWorld, apThis); } diff --git a/Code/client/Services/Debug/DebugService.cpp b/Code/client/Services/Debug/DebugService.cpp index c36d6693e0..8502d92fff 100644 --- a/Code/client/Services/Debug/DebugService.cpp +++ b/Code/client/Services/Debug/DebugService.cpp @@ -87,6 +87,8 @@ void __declspec(noinline) DebugService::PlaceActorInWorld() noexcept const auto pPlayerBaseForm = static_cast(PlayerCharacter::Get()->baseForm); auto pActor = Actor::Create(pPlayerBaseForm); + if (!pActor) + return; const Inventory inventory = PlayerCharacter::Get()->GetActorInventory(); pActor->SetActorInventory(inventory); diff --git a/Code/client/Services/Generic/CharacterService.cpp b/Code/client/Services/Generic/CharacterService.cpp index 7d3ac21af5..f24923eafd 100644 --- a/Code/client/Services/Generic/CharacterService.cpp +++ b/Code/client/Services/Generic/CharacterService.cpp @@ -227,6 +227,26 @@ void CharacterService::DeleteTempActor(const uint32_t aFormId) noexcept } } +namespace +{ +// Diagnostic for actors that loop between spawn and removal (e.g. a follower both saves share): where the actor +// is, and where the player is, at each step. +void LogActorPlacement(const char* acpWhat, uint32_t aFormId) noexcept +{ + auto* pActor = Cast(TESForm::GetById(aFormId)); + const auto* pPlayer = PlayerCharacter::Get(); + if (!pActor || !pPlayer) + return; + + const auto* pCell = pActor->GetParentCellEx(); + const auto* pPlayerCell = pPlayer->GetParentCellEx(); + spdlog::info("[ActorPlacement] {} {:X}: cell {:X}, worldspace {:X}, position ({:.0f}, {:.0f}, {:.0f}), remote {}; player cell {:X}, worldspace {:X}, position ({:.0f}, {:.0f}, {:.0f})", + acpWhat, aFormId, pCell ? pCell->formID : 0, pActor->GetWorldSpace() ? pActor->GetWorldSpace()->formID : 0, pActor->position.x, pActor->position.y, + pActor->position.z, pActor->GetExtension()->IsRemote(), pPlayerCell ? pPlayerCell->formID : 0, pPlayer->GetWorldSpace() ? pPlayer->GetWorldSpace()->formID : 0, + pPlayer->position.x, pPlayer->position.y, pPlayer->position.z); +} +} // namespace + void CharacterService::OnActorAdded(const ActorAddedEvent& acEvent) noexcept { Actor* pActor = Cast(TESForm::GetById(acEvent.FormId)); @@ -260,11 +280,15 @@ void CharacterService::OnActorAdded(const ActorAddedEvent& acEvent) noexcept m_world.emplace_or_replace(entity, acEvent.FormId); m_world.emplace_or_replace(entity); + LogActorPlacement("Added", acEvent.FormId); + ProcessNewEntity(entity); } void CharacterService::OnActorRemoved(const ActorRemovedEvent& acEvent) noexcept { + LogActorPlacement("Removing", acEvent.FormId); + if (auto* pActor = Cast(TESForm::GetById(acEvent.FormId))) pActor->GetExtension()->Reconciliation = ActorExtension::ReconciliationStage::None; @@ -485,8 +509,15 @@ void CharacterService::OnAssignCharacter(const AssignCharacterResponse& acMessag ReconcileActorData(cEntity, pActor, acMessage.OwnershipEpoch, actorData, true, false); + spdlog::info("[ActorPlacement] Moving remote {:X} to owner's worldspace {:X}:{:X}, cell {:X}:{:X}, position ({:.0f}, {:.0f}, {:.0f})", pActor->formID, + acMessage.WorldSpaceId.ModId, acMessage.WorldSpaceId.BaseId, acMessage.CellId.ModId, acMessage.CellId.BaseId, acMessage.Position.x, acMessage.Position.y, + acMessage.Position.z); + LogActorPlacement("Before move", pActor->formID); + MoveActor(pActor, acMessage.WorldSpaceId, acMessage.CellId, acMessage.Position); + LogActorPlacement("After move", pActor->formID); + // The owner's leveled pick rides the assignment response for actors we discovered ourselves ApplyLeveledNpcPick(pActor, acMessage.LeveledNpcPickId); } @@ -588,6 +619,11 @@ void CharacterService::OnCharacterSpawn(const CharacterSpawnRequest& acMessage) if (!pActor) { spdlog::error("Actor object {:X} could not be created.", acMessage.ServerId); + + // The custom form branch created this entity for the new actor + if (acMessage.FormId == GameId{} && entity) + m_world.destroy(*entity); + return; }