Changes requested in code review.

Author: rfortier

Code/client/Games/Skyrim/Actor.cpp

@@ -17,6 +17,7 @@
 #include <Events/MountEvent.h>
 #include <Events/DialogueEvent.h>
 #include <Events/HitEvent.h>
+#include <Events/RemoveSpellEvent.h>
 
 #include <Games/TES.h>
 #include <World.h>
@@ -103,6 +104,7 @@ void Actor::Save_Reversed(const uint32_t aChangeFlags, Buffer::Writer& aWriter)
 
 #endif
 
+TP_THIS_FUNCTION(TRemoveSpell, bool, Actor, MagicItem*);
 TP_THIS_FUNCTION(TCharacterConstructor, Actor*, Actor);
 TP_THIS_FUNCTION(TCharacterConstructor2, Actor*, Actor, uint8_t aUnk);
 TP_THIS_FUNCTION(TCharacterDestructor, Actor*, Actor);
@@ -119,6 +121,7 @@ TCharacterConstructor* RealCharacterConstructor;
 TCharacterConstructor2* RealCharacterConstructor2;
 TCharacterDestructor* RealCharacterDestructor;
 
+static TRemoveSpell* RealRemoveSpell = nullptr;
 static TAddInventoryItem* RealAddInventoryItem = nullptr;
 static TPickUpObject* RealPickUpObject = nullptr;
 static TDropObject* RealDropObject = nullptr;
@@ -352,17 +355,15 @@ void Actor::StopCombat() noexcept
     s_pStopCombat(this);
 }
 
-void Actor::RemoveSpell(MagicItem* apSpell) noexcept
+bool Actor::RemoveSpell(MagicItem* apSpell) noexcept
 {
-    TP_THIS_FUNCTION(TRemoveSpell, void, Actor, MagicItem*);
-    POINTER_SKYRIMSE(TRemoveSpell, removeSpell, 38717);
     if (!apSpell)
     {
-        spdlog::error("Actor::RemoveSpell: apSpell is null");
-        return;
+        spdlog::error(__FUNCTION__ ": apSpell is null");
+        return FALSE;
     }
-    // spdlog::info("Removing spell: {} from actor: {}", apSpell->formID, formID);
-    TiltedPhoques::ThisCall(removeSpell, this, apSpell);
+    // spdlog::info(__FUNCTION__ ": removing: {} from actor: {}", apSpell->formID, formID);
+    return TiltedPhoques::ThisCall(RealRemoveSpell, this, apSpell);
 }
 
 bool Actor::HasPerk(uint32_t aPerkFormId) const noexcept
@@ -382,6 +383,23 @@ uint8_t Actor::GetPerkRank(uint32_t aPerkFormId) const noexcept
     return TiltedPhoques::ThisCall(getPerkRank, this, pPerk);
 }
 
