Fix critical bugs and improve code quality in FakeParasiteClass

[Copilot]

Addresses multiple crash-inducing bugs and code quality issues in parasite attachment system: missing return values causing undefined behavior, use-after-free access, null pointer dereferences, incorrect timer values causing per-frame damage, and bitwise operations on negative values producing unexpected results.

Critical Fixes

Missing return values - _Load and _Save methods now return S_OK

Use-after-free in __Grapple_AI - Save references before calling __Uninfect():

// Before: this->Victim accessed after __Uninfect() invalidates it
FootClass* victimToSink = this->Victim;
this->__Uninfect();
victimToSink->Destroyed(this->Owner);  // this->Owner invalid

// After: save references before uninfect
FootClass* victimToSink = this->Victim;
FootClass* ownerSaved = this->Owner;
auto pVictimTypeExt = TechnoTypeExtContainer::Instance.Find(victimToSink->GetTechnoType());
this->__Uninfect();
victimToSink->Destroyed(ownerSaved);

Null pointer checks - Added guards for Owner, Victim, weapon, and weaponType->Warhead in __AI(), __Grapple_AI(), __Uninfect(), __Infect(), and __Detach()

Damage timer - Changed from Start(0) to Start(weaponType->ROF) to prevent per-frame damage application

Random offset calculation - Simplified buggy bitwise logic:

// Before: two's complement causes -4 & 0xFC = 252
int randomOffset = Random.RandomBool() ? -4 : 2;
randomOffset = (randomOffset & 0xFC) + 2;  // produces 254 or 2

// After: straightforward random offset
int randomOffset = Random.RandomBool() ? -64 : 64;

Code Quality Improvements

• Magic numbers extracted - Defined ParasiteConstants namespace for bridge height (410), animation frame count (10), Z-offsets (100), height thresholds (200), and overlay ranges (74-99, 205-230)

• Duplication eliminated - Added IsSpecialOverlay() for overlay range checks and ResetOwnerMission() for owner reinitialization logic, removing ~40 lines of duplicated code

• Consistent validation - Unified null checking pattern across all parasite lifecycle methods

Original prompt

FakeParasiteClass Bug Fixes and Improvements

Overview

This PR addresses multiple bugs, potential crashes, and code quality issues identified in the FakeParasiteClass implementation in src/Ext/Parasite/Body.cpp and src/Ext/Parasite/Body.h.

---

Critical Bug Fixes

1. Missing Return Values in _Load/_Save Methods

Location: src/Ext/Parasite/Body.h (lines 64-72)

Problem: The _Load and _Save methods have no return statements, causing undefined behavior since they return HRESULT.

Current Code:

HRESULT __stdcall _Load(IStream* pStm)
{
}

HRESULT __stdcall _Save(IStream* pStm, BOOL clearDirty)
{
}

Fix: Return S_OK or implement proper serialization:

HRESULT __stdcall _Load(IStream* pStm)
{
    return S_OK;
}

HRESULT __stdcall _Save(IStream* pStm, BOOL clearDirty)
{
    return S_OK;
}

---

2. Use-After-Uninfect Bug in __Grapple_AI

Location: src/Ext/Parasite/Body.cpp (lines 369-382)

Problem: After calling __Uninfect(), the code still accesses this->Owner and this->Victim which may be invalidated.

Current Code:

FootClass* victimToSink = this->Victim;
this->__Uninfect();

if (pVictimTypeExt->Sinkable_SquidGrab){
    victimToSink->IsSinking = true;
    victimToSink->Destroyed(this->Owner);  // this->Owner may be invalid!
    victimToSink->Stun();
}
else {
    auto damage = this->Victim->GetTechnoType()->Strength;  // this->Victim is nullptr!
    this->Victim->ReceiveDamage(&damage, 0, RulesClass::Instance->C4Warhead, this->Owner, true, false, this->Owner->Owner);
}

Fix: Save owner reference before uninfect and fix the else branch to use the saved victim:

FootClass* victimToSink = this->Victim;
FootClass* ownerSaved = this->Owner;
auto pVictimTypeExt = TechnoTypeExtContainer::Instance.Find(victimToSink->GetTechnoType());
this->__Uninfect();

if (pVictimTypeExt->Sinkable_SquidGrab) {
    victimToSink->IsSinking = true;
    victimToSink->Destroyed(ownerSaved);
    victimToSink->Stun();
} else {
    int damage = victimToSink->GetTechnoType()->Strength;
    victimToSink->ReceiveDamage(&damage, 0, RulesClass::Instance->C4Warhead, ownerSaved, true, false, ownerSaved ? ownerSaved->Owner : nullptr);
}

