feature(headless): Add ViewDummy for headless mode#2246
feature(headless): Add ViewDummy for headless mode#2246bobtista wants to merge 4 commits intoTheSuperHackers:mainfrom
Conversation
Greptile Overview
|
| Filename | Overview |
|---|---|
| Core/GameEngine/Include/GameClient/View.h | Adds ViewDummy no-op view for headless mode; needs explicit decision on save/load serialization compatibility and formatting/style consistency. |
| Generals/Code/GameEngine/Source/GameClient/InGameUI.cpp | Refactors InGameUI::init() to always create tactical view and only init/attach when TheDisplay exists; risk of leaving view defaults uninitialized in non-display paths. |
| Generals/Code/GameEngineDevice/Include/W3DDevice/GameClient/W3DInGameUI.h | Factory now returns ViewDummy when TheGlobalData->m_headless; unguarded TheGlobalData dereference can crash if called before global init. |
| GeneralsMD/Code/GameEngine/Source/GameClient/InGameUI.cpp | Same tactical view creation/attach refactor as vanilla; same risk around when setDefaultView() runs. |
| GeneralsMD/Code/GameEngineDevice/Include/W3DDevice/GameClient/W3DInGameUI.h | Same headless createView() change as vanilla; same unguarded TheGlobalData dereference risk. |
Sequence Diagram
sequenceDiagram
participant Caller as Game startup / load
participant UI as InGameUI::init()
participant W3DUI as W3DInGameUI::createView()
participant GD as TheGlobalData
participant View as View / ViewDummy / W3DView
participant Disp as TheDisplay
Caller->>UI: init()
UI->>W3DUI: createView()
W3DUI->>GD: read m_headless
alt headless
W3DUI-->>UI: new ViewDummy
else graphics
W3DUI-->>UI: new W3DView
end
UI->>View: (optional) setDefaultView(...)
alt TheDisplay exists
UI->>View: init()
UI->>Disp: attachView(View)
UI->>View: setWidth/Height(display)
else no display
UI-->>Caller: view exists but not attached
end
| // create the tactical view | ||
| if (TheDisplay) | ||
| TheTacticalView = createView(); | ||
| if (TheTacticalView && TheDisplay) | ||
| { | ||
| TheTacticalView = createView(); | ||
| TheTacticalView->init(); | ||
| TheDisplay->attachView( TheTacticalView ); | ||
|
|
||
| // make the tactical display the full screen width and height | ||
| TheTacticalView->setWidth( TheDisplay->getWidth() ); | ||
| TheTacticalView->setHeight( TheDisplay->getHeight() ); | ||
| TheTacticalView->setDefaultView(0.0f, 0.0f, 1.0f); | ||
| } |
There was a problem hiding this comment.
TheTacticalView->setDefaultView(...) is now only called inside the if (TheTacticalView && TheDisplay) block. In headless mode (or any scenario where TheDisplay == nullptr), this skips setting the view's default parameters, which can leave downstream code reading uninitialized/incorrect view defaults.
If the intent is “always create the view, always set its default parameters, but only init/attach/size when a display exists”, consider moving setDefaultView(0.0f, 0.0f, 1.0f) back outside the display check (guarded only by TheTacticalView).
Prompt To Fix With AI
This is a comment left during a code review.
Path: Generals/Code/GameEngine/Source/GameClient/InGameUI.cpp
Line: 1242:1253
Comment:
`TheTacticalView->setDefaultView(...)` is now only called inside the `if (TheTacticalView && TheDisplay)` block. In headless mode (or any scenario where `TheDisplay == nullptr`), this skips setting the view's default parameters, which can leave downstream code reading uninitialized/incorrect view defaults.
If the intent is “always create the view, always set its default parameters, but only init/attach/size when a display exists”, consider moving `setDefaultView(0.0f, 0.0f, 1.0f)` back outside the display check (guarded only by `TheTacticalView`).
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
This is an empty no-op function in the base class, so calling or not calling it makes no difference. In headless mode, a ViewDummy is used which doesn't need camera defaults since there's no camera.
| // TheSuperHackers @feature bobtista 31/01/2026 | ||
| // View that does nothing. Used for Headless Mode. | ||
| class ViewDummy : public View | ||
| { | ||
| public: | ||
| virtual Drawable *pickDrawable( const ICoord2D *screen, Bool forceAttack, PickType pickType ) { return nullptr; } | ||
| virtual Int iterateDrawablesInRegion( IRegion2D *screenRegion, Bool (*callback)( Drawable *draw, void *userData ), void *userData ) { return 0; } | ||
| virtual void forceRedraw() {} | ||
| virtual const Coord3D& get3DCameraPosition() const { static Coord3D zero = {0,0,0}; return zero; } | ||
| virtual WorldToScreenReturn worldToScreenTriReturn(const Coord3D *w, ICoord2D *s ) { return WTS_INVALID; } | ||
| virtual void screenToWorld( const ICoord2D *s, Coord3D *w ) {} | ||
| virtual void screenToTerrain( const ICoord2D *screen, Coord3D *world ) {} | ||
| virtual void screenToWorldAtZ( const ICoord2D *s, Coord3D *w, Real z ) {} | ||
| virtual void drawView( void ) {} | ||
| virtual void updateView(void) {} | ||
| virtual void stepView() {} | ||
| virtual void setGuardBandBias( const Coord2D *gb ) {} | ||
|
|
||
| protected: | ||
| virtual void xfer( Xfer *xfer ) {} | ||
| }; |
There was a problem hiding this comment.
ViewDummy::xfer() is overridden to a no-op, which will prevent the base View snapshot data from being serialized/deserialized. This can corrupt saves/replays if a headless-generated save is later loaded in a non-headless context, or if any code expects View::xfer() to run to keep snapshot CRC/state consistent.
If the goal is to skip only view blocks during save, it may be safer to keep calling View::xfer(xfer) (or gate the no-op on xfer->getXferMode() / a headless flag) rather than fully disabling serialization for this type.
Prompt To Fix With AI
This is a comment left during a code review.
Path: Core/GameEngine/Include/GameClient/View.h
Line: 334:354
Comment:
`ViewDummy::xfer()` is overridden to a no-op, which will prevent the base `View` snapshot data from being serialized/deserialized. This can corrupt saves/replays if a headless-generated save is later loaded in a non-headless context, or if any code expects `View::xfer()` to run to keep snapshot CRC/state consistent.
If the goal is to skip only view *blocks* during save, it may be safer to keep calling `View::xfer(xfer)` (or gate the no-op on `xfer->getXferMode()` / a headless flag) rather than fully disabling serialization for this type.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Do we ever plan on generating saves in headless mode, then loading them in full graphics mode? If yes, this is worth addressing
There was a problem hiding this comment.
Based on the context from PR #2139, I can see that the goal is to support saving games in headless mode and loading them later. The testing checklist even includes "Load the headless-created save in normal mode and verify it works" which directly answers your question.
Yes, you do plan to generate saves in headless mode and load them in full graphics mode. This is evident from:
- PR bugfix(savegame): Fix crashes when saving a game in headless mode #2139's testing requirements: The checklist specifically includes loading headless-created saves in normal mode
- The broader headless context: This appears to be for running dedicated servers or automated simulations that need to save state and potentially transfer it to clients
Given this, the empty ViewDummy::xfer() implementation is problematic because:
- When a headless save is loaded in graphics mode, the View snapshot data will be missing/corrupted
- The CRC mismatch could cause save validation failures
- Any code expecting View state to be preserved across save/load cycles will break
Recommendation: Call View::xfer(xfer) to preserve the base snapshot data, then conditionally skip only the graphics-specific blocks if needed. This ensures save compatibility while still avoiding unnecessary visual data serialization.
There was a problem hiding this comment.
Do we ever plan on generating saves in headless mode, then loading them in full graphics mode? If yes, this is worth addressing
Maybe not at the moment, perhaps useful in the future though for troubleshooting or generating specific scenarios programmatically or something like that?
| // TheSuperHackers @feature bobtista 31/01/2026 | ||
| // View that does nothing. Used for Headless Mode. | ||
| class ViewDummy : public View |
There was a problem hiding this comment.
The new ViewDummy block adds // TheSuperHackers @feature bobtista ... comments. If this repository is enforcing the standardized prologue/no-author-comments rule, these author-tag comments may violate that convention even though they aren't in the file prologue. Consider removing the author attribution and keeping only the functional description (e.g. “Dummy view used for headless mode”).
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!
Prompt To Fix With AI
This is a comment left during a code review.
Path: Core/GameEngine/Include/GameClient/View.h
Line: 334:336
Comment:
The new `ViewDummy` block adds `// TheSuperHackers @feature bobtista ...` comments. If this repository is enforcing the standardized prologue/no-author-comments rule, these author-tag comments may violate that convention even though they aren't in the file prologue. Consider removing the author attribution and keeping only the functional description (e.g. “Dummy view used for headless mode”).
<sub>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!</sub>
How can I resolve this? If you propose a fix, please make it concise.| virtual Drawable *pickDrawable( const ICoord2D *screen, Bool forceAttack, PickType pickType ) { return nullptr; } | ||
| virtual Int iterateDrawablesInRegion( IRegion2D *screenRegion, Bool (*callback)( Drawable *draw, void *userData ), void *userData ) { return 0; } | ||
| virtual void forceRedraw() {} | ||
| virtual const Coord3D& get3DCameraPosition() const { static Coord3D zero = {0,0,0}; return zero; } | ||
| virtual WorldToScreenReturn worldToScreenTriReturn(const Coord3D *w, ICoord2D *s ) { return WTS_INVALID; } | ||
| virtual void screenToWorld( const ICoord2D *s, Coord3D *w ) {} | ||
| virtual void screenToTerrain( const ICoord2D *screen, Coord3D *world ) {} | ||
| virtual void screenToWorldAtZ( const ICoord2D *s, Coord3D *w, Real z ) {} | ||
| virtual void drawView( void ) {} | ||
| virtual void updateView(void) {} | ||
| virtual void stepView() {} | ||
| virtual void setGuardBandBias( const Coord2D *gb ) {} | ||
|
|
There was a problem hiding this comment.
ViewDummy overrides many methods with one-liners on the same line as the signature (e.g. virtual void forceRedraw() {}). If the project rule is to keep bodies on separate lines for breakpoint placement, these should be expanded.
This pattern also appears in Generals*/Code/GameEngineDevice/Include/W3DDevice/GameClient/W3DInGameUI.h for createView().
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!
Prompt To Fix With AI
This is a comment left during a code review.
Path: Core/GameEngine/Include/GameClient/View.h
Line: 339:351
Comment:
`ViewDummy` overrides many methods with one-liners on the same line as the signature (e.g. `virtual void forceRedraw() {}`). If the project rule is to keep bodies on separate lines for breakpoint placement, these should be expanded.
This pattern also appears in `Generals*/Code/GameEngineDevice/Include/W3DDevice/GameClient/W3DInGameUI.h` for `createView()`.
<sub>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!</sub>
How can I resolve this? If you propose a fix, please make it concise.| // TheSuperHackers @fix bobtista 31/01/2026 Return dummy in headless mode | ||
| virtual View *createView( void ) { return TheGlobalData->m_headless ? NEW ViewDummy : NEW W3DView; } |
There was a problem hiding this comment.
createView() dereferences TheGlobalData unconditionally (TheGlobalData->m_headless). If this factory can run before global data initialization, this will crash in headless startup paths. If TheGlobalData is guaranteed non-null here, it may be worth asserting; otherwise guard with a null check (defaulting to W3DView or ViewDummy depending on intended behavior).
Also appears in GeneralsMD/Code/GameEngineDevice/Include/W3DDevice/GameClient/W3DInGameUI.h:73-74.
Prompt To Fix With AI
This is a comment left during a code review.
Path: Generals/Code/GameEngineDevice/Include/W3DDevice/GameClient/W3DInGameUI.h
Line: 73:74
Comment:
`createView()` dereferences `TheGlobalData` unconditionally (`TheGlobalData->m_headless`). If this factory can run before global data initialization, this will crash in headless startup paths. If `TheGlobalData` is guaranteed non-null here, it may be worth asserting; otherwise guard with a null check (defaulting to `W3DView` or `ViewDummy` depending on intended behavior).
Also appears in `GeneralsMD/Code/GameEngineDevice/Include/W3DDevice/GameClient/W3DInGameUI.h:73-74`.
How can I resolve this? If you propose a fix, please make it concise.| been moved to where all the other translators are attached in game client */ | ||
|
|
||
| // create the tactical view | ||
| if (TheDisplay) | ||
| TheTacticalView = createView(); |
There was a problem hiding this comment.
[P0] TheTacticalView is now always created, but setDefaultView() is only called when TheDisplay exists. Even if View::setDefaultView() is currently a no-op, derived view implementations (e.g., W3DView) may rely on it to initialize camera defaults; in headless mode this can leave view state uninitialized while still being used by non-display code paths.
Consider calling TheTacticalView->setDefaultView(0.0f, 0.0f, 1.0f); unconditionally after createView() (guarded only by TheTacticalView), and keep only init/attach/size under the display check.
Prompt To Fix With AI
This is a comment left during a code review.
Path: Generals/Code/GameEngine/Source/GameClient/InGameUI.cpp
Line: 1240:1243
Comment:
[P0] `TheTacticalView` is now always created, but `setDefaultView()` is only called when `TheDisplay` exists. Even if `View::setDefaultView()` is currently a no-op, derived view implementations (e.g., `W3DView`) may rely on it to initialize camera defaults; in headless mode this can leave view state uninitialized while still being used by non-display code paths.
Consider calling `TheTacticalView->setDefaultView(0.0f, 0.0f, 1.0f);` unconditionally after `createView()` (guarded only by `TheTacticalView`), and keep only `init/attach/size` under the display check.
How can I resolve this? If you propose a fix, please make it concise.| // TheSuperHackers @feature bobtista 31/01/2026 | ||
| // View that does nothing. Used for Headless Mode. | ||
| class ViewDummy : public View |
There was a problem hiding this comment.
[P0] ViewDummy no longer overrides xfer(), but it also doesn’t provide any serialization behavior of its own. If headless mode saves are expected to be loadable in non-headless mode, the view block still needs to be written/read in a compatible way (either by calling View::xfer(xfer) or by writing a compatible stub block). Otherwise, headless-created saves may be missing view data or desync snapshot CRC/state.
If the intent is “dummy view but still serialize base view state”, explicitly add virtual void xfer(Xfer* xfer) override { View::xfer(xfer); }.
Prompt To Fix With AI
This is a comment left during a code review.
Path: Core/GameEngine/Include/GameClient/View.h
Line: 334:336
Comment:
[P0] `ViewDummy` no longer overrides `xfer()`, but it also doesn’t provide any serialization behavior of its own. If headless mode saves are expected to be loadable in non-headless mode, the view block still needs to be written/read in a compatible way (either by calling `View::xfer(xfer)` or by writing a compatible stub block). Otherwise, headless-created saves may be missing view data or desync snapshot CRC/state.
If the intent is “dummy view but still serialize base view state”, explicitly add `virtual void xfer(Xfer* xfer) override { View::xfer(xfer); }`.
How can I resolve this? If you propose a fix, please make it concise.| protected: | ||
|
|
||
| /// factory for views |
There was a problem hiding this comment.
[P0] createView() dereferences TheGlobalData unconditionally (TheGlobalData->m_headless). If W3DInGameUI::init() (or any caller of createView()) can run before TheGlobalData is initialized, this will crash on startup.
Either guard (if (TheGlobalData && TheGlobalData->m_headless) ...) or add an assertion documenting the initialization order guarantee.
Prompt To Fix With AI
This is a comment left during a code review.
Path: Generals/Code/GameEngineDevice/Include/W3DDevice/GameClient/W3DInGameUI.h
Line: 70:72
Comment:
[P0] `createView()` dereferences `TheGlobalData` unconditionally (`TheGlobalData->m_headless`). If `W3DInGameUI::init()` (or any caller of `createView()`) can run before `TheGlobalData` is initialized, this will crash on startup.
Either guard (`if (TheGlobalData && TheGlobalData->m_headless) ...`) or add an assertion documenting the initialization order guarantee.
How can I resolve this? If you propose a fix, please make it concise.| // TheSuperHackers @feature bobtista 31/01/2026 | ||
| // View that does nothing. Used for Headless Mode. | ||
| class ViewDummy : public View |
There was a problem hiding this comment.
[P2] The new ViewDummy includes author-tag comments (// TheSuperHackers @feature bobtista ...). If this repo is trying to keep attribution out of code comments (and especially out of headers), consider replacing these with a short functional comment (e.g., // Dummy view used in headless mode). This keeps the header consistent with the rest of the codebase’s style conventions.
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!
Prompt To Fix With AI
This is a comment left during a code review.
Path: Core/GameEngine/Include/GameClient/View.h
Line: 334:336
Comment:
[P2] The new `ViewDummy` includes author-tag comments (`// TheSuperHackers @feature bobtista ...`). If this repo is trying to keep attribution out of code comments (and especially out of headers), consider replacing these with a short functional comment (e.g., `// Dummy view used in headless mode`). This keeps the header consistent with the rest of the codebase’s style conventions.
<sub>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!</sub>
How can I resolve this? If you propose a fix, please make it concise.| { | ||
| public: | ||
| virtual Drawable *pickDrawable( const ICoord2D *screen, Bool forceAttack, PickType pickType ) { return nullptr; } |
There was a problem hiding this comment.
[P3] Several ViewDummy overrides put the body on the same line as the signature (e.g., forceRedraw() {}). If you’re following the project’s breakpoint-friendly formatting rule, expand these to put the body on its own line.
Also appears in Generals*/.../W3DInGameUI.h for createView().
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!
Prompt To Fix With AI
This is a comment left during a code review.
Path: Core/GameEngine/Include/GameClient/View.h
Line: 337:339
Comment:
[P3] Several `ViewDummy` overrides put the body on the same line as the signature (e.g., `forceRedraw() {}`). If you’re following the project’s breakpoint-friendly formatting rule, expand these to put the body on its own line.
Also appears in `Generals*/.../W3DInGameUI.h` for `createView()`.
<sub>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!</sub>
How can I resolve this? If you propose a fix, please make it concise.
Summary
Related
Split from #2139 per @xezon's suggestion to review each dummy class separately.