Page MenuHomeWildfire Games

Avoid NaN in Foundation and Repairable
ClosedPublic

Authored by temple on Feb 12 2018, 5:07 AM.

Details

Summary

Games can go out of sync when structures are miraged because NaN is serialized differently for different players. See #5030 for an example of how to produce the oos.

Mirage serializes buildTime from both Foundation and Repairable, and if there's no builders or repairers then timeRemaining = 0/0 = NaN. Unfortunately NaN isn't reliably serialized (see #1879 and further explanation in #1828), so in the meantime let's avoid using it.

Test Plan

If timeLeft and rate are both 0, then it makes sense to return timeRemaining = 0 instead of 0/0 = NaN.
See #5030 for an example of how to produce the oos, and test that it doesn't happen with the patch.

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 12 2018, 5:07 AM
Vulcan added a subscriber: Vulcan.Feb 12 2018, 5:11 AM

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

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

temple edited the summary of this revision. (Show Details)Feb 12 2018, 5:14 AM
bb added a subscriber: bb.Feb 12 2018, 11:51 AM

Indeed if timeLeft == 0 => return 0,
however what if timeLeft>0 and rate == 0, from reading the tickets, we are not allowed to return infinity either (I guess one could test it by the same trick with a foundation)

elexis added a subscriber: elexis.Feb 12 2018, 5:16 PM

Ok to not serialize NaN and Infinity, but we should fix the serializer too so that noone else will have to reiterate all of that

binaries/data/mods/public/simulation/components/Repairable.js
83 ↗(On Diff #5772)

If the workaround is added, should have a comment explaining its purpose

mimo added a subscriber: mimo.Feb 12 2018, 7:16 PM

In the gui, these tooltips are only displayed when there are some builders, and i think it is good like that (even if the timeRemainingNew could be useful even if no builders for foundation).
So why in the first place computing that info when no builders (which represents 99% of the time for a standard structure). I'd rather have an early return in GetBuildTime when no builders, returning null or whatever. And thus rate will be always > 0.

There's lots of NaNs (2**53 - 2 of them?), but with serializing we want to choose just one, e.g. the regular 7ff8 guy. So, this seems to work:

Index: source/simulation2/serialization/BinarySerializer.h
===================================================================
--- source/simulation2/serialization/BinarySerializer.h	(revision 21185)
+++ source/simulation2/serialization/BinarySerializer.h	(working copy)
@@ -185,7 +185,13 @@
 
 	virtual void PutNumber(const char* name, double value)
 	{
-		m_Impl.Put(name, (const u8*)&value, sizeof(double));
+		if (isnan(value))
+		{
+			const u8 canonicalNaN[] = {0, 0, 0, 0, 0, 0, 0xf8, 0x7f};
+			m_Impl.Put(name, canonicalNaN, sizeof(double));
+		}
+		else
+			m_Impl.Put(name, (const u8*)&value, sizeof(double));
 	}
 
 	virtual void PutNumber(const char* name, fixed value)

But unfortunately I'm not at all familiar with this stuff. Will it work on all computers? (E.g., where does endianness come into play?) I don't know.

I agree with mimo that doing an early return would be nicer than catching the result of a div / 0 after it happened.

Still would be a great point in time to get this serialization fixed, so appreciate that you gave it a look temple.
We should aim at constructing a specific NaN without hardcoding the value (since that is preknowledge and the C++ internals could change).
Perhaps we can pick a specific NaN with something like std::nan("1")?
Also the same problem occurs for integer types I suppose.

In D1296#53067, @bb wrote:

Indeed if timeLeft == 0 => return 0,
however what if timeLeft>0 and rate == 0, from reading the tickets, we are not allowed to return infinity either (I guess one could test it by the same trick with a foundation)

I think infinity is fine. (There's +inf and -inf, but both are unique unlike NaN.)

In D1296#53089, @mimo wrote:

In the gui, these tooltips are only displayed when there are some builders, and i think it is good like that (even if the timeRemainingNew could be useful even if no builders for foundation).

We do have tooltips when no builders, exactly that timeRemainingNew.

In D1296#53209, @elexis wrote:

Perhaps we can pick a specific NaN with something like std::nan("1")?

Or NAN = std::nan(""), but they are "implementation-specific", which sounds like they could be different on different machines. Again, I don't know much about this!

Also the same problem occurs for integer types I suppose.

I think NaNs are only for doubles, since in BinarySerializer.cpp it first checks if it's an integer and otherwise uses double.

We could do our own implementation, adding one more bit to the serialization stating if it's NaN.
Another option is throwing an error in case of encountering NaN.
We could do the JS only fix for this release and the C++ action just after the release.

temple updated this revision to Diff 5798.Feb 16 2018, 10:04 PM

early return -> ternary, comment

temple updated this revision to Diff 5799.Feb 16 2018, 10:13 PM

rateNew always > 0, okay

elexis accepted this revision.Feb 16 2018, 10:35 PM

Patch correct and minimal as established so far.
If you want to copy this comment to Foundation.prototype.CalculateBuildMultiplier too, that might not hurt and could help finding these div/0 instances.

Blackbox testing: Can reproduce the bug with the recipe mentioned in the ticket and can confirm that it works.

I'm not confident with having the actual NaN serialization issue fixed so short before the release.
But it should be fixed, so maybe we can do that right afterwards.

Thanks for having solved that puzzle and found the ticket, proper care!

This revision is now accepted and ready to land.Feb 16 2018, 10:35 PM
This revision was automatically updated to reflect the committed changes.

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 (operator-linebreak):
|    | '&&' should be placed at the end of the line.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Foundation.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Foundation.js
| 209| 209| 					// If obstructing fauna is gaia or our own but doesn't have UnitAI, just destroy it
| 210| 210| 					var cmpOwnership = Engine.QueryInterface(ent, IID_Ownership);
| 211| 211| 					var cmpIdentity = Engine.QueryInterface(ent, IID_Identity);
| 212|    |-					if (cmpOwnership && cmpIdentity && cmpIdentity.HasClass("Animal")
| 213|    |-						&& (cmpOwnership.GetOwner() == 0 || cmpFoundationOwnership && cmpOwnership.GetOwner() == cmpFoundationOwnership.GetOwner()))
|    | 212|+					if (cmpOwnership && cmpIdentity && cmpIdentity.HasClass("Animal") &&
|    | 213|+						(cmpOwnership.GetOwner() == 0 || cmpFoundationOwnership && cmpOwnership.GetOwner() == cmpFoundationOwnership.GetOwner()))
| 214| 214| 						Engine.DestroyEntity(ent);
| 215| 215| 				}
| 216| 216| 

binaries/data/mods/public/simulation/components/Foundation.js
| 359| »   »   »   let·cmpOwnership·=·Engine.QueryInterface(this.entity,·IID_Ownership);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'cmpOwnership' is already declared in the upper scope.

binaries/data/mods/public/simulation/components/Foundation.js
| 213| »   »   »   »   »   »   &&·(cmpOwnership.GetOwner()·==·0·||·cmpFoundationOwnership·&&·cmpOwnership.GetOwner()·==·cmpFoundationOwnership.GetOwner()))
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '&&'; readers may interpret this as an expression boundary.

binaries/data/mods/public/simulation/components/Foundation.js
| 254| »   »   »   var·cmpFoundationOwnership·=·Engine.QueryInterface(this.entity,·IID_Ownership);
|    | [NORMAL] JSHintBear:
|    | 'cmpFoundationOwnership' is already defined.

binaries/data/mods/public/simulation/components/Foundation.js
| 328| »   »   var·pos·=·cmpPosition.GetPosition2D();
|    | [NORMAL] JSHintBear:
|    | 'pos' is already defined.

binaries/data/mods/public/simulation/components/Foundation.js
| 330| »   »   var·rot·=·cmpPosition.GetRotation();
|    | [NORMAL] JSHintBear:
|    | 'rot' is already defined.

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

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 (operator-linebreak):
|    | '&&' should be placed at the end of the line.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Foundation.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Foundation.js
| 209| 209| 					// If obstructing fauna is gaia or our own but doesn't have UnitAI, just destroy it
| 210| 210| 					var cmpOwnership = Engine.QueryInterface(ent, IID_Ownership);
| 211| 211| 					var cmpIdentity = Engine.QueryInterface(ent, IID_Identity);
| 212|    |-					if (cmpOwnership && cmpIdentity && cmpIdentity.HasClass("Animal")
| 213|    |-						&& (cmpOwnership.GetOwner() == 0 || cmpFoundationOwnership && cmpOwnership.GetOwner() == cmpFoundationOwnership.GetOwner()))
|    | 212|+					if (cmpOwnership && cmpIdentity && cmpIdentity.HasClass("Animal") &&
|    | 213|+						(cmpOwnership.GetOwner() == 0 || cmpFoundationOwnership && cmpOwnership.GetOwner() == cmpFoundationOwnership.GetOwner()))
| 214| 214| 						Engine.DestroyEntity(ent);
| 215| 215| 				}
| 216| 216| 