---

3. Missing Null Check for Warhead in __AI

Location: src/Ext/Parasite/Body.cpp (line 482)

Problem: weaponType->Warhead is accessed without null check.

Current Code:

victim->ParalysisTimer.Start(weaponType->Warhead->Paralyzes);

Fix: Add null check:

if (!weaponType || !weaponType->Warhead)
{
    return;
}
victim->ParalysisTimer.Start(weaponType->Warhead->Paralyzes);

---

4. Damage Timer Never Uses Weapon ROF

Location: src/Ext/Parasite/Body.cpp (line 479)

Problem: Timer always starts at 0 instead of using weapon ROF, causing damage every frame.

Current Code:

this->DamageDeliveryTimer.Start(0);

Fix: Use weapon's ROF value:

this->DamageDeliveryTimer.Start(weaponType->ROF);

---

5. Random Offset Bitwise Operation Bug

Location: src/Ext/Parasite/Body.cpp (lines 520-521)

Problem: Bitwise AND on negative value gives unexpected results due to two's complement representation.

Current Code:

int randomOffset = ScenarioClass::Instance->Random.RandomBool() ? -4 : 2;
randomOffset = (randomOffset & 0xFC) + 2;

Fix: Simplify the logic to achieve intended behavior:

int randomOffset = ScenarioClass::Instance->Random.RandomBool() ? -64 : 64;

---

Code Quality Improvements

6. Extract Magic Numbers to Named Constants

Location: src/Ext/Parasite/Body.cpp

Add constants at the top of the file or in the header:

namespace ParasiteConstants
{
    constexpr int BridgeHeightOffset = 410;
    constexpr int AnimationFrameCount = 10;
    constexpr int AnimationTimingBase = 128;
    constexpr int SmokeZOffset = 100;
    constexpr int MaxHeightForFalling = 200;
    
    // Overlay index ranges for ice/special terrain
    constexpr int SpecialOverlayRangeStart1 = 74;
    constexpr int SpecialOverlayRangeEnd1 = 99;
    constexpr int SpecialOverlayRangeStart2 = 205;
    constexpr int SpecialOverlayRangeEnd2 = 230;
}

Replace all magic numbers throughout the code with these constants.

---

7. Extract Duplicated Code to Helper Methods

Location: src/Ext/Parasite/Body.cpp

Create helper methods for repeated patterns:

// Add to FakeParasiteClass in Body.h
private:
    void RestoreOwnerToWorld(FootClass* owner, const CoordStruct& coord, DirType direction);
    bool IsSpecialOverlay(int overlayIndex) const;
    void ResetOwnerMission(FootClass* owner);

Implementation in Body.cpp:

bool FakeParasiteClass::IsSpecialOverlay(int overlayIndex) const
{
    return (overlayIndex >= ParasiteConstants::SpecialOverlayRangeStart1 && 
            overlayIndex <= ParasiteConstants::SpecialOverlayRangeEnd1) ||
           (overlayIndex >= ParasiteConstants::SpecialOve...

</details>



<!-- START COPILOT CODING AGENT SUFFIX -->

*This pull request was created as a result of the following prompt from Copilot chat.*
> # FakeParasiteClass Bug Fixes and Improvements
> 
> ## Overview
> This PR addresses multiple bugs, potential crashes, and code quality issues identified in the `FakeParasiteClass` implementation in `src/Ext/Parasite/Body.cpp` and `src/Ext/Parasite/Body.h`.
> 
> ---
> 
> ## Critical Bug Fixes
> 
> ### 1. Missing Return Values in _Load/_Save Methods
> **Location**: `src/Ext/Parasite/Body.h` (lines 64-72)
> 
> **Problem**: The `_Load` and `_Save` methods have no return statements, causing undefined behavior since they return `HRESULT`.
> 
> **Current Code**:
> ```cpp
> HRESULT __stdcall _Load(IStream* pStm)
> {
> }
> 
> HRESULT __stdcall _Save(IStream* pStm, BOOL clearDirty)
> {
> }
> ```
> 
> **Fix**: Return `S_OK` or implement proper serialization:
> ```cpp
> HRESULT __stdcall _Load(IStream* pStm)
> {
>     return S_OK;
> }
> 
> HRESULT __stdcall _Save(IStream* pStm, BOOL clearDirty)
> {
>     return S_OK;
> }
> ```
> 
> ---
> 
> ### 2. Use-After-Uninfect Bug in __Grapple_AI
> **Location**: `src/Ext/Parasite/Body.cpp` (lines 369-382)
> 
> **Problem**: After calling `__Uninfect()`, the code still accesses `this->Owner` and `this->Victim` which may be invalidated.
> 
> **Current Code**:
> ```cpp
> FootClass* victimToSink = this->Victim;
> this->__Uninfect();
> 
> if (pVictimTypeExt->Sinkable_SquidGrab){
>     victimToSink->IsSinking = true;
>     victimToSink->Destroyed(this->Owner);  // this->Owner may be invalid!
>     victimToSink->Stun();
> }
> else {
>     auto damage = this->Victim->GetTechnoType()->Strength;  // this->Victim is nullptr!
>     this->Victim->ReceiveDamage(&damage, 0, RulesClass::Instance->C4Warhead, this->Owner, true, false, this->Owner->Owner);
> }
> ```
> 
> **Fix**: Save owner reference before uninfect and fix the else branch to use the saved victim:
> ```cpp
> FootClass* victimToSink = this->Victim;
> FootClass* ownerSaved = this->Owner;
> auto pVictimTypeExt = TechnoTypeExtContainer::Instance.Find(victimToSink->GetTechnoType());
> this->__Uninfect();
> 
> if (pVictimTypeExt->Sinkable_SquidGrab) {
>     victimToSink->IsSinking = true;
>     victimToSink->Destroyed(ownerSaved);
>     victimToSink->Stun();
> } else {
>     int damage = victimToSink->GetTechnoType()->Strength;
>     victimToSink->ReceiveDamage(&damage, 0, RulesClass::Instance->C4Warhead, ownerSaved, true, false, ownerSaved ? ownerSaved->Owner : nullptr);
> }
> ```
> 
> ---
> 
> ### 3. Missing Null Check for Warhead in __AI
> **Location**: `src/Ext/Parasite/Body.cpp` (line 482)
> 
> **Problem**: `weaponType->Warhead` is accessed without null check.
> 
> **Current Code**:
> ```cpp
> victim->ParalysisTimer.Start(weaponType->Warhead->Paralyzes);
> ```
> 
> **Fix**: Add null check:
> ```cpp
> if (!weaponType || !weaponType->Warhead)
> {
>     return;
> }
> victim->ParalysisTimer.Start(weaponType->Warhead->Paralyzes);
> ```
> 
> ---
> 
> ### 4. Damage Timer Never Uses Weapon ROF
> **Location**: `src/Ext/Parasite/Body.cpp` (line 479)
> 
> **Problem**: Timer always starts at 0 instead of using weapon ROF, causing damage every frame.
> 
> **Current Code**:
> ```cpp
> this->DamageDeliveryTimer.Start(0);
> ```
> 
> **Fix**: Use weapon's ROF value:
> ```cpp
> this->DamageDeliveryTimer.Start(weaponType->ROF);
> ```
> 
> ---
> 
> ### 5. Random Offset Bitwise Operation Bug
> **Location**: `src/Ext/Parasite/Body.cpp` (lines 520-521)
> 
> **Problem**: Bitwise AND on negative value gives unexpected results due to two's complement representation.
> 
> **Current Code**:
> ```cpp
> int randomOffset = ScenarioClass::Instance->Random.RandomBool() ? -4 : 2;
> randomOffset = (randomOffset & 0xFC) + 2;
> ```
> 
> **Fix**: Simplify the logic to achieve intended behavior:
> ```cpp
> int randomOffset = ScenarioClass::Instance->Random.RandomBool() ? -64 : 64;
> ```
> 
> ---
> 
> ## Code Quality Improvements
> 
> ### 6. Extract Magic Numbers to Named Constants
> **Location**: `src/Ext/Parasite/Body.cpp`
> 
> Add constants at the top of the file or in the header:
> ```cpp
> namespace ParasiteConstants
> {
>     constexpr int BridgeHeightOffset = 410;
>     constexpr int AnimationFrameCount = 10;
>     constexpr int AnimationTimingBase = 128;
>     constexpr int SmokeZOffset = 100;
>     constexpr int MaxHeightForFalling = 200;
>     
>     // Overlay index ranges for ice/special terrain
>     constexpr int SpecialOverlayRangeStart1 = 74;
>     constexpr int SpecialOverlayRangeEnd1 = 99;
>     constexpr int SpecialOverlayRangeStart2 = 205;
>     constexpr int SpecialOverlayRangeEnd2 = 230;
> }
> ```
> 
> Replace all magic numbers throughout the code with these constants.
> 
> ---
> 
> ### 7. Extract Duplicated Code to Helper Methods
> **Location**: `src/Ext/Parasite/Body.cpp`
> 
> Create helper methods for repeated patterns:
> 
> ```cpp
> // Add to FakeParasiteClass in Body.h
> private:
>     void RestoreOwnerToWorld(FootClass* owner, const CoordStruct& coord, DirType direction);
>     bool IsSpecialOverlay(int overlayIndex) const;
>     void ResetOwnerMission(FootClass* owner);
> ```
> 
> Implementation in Body.cpp:
> ```cpp
> bool FakeParasiteClass::IsSpecialOverlay(int overlayIndex) const
> {
>     return (overlayIndex >= ParasiteConstants::SpecialOverlayRangeStart1 && 
>             overlayIndex <= ParasiteConstants::SpecialOverlayRangeEnd1) ||
>            (overlayIndex >= ParasiteConstants::SpecialOverlayRangeStart2 && 
>             overlayIndex <= ParasiteConstants::SpecialOverlayRangeEnd2);
> }
> 
> void FakeParasiteClass::ResetOwnerMission(FootClass* owner)
> {
>     if (!owner)
>         return;
>         
>     // Restore player control if applicable
>     if (owner->ShouldBeReselectOnUnlimbo && owner->Owner->ControlledByCurrentPlayer())
>     {
>         auto nCoord = owner->GetCoords();
>         VocClass::PlayIndexAtPos(
>             TechnoTypeExtContainer::Instance.Find(owner->GetTechnoType())->ParasiteExit_Sound.Get(),
>             &nCoord, 
>             false
>         );
>         owner->ShouldBeReselectOnUnlimbo = false;
>         owner->Select();
>     }
> 
>     // Rejoin team if applicable
>     if (TeamClass* team = owner->OldTeam)
>     {
>         team->AddMember(owner, false);
>     }
> 
>     // Reset mission if not busy
>     if (!owner->HaveMegaMission())
>     {
>         owner->SetArchiveTarget(nullptr);
>         owner->SetTarget(nullptr);
>         owner->SetDestination(nullptr, true);
>     }
> 
>     // Enter idle mode
>     owner->EnterIdleMode(false, true);
> }
> ```
> 
> ---
> 
> ### 8. Add Debug Logging for Edge Cases
> **Location**: `src/Ext/Parasite/Body.cpp`
> 
> Add logging in error paths for easier debugging:
> ```cpp
> // In __Uninfect when destroying owner
> Debug::LogInfo("Parasite[{}]: Owner destroyed - suppression timer active", (void*)this);
> 
> // In __Infect when failing to place
> Debug::LogInfo("Parasite[{}]: Failed to place owner after infection failure", (void*)this);
> 
> // In __Grapple_AI for invalid states
> Debug::LogInfo("Parasite[{}]: Invalid grapple state encountered", (void*)this);
> ```
> 
> ---
> 
> ### 9. Fix Inconsistent Null Checks
> **Location**: Throughout `src/Ext/Parasite/Body.cpp`
> 
> Ensure consistent null checking pattern:
> - Always check `this->Owner` before accessing
> - Always check `this->Victim` before accessing
> - Check weapon and warhead pointers before use
> 
> ---
> 
> ## Files to Modify
> - `src/Ext/Parasite/Body.h`
> - `src/Ext/Parasite/Body.cpp`
> 
> ## Testing Recommendations
> 1. Test parasite infection on various unit types (infantry, vehicles)
> 2. Test grapple attacks (squid) on naval units
> 3. Test save/load with active parasites
> 4. Test parasite detachment when victim is destroyed
> 5. Test parasite behavior on bridges and special terrain
> 6. Test parasite behavior when victim enters water/impassable terrain
> 

<!-- START COPILOT CODING AGENT TIPS -->
---

✨ Let Copilot coding agent [set things up for you](https://github.com/Otamaa/Phobos-Minimal/issues/new?title=✨+Set+up+Copilot+instructions&body=Configure%20instructions%20for%20this%20repository%20as%20documented%20in%20%5BBest%20practices%20for%20Copilot%20coding%20agent%20in%20your%20repository%5D%28https://gh.io/copilot-coding-agent-tips%29%2E%0A%0A%3COnboard%20this%20repo%3E&assignees=copilot) — coding agent works faster and does higher quality work when set up for your repo.