Paths

Table of Contentst

Clean CGame constructor arguments
ClosedPublic
Actions

Authored by elexis on Aug 22 2019, 3:43 PM.

Details

Reviewers: None
Commits: rP22785: Remove default CGame constructor values to make the code less error-prone, use…

Summary

The CGame constructor is spread throughout the codebase.
One problem with that is that it had received multiple arguments throughout it's timeline and they were made optional.
This meant that new code was shorter, but this also meant that it was easy to not check the other CGame constructor calls when the new arguments were introduced, since they were not part of the patch anymore but still affectected by the patch.
This has already caused some bugs where replays were written in atlas and other situations, for example in rP17689.

The responsibility of the decision whether or not to enable visual mode and write a replay is the one of the caller, so the caller should explicitly be forced to decide whether or not to do that rather than possibly not noticing the default value and copy pasting a constructor call.

Here all constructor calls with the values moved to the caller:

Declaration and Definition:

source/ps/Game.cpp:CGame::CGame(bool disableGraphics, bool replayLog):
source/ps/Game.h:	CGame(bool disableGraphics, bool replayLog);

Start a visual match from the UI:

source/ps/scripting/JSInterface_Game.cpp:	g_Game = new CGame(false, true);
source/ps/scripting/JSInterface_SavedGame.cpp:	g_Game = new CGame(false, true);
source/network/scripting/JSInterface_Network.cpp:	g_Game = new CGame(false, true);
source/network/scripting/JSInterface_Network.cpp:	g_Game = new CGame(false, true);

Start atlas:

source/tools/atlas/GameInterface/Handlers/MapHandlers.cpp:		g_Game = new CGame(false, false);

Autostart a visual or non-visual match:

source/ps/GameSetup/GameSetup.cpp:	g_Game = new CGame(nonVisual, !args.Has("autostart-disable-replay"));

Autostart a visual replay:

source/ps/GameSetup/GameSetup.cpp:	g_Game = new CGame(false, false);

Autostart a non-visual replay:

source/ps/Replay.cpp:	g_Game = new CGame(true, false);

Tests:

source/network/tests/test_Net.h:		CGame client1Game(true, false);
source/network/tests/test_Net.h:		CGame client2Game(true, false);
source/network/tests/test_Net.h:		CGame client3Game(true, false);
source/network/tests/test_Net.h:		CGame client1Game(true, false);
source/network/tests/test_Net.h:		CGame client2Game(true, false);
source/network/tests/test_Net.h:		CGame client3Game(true, false);
source/network/tests/test_Net.h:		CGame client2BGame(true, false);

But now where I see that, it would also be more consistent to speak of Visual mode (positively), always, rather than functions with "Visual" in their name having to pass nonvisual=false.
"true" represents a feature (renderer, sound) being enabled.

This patch is a preparation to make D2197 a bit cleaner.

Test Plan

If you do not already remember the revision history, you can blame for the constructor in CGame.h.
If you want to see some of the issues with that constructor, you (amongst other cases) blame for the following monster condition in GameSetup.cpp:

if (g_Game && g_Game->IsGameStarted() && !g_Game->IsVisualReplay() &&

g_AtlasGameLoop && !g_AtlasGameLoop->running && !nonVisual)

Diff Detail

Repository

rP 0 A.D. Public Repository

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

Event Timeline

elexis created this revision.Aug 22 2019, 3:43 PM

