Skip to content

tweak(Controlbar): Allow replay observer to view contained units if not following player #3122

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Mr-Sheerlock wants to merge 3 commits into
base: main
Choose a base branch
from
+51 −16
Open

tweak(Controlbar): Allow replay observer to view contained units if not following player #3122

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d63b811
tweak(Controlbar): Allow observer to view contained units if not foll…
Mr-Sheerlock Aug 13, 2026
fd01ff8
tweak(Controlbar): Disable control and unit buttons for observer
Mr-Sheerlock Aug 17, 2026
0d4cf8d
tweak(Controlbar): Add neutrality check to prevent observers from che…
Mr-Sheerlock Aug 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions Core/GameEngine/Include/GameClient/ControlBar.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -702,6 +702,7 @@ class ControlBar : public SubsystemInterface

Bool hasAnyShortcutSelection() const;
Bool canShowSpecialPowerShortcut() const;
Bool isApparentControllingPlayerNeutral(const Object* obj) const;
void showSpecialPowerShortcut();
void hideSpecialPowerShortcut();
void animateSpecialPowerShortcut( Bool isOn );
Expand Down Expand Up @@ -975,6 +976,7 @@ class ControlBar : public SubsystemInterface
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

@bobtista bobtista Aug 22, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Player *m_observerLookAtPlayer; ///< The current player we're looking at, Null if we're not looking at anyone.
Player *m_observedPlayer; ///< The current player we're observing, Null if we're not observing anyone.

Expand Down
61 changes: 47 additions & 14 deletions Core/GameEngine/Source/GameClient/GUI/ControlBar/ControlBar.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -884,6 +884,7 @@ ControlBar::ControlBar()
m_commandSets = nullptr;
m_controlBarSchemeManager = nullptr;
m_isObserverCommandBar = FALSE;
m_isReadOnly = FALSE;
m_observerLookAtPlayer = nullptr;
m_observedPlayer = nullptr;
m_buildToolTipLayout = nullptr;
Expand Down Expand Up @@ -1321,6 +1322,7 @@ void ControlBar::reset()
m_displayedOCLTimerSeconds = 0;

m_isObserverCommandBar = FALSE; // reset us to use a normal command bar
m_isReadOnly = FALSE;
m_observerLookAtPlayer = nullptr;
m_observedPlayer = nullptr;

Expand Down Expand Up @@ -1479,6 +1481,26 @@ void ControlBar::update()
exitPosition = obj->getObjectExitInterface()->getRallyPoint();

showRallyPoint(exitPosition);

ContainModuleInterface* observerContain = obj ? obj->getContain() : nullptr;
Bool showObserverInventory = (observerContain != nullptr && observerContain->getContainMax() > 0);

@bobtista bobtista Aug 22, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


if (showObserverInventory && m_observerLookAtPlayer == nullptr)
Comment on lines +1485 to +1488

@qodo-free-for-open-source-projects qodo-free-for-open-source-projects Bot Aug 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

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 (!isApparentControllingPlayerNeutral(obj)) {

@bobtista bobtista Aug 22, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

return;
}
Comment on lines +1488 to +1492

@qodo-free-for-open-source-projects qodo-free-for-open-source-projects Bot Aug 17, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remediation recommended

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


if (m_currContext != CB_CONTEXT_STRUCTURE_INVENTORY || m_currentSelectedDrawable != drawToEvaluateFor)
switchToContext(CB_CONTEXT_STRUCTURE_INVENTORY, drawToEvaluateFor);

@bobtista bobtista Aug 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Mr-Sheerlock Mr-Sheerlock Aug 17, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the review. After the new push, none of the buttons are enabled or can be pressed from the observer's POV.

else
updateContextStructureInventory();
}
else if (m_currContext != CB_CONTEXT_OBSERVER_LIST)
{
switchToContext(CB_CONTEXT_OBSERVER_LIST, nullptr);
}

return;
}

Expand Down Expand Up @@ -1805,24 +1827,12 @@ void ControlBar::evaluateContextUI()
ContainModuleInterface *contain = obj->getContain();
if( contain && contain->getContainMax() > 0 )
{

const Player *otherPlayer = contain->getApparentControllingPlayer(ThePlayerList->getLocalPlayer());
if (!otherPlayer)
otherPlayer = obj->getControllingPlayer();
Player *player = ThePlayerList->getLocalPlayer();

if( !player || !otherPlayer )
{
//Sanity.
return;
}
Relationship relation = player->getRelationship( otherPlayer->getDefaultTeam() );

Bool apparentControllingPlayerNeutral = isApparentControllingPlayerNeutral(obj);
//Note: All following checks already account for the fact that this object
//isn't ours.

//The only case we can actually see a non-controlled controlbar is a neutral garrisonable structure.
if( !contain->isGarrisonable() || relation != NEUTRAL )
if( !contain->isGarrisonable() || !apparentControllingPlayerNeutral)
{
//Can't peek inside enemy/allied containers period!
return;
Expand Down Expand Up @@ -2789,6 +2799,7 @@ void ControlBar::setControlBarSchemeByPlayer(Player *p)
if( !p->isPlayerActive() )
{
m_isObserverCommandBar = TRUE;
m_isReadOnly = TRUE;
Comment thread
qodo-free-for-open-source-projects[bot] marked this conversation as resolved.
switchToContext( CB_CONTEXT_OBSERVER_LIST, nullptr );
DEBUG_LOG(("We're loading the Observer Command Bar"));

Expand All @@ -2803,6 +2814,7 @@ void ControlBar::setControlBarSchemeByPlayer(Player *p)
{
switchToContext( CB_CONTEXT_NONE, nullptr );
m_isObserverCommandBar = FALSE;
m_isReadOnly = FALSE;

if (buttonPlaceBeacon)
buttonPlaceBeacon->winHide(
Expand Down Expand Up @@ -2834,6 +2846,7 @@ void ControlBar::setControlBarSchemeByPlayerTemplate( const PlayerTemplate *pt)
if(pt == ThePlayerTemplateStore->findPlayerTemplate(TheNameKeyGenerator->nameToKey("FactionObserver")))
{
m_isObserverCommandBar = TRUE;
m_isReadOnly = TRUE;
switchToContext( CB_CONTEXT_OBSERVER_LIST, nullptr );
DEBUG_LOG(("We're loading the Observer Command Bar"));

Expand All @@ -2848,6 +2861,7 @@ void ControlBar::setControlBarSchemeByPlayerTemplate( const PlayerTemplate *pt)
{
switchToContext( CB_CONTEXT_NONE, nullptr );
m_isObserverCommandBar = FALSE;
m_isReadOnly = FALSE;

if (buttonPlaceBeacon)
buttonPlaceBeacon->winHide(
Expand Down Expand Up @@ -3582,6 +3596,25 @@ Bool ControlBar::canShowSpecialPowerShortcut() const
return false;
}

//-------------------------------------------------------------------------------------------------
Bool ControlBar::isApparentControllingPlayerNeutral(const Object* obj) const
{
ContainModuleInterface* contain = obj->getContain();
const Player* otherPlayer = contain->getApparentControllingPlayer(ThePlayerList->getLocalPlayer());
if (!otherPlayer)
otherPlayer = obj->getControllingPlayer();
const Player* player = ThePlayerList->getLocalPlayer();

if (!player || !otherPlayer)
{
//Sanity.
return FALSE;
}

Relationship relation = player->getRelationship(otherPlayer->getDefaultTeam());
return relation == NEUTRAL;
}

//-------------------------------------------------------------------------------------------------
void ControlBar::updateSpecialPowerShortcut()
{
Expand Down
4 changes: 2 additions & 2 deletions Core/GameEngine/Source/GameClient/GUI/ControlBar/ControlBarStructureInventory.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ void ControlBar::populateButtonProc( Object *obj, void *userData )
GadgetButtonDrawOverlayImage( info->inventoryButtons[ info->buttonIndex ], image );

// Enable the button
info->inventoryButtons[ info->buttonIndex ]->winEnable( TRUE );
info->inventoryButtons[ info->buttonIndex ]->winEnable( !info->self->m_isReadOnly );

@bobtista bobtista Aug 22, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// move to the next button index
info->buttonIndex++;
Expand Down Expand Up @@ -170,7 +170,7 @@ void ControlBar::populateStructureInventory( Object *building )
m_commandWindows[ STOP_ID ]->winHide( FALSE );

// if there is at least one item in there enable the evacuate and stop buttons
if( contain->getContainCount() != 0 )
if(!m_isReadOnly && contain->getContainCount() != 0 )
{
m_commandWindows[ EVACUATE_ID ]->winEnable( TRUE );
m_commandWindows[ STOP_ID ]->winEnable( TRUE );
Expand Down
Loading