Skip to content

[ZH] Implement Headless Mode #651

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.

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
wants to merge 51 commits into
base: main
Choose a base branch
from

Conversation

helmutbuhler
Copy link

@helmutbuhler helmutbuhler commented Apr 11, 2025

This PR adds a commandline option -headless which runs the game without any graphics.
This is necessary to implement a silent replay checker (see issue #565). Note that I have that replay checker already implemented, but I will push that in another PR. This is just preparation for that.

Note that right now, if you pass in -headless, the game just runs the Shellmap. Since it's headless, you won't be able to see or do anything. But if you run it in Debug, you can enjoy looking at the log file getting bigger.

Headless mode is implemented here by essentially only running GameLogic, and not the GameClient. Unfortunately, GameLogic depends on the GameClient in some small hacky ways, which makes this a non-trivial task. See also this comment I added:

// TheSuperHackers @logic-client-separation helmutbuhler 04/11/2025
// Some information about the architecture and headless mode:
// The game is structurally separated into GameLogic and GameClient.
// The Logic is responsible for everything that affects the game mechanic and what is synchronized over
// the network. The Client is responsible for rendering, input, audio and similar stuff.
// 
// Unfortunately there are some places in the code that make the Logic depend on the Client.
// (Search for @logic-client-separation)
// That means if we want to run the game headless, we cannot just disable the Client. We need to disable
// the parts in the Client that don't work in headless mode and need to keep the parts that are needed
// to run the Logic.
// The following describes which parts we disable in headless mode:
//
//	GameEngine:
//		TheGameClient is partially disabled:
//			TheKeyboard = NULL
//			TheMouse = NULL
//			TheDisplay is partially disabled:
//				m_3DInterfaceScene = NULL
//				m_2DScene = NULL
//				m_3DScene = NULL
//				(m_assetManager remains!)
//			TheWindowManager = NULL
//			TheIMEManager = NULL
//			TheTerrainVisual is partially disabled:
//				TheTerrainTracksRenderObjClassSystem = NULL
//				TheW3DShadowManager = NULL
//				TheWaterRenderObj = NULL
//				TheSmudgeManager = NULL
//				TheTerrainRenderObject is partially disabled:
//					m_treeBuffer = NULL
//					m_propBuffer = NULL
//					m_bibBuffer = NULL
//					m_bridgeBuffer = NULL
//					m_waypointBuffer = NULL
//					m_roadBuffer = NULL
//					m_shroud = NULL
//		TheRadar = RadarDummy

As it is now, this PR sets all the client components as listed above to NULL and adds all the appropriate NULL-checks (which are sadly quite a few).
For TheRadar I added a dummy implementation to avoid the NULL-checks in the many callers (this doesn't make much sense in the other components though).

If we decide to fix these separation issues properly in the future, we will be able to revert many of the changes in this PR in the future.

Alternatively, we can also decide to just hide the window in headless mode and initialize DX without using it. Then almost all the changes in this PR become obsolete as well. The question is then whether this will still run in CI.

I would prefer to keep the changes in this PR and to clean up these separation issues properly long-term. Having logic and client properly separated can help in the future when we decide to increase the framerate of the client, or if we want to implement a headless server. It's also nice to automatically test that the logic works without the client.

…th logging enabled (if you define RELEASE_DEBUG_LOGGING in Debug.h)
…in GameLogic::startNewGame to improve CRC Logging
…e with -SaveDebugCRCPerFrame.

Also remove Bool dumped (wasn't really used and was in the way for this new code)
…d and not intended because it spams the crclogging with mostly useless data.
…ewGame instead of GameLogic::prepareNewGame so it works when loading a game.
# Conflicts:
#	GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Object.cpp
@@ -1652,7 +1654,7 @@ const Image *ControlBar::getStarImage(void )
else
m_lastFlashedAtPointValue = ThePlayerList->getLocalPlayer()->getSciencePurchasePoints();

GameWindow *win= TheWindowManager->winGetWindowFromId( NULL, TheNameKeyGenerator->nameToKey( "ControlBar.wnd:ButtonGeneral" ) );
GameWindow *win= TheWindowManager ? TheWindowManager->winGetWindowFromId( NULL, TheNameKeyGenerator->nameToKey( "ControlBar.wnd:ButtonGeneral" ) ) : NULL;
Copy link

Choose a reason for hiding this comment

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

Couldn't you do a check for headless at the beginning as you do in ControlBar::update ?

@helmutbuhler helmutbuhler marked this pull request as ready for review April 15, 2025 20:51
@helmutbuhler
Copy link
Author

Alright, PR #544 is merged and I updated this PR, it's ready for review now. Please read the first post before you review though.

@helmutbuhler helmutbuhler requested a review from xezon April 19, 2025 18:58
@helmutbuhler
Copy link
Author

Quick update with merge conflict fix and an added previously missed null pointer check

@xezon xezon added Enhancement Is new feature or request Major Severity: Minor < Major < Critical < Blocker labels Apr 21, 2025
Copy link

@xezon xezon left a comment

Choose a reason for hiding this comment

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

I would like to see more null implementations where so possible. I suspect the null checks were only placed to avoid crashing, right? I suspect it still runs a lot of logic that is not necessary to run in headless?

@@ -457,12 +457,15 @@ W3DDisplay::~W3DDisplay()

// shutdown
Debug_Statistics::Shutdown_Statistics();
W3DShaderManager::shutdown();
if (!TheGlobalData->m_headless)
Copy link

Choose a reason for hiding this comment

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

This looks suspicious. I would expect W3DDisplay to not even be created in Headless. It looks to be necessary for asset manager and ww math.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, it's unfortunate, but TerrainLogic uses the Asset Manager to get the Terrain data. When we move that data to the TerrainLogic, we'll be able to skip creation of W3DDisplay in headless mode and remove all those checks in that file.

@helmutbuhler
Copy link
Author

helmutbuhler commented Apr 21, 2025

@xezon
Thanks for the review! Hopefully I can implement those requested changes soon.
But in the meantime, maybe it would be a good idea to also test whether these changes allow the game to run in a Github actions container without gpu support. Unfortunately I don't have much experience there, maybe someone else can do that?

Copy link

@xezon xezon left a comment

Choose a reason for hiding this comment

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

I do wonder if more of the null checks can be avoided with early checks

if (TheGlobalData->m_headless)
  return;

or

if (!TheGlobalData->m_headless)
  doRendering();

The advantage of these checks is

  1. avoid more unecessary computations
  2. make our headless related checks easier to find

I am also fond of the dummy implementations.

@@ -281,6 +281,8 @@ Real W3DTerrainLogic::getGroundHeight( Real x, Real y, Coord3D* normal ) const
{
#define USE_THE_TERRAIN_OBJECT
#ifdef USE_THE_TERRAIN_OBJECT
// TheSuperHackers @logic-client-separation helmutbuhler 04/11/2025
Copy link

Choose a reason for hiding this comment

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

Are these logic-client-separation comments complete or are some potentially missing?

// m_waypointBuffer = NULL
// m_roadBuffer = NULL
// m_shroud = NULL
// TheRadar = RadarDummy
Copy link

Choose a reason for hiding this comment

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

RadarDummy, DummyGameWindow, DummyGameWindowManager are not consistently named.

I suggest to put "Dummy" at the end so that it groups properly with the normal class.

Radar
RadarDummy
GameWindow
GameWindowDummy
GameWindowManager
GameWindowManagerDummy

Alternatively can also use "Null" instead of "Dummy". Either name is fine.

@@ -380,6 +380,49 @@ extern WindowMsgHandledType PassMessagesToParentSystem( GameWindow *window,
WindowMsgData mData2 );


// TheSuperHackers @feature helmutbuhler 24/04/2025
// GameWindow that does nothing. Used for Headless Mode.
Copy link

Choose a reason for hiding this comment

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

I suggest to move these dummies into new header + source files because they are a bit on the larger side.

@@ -616,7 +616,7 @@ void W3DView::setCameraTransform( void )
buildCameraTransform( &cameraTransform );
m_3DCamera->Set_Transform( cameraTransform );

if (TheTerrainRenderObject)
if (TheTerrainRenderObject && W3DDisplay::m_3DScene != NULL)
Copy link

Choose a reason for hiding this comment

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

Is a call into W3DView::setCameraTransform needed at all? Can this call not be prevented higher up?

@@ -457,7 +458,7 @@ void W3DGhostObject::restoreParentObject(void)
//removed by the ghost object manager, so restore it. If we have a render
//object that is in the scene, then it was probably added because the model
//changed while the object was ghosted (for damage states, garrison, etc.).
if (robj->Peek_Scene() == NULL)
if (robj->Peek_Scene() == NULL && W3DDisplay::m_3DScene != NULL)
Copy link

Choose a reason for hiding this comment

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

Is a call to W3DGhostObject::restoreParentObject required? Perhaps this call can be prevented higher up?

@@ -655,13 +658,17 @@ Bool W3DTerrainVisual::load( AsciiString filename )
it = NULL;
}
// add our terrain render object to the scene
W3DDisplay::m_3DScene->Add_Render_Object( m_terrainRenderObject );
if (W3DDisplay::m_3DScene != NULL)
Copy link

Choose a reason for hiding this comment

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

Looking at all these m_3DScene tests, I wonder if a few m_headless tests can be placed higher up the call stack or around these m_3DScene usages to avoid the bulk of rendering logic. Assuming that the rendering does not affect game logic, it should be possible to strip a lot of the render updates, no?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Is new feature or request Major Severity: Minor < Major < Critical < Blocker
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants