Page MenuHomeWildfire Games

Remove excess argument from shift call
AcceptedPublic

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

Details

Reviewers
smiley
elexis
Trac Tickets
#5345
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
Branch
/ps/trunk
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 6460
Build 10700: Vulcan BuildJenkins
Build 10699: arc lint + arc unit

Event Timeline

FeXoR created this revision.Fri, Nov 16, 11:49 PM
smiley added a subscriber: smiley.Sat, Nov 17, 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
150

inline this probably.

smiley accepted this revision.EditedSat, Nov 17, 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.Sat, Nov 17, 4:54 AM
FeXoR added a comment.Sat, Nov 17, 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.Sat, Nov 17, 5:04 AM
FeXoR added inline comments.
binaries/data/mods/public/maps/random/rmgen/math.js
150

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

smiley added inline comments.Sat, Nov 17, 5:09 AM
binaries/data/mods/public/maps/random/rmgen/math.js
150

Ah, indeed. Missed that line.

Stan added a subscriber: Stan.Tue, Nov 20, 3:34 PM
Stan added inline comments.
binaries/data/mods/public/maps/random/rmgen/math.js
143

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.Tue, Nov 20, 4:05 PM
elexis added inline comments.
binaries/data/mods/public/maps/random/rmgen/math.js
144

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

FeXoR marked an inline comment as done.Tue, Nov 20, 8:26 PM
FeXoR added inline comments.
binaries/data/mods/public/maps/random/rmgen/math.js
143

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

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

Added using Vector2D.distanceTo() to simplify code.

FeXoR marked an inline comment as done.Tue, Nov 20, 9:27 PM
FeXoR added inline comments.
binaries/data/mods/public/maps/random/rmgen/math.js
144

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

FeXoR requested review of this revision.Tue, Nov 20, 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.Tue, Nov 20, 9:58 PM
binaries/data/mods/public/maps/random/rmgen/math.js
144

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.Tue, Nov 20, 10:07 PM
binaries/data/mods/public/maps/random/rmgen/math.js
143

Oh Kernighan–Lin algorithm

FeXoR marked an inline comment as done.Tue, Nov 20, 10:21 PM
FeXoR added inline comments.
binaries/data/mods/public/maps/random/rmgen/math.js
144

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.Wed, Nov 21, 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.Wed, Nov 21, 5:58 PM

No issues still. Generated caledonian_meadows and roads are correct.

binaries/data/mods/public/maps/random/rmgen/math.js
144

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.Wed, Nov 21, 5:58 PM
elexis accepted this revision.Wed, Nov 21, 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–123

(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.Wed, Nov 21, 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.Wed, Nov 21, 11:01 PM
binaries/data/mods/public/maps/random/rmgen/math.js
121–123

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.Thu, Nov 22, 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.