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)
Details
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Branch
- /ps/trunk
- Lint
Lint OK - Unit
No Unit Test Coverage - Build Status
Buildable 6458 Build 10696: arc lint + arc unit
Event Timeline
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 | inline this probably. |
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.
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.
binaries/data/mods/public/maps/random/rmgen/math.js | ||
---|---|---|
148 | Nonono! The length changes while the loop roles: |
binaries/data/mods/public/maps/random/rmgen/math.js | ||
---|---|---|
148 | Ah, indeed. Missed that line. |
binaries/data/mods/public/maps/random/rmgen/math.js | ||
---|---|---|
141 | 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 ? |
binaries/data/mods/public/maps/random/rmgen/math.js | ||
---|---|---|
142 | (offtopic) points[order[i]].distanceTo(points[order[i - 1]]) |
binaries/data/mods/public/maps/random/rmgen/math.js | ||
---|---|---|
141 | The comment two lines earlier tells you: Just add the first 3 points ;) |
binaries/data/mods/public/maps/random/rmgen/math.js | ||
---|---|---|
142 | Good catch! And while we're at it ;) |
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.
binaries/data/mods/public/maps/random/rmgen/math.js | ||
---|---|---|
142 | 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) |
binaries/data/mods/public/maps/random/rmgen/math.js | ||
---|---|---|
141 | Oh Kernighan–Lin algorithm |
binaries/data/mods/public/maps/random/rmgen/math.js | ||
---|---|---|
142 | Yup. Should originate from hightmap.js L98. |
@Stan Maybe...
I wrote if myself but I'm pretty sure someone else did wright something similar before ;D
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/differential/790/
No issues still. Generated caledonian_meadows and roads are correct.
binaries/data/mods/public/maps/random/rmgen/math.js | ||
---|---|---|
142 | Indeed, maybe there should be a vector check in places like these. But that's pretty out of scope. |
- 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 | (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) |
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/differential/792/
binaries/data/mods/public/maps/random/rmgen/math.js | ||
---|---|---|
121 | If you want to add the param explanations,
http://usejsdoc.org/tags-param.html |
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.