+bool TP_MAKE_THISCALL(HookRemoveSpell, Actor, MagicItem* apSpell)
+{
+    bool result = TiltedPhoques::ThisCall(RealRemoveSpell, apThis, apSpell);
+    if (apThis->GetExtension()->IsLocalPlayer() && result)
+    {
+        // Log spell info
+        // spdlog::info("Removing spell {}, ID: {} from local player", apSpell->GetName() , apSpell->formID);
+        RemoveSpellEvent removalEvent;
+
+        removalEvent.TargetId = apThis->formID;
+        removalEvent.SpellId = apSpell->formID;
+        World::Get().GetRunner().Trigger(removalEvent);
+    }
+
+    return result;
+}
+
 Actor* TP_MAKE_THISCALL(HookCharacterConstructor, Actor)
 {
     TP_EMPTY_HOOK_PLACEHOLDER;
@@ -1246,6 +1264,7 @@ static TiltedPhoques::Initializer s_actorHooks(
     {
         POINTER_SKYRIMSE(TActorProcess, s_actorProcess, 37356);
         POINTER_SKYRIMSE(TSetPosition, s_setPosition, 19790);
+        POINTER_SKYRIMSE(TRemoveSpell, s_removeSpell, 38717);
         POINTER_SKYRIMSE(TCharacterConstructor, s_characterCtor, 40245);
         POINTER_SKYRIMSE(TCharacterConstructor2, s_characterCtor2, 40246);
         POINTER_SKYRIMSE(TCharacterDestructor, s_characterDtor, 37175);
@@ -1268,6 +1287,7 @@ static TiltedPhoques::Initializer s_actorHooks(
 
         RealActorProcess = s_actorProcess.Get();
         RealSetPosition = s_setPosition.Get();
+        RealRemoveSpell = s_removeSpell.Get();
         FUNC_GetActorLocation = s_GetActorLocation.Get();
         RealCharacterConstructor = s_characterCtor.Get();
         RealCharacterConstructor2 = s_characterCtor2.Get();
@@ -1289,6 +1309,7 @@ static TiltedPhoques::Initializer s_actorHooks(
 
         TP_HOOK(&RealActorProcess, HookActorProcess);
         TP_HOOK(&RealSetPosition, HookSetPosition);
+        TP_HOOK(&RealRemoveSpell, HookRemoveSpell);
         TP_HOOK(&RealCharacterConstructor, HookCharacterConstructor);
         TP_HOOK(&RealCharacterConstructor2, HookCharacterConstructor2);
         TP_HOOK(&RealForceState, HookForceState);

Code/client/Games/Skyrim/Actor.h

@@ -250,7 +250,7 @@ struct Actor : TESObjectREFR
     void StopCombat() noexcept;
     bool PlayIdle(TESIdleForm* apIdle) noexcept;
     void FixVampireLordModel() noexcept;
-    void RemoveSpell(MagicItem* apSpell) noexcept;
+    bool RemoveSpell(MagicItem* apSpell) noexcept;
 
     enum ActorFlags
     {

Code/client/Games/Skyrim/Magic/MagicTarget.cpp

@@ -17,8 +17,6 @@
 
 #include <Effects/ActiveEffect.h>
 
-#include <Events/RemoveSpellEvent.h>
-
 TP_THIS_FUNCTION(TAddTarget, bool, MagicTarget, MagicTarget::AddTargetData& arData);
 TP_THIS_FUNCTION(TRemoveSpell, bool, Actor, MagicItem* apSpell);
 TP_THIS_FUNCTION(TCheckAddEffectTargetData, bool, MagicTarget::AddTargetData, void* arArgs, float afResistance);
@@ -185,26 +183,6 @@ bool TP_MAKE_THISCALL(HookAddTarget, MagicTarget, MagicTarget::AddTargetData& ar
     }
 }
 
-// Designed to run when we hook removespell (AddressLib ID is 38717)
-// Sends a message to the server to remove the spell from this player on other clients
-bool TP_MAKE_THISCALL(HookRemoveSpell, Actor, MagicItem* apSpell)
-{   
-    bool result = TiltedPhoques::ThisCall(RealRemoveSpell, apThis, apSpell);
-    if (apThis->GetExtension()->IsLocalPlayer() && result)
-    {
-        // Log spell info
-        //spdlog::info("Removing spell {}, ID: {} from local player", apSpell->GetName() , apSpell->formID);
-        RemoveSpellEvent removalEvent;
-
-        removalEvent.TargetId = apThis->formID;
-        removalEvent.SpellId = apSpell->formID;
-        World::Get().GetRunner().Trigger(removalEvent);
-    }
-
-    return result;
-}
-
-
 bool TP_MAKE_THISCALL(HookCheckAddEffectTargetData, MagicTarget::AddTargetData, void* arArgs, float afResistance)
 {
     if (s_autoSucceedEffectCheck)
@@ -267,7 +245,6 @@ static TiltedPhoques::Initializer s_magicTargetHooks([]() {
     RealGetPerkRank = getPerkRank.Get();
 
     TP_HOOK(&RealAddTarget, HookAddTarget);
-    TP_HOOK(&RealRemoveSpell, HookRemoveSpell);
     TP_HOOK(&RealCheckAddEffectTargetData, HookCheckAddEffectTargetData);
     TP_HOOK(&RealFindTargets, HookFindTargets);
     TP_HOOK(&RealAdjustForPerks, HookAdjustForPerks);

Code/encoding/Messages/RemoveSpellRequest.h

@@ -9,7 +9,7 @@ struct RemoveSpellRequest final: ClientMessage
     virtual ~RemoveSpellRequest() = default;
     void SerializeRaw(TiltedPhoques::Buffer::Writer& aWriter) const noexcept override;
     void DeserializeRaw(TiltedPhoques::Buffer::Reader& aReader) noexcept override;
-    bool operator==(const RemoveSpellRequest& achRhs) const noexcept { return TargetId == achRhs.TargetId && SpellId == achRhs.SpellId && Opcode == achRhs.Opcode; }
+    bool operator==(const RemoveSpellRequest& achRhs) const noexcept { return  Opcode == achRhs.Opcode && TargetId == achRhs.TargetId && SpellId == achRhs.SpellId; }
 
     uint32_t TargetId{};
     GameId SpellId{};