refactor(scorch): Store scorch structs in a deque and various minor fixes

[stephanmeesters]

Merge by rebase

• Fixes minor issue where a rejected (duplicated) scorch would still remove an old scorch unnecessarily
• Correctness fix by releasing old scorch buffers before allocating a new one
• Remove debug scorch marks, as they are not very useful
• Refactor scorch deduplication and scorch buffer writes by extracting into their own functions
• Refactor scorch objects by use of a std::deque instead of a fixed-size array

The std::deque is preferable because 1) it allows for a dynamic number of max scorches (if we were to increase in the future, and make quality level dependent); 2) performance.

Todo

• Check that each commit compiles
• Add pull ID to each commit

[qodo-free-for-open-source-projects]

PR Summary by Qodo

Scorch mark queue: dedupe before eviction and store scorches in std::deque

🐞 Bug fix ✨ Enhancement 🕐 20-40 Minutes

AI Description

• Fix scorch deduplication to avoid evicting an existing scorch on rejected duplicates.
• Prevent buffer leaks by releasing existing scorch buffers before reallocation.
• Refactor scorch storage to a deque and extract buffer-write logic for clarity.

Diagram

graph TD
A["addScorch()"] --> B{"Duplicate?"} -->|"yes"| C["Drop request"]
B -->|"no"| D["Scorch deque"] --> E["invalidateBuffers()"] --> F{"Needs recompute?"} -->|"yes"| G["writeScorchToBuffer()"] --> H[("DX8 VB/IB")]
F -->|"no"| I["Draw existing"]
subgraph Legend
  direction LR
  _mod["Module/Function"] ~~~ _dec{"Decision"} ~~~ _buf[("Buffer")]
end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Fixed-size ring buffer (circular array)

• ➕ No per-element allocations; better cache locality than std::deque
• ➕ Keeps stable memory footprint (important in render paths)
• ➕ O(1) push/pop with explicit head/tail indices
• ➖ More custom logic to maintain (wraparound, iteration order)
• ➖ Harder to read than deque; higher bug risk if not well-tested

2. Keep array + head index (avoid shifting)

• ➕ Minimal change from original approach
• ➕ Avoids O(n) shifts while keeping fixed storage
• ➖ Iteration becomes slightly more complex (index mapping)
• ➖ Still custom bookkeeping compared to deque

3. std::vector + erase/front index

• ➕ Contiguous storage (best cache) if managed carefully
• ➕ Familiar container semantics
• ➖ Front erases are O(n) unless using a head index (which reintroduces custom logic)
• ➖ Potential reallocations unless reserved and carefully managed

Recommendation: Current deque-based approach is a reasonable readability/correctness tradeoff for a capped queue (MAX_SCORCH_MARKS). If profiling later shows scorch management is hot or allocation-heavy, consider a fixed-size ring buffer to remove deque block allocations and improve locality—while keeping the same dedupe-before-eviction behavior introduced here.

Files changed (2) +128 / -134

Bug fix (1) +120 / -131

W3DScorch.cppFix dedupe/eviction ordering, free buffers before reallocate, and extract buffer writer +120/-131

Fix dedupe/eviction ordering, free buffers before reallocate, and extract buffer writer

• Ensures duplicate scorches are rejected before evicting the oldest scorch, preventing unintended removals. Calls freeBuffers() at the start of allocateBuffers() to release old GPU resources before allocating new ones, and removes debug-only sample scorches. Refactors buffer rebuild to be gated by m_needBufferRecompute and extracts the per-scorch buffer population into writeScorchToBuffer() with explicit bounds failure signaling.

Core/GameEngineDevice/Source/W3DDevice/GameClient/W3DScorch.cpp

Refactor (1) +8 / -3

W3DScorch.hReplace fixed scorch array with std::deque and new helper declarations +8/-3

Replace fixed scorch array with std::deque and new helper declarations

• Introduces std::deque-backed scorch storage and replaces the previous (array + counters) model with a single container plus a dirty flag (m_needBufferRecompute). Adds private helpers for duplicate detection and writing a scorch into mapped vertex/index buffers.

Core/GameEngineDevice/Include/W3DDevice/GameClient/W3DScorch.h

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Signed diffuse parameter ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

writeScorchToBuffer takes diffuse as signed Int even though it is computed as UnsignedInt
and stored in VertexFormatXYZDUV1::diffuse (unsigned). This introduces an implementation-defined
unsigned→signed conversion and makes the API type-inaccurate (can also trigger signedness warnings).

Code

Core/GameEngineDevice/Include/W3DDevice/GameClient/W3DScorch.h[R80-81]

+	Bool writeScorchToBuffer(const TScorch& scorch, WorldHeightMap& map, Int diffuse,
+	                         VertexFormatXYZDUV1* curVb, UnsignedShort* curIb);

Evidence

The PR introduces a helper whose signature uses Int diffuse, but the value is produced as an
UnsignedInt color and ultimately assigned to an unsigned vertex diffuse field; this mismatch
forces an unsigned→signed conversion before storing back to unsigned.

Core/GameEngineDevice/Include/W3DDevice/GameClient/W3DScorch.h[78-82]
Core/GameEngineDevice/Source/W3DDevice/GameClient/W3DScorch.cpp[154-165]
Core/Libraries/Source/WWVegas/WW3D2/dx8fvf.h[146-154]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`writeScorchToBuffer(...)` accepts `diffuse` as `Int`, but the caller produces an `UnsignedInt` color and the vertex format stores `diffuse` as unsigned. Keeping this parameter signed forces an implementation-defined conversion and misrepresents what the value is.
### Issue Context
- `updateScorches` computes `UnsignedInt diffuse = DX8Wrapper::Convert_Color_Clamp(...)` and passes it to `writeScorchToBuffer`.
- `VertexFormatXYZDUV1::diffuse` is declared as `unsigned`.
### Fix
- Change the `diffuse` parameter type in both declaration and definition from `Int` to `UnsignedInt` (preferred, since it matches the rest of the codebase) or to `unsigned`.
- Ensure the call site remains unchanged (it already has `UnsignedInt diffuse`).
### Fix Focus Areas
- Core/GameEngineDevice/Include/W3DDevice/GameClient/W3DScorch.h[78-82]
- Core/GameEngineDevice/Source/W3DDevice/GameClient/W3DScorch.cpp[154-170]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Tip of the day

💡 Did you know, you can add REVIEW.md to your repo root and Qodo follows it on every PR

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[Caball009]

The PR description doesn't include why std::deque would be preferable here. Can you include that?

[stephanmeesters]

The PR description doesn't include why std::deque would be preferable here. Can you include that?

Done

[Caball009]

It's a bit of a tradeoff. It's clearly a win for the eviction of the oldest scorch mark, but a loss for memory access. An array + head & tail would be faster but make the code more complex so I wouldn't necessarily recommend it.