Skip to content

fix: surface speed effect removal, type correctness, and code cleanup#226

Merged
plenarius merged 1 commit into
masterfrom
fix/surface-speed-effects-and-cleanup
Apr 19, 2026
Merged

fix: surface speed effect removal, type correctness, and code cleanup#226
plenarius merged 1 commit into
masterfrom
fix/surface-speed-effects-and-cleanup

Conversation

@plenarius

Copy link
Copy Markdown
Owner

Summary

  • Fix Expeditious Retreat being stripped by road transitions: Effect removal now uses RemoveEffectByTag with ID-based removal instead of deferred AIMasterEvent::RemoveEffect with raw CGameEffect* pointers, which could match and remove unrelated MovementSpeedIncrease effects
  • Fix undefined engine behavior for slowing surfaces: Uses MovementSpeedDecrease with std::abs(effectChange) for negative speed modifiers (mud, water, snow, sand) instead of MovementSpeedIncrease with negative values
  • Fix unbounded map growth: Uses find() on m_SurfaceMaterialSpeeds instead of operator[] which silently inserted default entries for every unknown surface material
  • Fix RemoveEffectByTag calling RemoveEffectById(0) 128 times: vector(128) pre-filled 128 zeros before real IDs; changed to reserve(128)
  • Fix server crash on malformed rune tags: Extracted ParseRuneTag helper with bounds checking; callers guard against sentinel return
  • Fix OOB vector access in NextColor/PreviousColor: Added bounds checks before accessing v[index] at vector boundaries
  • Allow weapon color 0: SetItemAppearance now accepts val >= 0 and NextColor wrap-around searches from 0
  • Code cleanup: Extracted GetRaceVFXOffset helper (3x dedup), removed dead m_GetItemInSlotHook member, removed duplicate #include, fixed %d%zu for size_t

Fixes nwnxee#374

Test plan

  • Cast Expeditious Retreat, walk onto a road tile and back off — speed bonus should persist
  • Walk through mud/water/snow/sand — movement should slow correctly
  • Walk between road and non-road tiles rapidly — no effect stacking
  • Use rune system (apply, combine, describe) — functionality unchanged
  • Cycle weapon colors forward/backward at boundaries — no crash
  • Set weapon color to 0 — should work

- Fix effect removal stripping unrelated MovementSpeedIncrease effects
  (e.g. Expeditious Retreat) by using RemoveEffectByTag with ID-based
  removal instead of deferred AIMaster RemoveEffect with raw pointers
- Use MovementSpeedDecrease for negative speed modifiers (mud, water,
  snow, sand) instead of MovementSpeedIncrease with negative values
- Use find() on m_SurfaceMaterialSpeeds to avoid unbounded map growth
  from operator[] inserting defaults for unknown surface materials
- Fix vector(128) pre-filling 128 zeros in RemoveEffectByTag; use
  reserve(128) instead
- Extract ParseRuneTag helper with bounds checking to eliminate 4x
  duplicated tag parsing across ApplyRune, CombineRunes,
  GetRuneDescription, and prevent server crash on malformed tags
- Extract GetRaceVFXOffset helper to eliminate 3x duplicated
  race-to-VFX-offset mapping
- Fix NextColor/PreviousColor OOB vector access at boundaries
- Allow weapon color 0 in SetItemAppearance and NextColor wrap-around
- Remove unused m_GetItemInSlotHook member, duplicate include, fix
  LOG_INFO format specifier for size_t

Fixes nwnxee#374
@plenarius
plenarius merged commit 6847fac into master Apr 19, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant