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 9 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}
| 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
|
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?
97e5f11 to
0d4cf8d
Compare
|
|
||
| if (showObserverInventory && m_observerLookAtPlayer == nullptr) | ||
| { | ||
| if (!isApparentControllingPlayerNeutral(obj)) { |
There was a problem hiding this comment.
This return sits above the else if that restores CB_CONTEXT_OBSERVER_LIST, so a failed neutrality check leaves the previous container's inventory on screen. It's reachable for a defeated player who becomes an observer, though I don't think for a replay observer if that helps.
Maybe
Bool showObserverInventory = observerContain != nullptr
&& observerContain->getContainMax() > 0
&& m_observerLookAtPlayer == nullptr
&& isApparentControllingPlayerNeutral(obj);
if (showObserverInventory)
| showRallyPoint(exitPosition); | ||
|
|
||
| ContainModuleInterface* observerContain = obj ? obj->getContain() : nullptr; | ||
| Bool showObserverInventory = (observerContain != nullptr && observerContain->getContainMax() > 0); |
There was a problem hiding this comment.
Is including non-garrisonable containers intentional? If yes, the fixed 10-slot layout needs bounding (there's a bot comment on populateButtonProc). If not, matching the isGarrisonable() gate resolves both.
There was a problem hiding this comment.
No, I missed this case.
I will go with the bounding solution for generality.
|
|
||
| // Enable the button | ||
| info->inventoryButtons[ info->buttonIndex ]->winEnable( TRUE ); | ||
| info->inventoryButtons[ info->buttonIndex ]->winEnable( !info->self->m_isReadOnly ); |
There was a problem hiding this comment.
The DEBUG_ASSERTCRASH at line 68 is compiled out in release, so buttonIndex keeps incrementing past MAX_STRUCTURE_INVENTORY_BUTTONS (10).
An early return once buttonIndex >= MAX_STRUCTURE_INVENTORY_BUTTONS would make the limit enforced rather than asserted.
| Color m_buildUpClockColor; | ||
|
|
||
| Bool m_isObserverCommandBar; ///< If this is true, the command bar behaves greatly different | ||
| Bool m_isReadOnly; ///< If this is true, the command bar will not allow any commands to be issued |
There was a problem hiding this comment.
m_isReadOnly tracks m_isObserverCommandBar - both are set at the same six sites, always to the same value. Could we have the two inventory call sites use m_isObserverCommandBar directly (or isObserverControlBarOn()) and drop this member?
|
Comments are left unaddressed. Needs push. |
|
Will try to push asap |
…cking inventory during multiplayer
0d4cf8d to
1576210
Compare
|
| Filename | Overview |
|---|---|
| Core/GameEngine/Include/GameClient/ControlBar.h | Declares neutral-controller helpers and the observer inventory population interface. |
| Core/GameEngine/Source/GameClient/GUI/ControlBar/ControlBar.cpp | Selects observer inventory contexts and centralizes actual and apparent controller-neutrality checks. |
| Core/GameEngine/Source/GameClient/GUI/ControlBar/ControlBarCommand.cpp | Makes command, transport-inventory, and production-queue controls non-interactive while preserving observer-visible coloring. |
| Core/GameEngine/Source/GameClient/GUI/ControlBar/ControlBarStructureInventory.cpp | Makes structure inventory read-only for observers, but leaves observer-specific Evacuate and Stop rendering state set after observer mode ends. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Replay observer selects object] --> B{Following a player?}
B -->|Yes| C[Show observer list]
B -->|No| D{Object has capacity and neutral team?}
D -->|No| C
D -->|Yes| E{Garrisonable with no command set?}
E -->|Yes| F[Show read-only structure inventory]
E -->|No| G[Show read-only command context]
Prompt To Fix All With AI
### Issue 1
Core/GameEngine/Source/GameClient/GUI/ControlBar/ControlBarStructureInventory.cpp:178-182
**Observer color state persists**
When replay observation sets `WIN_STATUS_ALWAYS_COLOR` on the Evacuate and Stop controls, the non-observer path never clears it. The same command windows are reused when switching to an active player, so disabled Evacuate and Stop controls keep the observer-specific colored rendering instead of their normal disabled appearance. This is a non-blocking visual inconsistency that can make their state less clear.
```suggestion
if(isObserverControlBarOn())
{
m_commandWindows[ EVACUATE_ID ]->winSetStatus(WIN_STATUS_ALWAYS_COLOR);
m_commandWindows[ STOP_ID ]->winSetStatus(WIN_STATUS_ALWAYS_COLOR);
}
else
{
m_commandWindows[ EVACUATE_ID ]->winClearStatus(WIN_STATUS_ALWAYS_COLOR);
m_commandWindows[ STOP_ID ]->winClearStatus(WIN_STATUS_ALWAYS_COLOR);
}
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (5): Last reviewed commit: "Tweak(ControlBar): Show stop and evacuat..." | Re-trigger Greptile
… to m_isObserverCommandBar
1576210 to
b8bc9c5
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b8bc9c5851
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Bool showObserverInventory = observerContain != nullptr | ||
| && observerContain->getContainMax() > 0 | ||
| && m_observerLookAtPlayer == nullptr | ||
| && isApparentControllingPlayerNeutral(obj); |
There was a problem hiding this comment.
In a live multiplayer match, an observer who is not following a player can satisfy this condition for a civilian building occupied exclusively by undetected stealth units. GarrisonContain::getApparentControllingPlayer() deliberately reports the building's original neutral owner to non-allies in that case, so this check opens the inventory and reveals the hidden occupants' images and veterancy. Add an explicit replay/playback guard, or verify the actual controller before exposing the inventory, to preserve the stealth-garrison mechanic for live observers.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I think the only unit that triggers such behavior is Jarmen Kell (When it's the only unit garrisoned in building) whose place can be discovered using other cues.
You can't view the inventory of the building occupied by an enemy Jarmen
Otherwise all stealth units trigger building garrison apparent to other players too.
| if (m_currContext != CB_CONTEXT_STRUCTURE_INVENTORY || m_currentSelectedDrawable != drawToEvaluateFor) | ||
| switchToContext(CB_CONTEXT_STRUCTURE_INVENTORY, drawToEvaluateFor); |
There was a problem hiding this comment.
When a replay transport carries a passenger whose TransportSlotCount is greater than one, this routes it through populateStructureInventory(), which displays getContainMax() slots without subtracting getExtraSlotsInUse(). The existing transport path in doTransportInventoryUI() explicitly performs that subtraction, so the observer view now shows too many empty slots and misrepresents the transport's remaining capacity. Use the transport inventory accounting here or hide the corresponding extra slots.
Useful? React with 👍 / 👎.
7219d2d to
dde9ebc
Compare
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if(isObserverControlBarOn()) | ||
| { | ||
| m_commandWindows[ EVACUATE_ID ]->winSetStatus(WIN_STATUS_ALWAYS_COLOR); | ||
| m_commandWindows[ STOP_ID ]->winSetStatus(WIN_STATUS_ALWAYS_COLOR); | ||
| } |
There was a problem hiding this comment.
When replay observation sets WIN_STATUS_ALWAYS_COLOR on the Evacuate and Stop controls, the non-observer path never clears it. The same command windows are reused when switching to an active player, so disabled Evacuate and Stop controls keep the observer-specific colored rendering instead of their normal disabled appearance. This is a non-blocking visual inconsistency that can make their state less clear.
| if(isObserverControlBarOn()) | |
| { | |
| m_commandWindows[ EVACUATE_ID ]->winSetStatus(WIN_STATUS_ALWAYS_COLOR); | |
| m_commandWindows[ STOP_ID ]->winSetStatus(WIN_STATUS_ALWAYS_COLOR); | |
| } | |
| if(isObserverControlBarOn()) | |
| { | |
| m_commandWindows[ EVACUATE_ID ]->winSetStatus(WIN_STATUS_ALWAYS_COLOR); | |
| m_commandWindows[ STOP_ID ]->winSetStatus(WIN_STATUS_ALWAYS_COLOR); | |
| } | |
| else | |
| { | |
| m_commandWindows[ EVACUATE_ID ]->winClearStatus(WIN_STATUS_ALWAYS_COLOR); | |
| m_commandWindows[ STOP_ID ]->winClearStatus(WIN_STATUS_ALWAYS_COLOR); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: Core/GameEngine/Source/GameClient/GUI/ControlBar/ControlBarStructureInventory.cpp
Line: 178-182
Comment:
**Observer color state persists**
When replay observation sets `WIN_STATUS_ALWAYS_COLOR` on the Evacuate and Stop controls, the non-observer path never clears it. The same command windows are reused when switching to an active player, so disabled Evacuate and Stop controls keep the observer-specific colored rendering instead of their normal disabled appearance. This is a non-blocking visual inconsistency that can make their state less clear.
```suggestion
if(isObserverControlBarOn())
{
m_commandWindows[ EVACUATE_ID ]->winSetStatus(WIN_STATUS_ALWAYS_COLOR);
m_commandWindows[ STOP_ID ]->winSetStatus(WIN_STATUS_ALWAYS_COLOR);
}
else
{
m_commandWindows[ EVACUATE_ID ]->winClearStatus(WIN_STATUS_ALWAYS_COLOR);
m_commandWindows[ STOP_ID ]->winClearStatus(WIN_STATUS_ALWAYS_COLOR);
}
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
4 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.