Page MenuHomeWildfire Games
Differential D1548

Danubius OOS on rejoin Vector2D hotfix
ClosedPublic
Actions

Authored by elexis on Jun 2 2018, 11:24 AM.

Details

Reviewers
temple
fatherbushido
Commits
rP21834: Fix an OOS on rejoin on Danubius following rP20957, refs #4855, fixes #5198.
Trac Tickets
#4698
#5162
#5198
Summary

As reported in #5162 there is an OOS on rejoin on Danubius when ships want to ungarrison,
since this.riverDirection is a Vector2D and we have code to serialize but not deserialize that type properly, see #4698.

Test Plan

The error occurs when ships want to ungarrison.
Notice there is no OOS without rejoin - even if one applied this patch and the other didn't.
Notice there is an OOS on rejoin if the rejoiner didn't apply the patch, no matter if the host has it or not.
Notice that there is no OOS on rejoin if the rejoiner has the patch, independent of the host having it.

Pass Engine.SetSimRate(20); for every client in the JS console to fast forward.
Use Engine.GetGUIObjectByName("devCommands").hidden=false; to see the map if you didn't set yourself as observer.

So this hotfix doesn't make it worse in any situation for players while making it better in the average case.

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.Jun 2 2018, 11:24 AM
elexis added inline comments.Jun 2 2018, 11:25 AM
binaries/data/mods/public/maps/random/danubius_triggers.js
489 ↗(On Diff #6699)

From https://en.wikipedia.org/wiki/Cross_product

The cross product is anticommutative (i.e., a × b = − b × a)

Vulcan added a subscriber: Vulcan.Jun 2 2018, 11:26 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 (comma-spacing):
|    | A space is required after ','.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/danubius_triggers.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/danubius_triggers.js
|  14|  14| 
|  15|  15| const danubiusAttackerTemplates = deepfreeze({
|  16|  16| 	"ships": TriggerHelper.GetTemplateNamesByClasses("Warship", "gaul", undefined, undefined, true),
|  17|    |-	"siege": TriggerHelper.GetTemplateNamesByClasses("Siege","gaul", undefined, undefined, true),
|    |  17|+	"siege": TriggerHelper.GetTemplateNamesByClasses("Siege", "gaul", undefined, undefined, true),
|  18|  18| 	"females": TriggerHelper.GetTemplateNamesByClasses("FemaleCitizen","gaul", undefined, undefined, true),
|  19|  19| 	"healers": TriggerHelper.GetTemplateNamesByClasses("Healer","gaul", undefined, undefined, true),
|  20|  20| 	"champions": TriggerHelper.GetTemplateNamesByClasses("Champion", "gaul", undefined, undefined, true),
|    | [NORMAL] ESLintBear (comma-spacing):
|    | A space is required after ','.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/danubius_triggers.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/danubius_triggers.js
|  15|  15| const danubiusAttackerTemplates = deepfreeze({
|  16|  16| 	"ships": TriggerHelper.GetTemplateNamesByClasses("Warship", "gaul", undefined, undefined, true),
|  17|  17| 	"siege": TriggerHelper.GetTemplateNamesByClasses("Siege","gaul", undefined, undefined, true),
|  18|    |-	"females": TriggerHelper.GetTemplateNamesByClasses("FemaleCitizen","gaul", undefined, undefined, true),
|    |  18|+	"females": TriggerHelper.GetTemplateNamesByClasses("FemaleCitizen", "gaul", undefined, undefined, true),
|  19|  19| 	"healers": TriggerHelper.GetTemplateNamesByClasses("Healer","gaul", undefined, undefined, true),
|  20|  20| 	"champions": TriggerHelper.GetTemplateNamesByClasses("Champion", "gaul", undefined, undefined, true),
|  21|  21| 	"champion_infantry": TriggerHelper.GetTemplateNamesByClasses("Champion+Infantry", "gaul", undefined, undefined, true),
|    | [NORMAL] ESLintBear (comma-spacing):
|    | A space is required after ','.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/danubius_triggers.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/danubius_triggers.js
|  16|  16| 	"ships": TriggerHelper.GetTemplateNamesByClasses("Warship", "gaul", undefined, undefined, true),
|  17|  17| 	"siege": TriggerHelper.GetTemplateNamesByClasses("Siege","gaul", undefined, undefined, true),
|  18|  18| 	"females": TriggerHelper.GetTemplateNamesByClasses("FemaleCitizen","gaul", undefined, undefined, true),
|  19|    |-	"healers": TriggerHelper.GetTemplateNamesByClasses("Healer","gaul", undefined, undefined, true),
|    |  19|+	"healers": TriggerHelper.GetTemplateNamesByClasses("Healer", "gaul", undefined, undefined, true),
|  20|  20| 	"champions": TriggerHelper.GetTemplateNamesByClasses("Champion", "gaul", undefined, undefined, true),
|  21|  21| 	"champion_infantry": TriggerHelper.GetTemplateNamesByClasses("Champion+Infantry", "gaul", undefined, undefined, true),
|  22|  22| 	"citizen_soldiers": TriggerHelper.GetTemplateNamesByClasses("CitizenSoldier", "gaul", undefined, "Basic", true),
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 4 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/danubius_triggers.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/danubius_triggers.js
| 288| 288| 
| 289| 289| 		let animations = ritualAnimations[
| 290| 290| 			cmpIdentity.HasClass("Healer") ? "healer" :
| 291|    |-			cmpIdentity.HasClass("Female") ? "female" : "male"];
|    | 291|+				cmpIdentity.HasClass("Female") ? "female" : "male"];
| 292| 292| 
| 293| 293| 		let cmpVisual = Engine.QueryInterface(ent, IID_Visual);
| 294| 294| 		if (!cmpVisual)

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

Harbormaster completed remote builds in B6198: Diff 6699.Jun 2 2018, 11:26 AM
Stan added a subscriber: Stan.Jun 2 2018, 11:27 AM
Stan added inline comments.
binaries/data/mods/public/maps/random/danubius_triggers.js
626 ↗(On Diff #6699)

Why not new Vector2D here ?

elexis added inline comments.Jun 2 2018, 11:52 AM
binaries/data/mods/public/maps/random/danubius_triggers.js
626 ↗(On Diff #6699)

See that comment in the line above that.

If we wanted this.riverDirection to be Vector2D then we wouldn't have to change anything as Vector2D.sub already yields a Vector2D.

temple added inline comments.Jun 2 2018, 4:36 PM
binaries/data/mods/public/maps/random/danubius_triggers.js
626 ↗(On Diff #6699)

This line isn't necessary, and notice that this.mapCenter is also a Vector2D so maybe put the comment above that.

elexis updated this revision to Diff 6710.Jun 2 2018, 9:30 PM

Address the other Vector2D variable too.
Use clone to convert to objects.
This way it would bug immediately if some new code tried to use it as a vector.

Vulcan added a comment.Jun 2 2018, 9:37 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 (comma-spacing):
|    | A space is required after ','.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/danubius_triggers.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/danubius_triggers.js
|  14|  14| 
|  15|  15| const danubiusAttackerTemplates = deepfreeze({
|  16|  16| 	"ships": TriggerHelper.GetTemplateNamesByClasses("Warship", "gaul", undefined, undefined, true),
|  17|    |-	"siege": TriggerHelper.GetTemplateNamesByClasses("Siege","gaul", undefined, undefined, true),
|    |  17|+	"siege": TriggerHelper.GetTemplateNamesByClasses("Siege", "gaul", undefined, undefined, true),
|  18|  18| 	"females": TriggerHelper.GetTemplateNamesByClasses("FemaleCitizen","gaul", undefined, undefined, true),
|  19|  19| 	"healers": TriggerHelper.GetTemplateNamesByClasses("Healer","gaul", undefined, undefined, true),
|  20|  20| 	"champions": TriggerHelper.GetTemplateNamesByClasses("Champion", "gaul", undefined, undefined, true),
|    | [NORMAL] ESLintBear (comma-spacing):
|    | A space is required after ','.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/danubius_triggers.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/danubius_triggers.js
|  15|  15| const danubiusAttackerTemplates = deepfreeze({
|  16|  16| 	"ships": TriggerHelper.GetTemplateNamesByClasses("Warship", "gaul", undefined, undefined, true),
|  17|  17| 	"siege": TriggerHelper.GetTemplateNamesByClasses("Siege","gaul", undefined, undefined, true),
|  18|    |-	"females": TriggerHelper.GetTemplateNamesByClasses("FemaleCitizen","gaul", undefined, undefined, true),
|    |  18|+	"females": TriggerHelper.GetTemplateNamesByClasses("FemaleCitizen", "gaul", undefined, undefined, true),
|  19|  19| 	"healers": TriggerHelper.GetTemplateNamesByClasses("Healer","gaul", undefined, undefined, true),
|  20|  20| 	"champions": TriggerHelper.GetTemplateNamesByClasses("Champion", "gaul", undefined, undefined, true),
|  21|  21| 	"champion_infantry": TriggerHelper.GetTemplateNamesByClasses("Champion+Infantry", "gaul", undefined, undefined, true),
|    | [NORMAL] ESLintBear (comma-spacing):
|    | A space is required after ','.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/danubius_triggers.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/danubius_triggers.js
|  16|  16| 	"ships": TriggerHelper.GetTemplateNamesByClasses("Warship", "gaul", undefined, undefined, true),
|  17|  17| 	"siege": TriggerHelper.GetTemplateNamesByClasses("Siege","gaul", undefined, undefined, true),
|  18|  18| 	"females": TriggerHelper.GetTemplateNamesByClasses("FemaleCitizen","gaul", undefined, undefined, true),
|  19|    |-	"healers": TriggerHelper.GetTemplateNamesByClasses("Healer","gaul", undefined, undefined, true),
|    |  19|+	"healers": TriggerHelper.GetTemplateNamesByClasses("Healer", "gaul", undefined, undefined, true),
|  20|  20| 	"champions": TriggerHelper.GetTemplateNamesByClasses("Champion", "gaul", undefined, undefined, true),
|  21|  21| 	"champion_infantry": TriggerHelper.GetTemplateNamesByClasses("Champion+Infantry", "gaul", undefined, undefined, true),
|  22|  22| 	"citizen_soldiers": TriggerHelper.GetTemplateNamesByClasses("CitizenSoldier", "gaul", undefined, "Basic", true),
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 4 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/danubius_triggers.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/danubius_triggers.js
| 288| 288| 
| 289| 289| 		let animations = ritualAnimations[
| 290| 290| 			cmpIdentity.HasClass("Healer") ? "healer" :
| 291|    |-			cmpIdentity.HasClass("Female") ? "female" : "male"];
|    | 291|+				cmpIdentity.HasClass("Female") ? "female" : "male"];
| 292| 292| 
| 293| 293| 		let cmpVisual = Engine.QueryInterface(ent, IID_Visual);
| 294| 294| 		if (!cmpVisual)

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

Harbormaster completed remote builds in B6207: Diff 6710.Jun 2 2018, 9:37 PM
temple accepted this revision.Jun 2 2018, 9:41 PM

Okay. (I also looked for Vector2D that might have been serialized elsewhere, not comprehensive but didn't find anything.)

This revision is now accepted and ready to land.Jun 2 2018, 9:41 PM
elexis added a comment.Jun 3 2018, 12:50 AM
In D1548#62806, @temple wrote:

Okay. (I also looked for Vector2D that might have been serialized elsewhere, not comprehensive but didn't find anything.)

Thanks for the review and the responsible search.
It's a bit tough to do a complete search, since C++ can convert the Vector2D.h type to the JS vector type.
I guess we would have found them sooner if there were some (but OOS hunting season apparently isn't over yet either).

elexis updated the Trac tickets for this revision.Jun 3 2018, 1:02 AM
Closed by commit rP21834: Fix an OOS on rejoin on Danubius following rP20957, refs #4855, fixes #5198. (authored by elexis). · Explain WhyJun 3 2018, 1:52 PM
This revision was automatically updated to reflect the committed changes.
Owners added a subscriber: Restricted Owners Package.Jun 3 2018, 1:52 PM

Revision Contents
Changeset List

PathSize
ps/
trunk/
binaries/
data/
mods/
public/
maps/
random/
10 lines

Diff 6714

View Options

ps/trunk/binaries/data/mods/public/maps/random/danubius_triggers.js

Loading...
Wildfire Games · Phabricator