Paths

Table of Contentst

Initialise PlayerData in Atlas' ActorViewer
ClosedPublic
Actions

Authored by s0600204 on Feb 1 2018, 6:04 PM.

Details

Reviewers

Stan
elexis

Commits

rP21167: Load and Initialise PlayerData in Atlas' ActorViewer

Trac Tickets

#4900

Summary

As described in ticket #4900, selecting an item from the list in the ActorViewer within Atlas resulted in warnings concerning undefined properties.

This appears to be because the ActorViewer has its own simulation instance, separate from Atlas' main instance. And inside the ActorViewer's instance, players were neither defined nor initialised.

What follows is one possible solution.

Test Plan

Open Atlas
Go to the entity tab
Open the actor viewer
Pick an actor
Confirm that the warnings reported in #4900 no longer appear.

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

s0600204 created this revision.Feb 1 2018, 6:04 PM

Owners added a subscriber: Restricted Owners Package.Feb 1 2018, 6:04 PM

elexis awarded a token.Feb 1 2018, 6:46 PM

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

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (309 tests).....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (309 tests).....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

Executing section Default...
Executing section Source...

source/tools/atlas/GameInterface/ActorViewer.cpp
|  33| #include·"graphics/Terrain.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: ''.

source/tools/atlas/GameInterface/ActorViewer.cpp
|  33| #include·"graphics/Terrain.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: 'MESSAGES_SKIP_STRUCTS'.

source/tools/atlas/GameInterface/ActorViewer.cpp
|  33| #include·"graphics/Terrain.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: '_MSC_VER'.
Executing section JS...

Harbormaster completed remote builds in B4703: Diff 5531.Feb 1 2018, 6:49 PM

I was wondering if it shouldn't receive independent player data, default player data, since the sim state of the main game should not influence the actor viewer, right? Like it shouldn't matter which player color and civ was chosen or if some promotion technology was researched.
What's the worst that could happen - the actor viewer displaying a different actor depending on the researched techs?
But this proposal, if it works as intended, seems more than sufficient and fixes a huge error.
Thanks for investigating and writing up a fix!

Testing that every added line is correct shouldn't be hard, testing that the patch is complete should not be forgotton if we want to achieve the best quality. One could compare which other simulation2 init calls exist in similar places. I found PreInitGame quickly but didn't investigate, do we need that? Do we miss out anything else? Maybe some destructors?
If those questions are answered and the patch tested quickly, I think this can go in.

