tweak(Controlbar): Allow observer to view units and structures if not following player - #3122
tweak(Controlbar): Allow observer to view units and structures if not following player#3122Mr-Sheerlock wants to merge 14 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 observer-aware context helpers and makes observer-state accessors const-correct. |
| Core/GameEngine/Source/GameClient/GUI/ControlBar/ControlBar.cpp | Extends context evaluation to observer selections, although the previously reported live-observer information disclosure remains outstanding. |
| Core/GameEngine/Source/GameClient/GUI/ControlBar/ControlBarCommand.cpp | Populates observer-visible command and production controls in a disabled state. |
| Core/GameEngine/Source/GameClient/GUI/ControlBar/ControlBarStructureInventory.cpp | Makes container contents read-only for observers, but the previously reported persistent observer color state remains. |
| Core/GameEngine/Source/GameClient/GUI/ControlBar/ControlBarUnderConstruction.cpp | Disables cancellation while observing, but the previously reported failure to restore the reused button remains. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Selection changes] --> B{Observer control bar active?}
B -- No --> C[Apply ownership and relationship checks]
B -- Yes --> D{Following a player?}
D -- Yes --> E[Show followed-player observer context]
D -- No --> F[Evaluate selected object context]
F --> G[Show inventory, commands, or construction details]
G --> H[Populate controls in disabled read-only state]
C --> I[Show normal player context]
Reviews (11): Last reviewed commit: "tweak(Controlbar): Relax neutrality cons..." | 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!
Does it matter if observers can see that? They can already see the number of pips if there are any occupants. |
This begs the question whether it is ok that Observer in Multiplayer should behave different from Observer in Replay. It probably should not. If we want to restrict Observer capabilities in Multiplayer, then this likely will need to be a much broader feature that confines an observer to a team view. |
|
|
||
| showRallyPoint(exitPosition); | ||
|
|
||
| ContainModuleInterface* observerContain = obj ? obj->getContain() : nullptr; |
There was a problem hiding this comment.
I am surprised by this code block here. Why does it not go through the regular code path to see the inventories?
The issue with replicating all the UI stuff for Observer is that another layer of complexity is added, plus anything not covered here will remain unsupported, for example: CB_CONTEXT_OCL_TIMER, CB_CONTEXT_UNDER_CONSTRUCTION and whatever else.
There was a problem hiding this comment.
tried to address this issue in the latest push.
|
I think it's up for vote. My personal opinion would be be on the more conservative side: to make it opaque for enemy observer in multiplayer to limit communication on voice channels during gameplay. |
What we need is a real Observer mode for competitive play, which is a separate concern (unrelated to this change). For the regular Observer, I do expect that it needs to behave the same in Replay and Match. For example we would also like a Live Streamer to benefit from the same new Observer abilities, right? Otherwise he could only present the full experience in Replay Mode. |
| return localPlayer->getRelationship(otherPlayer->getDefaultTeam()) == NEUTRAL; | ||
| } | ||
|
|
||
| Bool ControlBar::isControllingPlayerNeutral(const Object* obj) const |
There was a problem hiding this comment.
I find these 2 isolated function quite a bit confusing by their name. They sound as if they do almost the same, but they also look substantially different. Do their name accurately represent what they do?
I am asking because if these portions intend to do the same thing, then the code can be streamlined more.
There was a problem hiding this comment.
Yes their names correspond to what they do.
In case of Apparent, it takes into consideration whether the stealth unit inside it wouldn't change the behaviour for an observing player (active and non-active)
| Bool ControlBar::isControllingPlayerNeutral(const Object* obj) const | ||
| { | ||
| const Player* player = ThePlayerList->getLocalPlayer(); | ||
| return player->getRelationship(obj->getTeam()) == NEUTRAL; |
There was a problem hiding this comment.
Is the different with getRelationship(otherPlayer->getDefaultTeam()) and getRelationship(obj->getTeam()) between the two function intentional? What is the difference between that (ignoring getApparentControllingPlayer)?
There was a problem hiding this comment.
No I just abstracted the both of the original implementations before the PR into a function it as is because I was using it elsewhere.
There was a problem hiding this comment.
I see that, but I wonder if there is more to do. The way the functions differ is quite confusing. I wonder if isControllingPlayerNeutral can be a subset of isApparentControllingPlayerNeutral.
There was a problem hiding this comment.
Example:
Bool ControlBar::isApparentControllingPlayerNeutral(const Object* obj) const
{
if (ContainModuleInterface* contain = obj->getContain())
{
Player* localPlayer = ThePlayerList->getLocalPlayer();
if (const Player* otherPlayer = contain->getApparentControllingPlayer(localPlayer))
return localPlayer->getRelationship(otherPlayer->getDefaultTeam()) == NEUTRAL;
}
return isControllingPlayerNeutral(obj);
}
Bool ControlBar::isControllingPlayerNeutral(const Object* obj) const
{
const Player* player = ThePlayerList->getLocalPlayer();
return player->getRelationship(obj->getTeam()) == NEUTRAL;
}Not clear to me if identical. Maybe it is.
Ok I will remove neutrality check on observers if so. |
| if (!isControlEnabled()) { | ||
| win->winSetStatus(WIN_STATUS_ALWAYS_COLOR); | ||
| win->winEnable(FALSE); | ||
| } |
There was a problem hiding this comment.
When control switches from replay-observer mode to an active player, this reused cancel-construction button remains disabled and retains WIN_STATUS_ALWAYS_COLOR. The observer branch sets both states, but the active-player path does not call winEnable(TRUE) or clear the status. As a result, selecting an under-construction object after the switch can leave Cancel Construction unusable.
| if (!isControlEnabled()) { | |
| win->winSetStatus(WIN_STATUS_ALWAYS_COLOR); | |
| win->winEnable(FALSE); | |
| } | |
| if (!isControlEnabled()) | |
| { | |
| win->winSetStatus(WIN_STATUS_ALWAYS_COLOR); | |
| win->winEnable(FALSE); | |
| } | |
| else | |
| { | |
| win->winClearStatus(WIN_STATUS_ALWAYS_COLOR); | |
| win->winEnable(TRUE); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: Core/GameEngine/Source/GameClient/GUI/ControlBar/ControlBarUnderConstruction.cpp
Line: 90-93
Comment:
**Cancel Button Stays Disabled**
When control switches from replay-observer mode to an active player, this reused cancel-construction button remains disabled and retains `WIN_STATUS_ALWAYS_COLOR`. The observer branch sets both states, but the active-player path does not call `winEnable(TRUE)` or clear the status. As a result, selecting an under-construction object after the switch can leave Cancel Construction unusable.
```suggestion
if (!isControlEnabled())
{
win->winSetStatus(WIN_STATUS_ALWAYS_COLOR);
win->winEnable(FALSE);
}
else
{
win->winClearStatus(WIN_STATUS_ALWAYS_COLOR);
win->winEnable(TRUE);
}
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
| @@ -1927,16 +1924,16 @@ void ControlBar::evaluateContextUI() | |||
| } | |||
| else if( obj->getCommandSetString().isEmpty() == FALSE ) | |||
| { | |||
|
|
|||
| switchToContext( CB_CONTEXT_COMMAND, drawToEvaluateFor ); | |||
| if (obj->isLocallyControlled() == TRUE || isObserverControlBarOn() || isControllingPlayerNeutral(obj)) | |||
There was a problem hiding this comment.
The observer-mode exemption also applies to live multiplayer observers. When one selects an allied or enemy container, this condition opens its structure-inventory context, the update guard is bypassed, and the inventory UI displays each contained unit's image and veterancy without another visibility check. The same exemption exposes command and production-queue state, leaking player information that multiplayer observers should not be able to inspect.
How this was verified: Live observers enter observer-control-bar mode, which bypasses the ownership checks before the inventory iterator renders each contained object.
Prompt To Fix With AI
This is a comment left during a code review.
Path: Core/GameEngine/Source/GameClient/GUI/ControlBar/ControlBar.cpp
Line: 1917-1927
Comment:
**Live Observers Reveal Inventories**
The observer-mode exemption also applies to live multiplayer observers. When one selects an allied or enemy container, this condition opens its structure-inventory context, the update guard is bypassed, and the inventory UI displays each contained unit's image and veterancy without another visibility check. The same exemption exposes command and production-queue state, leaking player information that multiplayer observers should not be able to inspect.
**How this was verified:** Live observers enter observer-control-bar mode, which bypasses the ownership checks before the inventory iterator renders each contained object.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Mr-Sheerlock
changed the title
Mr-Sheerlock
changed the title
5a726f9 to
ff1024c
Compare
5 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:
limitations:
a limitation I just thought about: would be lovely if we can select a garrisoned unit and view its own garrisoned units too.