Page MenuHomeWildfire Games

Fix OOS on rejoin by adding Identity classes to the mirage filter rather than the Mirage component
ClosedPublic

Authored by temple on Feb 23 2018, 8:12 PM.

Details

Summary

Identity classes aren't serialized because we read them from the templates (which shouldn't change), so we probably shouldn't serialize them in the Mirage component either.
Anyway, this caused an OOS because classesList was sometimes serialized as an array and other times as a reference, as described in #5048.
The change was made in D750/rP19987.

Test Plan

Verify that the cursor issue in D750/rP19987 is still fixed.

Perhaps all of Identity should be merged rather than just some elements?
Before the filters were moved to xml in D215, there was this comment in ps/TemplateLoader.cpp:

// Select a subset of identity data. We don't want to have, for example, a CC mirage
// that has also the CC class and then prevents construction of other CCs

It's not clear how this particular scenario would happen (or if it's the correct behavior), for example. Entity limits and limit changers seem to work as before.

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

temple created this revision.Feb 23 2018, 8:12 PM
Vulcan added a subscriber: Vulcan.Feb 23 2018, 8:14 PM

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

Linter detected issues:
Executing section Default...
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (spaced-comment):
|    | Expected space or tab after '//' in comment.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Attack.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Attack.js
| 495| 495| 
| 496| 496| 		let horizSpeed = +this.template[type].ProjectileSpeed;
| 497| 497| 		let gravity = +this.template[type].Gravity;
| 498|    |-		//horizSpeed /= 2; gravity /= 2; // slow it down for testing
|    | 498|+		// horizSpeed /= 2; gravity /= 2; // slow it down for testing
| 499| 499| 
| 500| 500| 		let cmpPosition = Engine.QueryInterface(this.entity, IID_Position);
| 501| 501| 		if (!cmpPosition || !cmpPosition.IsInWorld())
|    | [NORMAL] ESLintBear (no-trailing-spaces):
|    | Trailing spaces not allowed.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Attack.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Attack.js
| 544| 544| 		let launchPoint = selfPosition.clone();
| 545| 545| 		// TODO: remove this when all the ranged unit templates are updated with Projectile/Launchpoint
| 546| 546| 		launchPoint.y += 3;
| 547|    |-		
|    | 547|+
| 548| 548| 		let cmpVisual = Engine.QueryInterface(this.entity, IID_Visual);
| 549| 549| 		if (cmpVisual)
| 550| 550| 		{
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'else'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Attack.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Attack.js
| 615| 615| 			});
| 616| 616| 	}
| 617| 617| 	else
| 618|    |-	{
|    | 618|+	
| 619| 619| 		// Melee attack - hurt the target immediately
| 620| 620| 		cmpDamage.CauseDamage({
| 621| 621| 			"strengths": this.GetAttackStrengths(type),
| 625| 625| 			"type": type,
| 626| 626| 			"attackerOwner": attackerOwner
| 627| 627| 		});
| 628|    |-	}
|    | 628|+	
| 629| 629| };
| 630| 630| 
| 631| 631| /**

binaries/data/mods/public/simulation/components/Attack.js
| 485| ·»   let·cmpDamage·=·Engine.QueryInterface(SYSTEM_ENTITY,·IID_Damage);
|    | [NORMAL] ESLintBear (no-mixed-spaces-and-tabs):
|    | Mixed spaces and tabs.

binaries/data/mods/public/simulation/components/Attack.js
| 590| »   »   cmpTimer.SetTimeout(SYSTEM_ENTITY,·IID_Damage,·"MissileHit",·timeToTarget·*·1000·+·+this.template.Ranged.Delay,·data);
|    | [NORMAL] JSHintBear:
|    | Confusing plusses.

Link to build: https://jenkins.wildfiregames.com/job/differential/78/display/redirect

bb added a comment.Feb 24 2018, 7:04 PM

I should be totally the wrong reviewer in this case, but works as advertised

IIRC after the commit we had some discussion whether this proposed change wouldn't be better than the committed one, but we said that classes might eventually be changed by a tech or so. But allowing that needs some more serializing stuff. Maybe the changing class cases could be caught by some upgrade or so, so I @coin for this patch and serializing classes everywhere.

elexis retitled this revision from Add Identity classes to the mirage filter rather than the Mirage component to Fix OOS on rejoin by adding Identity classes to the mirage filter rather than the Mirage component.Feb 24 2018, 7:11 PM
temple added inline comments.Feb 24 2018, 7:17 PM
binaries/data/mods/public/simulation/components/Mirage.js
70 ↗(On Diff #5893)

An alternative solution would be to clone here.

My memory's not too good, apparently: D1078. Some of what I wrote there is nonsense though.

I should be totally the wrong reviewer in this case

Introducing an OOS qualifies for reviewing the fix.

Unless I miss a critical clue, I agree with the analysis in D1078 stating that CopyIdentity is pointless if we already have the special XML file copying a subset of Identity.
So this option seems much better than adding a clone.
(The commit message should state that it's not only an OOS fix but also removal of indirection / pointlessness and the other revision proposal should be marked as fixed.)

Select a subset of identity data. We don't want to have, for example, a CC mirage
that has also the CC class and then prevents construction of other CCs

It's not clear how this particular scenario would happen (or if it's the correct behavior), for example. Entity limits and limit changers seem to work as before.

I'm expecting it to happen if mirage.xml would also copy the TrainingRestrictions or BuildRestrictions component to the miraged entity.
The TemplateLoader.cpp comment was added in rP15612, which also introduced the entirety of CopyMirageSubset but also didn't copy these two components. So it looks like the comment was misleading all along.

(Looks like a general rule should be that Mirage only copies properties in CopyFoo if that property can be changed during a game. So we perhaps there are some that could be copied in the special filter template instead. Just to have stated it)

So patch looks correct to me, althogh I didn't test. We should discuss bb's concern mentioned in D1078 that something like StatisticTracker might be broken elsewhere.

binaries/data/mods/public/simulation/components/Attack.js
265 ↗(On Diff #5893)

(correct, as the QueryMiragedInterface returns the stuff copied in the CopyFoo functions with priority, but we don't copy it anymore there).

binaries/data/mods/public/simulation/templates/special/filter/mirage.xml
10 ↗(On Diff #5893)

If I understand the matter correctly, this will cause a copy of classes to be written to the Mirage component. So the Identity classes list is not serialized, while the Mirage classes list is. This still solves the OOS because the AddEntity call in Fogging.prototype.LoadMirage yields a deepcopy, whereas now we only copy over a reference.

temple added a comment.Mar 7 2018, 2:21 AM

The miraged entity is missing a lot of components so most of the time there's not a problem. But it does have Ownership and Identity, so we have to be careful when matching classes, etc. For example as bb said in D1078 this causes problems with conquest checks, since the miraged entities now have the same classes as the unmiraged ones. And also in EntityLimits it does change the changers (annoyingly I had confused Foundation and Footprint in the D1078 summary) so for example placing a kennel foundation in the fog of war will let you train ten more dogs. An easy solution to these is to quit if Engine.QueryInterface(msg.entity, IID_Mirage) exists, but can we find everywhere where we need to put that? (Maybe we're lucky and it's only those two spots.)

I don't know the best solution.

binaries/data/mods/public/simulation/templates/special/filter/mirage.xml
10 ↗(On Diff #5893)

This doesn't do anything to the Mirage component, instead the miraged entity will have this.classesList in its Identity component (and nothing is serialized there).

(Currently, the miraged entity has a Mirage component with this.classesList which is copied from the original entity (in Fogging, as you said), but it also has an Identity component that's filled out with the things that are merged here, like Civ and GenericName. Doing QueryMiragedInterface will get the Mirage component while Engine.QueryInterface will get the Identity (or whatever) component. (For non-mirage entities, QueryMiragedInterface is the same as Engine.QueryInterface. Added in rP16607.) It's a bit weird that Identity is split this way when the other miraged components like Health aren't. So maybe that's an argument for this patch.)

As far as I see we have no other choice but copying the classeslist to the mirage. (Looking up the miraged entity to determine the cursor won't work if the miraged entity was destroyed meanwhile).
So we have to find all places that could break.
Subsequently there doesn't seem to be an alternative to ignoring mirages explicitly when looking for entities with Identity classes and Ownership.
-> grep time

binaries/data/mods/public/simulation/templates/special/filter/mirage.xml
10 ↗(On Diff #5893)

Sorry I meant mirage entity, not mirage component.
But the mirage entity is serialized and the mirage still serializes this classes list, but it's a deepcopy, hence no OOS, right?

Yes I liked that the patch at the other revision proposal unified Identity, but we certainly have enough complexity to review already in this iteration IMO

temple updated this revision to Diff 6061.Mar 8 2018, 3:21 AM

It seems better to simply clone the array to avoid the OOS issue. That way we don't have to figure out all the places where we need to add code to ignore mirages.

(I'm still not clear on why those who rejoin get an array rather than a reference to an array. I noticed that when the parent entity of a mirage is destroyed, then this.classesList in Mirage changes from a reference to an array, when I might've expected it to instead be destroyed as well. Somewhere there's code that explains all of this but I haven't found it yet.)

Vulcan added a comment.Mar 8 2018, 4:13 AM

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

Linter detected issues:
Executing section Default...
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (semi):
|    | Missing semicolon.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Mirage.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Mirage.js
|  73|  73| 	this.classesList = clone(cmpIdentity.GetClassesList());
|  74|  74| };
|  75|  75| 
|  76|    |-Mirage.prototype.GetClassesList = function() { return this.classesList };
|    |  76|+Mirage.prototype.GetClassesList = function() { return this.classesList; };
|  77|  77| 
|  78|  78| // Foundation data
|  79|  79| 

binaries/data/mods/public/simulation/components/Mirage.js
|  76| Mirage.prototype.GetClassesList·=·function()·{·return·this.classesList·};
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.

Link to build: https://jenkins.wildfiregames.com/job/differential/177/display/redirect

In D1325#55955, @temple wrote:

It seems better to simply clone the array to avoid the OOS issue. That way we don't have to figure out all the places where we need to add code to ignore mirages.

Not only easier and less error prone for us now but also easier and less error prone for future code.

I'm still not clear on why those who rejoin get an array rather than a reference to an array. I noticed that when the parent entity of a mirage is destroyed, then this.classesList in Mirage changes from a reference to an array, when I might've expected it to instead be destroyed as well. Somewhere there's code that explains all of this but I haven't found it yet.

This sounds for once like SpiderMonkey counting references and not destroying an object until all references are deleted and secondly the serialization code inserting an array where it's constructed the first time and references to that only afterwards and the rejoined client constructing the array in the mirage entity first before the GUI or whatever event creates the Identity classeslist. (But that's a guess and in every post I said something wrong, so.)
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Memory_Management#Garbage_collection

binaries/data/mods/public/simulation/components/Mirage.js
72 ↗(On Diff #6061)

Should also mention why the template based approach is not used IMO

binaries/data/mods/public/simulation/templates/special/filter/mirage.xml
10 ↗(On Diff #5893)

But the mirage entity is serialized and the mirage still serializes this classes list, but it's a deepcopy, hence no OOS, right?

In the non-clone / special filter variant iteration of the patch the classeslist of the mirage entity is part of the template and templates are not serialized. It's a new object (which is what I meant with deepcopy), hence indeed no OOS.

temple updated this revision to Diff 6071.Mar 8 2018, 9:24 PM
In D1325#55964, @elexis wrote:
In D1325#55955, @temple wrote:

I'm still not clear on why those who rejoin get an array rather than a reference to an array. I noticed that when the parent entity of a mirage is destroyed, then this.classesList in Mirage changes from a reference to an array, when I might've expected it to instead be destroyed as well. Somewhere there's code that explains all of this but I haven't found it yet.

This sounds for once like SpiderMonkey counting references and not destroying an object until all references are deleted and secondly the serialization code inserting an array where it's constructed the first time and references to that only afterwards and the rejoined client constructing the array in the mirage entity first before the GUI or whatever event creates the Identity classeslist. (But that's a guess and in every post I said something wrong, so.)
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Memory_Management#Garbage_collection

Thanks for the explanation/guess.

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

Linter detected issues:
Executing section Default...
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (semi):
|    | Missing semicolon.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Mirage.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Mirage.js
|  77|  77| 	this.classesList = clone(cmpIdentity.GetClassesList());
|  78|  78| };
|  79|  79| 
|  80|    |-Mirage.prototype.GetClassesList = function() { return this.classesList };
|    |  80|+Mirage.prototype.GetClassesList = function() { return this.classesList; };
|  81|  81| 
|  82|  82| // Foundation data
|  83|  83| 

binaries/data/mods/public/simulation/components/Mirage.js
|  80| Mirage.prototype.GetClassesList·=·function()·{·return·this.classesList·};
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.

Link to build: https://jenkins.wildfiregames.com/job/differential/185/display/redirect

elexis accepted this revision.Mar 9 2018, 11:34 AM

I didn't test the patch but you have shown to us that you have tested this well. Also I don' t read any possibility for this code not to work.

We should have the green light from @bb too as he gave us the opportunity to examine this piece of code :-)

Thanks temple for being here and having spent the time and nerves on this issue!

binaries/data/mods/public/simulation/components/Mirage.js
77 ↗(On Diff #6071)

It's an art in itself to keep comments short while still carrying all the necessary information for future developers to be able to decide things without having to dig much. Ok for me if you want to shorten it a bit or leave it as is. Certainly better too much than too few here IMO.

I believe this would summarize the information you have posted, but as I mentioned take which variant you want. I'm not claiming this to be well phrased :-)

// In almost all cases we want to ignore mirage entities when querying Identity components of owned entities.
// To avoid adding a test everywhere, we don't transfer the classeslist in the template but here.
// We clone this since the classes list is not synchronized and since the mirage should be a snapshot of the entity at the given time.
This revision is now accepted and ready to land.Mar 9 2018, 11:34 AM
bb added a comment.Mar 9 2018, 9:01 PM

verified that the cursor still works and the oos is gone

I got wondering why this is the first such oos in the mirage component, while all (but one) other values are not cloned, probably due to the fact that this is the first array like object added here, perhaps a comment (or part of the comment already in the patch) could be placed in the file head to explain we need to clone those stuff to avoid oos's (and hope that will stop ppl like me to implement them).

elexis added 1 blocking reviewer(s): bb.Mar 9 2018, 9:27 PM
In D1325#56294, @bb wrote:

I got wondering why this is the first such oos in the mirage component, while all (but one) other values are not cloned, probably due to the fact that this is the first array like object added here, perhaps a comment (or part of the comment already in the patch) could be placed in the file head to explain we need to clone those stuff to avoid oos's (and hope that will stop ppl like me to implement them).

That's the "since the classes list is not synchronized" part of the comment, perhaps that could also be "not serialized".
You see that Identity.prototype.Serialize = null; // we have no dynamic state to save line and that this.classesListis initialized on Init.
So Identity of rejoined clients has an object created at the time the entity was created the first time, while the non-rejoined clients create an object at the time of the rejoin. Hence the objects are initialized in a different order, thus receive different references. The objects are equal but not identical.
This difference becomes relevant with the Mirage component serializing these objects.
The other CopyFoo properties in Mirage are numbers strings or booleans, so even if they were non-serialized, there are no references to those around, only copies.
Lesson learnt: Don't serialize non-serialized component properties, i.e. care to search if that pattern is done in a patch in the future, right?
Did I get some detail wrong?

This revision now requires review to proceed.Mar 9 2018, 9:27 PM
temple added a comment.Mar 9 2018, 9:58 PM

The other clone was in rP17092.
It seems like this.marketType which is a reference to a set in Market could be a problem but I couldn't get it to bug and it was added a couple years ago in rP18172, so apparently not.

binaries/data/mods/public/simulation/components/Mirage.js
77 ↗(On Diff #6071)

Sounds fine.

temple updated this revision to Diff 6095.Mar 9 2018, 9:58 PM

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

Linter detected issues:
Executing section Default...
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (semi):
|    | Missing semicolon.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Mirage.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Mirage.js
|  73|  73| 	this.classesList = clone(cmpIdentity.GetClassesList());
|  74|  74| };
|  75|  75| 
|  76|    |-Mirage.prototype.GetClassesList = function() { return this.classesList };
|    |  76|+Mirage.prototype.GetClassesList = function() { return this.classesList; };
|  77|  77| 
|  78|  78| // Foundation data
|  79|  79| 

binaries/data/mods/public/simulation/components/Mirage.js
|  76| Mirage.prototype.GetClassesList·=·function()·{·return·this.classesList·};
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.

Link to build: https://jenkins.wildfiregames.com/job/differential/201/display/redirect

bb accepted this revision.Mar 9 2018, 11:34 PM

MarketType works since it is serialized in the market component

This revision is now accepted and ready to land.Mar 9 2018, 11:34 PM
In D1325#56342, @bb wrote:

MarketType works since it is serialized in the market component

Right. Thanks for the review, elexis too. Hopefully we won't have to think about this again.

This revision was automatically updated to reflect the committed changes.