I would prefer using enum-flags to make these parameters more readable then.

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/ps/Game.h
|  41| class·CGame
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCGame{' is invalid C code. Use --std or --language to configure the language.

source/ps/scripting/JSInterface_Game.cpp
|   1| /*·Copyright·(C)·2018·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2018"

source/network/tests/test_Net.h
|  37| class·TestNetComms·:·public·CxxTest::TestSuite
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classTestNetComms:' is invalid C code. Use --std or --language to configure the language.
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/450/display/redirect

Harbormaster completed remote builds in B9000: Diff 9439.Aug 22 2019, 3:55 PM

I don't mind much what our personal opinions are, it matters what the effect on future readers is that have never seen or met us.
So we need to stop arguing with saying "I like X", since it hides the argument.
A disadvantage of enum flags is that they define a new data structure and that they require parsing that is sometimes done uglily using switch and even loops.
But in this case those are only two booleans, two bits, so it should be possible to get away with one arg & const each and it provides a readable name.
So I'm afraid the flags are preferable in this single instance even if you had the opposite opinion or if flags are worse in other cases.

Also renaming GraphicsDisabeld to IsVisual and nonvisual to IsVisual while at it.

It would be great if one could avoid passing the first argument altogether and just check if a renderer exists, but it seems to be the case that its not known at the time of calling the constructor.

Looking further, we can obtain it directly whether "visual" mode is enabled or not. Visual means we have a g_Renderer.

And with that it's only one boolean which should be easy enough to keep track of as long as one doesn't hide the default in header remote to the calling files.

Test whether the renderer is open instead of passing this information around under different names.

elexis added inline comments.Aug 22 2019, 4:37 PM

source/network/tests/test_Net.h
214 ↗	(On Diff #9440)	Here you see another reason why I consider default values to be an anti-pattern. It looks like a conscious decision to change a boolean from true to false, but in fact the first argument was deleted and the second argument takes it place without having been changed, while in other callers that provided all arguments you can see the change directly without having to look elsewhere, thus also making it easier to audit, not only maintain too.
source/renderer/Renderer.h
126 ↗	(On Diff #9440)	`CRendererInternals` has not been declared and not been implemented yet, so the implementation has to go to cpp or tons of code moved.

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/ps/Game.h
|  41| class·CGame
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCGame{' is invalid C code. Use --std or --language to configure the language.

source/ps/scripting/JSInterface_Game.cpp
|   1| /*·Copyright·(C)·2018·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2018"

source/renderer/Renderer.h
|  69| class·CRenderer·:
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCRenderer:' is invalid C code. Use --std or --language to configure the language.

source/network/tests/test_Net.h
|  37| class·TestNetComms·:·public·CxxTest::TestSuite
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classTestNetComms:' is invalid C code. Use --std or --language to configure the language.

source/tools/atlas/GameInterface/Handlers/MapHandlers.cpp
| 211| »   »   g_Renderer.GetWaterManager(),·g_Renderer.GetSkyManager(),
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Unmatched '}'. Configuration: 'MESSAGESSETUP_NOTFIRST'.
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/451/display/redirect

Harbormaster completed remote builds in B9001: Diff 9440.Aug 22 2019, 4:47 PM

elexis mentioned this in rP22432: Adds a possibility to disable saving of replay in autostart mode..Aug 22 2019, 5:02 PM

Use Singleton method CRenderer::IsInitialised() instead of adding a Renderer boolean getter and doing something that crashes everything immediately all the time.

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/ps/Game.h
|  41| class·CGame
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCGame{' is invalid C code. Use --std or --language to configure the language.

source/network/tests/test_Net.h
|  37| class·TestNetComms·:·public·CxxTest::TestSuite
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classTestNetComms:' is invalid C code. Use --std or --language to configure the language.

source/ps/scripting/JSInterface_Game.cpp
|   1| /*·Copyright·(C)·2018·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2018"

source/tools/atlas/GameInterface/Handlers/MapHandlers.cpp
| 211| »   »   g_Renderer.GetWaterManager(),·g_Renderer.GetSkyManager(),
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Unmatched '}'. Configuration: 'MESSAGESSETUP_NOTFIRST'.
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/452/display/redirect

Harbormaster completed remote builds in B9002: Diff 9441.Aug 22 2019, 5:15 PM

elexis mentioned this in rP18613: Always save the replay metadata (summary screen info) when ending the….Aug 22 2019, 5:16 PM

elexis mentioned this in rP19645: Add a -autostart-nonvisual option. Patch by sacha_vrand. Fixes #4577..Aug 22 2019, 5:32 PM

elexis edited the summary of this revision. (Show Details)

Notice that there are 36 occurrences of CRenderer::IsInitialised() after this patch (and 30 before), so it's not a new thing to test for this at all.
It just shows that we made up some random concepts in our mind ("visual mode") that doesn't really exist as something different from "renderering enabled".

One might argue that this patch prohibits running multiple games in parallel non-visually while we have a renderer.
But that's not true, because if one would want to consider this to be a feature, then the logic itself needs to be changed as well. So the according lines where nonVisual is tested have to be changed then too and many other places probably do.
Mostly all of those CRenderer::IsInitialised() checks that already exist would have to become CRenderer::IsInitialised() && weWantToUseItToo then.

So I suggest to remove the dead limb and one can attach a healthy different one in a different place if one wants to instantiate multiple CGames, or instantiate them without renderer use if the renderer was enabled.

source/ps/Game.h
84 ↗	(On Diff #9441)	disableGraphics from rP7653. replayLog argument from rP16727. using the default value incorrectly for atlas fixed by rP17689.
152 ↗	(On Diff #9441)	From rP19645.

elexis added inline comments.Aug 25 2019, 1:02 PM

source/ps/GameSetup/GameSetup.cpp
752 ↗	(On Diff #9441)	I suppose one could avoid the local variable, but it's more obvious that the `const bool` is not affected by the destructor call than `CRenderer::IsInitialised()` being unaffected by calling its destructor, and it looks a bit strange `IsInitialized = true` after destructor.