binaries/data/mods/public/simulation/components/Foundation.js
| 359| »   »   »   let·cmpOwnership·=·Engine.QueryInterface(this.entity,·IID_Ownership);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'cmpOwnership' is already declared in the upper scope.

binaries/data/mods/public/simulation/components/Foundation.js
| 213| »   »   »   »   »   »   &&·(cmpOwnership.GetOwner()·==·0·||·cmpFoundationOwnership·&&·cmpOwnership.GetOwner()·==·cmpFoundationOwnership.GetOwner()))
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '&&'; readers may interpret this as an expression boundary.

binaries/data/mods/public/simulation/components/Foundation.js
| 254| »   »   »   var·cmpFoundationOwnership·=·Engine.QueryInterface(this.entity,·IID_Ownership);
|    | [NORMAL] JSHintBear:
|    | 'cmpFoundationOwnership' is already defined.

binaries/data/mods/public/simulation/components/Foundation.js
| 328| »   »   var·pos·=·cmpPosition.GetPosition2D();
|    | [NORMAL] JSHintBear:
|    | 'pos' is already defined.

binaries/data/mods/public/simulation/components/Foundation.js
| 330| »   »   var·rot·=·cmpPosition.GetRotation();
|    | [NORMAL] JSHintBear:
|    | 'rot' is already defined.

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