.pr_agent_accepted_suggestions

PR 3159 (2026-08-16)

[maintainability] Signed diffuse parameter

---

Signed diffuse parameter

`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).

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]

---

---

PR 3158 (2026-08-16)

[maintainability] Flag semantic mismatch

---

Flag semantic mismatch

Shell::popImmediate(Bool suppressInit) forwards suppressInit into doPop(Bool impendingPush), where the parameter name/meaning differs (it controls whether newTop->runInit runs). This semantic mismatch is introduced by the PR and makes the public API intent harder to follow at the call site.

Issue description

popImmediate(Bool suppressInit) passes its new flag directly into doPop(Bool impendingPush). Although the polarity currently matches the desired behavior (TRUE => skip newTop->runInit), the different naming/abstraction (suppressInit vs impendingPush) is confusing and brittle.

Issue Context

doPop currently uses its boolean to decide whether to run init on the newly exposed top layout. The PR repurposes this parameter by forwarding suppressInit positionally, which obscures intent.

Fix Focus Areas

• Core/GameEngine/Source/GameClient/GUI/Shell/Shell.cpp[428-714]
• Core/GameEngine/Include/GameClient/Shell.h[131-170]

Suggested approach

Pick one:

1. Rename the doPop parameter (declaration + definition) to reflect what it actually controls (e.g., suppressInit or runInit), and update call sites accordingly.
2. Keep doPop(impendingPush) but make the mapping explicit in popImmediate, e.g.:

• doPop(/*impendingPush=*/suppressInit); (or)
• const Bool impendingPush = suppressInit; doPop(impendingPush); This preserves behavior while making intent unambiguous.

---

---

PR 3140 (2026-08-14)

[correctness] Stale smudge count

---

Stale smudge count

The new early-return path in W3DSmudgeManager::render exits before updating m_smudgeCountLastFrame, so SmudgeManager::getSmudgeCountLastFrame() can return a previous-frame value when there are no smudges/sets. This breaks the contract of a “last frame” counter on frames where the global smudge set is empty.

Issue description

W3DSmudgeManager::render now returns early when there are no smudge sets or no smudges in the global set, but it does so before updating m_smudgeCountLastFrame. This leaves SmudgeManager::getSmudgeCountLastFrame() returning a stale non-zero value on subsequent empty frames.

Issue Context

m_smudgeCountLastFrame is only assigned later in render after the visibility pass. With the new early return, that assignment is skipped entirely.

Fix Focus Areas

• Core/GameEngineDevice/Source/W3DDevice/GameClient/W3DSmudge.cpp[311-317]
• Core/GameEngineDevice/Source/W3DDevice/GameClient/W3DSmudge.cpp[429-436]

Proposed fix

Set m_smudgeCountLastFrame = 0; before returning from the new early-return condition (or otherwise ensure it is updated on all return paths that represent “no smudges rendered this frame”).

---

[reliability] Support check never runs

---

Support check never runs

The early return can prevent testHardwareSupport() from ever being called when the global smudge set remains empty, leaving m_hardwareSupportStatus stuck at SMUDGE_SUPPORT_UNKNOWN. Since getHardwareSupport() treats UNKNOWN as supported, upstream code may keep smudge-related processing enabled even on unsupported hardware until at least one smudge exists.

Issue description

W3DSmudgeManager::render now returns before calling testHardwareSupport() when the smudge-set list is empty or the global set has zero smudges. This can leave m_hardwareSupportStatus as SMUDGE_SUPPORT_UNKNOWN indefinitely, while getHardwareSupport() reports support for all states except SMUDGE_SUPPORT_NO.

Issue Context

• SmudgeManager initializes m_hardwareSupportStatus to SMUDGE_SUPPORT_UNKNOWN.
• getHardwareSupport() returns true for UNKNOWN.
• Multiple call sites gate smudge work on getHardwareSupport().
• testHardwareSupport() is the codepath that transitions UNKNOWN -> YES/NO.

Fix Focus Areas

• Core/GameEngineDevice/Source/W3DDevice/GameClient/W3DSmudge.cpp[311-320]
• Core/GameEngineDevice/Source/W3DDevice/GameClient/W3DSmudge.cpp[203-221]
• Core/GameEngine/Include/GameClient/Smudge.h[106-113]
• Core/GameEngine/Source/GameClient/System/Smudge.cpp[36-40]

Proposed fix

Preserve the perf win (skipping Flush/visibility pass/backbuffer copy) while still allowing capability detection by:

1. Calling testHardwareSupport() before the early-return check (it is effectively one-time work due to internal caching), or
2. Calling testHardwareSupport() only when m_hardwareSupportStatus == SMUDGE_SUPPORT_UNKNOWN even if returning early. Either approach avoids leaving hardware support permanently UNKNOWN in smudge-free sessions.

---

---