Page MenuHomeWildfire Games

Remove excess argument from shift call
ClosedPublic

Authored by FeXoR on Nov 16 2018, 11:49 PM.

Details

Test Plan

Test the behavior of shift() with and without an argument. (Or generate a random map that uses sortPointsShortestCycle() to determine there's no change in player or road placement)

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

FeXoR created this revision.Nov 16 2018, 11:49 PM
smiley added a subscriber: smiley.Nov 17 2018, 4:35 AM

On an unrelated note, what would be the impact of using squared distances here. Havent looked closely, but as far as I can tell, it’s only distance to distance comparisons.

binaries/data/mods/public/maps/random/rmgen/math.js
148 ↗(On Diff #6990)

inline this probably.

smiley accepted this revision.EditedNov 17 2018, 4:54 AM

Array.prototype.shift does not take any arguments according to https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/shift but JavaScript being JavaScript, it wont throw an error even if one is present.
This could have been intentional by the author in an attempt to shift by n, but that would make the algorithm wrong.
Either way, it wont break anything. Tested to be sure.

This revision is now accepted and ready to land.Nov 17 2018, 4:54 AM
FeXoR added a comment.Nov 17 2018, 5:01 AM

I was the original author and am to blame. I checked the algorithm and, no, it isn't meant to shift by n so just an oversight.

FeXoR marked an inline comment as done.Nov 17 2018, 5:04 AM
FeXoR added inline comments.
binaries/data/mods/public/maps/random/rmgen/math.js
148 ↗(On Diff #6990)

Nonono! The length changes while the loop roles:
pointsToAdd.shift();

smiley added inline comments.Nov 17 2018, 5:09 AM
binaries/data/mods/public/maps/random/rmgen/math.js
148 ↗(On Diff #6990)

Ah, indeed. Missed that line.

Stan added a subscriber: Stan.Nov 20 2018, 3:34 PM
Stan added inline comments.
binaries/data/mods/public/maps/random/rmgen/math.js
141 ↗(On Diff #6990)

Not sure but if (i !=0) might be more readable there don't you think ? Or an early return.

Could also be a function. What does 3 represent here ?

elexis added a subscriber: elexis.Nov 20 2018, 4:05 PM
elexis added inline comments.
binaries/data/mods/public/maps/random/rmgen/math.js
142 ↗(On Diff #6990)

(offtopic) points[order[i]].distanceTo(points[order[i - 1]])

FeXoR marked an inline comment as done.Nov 20 2018, 8:26 PM
FeXoR added inline comments.
binaries/data/mods/public/maps/random/rmgen/math.js
141 ↗(On Diff #6990)

The comment two lines earlier tells you: Just add the first 3 points ;)

FeXoR updated this revision to Diff 6994.Nov 20 2018, 9:24 PM

Added using Vector2D.distanceTo() to simplify code.

FeXoR marked an inline comment as done.Nov 20 2018, 9:27 PM
FeXoR added inline comments.
binaries/data/mods/public/maps/random/rmgen/math.js
142 ↗(On Diff #6990)

Good catch! And while we're at it ;)

FeXoR requested review of this revision.Nov 20 2018, 9:31 PM

Now this should really be tested in action with maps but I don't know how to change the test plan or the title.
Caledonian Meadows uses this. Generate some maps, if no paths cross each other it should be fine.

elexis added inline comments.Nov 20 2018, 9:58 PM
binaries/data/mods/public/maps/random/rmgen/math.js
142 ↗(On Diff #6990)

Sure this is a Vector2D already? Because getPointsByHeight uses { "x": x, "y": y , "dist": minDistance} (if the distance property is in fact unavoidable, it can become { position: fooVec2D, distance: bar} instead of fooVec2D)

Stan added inline comments.Nov 20 2018, 10:07 PM
binaries/data/mods/public/maps/random/rmgen/math.js
141 ↗(On Diff #6990)

Oh Kernighan–Lin algorithm

FeXoR marked an inline comment as done.Nov 20 2018, 10:21 PM
FeXoR added inline comments.
binaries/data/mods/public/maps/random/rmgen/math.js
142 ↗(On Diff #6990)

Yup. Should originate from hightmap.js L98.
I also tested this so this would have fatally failed otherwise AFAICT ;)

@Stan Maybe...
I wrote if myself but I'm pretty sure someone else did wright something similar before ;D

Vulcan added a subscriber: Vulcan.Nov 21 2018, 2:34 PM

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

Link to build: https://jenkins.wildfiregames.com/job/differential/790/

smiley accepted this revision.Nov 21 2018, 5:58 PM

No issues still. Generated caledonian_meadows and roads are correct.

binaries/data/mods/public/maps/random/rmgen/math.js
142 ↗(On Diff #6990)

Indeed, maybe there should be a vector check in places like these. But that's pretty out of scope.

This revision is now accepted and ready to land.Nov 21 2018, 5:58 PM
elexis accepted this revision.Nov 21 2018, 6:24 PM
  • Confirmed with warn(uneval()) that the line is actually executed
  • The patch must be correct for any mapsettings, because there is only one call to sortPointsShortestCycle, that receives the argument from groupPlayersCycle, that receives the argument from getStartLocationsByHeightmap on wild lake and caledonian meadows and that function always returns a Vector2D array.
binaries/data/mods/public/maps/random/rmgen/math.js
121 ↗(On Diff #6994)

(x, y) is undefined syntax and can be nuked (points itself should be sufficient since all points are currently Vector2D, so one doesnt have to mention it in every function)

FeXoR updated this revision to Diff 6995.Nov 21 2018, 9:54 PM

Documentation correction

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

Link to build: https://jenkins.wildfiregames.com/job/differential/792/

elexis added inline comments.Nov 21 2018, 11:01 PM
binaries/data/mods/public/maps/random/rmgen/math.js
121 ↗(On Diff #6994)

If you want to add the param explanations,

  • @param {Vector2D[]} points - (text)

http://usejsdoc.org/tags-param.html
refs #4831
brackets [ ] are for optional arguments if Im not mistaken

FeXoR updated this revision to Diff 6997.Nov 22 2018, 11:39 AM

Change documentation once more

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

Link to build: https://jenkins.wildfiregames.com/job/differential/793/

Thought I find the Obect[] syntax quite misleading for the content of the array is more prominent than the array itself this way.

This revision was automatically updated to reflect the committed changes.