Page MenuHomeWildfire Games

Remove duplicate hardcoded StatisticsTracker unit and building classes
ClosedPublic

Authored by elexis on Oct 20 2019, 5:35 PM.

Details

Summary

rP14703 added StatisticsTracker code to count distinct classes of buildings and units, but hardcoded these classes in the code. Furthermore it duplicated those, so that it becomes much harder and complex to insert or maintain it.
Template values should be in the templates, that's why the template system exists to begin with.

Test Plan

Notice that these classes are hardcoded in reportGame of session.js and in source/tools/lobbybots/. However there shouldn't be a comment in the simulation stating that the lobby bot code should be kept in sync with this,
because the lobby code should be considered to be independent. If anything, the lobbybot code should be kept in sync with the 0ad templates, not the other way around.
Also modders who change the template values shouldn't have to modify lobby bot code.
If the lobby bot code doesnt account for mods, its the problem of that code and not to be addressed in the simulation code.

Ensure that all objects have the same values as before by reading really well. Notice there are many issues with Domestic classes and related code (see all the comments posted on Phabricator today, http://irclogs.wildfiregames.com/2019-10/2019-10-20-QuakeNet-%230ad-dev.log)
But that should be addressed independently to limit the complexity and improve auditability.
Train all those unit types and see that the summary screen shows the correct value.
Notice that UpdateSequenceInterval is not serialized, because its not an owned property (see C++ script serialization code somwhere), and be aware that if it was to serialize non-owned properties, it would also serialize the Schema.

The comment // Exclude gaia animals but not gaia soldiers was added following rP21548 to clarify why this is the only line using "Animal" while every other line uses "Domestic".
This may be clarified, but not here, not now.

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.Oct 20 2019, 5:35 PM

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/476/display/redirect

Build failure - The Moirai have given mortals hearts that can endure.

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

elexis updated this revision to Diff 10186.Oct 20 2019, 5:51 PM

Fix tests.

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

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/477/display/redirect

elexis added inline comments.Oct 20 2019, 5:53 PM
binaries/data/mods/public/simulation/components/tests/test_StatisticsTracker.js
41 ↗(On Diff #10186)

Notice that PushValue just pushes everything as is, i.e. also Worker and Infantry even if this is not part of the so called tracked classes.
However the StatisticsTracker and the tests are the only function calling the Push function, and the code should not be written to suit the tests, but the tests should test the code.

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

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

Freagarach added inline comments.
binaries/data/mods/public/simulation/components/StatisticsTracker.js
10–13 ↗(On Diff #10186)

+\t

16–19 ↗(On Diff #10186)

+\t

23 ↗(On Diff #10186)

Might want to add that it is in ms? (For it looks weird to have a simple calculation here.)

45 ↗(On Diff #10186)

+.

252 ↗(On Diff #10186)

+.
Why specifically gaia animals? It just ignores all animals, right?

Mostly wondering whether there would be a reason to deem this patch bad, trying to identify a reason why this should not be in the templates but should be in the code.
I fail to find such a reason and it's actually bad to hardcode values in the code. Perhaps one could argue that while the purpose of templates is store values for components, the superseding purpose of templates is to remain moddable while these values are not freely moddable, because the summary screen and lobby bot require some of these values to be present. But I would counter to that that the JS arrays and objects in the summary screen code also look quite modifiable, both in the StatisticsTracker and reportGame in session.js and equally appear like a manual selection of nice classes like here in the template.
So if someone was to hold that point, consequentially the summary screen would have to be changed to work without hardcoded classes, without making any expectations as to what is in the playerState. Then this patch is still correct and not adding a regression, but its the summary screen hardcoding that would be broken. Hence must commit, no?

binaries/data/mods/public/simulation/components/StatisticsTracker.js
252 ↗(On Diff #10186)

After this patch, I think only Unit, Structure, Domestic are remaining, and this line which looks like it could use Domestic, but it has to use Animal.

It does exclude all animals, but players can only own domestic animals (at least the code was always written with that assumption, including the one in this file), so it mentions only the relevant subset of the truth even if it doesnt state the whole truth, as that's meant to point out the more relevant / less obvious part of it.

I can make that // Exclude animals but not gaia soldiers, but it sounds a step weirder than // Exclude gaia animals but not gaia soldiers.

Or // Do not only exclude Domestic animals but all gaia animals, but don't exclude gaia soldiers. Or if you have a better phrasing Im open for that too.

Freagarach added inline comments.Oct 20 2019, 7:41 PM
binaries/data/mods/public/simulation/components/StatisticsTracker.js
252 ↗(On Diff #10186)

I'm sorry, but I miss why GAIA has to be mentioned?
Is it because the prasing // Exclude animals. would not give enough information? (Why should we exclude animals? Elephants are animals? Cavalry rides animals? Also GAIA Cavalry rides animals.)

elexis added inline comments.Oct 20 2019, 7:46 PM
binaries/data/mods/public/simulation/components/StatisticsTracker.js
252 ↗(On Diff #10186)

Because Domestic is the only unit class remaining after this diff, except this line which uses Animal,
therefore the first thought that one should have is to replace Animal with Domestic to reduce the number of classes to 1 (Unit and Stucture dont count).

But doing so would mean that non-domestic gaia animals (most if not all of them) would count as killed enemies.

And excluding gaia altogether revert the commit mentioned in the test plan (rP21548), i.e. not counting the gaia soldiers on survival of the fittest or jebel barkal.

if (unitClass != "Domestic" || counterName == "unitsTrained")
if (!cmpUnitEntityIdentity.HasClass("Domestic"))
if (cmpUnitEntityIdentity.HasClass("Domestic") && costs)
if (cmpLostEntityIdentity.HasClass("Unit") && !cmpLostEntityIdentity.HasClass("Domestic"))

For default serialization:
Serialization of these properties could be left out, but since there is only one of these per components per player, it has no significant performance or memory footprint impact. Implementing a custom serialization function that copies all owned properties except these two means that the reader will have to read through the entire component code in order to determine / verify whether the list is complete.

For custom serialization:
While I write this, I can't deny that it's not the truth. The serialization is easy to verify because the component code is short enough, for every (these two) blacklisted members, it's obvious that they depend on the template and are valid to exclude, and for every whitelisted member (every existing member) its obvious that it depens on the simulation state and thus must be serialized.

So must implement the custom serialization. So I did:

StatisticsTracker.prototype.Serialize = function()
{
	let state = {};
	for (let key in this)
		if (this.hasOwnProperty(key) && key != "unitsClasses" && key!= "buildingsClasses")
			state[key] = this[key];

	return state;
};

No deserialization function needed.

Now one however also needs to verify that Init is called after deserialization or implement a Deserialization function that reinitializes the templates data.

So while the Init function is called when loading a simstate:

bool CComponentManager::AddComponent(CEntityHandle ent, ComponentTypeId cid, const CParamNode& paramNode)
{
	IComponent* component = ConstructComponent(ent, cid);
	if (!component)
		return false;

	component->Init(paramNode);
	return true;
}

However I think the entity is deleted and when deserialization takes place the new one is created with the Deserialize function having to perform the Init function if it wants it. We see:

GuiInterface.prototype.Deserialize = function(data)
{
	this.Init();
	this.timeNotifications = data.timeNotifications;
	this.timeNotificationID = data.timeNotificationID;
};

Further confirmation by warn(uneval(this))

WARNING: StatisticsTracker.prototype.Init
WARNING: StatisticsTracker.prototype.Init
WARNING: StatisticsTracker.prototype.Deserialize
WARNING: ({})

So it would need this:

StatisticsTracker.prototype.Deserialize = function(data)
{
	for (let key in data)
		this[key] = data[key];

	this.unitsClasses = this.template.UnitClasses._string.split(/\s+/);
	this.buildingsClasses = this.template.StructureClasses._string.split(/\s+/);
};

which duplicates the template setter.
That should be avoided and there would have to be a helper function to remove the duplication or calling Init again (which does redundant useless work that might have avert side effects).

So noone can say I was to lazy or not focused on optimizing the game, this serialization does not appear warranted or benefitial to me for this component in the current form.

binaries/data/mods/public/simulation/templates/special/player/player.xml
92 ↗(On Diff #10186)

<!-- The summary screen and lobby rankings expect these classes, but additional classes may be added -->

After this patch, the following Identity classes remain:

  • vegetarianFood, if (type == "food" && (specificType == "fruit" || specificType == "grain"))
  • Domestic, Animal
  • Unit, Structure

These are hardcodings too and prevent the game from being more moddable.

The Unit/Structure distinction may be kept, since it's a core concept of the code currently.

But the vegetarian food one can be abstracted by recording all, not only one specific subtypes of the resources.

		"fish": "Fish",
		"fruit": "Fruit",
		"grain": "Grain",
		"meat": "Meat"

It sounds however like these unused(?) resource types should vanish then.
If theyre never used anywhere at all, then they only waste space if they were tracked.
If they were used, somewhere at all, then it should also be communicated to the player in tooltips and the summary screen.

But not in this patch anyway.

The Domestic/Animal distinction would be good to abstract as well. But that requires bugfixing it first! as mentioned in the summary (or rebranding "total" to "net", or making statistics more relevant without making them less truthful)!

binaries/data/mods/public/simulation/components/StatisticsTracker.js
48 ↗(On Diff #10186)
			// Domestic units are only counted for training
			if (unitClass != "Domestic" || counterName == "unitsTrained")

Instead of that, it could also be removed from the template list and added below the loop.

That would look nicer, but it would also remove the freedom to remove Domestic from being counted without having to modify the code (the Domestic checks are only skips, so support both that counter from being present or dropped).

On the other side Domestic is the only counter of the templates that isnt added to all counter types.

Three empty column ("unitsLost", "enemyUnitsKilled", "unitsCaptured") shouldnt be added to the snapshots, because there is a significant amount of them (memory footprint).

Then again the same footprint is added if one just adds one counted class to the summary screen, meh.

Well thats open to reconsideration for the next Domestic rework in this file. Perhaps the units could be counted and shown in the summary screen too, the unit just being treated like other units instead of making the hardcoding drama in this component.

For rP14703 there was also the debate whether to unify buildings and structures. Not now.

Verified the patch by 1. comparing all removed arrays and objects line by line again with the templates and 2. noticing that removing one item in the template list causes hard summary screen errors, and playing a cheatgame EOF

binaries/data/mods/public/simulation/components/tests/test_StatisticsTracker.js
30 ↗(On Diff #10186)

should be [29], see rP21250

This revision was not accepted when it landed; it landed in state Needs Review.Oct 21 2019, 10:48 AM
This revision was automatically updated to reflect the committed changes.