source/tools/atlas/GameInterface/ActorViewer.cpp
1 ↗	(On Diff #5531)	8

Stan awarded a token.Feb 1 2018, 8:13 PM

I'll try to test this depending on my schedule.
@elexis might be better at reviewing cpp code as I'm not that familiar with it.

I agree with him that colors should be independent of the main simulation.

Indeed one should consider memory leaks which are(were) a common thing with Atlas.

Actually the actor viewer should maybe split from Atlas and merged with the actor editor or both merged with Atlas. Need to create a ticket about getting the anims from the actors instead of that ugly hardcoding.

In D1276#51977, @elexis wrote:

I found PreInitGame quickly but didn't investigate, do we need that?

In source/ps/Game.cpp, a developer wrote:

// Perform some simulation initializations (replace skirmish entities, explore territories, etc.)
// that needs to be done before setting up the AI and shouldn't be done in Atlas
if (!g_AtlasGameLoop->running)
	m_Simulation2->PreInitGame();

In D1276#51977, @elexis wrote:

Testing that every added line is correct shouldn't be hard,

I've added some (rather lengthy, sorry) line comments explaining my reasoning behind them. Hopefully that will help with determining the "correctness" of each commented-on added line.

Do we miss out anything else? Maybe some destructors?

Don't think so. (To both questions).

In D1276#51992, @Stan wrote:

@elexis might be better at reviewing cpp code as I'm not that familiar with it.

Nor am I. And weren't you wanting to learn ?? Take your time if you need it. This issue's been around for a few years - it can wait.

Indeed one should consider memory leaks which are(were) a common thing with Atlas.

I remember.

I don't think this adds a memory leak. And when ActorViewer is not the "active tool", I believe ActorViewer's simulation stops/pauses. (Heck, the simulation appears to only run when there's an animation playing.) That hasn't been changed here.

source/tools/atlas/GameInterface/ActorViewer.cpp
1 ↗	(On Diff #5531)	Heh, oops. ~~Can~~ Will be fixed on commit, if an updated patch proves unnecessary.
296 ↗	(On Diff #5531)	In D1276#51977, @elexis wrote: I was wondering if it shouldn't receive independent player data, default player data, [...] In D1276#51992, @Stan wrote: I agree with him that colors should be independent of the main simulation. If this line wasn't included, then `simulation/helpers/Player.js` (a function from which is called with the next line) would fallback to loading default player data from the appropriate source. Unfortunately a warning is emitted when this happens, presumably to catch maps that don't have any player data (so IMO worth keeping). And as the point of this revision was to deal with why the current warnings are given, replacing one warning with another seems to run counter to intent.
297 ↗	(On Diff #5531)	This is the important line of this revision. The other added lines handle things that could be addressed with alterations elsewhere, but this line is the line that resolves the main issue that this revision addresses.
310 ↗	(On Diff #5531)	This line creates an empty set of init settings. The only other location in cpp[1] that the function on the next line (`InitGame`) gets called is `source/ps/Game.cpp`. [2] There, the init settings are requested from `simulation2`, and then passed (without modification) in order to init the game. The reason why we don't fetch the init settings from `simulation2` here is because we haven't set any to begin with (and thus requesting them would give us the same thing as we're creating, but do so in a more long-winded way). (One might wonder why we need to pass an argument to a function containing data that is already accessible to said function.) [1] - `ack --type=cpp "[\>\.]InitGame"` [2] - There is also a similarly named function in `source/tools/atlas/GameInterface/Handlers/MapHandlers.cpp` which (re)initialises the `g_Game` global.
311 ↗	(On Diff #5531)	We run `InitGame` so that components' `OnInitGame` and/or `OnGlobalInitGame` function(s) get called. This includes the relevant function in the JS component `StatisticsTracker`. I single this out for special mention as this component includes a function that gets called every so often (currently every 30s) on a timer[1]. The function that gets called by the timer tries to access the value of a member-object (`this.sequences`) that is specified in `StatisticsTracker`'s `Init()` but only defined/given a value in `OnGlobalInitGame()`. Needless to say, if the object has not been defined, multiple error messages appear (one for each player) - once initially when the `ActorViewer` becomes the "active tool", and then again every 30 seconds (so long as the `ActorViewer` is the "active tool" and there's an animation running). [1] - It might be possible to set this timer up in `OnGlobalInitGame` rather than in `Init`, but that is beyond the scope of this revision.

elexis added inline comments.Feb 4 2018, 5:52 PM

source/tools/atlas/GameInterface/ActorViewer.cpp
310 ↗	(On Diff #5531)	I can't see a reason just from the code either why the matchsettings are first set by `CGame::ReallyStartGame` using `m_Simulation2->InitGame(settings);`, exposed via `CSimulation2::GetInitAttributes()` but then changing ones mind and starting the simulation with the the settings passed to `Simulation2.InitGame`. (To decide, one could lookup the commit history to estimate if that is historical leftover.)
311 ↗	(On Diff #5531)	Ah, good that you found this missing call. That should be the fix to #4996 then.

Tested, works. Finally the number of errors per selection is 0 instead of 1. Thanks a lot!
The playerdefault loading can be considered before or after a commit, maybe, maybe not in a ticket.

source/tools/atlas/GameInterface/ActorViewer.cpp
296 ↗	(On Diff #5531)	Could `CSimulation2::GetPlayerDefaults()` be used to load and set the defaults without the warning being emitted in the next call?
311 ↗	(On Diff #5531)	I could still reproduce the bug described in that ticket.

This revision is now accepted and ready to land.Feb 7 2018, 4:36 PM

Fetch PlayerData from defaults file, rather than copying from Atlas' main simulation instance.

source/tools/atlas/GameInterface/ActorViewer.cpp
296 ↗	(On Diff #5531)	Yes, that works... (with a small alteration to `helpers/Player.js` so it doesn't try prepending gaia when it is already present...) ...well spotted.

Cute! If you made sure that it still works, can commit.

binaries/data/mods/public/simulation/helpers/Player.js
18 ↗	(On Diff #5710)	If the condition is added, the comment might contain the word ActorViewer, so people don't have a hard time comprehending the origin of this code

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

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (309 tests).....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (309 tests).....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

Executing section Default...
Executing section Source...

source/tools/atlas/GameInterface/ActorViewer.cpp
|  33| #include·"graphics/Terrain.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: ''.

source/tools/atlas/GameInterface/ActorViewer.cpp
|  33| #include·"graphics/Terrain.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: 'MESSAGES_SKIP_STRUCTS'.

source/tools/atlas/GameInterface/ActorViewer.cpp
|  33| #include·"graphics/Terrain.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: '_MSC_VER'.
Executing section JS...
|    | [NORMAL] ESLintBear (spaced-comment):
|    | Expected space or tab after '//' in comment.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/helpers/Player.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/helpers/Player.js
|  91|  91| 
|  92|  92| 		// Note: this is not yet implemented but I leave it commented to highlight it's easy
|  93|  93| 		// If anyone ever adds handicap.
|  94|    |-		//if (getSetting(playerData, playerDefaults, i, "GatherRateMultiplier") !== undefined)
|    |  94|+		// if (getSetting(playerData, playerDefaults, i, "GatherRateMultiplier") !== undefined)
|  95|  95| 		//	cmpPlayer.SetGatherRateMultiplier(getSetting(playerData, playerDefaults, i, "GatherRateMultiplier"));
|  96|  96| 
|  97|  97| 		if (getSetting(playerData, playerDefaults, i, "PopulationLimit") !== undefined)
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'for' condition.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/helpers/Player.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/helpers/Player.js
| 130| 130| 
| 131| 131| 			// Set all but self as enemies as SetTeam takes care of allies
| 132| 132| 			for (var j = 0; j < numPlayers; ++j)
| 133|    |-			{
|    | 133|+			
| 134| 134| 				if (i == j)
| 135| 135| 					cmpPlayer.SetAlly(j);
| 136| 136| 				else
| 137| 137| 					cmpPlayer.SetEnemy(j);
| 138|    |-			}
|    | 138|+			
| 139| 139| 			cmpPlayer.SetTeam(myTeam === undefined ? -1 : myTeam);
| 140| 140| 		}
| 141| 141| 
|    | [NORMAL] ESLintBear (comma-spacing):
|    | A space is required after ','.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/helpers/Player.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/helpers/Player.js
| 301| 301| 	return IsOwnedByHelper(player, target, "IsMutualAlly");
| 302| 302| }
| 303| 303| 
| 304|    |-function IsOwnedByNeutralOfPlayer(player,target)
|    | 304|+function IsOwnedByNeutralOfPlayer(player, target)
| 305| 305| {
| 306| 306| 	return IsOwnedByHelper(player, target, "IsNeutral");
| 307| 307| }

binaries/data/mods/public/simulation/helpers/Player.js
|  72| »   »   let·cmpPlayer·=·QueryPlayerIDInterface(i);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'cmpPlayer' is already declared in the upper scope.

binaries/data/mods/public/simulation/helpers/Player.js
| 154| »   »   for·(let·i·=·0;·i·<·numPlayers;·++i)
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.

binaries/data/mods/public/simulation/helpers/Player.js
|  61| »   »   var·entID·=·cmpPlayerManager.GetPlayerByID(i);
|    | [NORMAL] JSHintBear:
|    | 'entID' is already defined.

binaries/data/mods/public/simulation/helpers/Player.js
|  70| »   for·(var·i·=·0;·i·<·numPlayers;·++i)
|    | [NORMAL] JSHintBear:
|    | 'i' is already defined.

binaries/data/mods/public/simulation/helpers/Player.js
| 132| »   »   »   for·(var·j·=·0;·j·<·numPlayers;·++j)
|    | [NORMAL] JSHintBear:
|    | 'j' is already defined.

Harbormaster completed remote builds in B4863: Diff 5710.Feb 7 2018, 11:25 PM

s0600204 mentioned this in D1289: Remove unnecessary argument from CSimulation2::InitGame.Feb 8 2018, 9:13 PM

Closed by commit rP21167: Load and Initialise PlayerData in Atlas' ActorViewer (authored by s0600204). · Explain WhyFeb 9 2018, 10:49 PM

This revision was automatically updated to reflect the committed changes.

s0600204 marked an inline comment as done.

elexis added inline comments.Feb 10 2018, 1:51 PM