tweak(Controlbar): Allow replay observer to view contained units if not following player - #3122
tweak(Controlbar): Allow replay observer to view contained units if not following player#3122Mr-Sheerlock wants to merge 3 commits into
Conversation
PR Summary by QodoObserver ControlBar: read-only structure inventory view when not following a player
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
| ContainModuleInterface* observerContain = obj ? obj->getContain() : nullptr; | ||
| Bool showObserverInventory = (observerContain != nullptr && observerContain->getContainMax() > 0); | ||
|
|
||
| if (showObserverInventory && m_observerLookAtPlayer == nullptr) |
There was a problem hiding this comment.
1. Inventory slots overflow 🐞 Bug ☼ Reliability
ControlBar::update() now switches observers into CB_CONTEXT_STRUCTURE_INVENTORY for any selected object with getContainMax() > 0, but the structure inventory UI only supports 10 occupant slots. If a container ever has >10 contained objects (e.g., tunnel networks when MaxTunnelCapacity is configured above 10), populateStructureInventory() will call populateButtonProc() past the supported slot count, tripping the MAX_STRUCTURE_INVENTORY_BUTTONS assert and/or overwriting non-inventory buttons.
Agent Prompt
### Issue description
Observer mode now routes any selectable container (ContainMax > 0) into `CB_CONTEXT_STRUCTURE_INVENTORY`. The structure inventory UI is hard-limited to `MAX_STRUCTURE_INVENTORY_BUTTONS` (10). If `iterateContained()` yields more than 10 occupants, `populateButtonProc()` hits its `DEBUG_ASSERTCRASH` (and in non-assert builds can start repurposing the Stop/Evacuate buttons and potentially go beyond UI expectations).
### Issue Context
- `ControlBar::update()` (observer branch) uses only `getContainMax() > 0` as the gate.
- `populateStructureInventory()` iterates *all* contained objects and calls `populateButtonProc()`.
- `populateButtonProc()` asserts `buttonIndex < MAX_STRUCTURE_INVENTORY_BUTTONS`.
- Tunnel network capacity is configurable via `GlobalData::m_maxTunnelCapacity` (INI: `MaxTunnelCapacity`), so it can exceed 10.
### Fix Focus Areas
- Core/GameEngine/Source/GameClient/GUI/ControlBar/ControlBar.cpp[1483-1496]
- Core/GameEngine/Source/GameClient/GUI/ControlBar/ControlBarStructureInventory.cpp[63-90]
- Core/GameEngine/Source/GameClient/GUI/ControlBar/ControlBarStructureInventory.cpp[179-196]
### What to change
Implement *one* of these safe guards (preferably both A and B):
1. **A (UI-level hardening):** In `populateButtonProc()`, if `buttonIndex >= MAX_STRUCTURE_INVENTORY_BUTTONS`, return early (do not write into `m_containData` / do not enable controls). This prevents asserts/crashes and prevents Stop/Evacuate slots from being repurposed.
2. **B (observer routing guard):** In observer `update()`, only route to `CB_CONTEXT_STRUCTURE_INVENTORY` when `observerContain->getContainCount() <= MAX_STRUCTURE_INVENTORY_BUTTONS` (or clamp display to 10 with a clear rule). If count exceeds, fall back to `CB_CONTEXT_OBSERVER_LIST` or add paging/scrolling support.
Include an explicit comment explaining the 10-slot UI limitation so future changes to tunnel capacity don’t reintroduce the problem.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| if (showObserverInventory && m_observerLookAtPlayer == nullptr) | ||
| { | ||
| if (m_currContext != CB_CONTEXT_STRUCTURE_INVENTORY || m_currentSelectedDrawable != drawToEvaluateFor) | ||
| switchToContext(CB_CONTEXT_STRUCTURE_INVENTORY, drawToEvaluateFor); |
There was a problem hiding this comment.
Could the observer inventory be made read-only at the UI boundary? populateStructureInventory() enables the occupant, Evacuate, and Stop buttons, and assigning a real m_currentSelectedDrawable lets clicks reach processCommandUI() and emit MSG_EXIT, MSG_EVACUATE, or MSG_DO_STOP.
To be fair, this doesn't currently do anything, but coincidentally so - would be good to make that intentional. Qodo also commented about this
There was a problem hiding this comment.
Thank you for the review. After the new push, none of the buttons are enabled or can be pressed from the observer's POV.
|
The buttons should not be clickable for an observer / non-owning player. Please fix other issues when there are. |
f3f0ec5 to
cdd3316
Compare
cdd3316 to
fd01ff8
Compare
Mr-Sheerlock
changed the title
| if (showObserverInventory && m_observerLookAtPlayer == nullptr) | ||
| { | ||
| if (!isApparentControllingPlayerNeutral(obj)) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
2. Observer inventory early return 🐞 Bug ≡ Correctness
In observer-mode ControlBar::update(), selecting a container that fails isApparentControllingPlayerNeutral(obj) returns immediately, bypassing the fallback that switches the UI back to CB_CONTEXT_OBSERVER_LIST. This can leave stale structure-inventory UI visible after selecting a disallowed container.
Agent Prompt
## Issue description
Observer-mode `ControlBar::update()` returns early when a selected container is not neutral. That return occurs before the code that restores `CB_CONTEXT_OBSERVER_LIST`, so the control bar can remain in a previous context (e.g., structure inventory) even though the current selection is disallowed.
## Issue Context
This is in the `if (m_isObserverCommandBar)` update path and only triggers for selected objects with a contain module.
## Fix Focus Areas
- Core/GameEngine/Source/GameClient/GUI/ControlBar/ControlBar.cpp[1485-1502]
## Suggested fix
Replace the early `return` with a controlled fallback:
- Either switch to `CB_CONTEXT_OBSERVER_LIST` (or `CB_CONTEXT_NONE`) before returning, or
- Restructure the logic so the existing `else if (m_currContext != CB_CONTEXT_OBSERVER_LIST)` branch remains reachable when the neutral check fails.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit 9660d40 |
9660d40 to
97e5f11
Compare
|
Okay I think the PR should be ready for review. |
|
However I also found one detail regarding the Jarmen Kell and didn't find it anywhere in the issues. |
| else | ||
| { | ||
| switchToContext( CB_CONTEXT_NONE, nullptr ); | ||
| m_isObserverCommandBar = FALSE; |
There was a problem hiding this comment.
Should clear m_isReadOnly here too right?
…cking inventory during multiplayer
97e5f11 to
0d4cf8d
Compare
3 participants
This PR is an enhancement/suggestion for observer Controlbar functionality. It allows an observer to view exact units contained by a building/unit.
I added the
m_observerLookAtPlayernull check because otherwise when following a player and the player selects a containing building/unit the UI doesn't support a way to stop following the player.Verification:
1: For replays:
2: For multiplayer
Potential issues:
and also a limitation I just thought about: would be lovely if we can select a garrisoned unit and view its own garrisoned units too.
Edit:
Added more tests in the verification.