From b63023bf5a5bcfad28ad1aa4c8b7ac19111de7fb Mon Sep 17 00:00:00 2001 From: rfortier Date: Wed, 23 Oct 2024 19:22:06 -0400 Subject: [PATCH 1/5] Remove redundant UnEquipAll() call which seems to trigger a Skyrim concurrency bug. --- Code/client/Games/Skyrim/Actor.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Code/client/Games/Skyrim/Actor.cpp b/Code/client/Games/Skyrim/Actor.cpp index f587dcab1..8f67f5cb1 100644 --- a/Code/client/Games/Skyrim/Actor.cpp +++ b/Code/client/Games/Skyrim/Actor.cpp @@ -395,7 +395,9 @@ void Actor::SetActorInventory(const Inventory& aInventory) noexcept { spdlog::info("Setting inventory for actor {:X}", formID); - UnEquipAll(); + // The UnEquipAll() that used to be here is redundant, + // as RemoveAllItems() unequips every item if needed. + // Placing this UnEquipAll() here seems to trigger a Skyrim bug/race. SetInventory(aInventory); SetMagicEquipment(aInventory.CurrentMagicEquipment); From 49bfc73267a3b4ca55d66928c2e2793982416d62 Mon Sep 17 00:00:00 2001 From: rfortier Date: Mon, 21 Oct 2024 21:49:54 -0400 Subject: [PATCH 2/5] Don't accept OnRemoteSpawnDataReceived data for the local PlayerCharacter that the remote system is obviously not authoratative for. From the code comment: Only accept spawn position and inventory updates if not for local player The remote system is simply not authoritative for this info, and trying to take it from the remote can result in concurrent update corruption That's the real problem, either s_pRemoveAllItems or s_unequipAll have async behavior themselves or undocumented behavior that causes concurrency issues. --- .../Services/Generic/CharacterService.cpp | 24 ++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-) diff --git a/Code/client/Services/Generic/CharacterService.cpp b/Code/client/Services/Generic/CharacterService.cpp index 0bb9f705f..d1c852cf6 100644 --- a/Code/client/Services/Generic/CharacterService.cpp +++ b/Code/client/Services/Generic/CharacterService.cpp @@ -512,9 +512,27 @@ void CharacterService::OnRemoteSpawnDataReceived(const NotifySpawnData& acMessag if (!pActor) return; - pActor->SetActorValues(acMessage.NewActorData.InitialActorValues); - pActor->SetActorInventory(acMessage.NewActorData.InitialInventory); - m_weaponDrawUpdates[pActor->formID] = {acMessage.NewActorData.IsWeaponDrawn}; + // Only accept spawn position and inventory updates if not for local player + // The remote system is simply not authoritative for this info, and trying + // to take it from the remote can result in concurrent update corruption + // That's the real problem, either s_pRemoveAllItems or s_unequipAll + // have async behavior themselves or undocumented behavior that + // causes concurrency issues. + if (pActor->GetExtension()->IsPlayer() && !pActor->GetExtension()->IsRemote()) + { +#if TP_SKYRIM64 + auto pNpc = Cast(pActor); + auto pName = pNpc ? static_cast(pNpc->fullName.value) : ""; + spdlog::warn(__FUNCTION__ ": rejecting non-authoritative remote spawn update for local player pActor{:X}, formID{:X}, name \"{}\"", + (uintptr_t)pActor, pActor->formID, pName); +#endif + } + else + { + pActor->SetActorValues(acMessage.NewActorData.InitialActorValues); + pActor->SetActorInventory(acMessage.NewActorData.InitialInventory); + m_weaponDrawUpdates[pActor->formID] = {acMessage.NewActorData.IsWeaponDrawn}; + } if (pActor->IsDead() != acMessage.NewActorData.IsDead) acMessage.NewActorData.IsDead ? pActor->Kill() : pActor->Respawn(); From ed79bf1d44ef5cd174872ff3f5349f48db4ccfb7 Mon Sep 17 00:00:00 2001 From: rfortier Date: Tue, 22 Oct 2024 17:47:17 -0400 Subject: [PATCH 3/5] Loosen the check ALL local NPCs should reject updated remote spawn data looping back to themselves. --- .../Services/Generic/CharacterService.cpp | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/Code/client/Services/Generic/CharacterService.cpp b/Code/client/Services/Generic/CharacterService.cpp index d1c852cf6..e0ecf6633 100644 --- a/Code/client/Services/Generic/CharacterService.cpp +++ b/Code/client/Services/Generic/CharacterService.cpp @@ -512,13 +512,18 @@ void CharacterService::OnRemoteSpawnDataReceived(const NotifySpawnData& acMessag if (!pActor) return; - // Only accept spawn position and inventory updates if not for local player - // The remote system is simply not authoritative for this info, and trying - // to take it from the remote can result in concurrent update corruption - // That's the real problem, either s_pRemoveAllItems or s_unequipAll - // have async behavior themselves or undocumented behavior that + // Only accept spawn position and inventory updates for REMOTE Npcs. + // The remote system is simply not authoritative for local Npc info, and trying + // to take it from the remote can result in concurrent update corruption. + // This is true whether it is a local Player (and therefor remote from the + // Leader), or a local NPC that is with their leader or is owned instead by + // the local player and their own data, possibly out-of-date, is looping + // back in OnRemoteSpawnDataReceived + // + // The underlying problem, either s_pRemoveAllItems or s_unequipAll + // have async behavior themselves or undocumented behavior that // causes concurrency issues. - if (pActor->GetExtension()->IsPlayer() && !pActor->GetExtension()->IsRemote()) + if (!pActor->GetExtension()->IsRemote()) { #if TP_SKYRIM64 auto pNpc = Cast(pActor); From 90946c7288fda7f8988a59ca3b05101a9a826090 Mon Sep 17 00:00:00 2001 From: rfortier Date: Sat, 26 Oct 2024 13:32:41 -0400 Subject: [PATCH 4/5] Revert "Loosen the check ALL local NPCs should reject updated remote spawn data looping back to themselves." This reverts commit ed79bf1d44ef5cd174872ff3f5349f48db4ccfb7. --- .../Services/Generic/CharacterService.cpp | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/Code/client/Services/Generic/CharacterService.cpp b/Code/client/Services/Generic/CharacterService.cpp index e0ecf6633..d1c852cf6 100644 --- a/Code/client/Services/Generic/CharacterService.cpp +++ b/Code/client/Services/Generic/CharacterService.cpp @@ -512,18 +512,13 @@ void CharacterService::OnRemoteSpawnDataReceived(const NotifySpawnData& acMessag if (!pActor) return; - // Only accept spawn position and inventory updates for REMOTE Npcs. - // The remote system is simply not authoritative for local Npc info, and trying - // to take it from the remote can result in concurrent update corruption. - // This is true whether it is a local Player (and therefor remote from the - // Leader), or a local NPC that is with their leader or is owned instead by - // the local player and their own data, possibly out-of-date, is looping - // back in OnRemoteSpawnDataReceived - // - // The underlying problem, either s_pRemoveAllItems or s_unequipAll - // have async behavior themselves or undocumented behavior that + // Only accept spawn position and inventory updates if not for local player + // The remote system is simply not authoritative for this info, and trying + // to take it from the remote can result in concurrent update corruption + // That's the real problem, either s_pRemoveAllItems or s_unequipAll + // have async behavior themselves or undocumented behavior that // causes concurrency issues. - if (!pActor->GetExtension()->IsRemote()) + if (pActor->GetExtension()->IsPlayer() && !pActor->GetExtension()->IsRemote()) { #if TP_SKYRIM64 auto pNpc = Cast(pActor); From c53d7c86a93333cc758a31a02ff0a44f619bebbf Mon Sep 17 00:00:00 2001 From: rfortier Date: Sat, 26 Oct 2024 13:33:22 -0400 Subject: [PATCH 5/5] Revert "Don't accept OnRemoteSpawnDataReceived data for the local PlayerCharacter that the remote system is obviously not authoratative for." This reverts commit 49bfc73267a3b4ca55d66928c2e2793982416d62. --- .../Services/Generic/CharacterService.cpp | 24 +++---------------- 1 file changed, 3 insertions(+), 21 deletions(-) diff --git a/Code/client/Services/Generic/CharacterService.cpp b/Code/client/Services/Generic/CharacterService.cpp index d1c852cf6..0bb9f705f 100644 --- a/Code/client/Services/Generic/CharacterService.cpp +++ b/Code/client/Services/Generic/CharacterService.cpp @@ -512,27 +512,9 @@ void CharacterService::OnRemoteSpawnDataReceived(const NotifySpawnData& acMessag if (!pActor) return; - // Only accept spawn position and inventory updates if not for local player - // The remote system is simply not authoritative for this info, and trying - // to take it from the remote can result in concurrent update corruption - // That's the real problem, either s_pRemoveAllItems or s_unequipAll - // have async behavior themselves or undocumented behavior that - // causes concurrency issues. - if (pActor->GetExtension()->IsPlayer() && !pActor->GetExtension()->IsRemote()) - { -#if TP_SKYRIM64 - auto pNpc = Cast(pActor); - auto pName = pNpc ? static_cast(pNpc->fullName.value) : ""; - spdlog::warn(__FUNCTION__ ": rejecting non-authoritative remote spawn update for local player pActor{:X}, formID{:X}, name \"{}\"", - (uintptr_t)pActor, pActor->formID, pName); -#endif - } - else - { - pActor->SetActorValues(acMessage.NewActorData.InitialActorValues); - pActor->SetActorInventory(acMessage.NewActorData.InitialInventory); - m_weaponDrawUpdates[pActor->formID] = {acMessage.NewActorData.IsWeaponDrawn}; - } + pActor->SetActorValues(acMessage.NewActorData.InitialActorValues); + pActor->SetActorInventory(acMessage.NewActorData.InitialInventory); + m_weaponDrawUpdates[pActor->formID] = {acMessage.NewActorData.IsWeaponDrawn}; if (pActor->IsDead() != acMessage.NewActorData.IsDead) acMessage.NewActorData.IsDead ? pActor->Kill() : pActor->Respawn();