Page MenuHomeWildfire Games
Differential D114

Schwarzwald tiny map - tile is undefined
ClosedPublic
Actions

Authored by FeXoR on Jan 31 2017, 12:11 AM.

Details

Reviewers
vladislavbelov
elexis
Stan
Commits
rP19288: Fixes a bug that caused an error when distributeEntitiesByHeight() in hightmap.
Trac Tickets
#4152
Summary

Random map Schwarzwald with mapsize set to tiny raises an error and doesn't start the match at all.

Test Plan

Execute Atlas, set random map to generate to Schwarzwald, optionally change seed and number of players (2 are advisable) to your liking, generate

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

There are a very large number of changes, so older changes are hidden. Show Older Changes
FeXoR added a comment.Jan 31 2017, 1:16 PM

And another one
Changed SVN-diff to use slash instead of backslash...

FeXoR updated this revision to Diff 409.Jan 31 2017, 1:18 PM

Reverted changes
Commented out svn-diff again...

FeXoR updated this revision to Diff 410.Jan 31 2017, 1:26 PM

Aaand another attempt
Test if svn-diff is linked correctly... sorry...

FeXoR updated this revision to Diff 411.Jan 31 2017, 1:28 PM

And another one...
Yea, I know...

FeXoR updated this revision to Diff 412.Jan 31 2017, 1:34 PM

yet another
Jepeee

Harbormaster completed remote builds in B422: Diff 412.Jan 31 2017, 1:34 PM
FeXoR updated this revision to Diff 413.Jan 31 2017, 1:35 PM

Out of ideas
Well, no context available for now...

Vulcan added a comment.Jan 31 2017, 1:51 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running debug tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/285/ for more details.

Harbormaster completed remote builds in B416: Diff 406.Jan 31 2017, 1:51 PM
Stan added inline comments.Jan 31 2017, 2:33 PM
binaries/data/mods/public/maps/random/heightmap/heightmap.js
156 ↗(On Diff #413)

Dont you log it twice ?

122 ↗(On Diff #403)

Perhaps we could write a IsBetween (lowerBound, higherBound);
function put it in the global scripts. An simply call !IsBetween here

Vulcan added a comment.Jan 31 2017, 2:35 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running debug tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/286/ for more details.

Harbormaster completed remote builds in B417: Diff 407.Jan 31 2017, 2:35 PM
Vulcan added a comment.Jan 31 2017, 3:18 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running debug tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/287/ for more details.

Harbormaster completed remote builds in B418: Diff 408.Jan 31 2017, 3:18 PM
Vulcan added a comment.Jan 31 2017, 4:02 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running debug tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/288/ for more details.

Harbormaster completed remote builds in B419: Diff 409.Jan 31 2017, 4:02 PM
FeXoR updated this revision to Diff 415.Jan 31 2017, 5:09 PM

Another try
svn diff now definitely gives full context. Let's see if this works out...

Vulcan added a comment.Jan 31 2017, 5:31 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running debug tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/289/ for more details.

Harbormaster completed remote builds in B420: Diff 410.Jan 31 2017, 5:31 PM
FeXoR updated this revision to Diff 416.Jan 31 2017, 5:40 PM

And another try
Maybe a Windows cache update problem?

FeXoR updated this revision to Diff 417.Jan 31 2017, 5:54 PM

Another try
"svn diff" definitely shows full context ;/

FeXoR updated this revision to Diff 418.Jan 31 2017, 5:57 PM

Resetting to best chice I found
So no context for now...

Vulcan added a comment.Jan 31 2017, 6:25 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running debug tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/290/ for more details.

Harbormaster completed remote builds in B421: Diff 411.Jan 31 2017, 6:25 PM
Vulcan added a comment.Jan 31 2017, 7:11 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running debug tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/291/ for more details.

Harbormaster completed remote builds in B422: Diff 412.Jan 31 2017, 7:11 PM
Vulcan added a comment.Jan 31 2017, 8:41 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running debug tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/292/ for more details.

Harbormaster completed remote builds in B423: Diff 413.Jan 31 2017, 8:41 PM
Vulcan added a comment.Jan 31 2017, 9:33 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running debug tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/293/ for more details.

Harbormaster completed remote builds in B425: Diff 415.Jan 31 2017, 9:33 PM
Vulcan added a comment.Jan 31 2017, 10:17 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running debug tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/294/ for more details.

Harbormaster completed remote builds in B426: Diff 416.Jan 31 2017, 10:17 PM
Vulcan added a comment.Jan 31 2017, 11:01 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running debug tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/295/ for more details.

Harbormaster completed remote builds in B427: Diff 417.Jan 31 2017, 11:01 PM
Vulcan added a comment.Jan 31 2017, 11:02 PM

Build has FAILED

Link to build: http://jw:8080/job/phabricator/296/
See console output for more information: http://jw:8080/job/phabricator/296/console

Harbormaster failed remote builds in B428: Diff 418!Jan 31 2017, 11:02 PM
FeXoR added a reviewer: vladislavbelov.Feb 4 2017, 6:53 PM
FeXoR added inline comments.Feb 7 2017, 5:48 PM
binaries/data/mods/public/maps/random/heightmap/heightmap.js
156 ↗(On Diff #413)

Yes. The fist will be logged if there is no valid point on the map any placement is possible. The second will occur if there are points where placement could happen but still nothing has been placed. The second should never happen, it's just there to be sure if it happens it's logged. They will never occur both ar the same call (for the function returns if the first one triggers).

elexis edited edge metadata.Feb 23 2017, 6:23 PM

Despite the fact that the last change was in october 2016 (rP18840), the patch doesn't apply somehow.
The one on the ticket still works: http://trac.wildfiregames.com/attachment/ticket/4152/heightmapFix2016-12-6.patch

FeXoR added inline comments.Feb 23 2017, 8:10 PM
binaries/data/mods/public/maps/random/heightmap/heightmap.js
122 ↗(On Diff #403)

That indeed might be a good idea. Thanks!

FeXoR updated this revision to Diff 586.Feb 23 2017, 8:23 PM

Try another time since the last diff didn't apply for some reason

Harbormaster completed remote builds in B576: Diff 586.Feb 23 2017, 8:23 PM
elexis edited the summary of this revision. (Show Details)Feb 23 2017, 9:24 PM
elexis removed reviewers: niektb, mimo.
Vulcan added a comment.Feb 23 2017, 10:06 PM

Build has FAILED

Link to build: http://jw:8080/job/phabricator/395/
See console output for more information: http://jw:8080/job/phabricator/395/console

Harbormaster failed remote builds in B576: Diff 586!Feb 23 2017, 10:06 PM
elexis requested changes to this revision.Feb 28 2017, 4:16 PM

Patch works as it now doesn't assume that we always find a valid tile to place the mines and instead prints a logmessage and goes on with the mapgen.
Few code style improvements proposed.

binaries/data/mods/public/maps/random/heightmap/heightmap.js
156 ↗(On Diff #413)

Two spaces

113 ↗(On Diff #586)

Add a newline somewhere in the argument list.
The ref to g_Gaia should be avoided, the argument should become mandatory and the 2 users of that function should pass it instead.

118 ↗(On Diff #586)

dont add that pair of braces, not needed nor useful IMO

123 ↗(On Diff #586)

Okay (mentioned the destructuring assignment in irc few days ago, i.e. let [cX, cY] = [x + 0.5, y + 0.5], but I didn't see you push the checkpoint, so it's all good)

125 ↗(On Diff #586)

newline after the continue statements
(I suggested to merge those if statements in irc few days ago, but it seems more readable in the current state, so you can keep it the way it is)

126 ↗(On Diff #586)

Avoid "Avoid avoidPoints", rephrase

128 ↗(On Diff #586)

It bugged here:

ERROR: JavaScript error: maps/random/heightmap/heightmap.js line 128
TypeError: tile is undefined
  distributeEntitiesByHeight@maps/random/heightmap/heightmap.js:128:1
  @maps/random/schwarzwald.js:266:1

as it assumed that we can find a valid tile.

135 ↗(On Diff #586)

Two spaces.
Also doesn't show any arguments, only the callstack. The callstack is useful as other maps might use this function in many places.
I'd add uneval(entityList) and an "\n" before the callstack.
Perhaps you can use print instead of log, so that it not only ends up in the logfile but also in the terminal output.

146 ↗(On Diff #586)

Use the new pickRandom function here

This revision now requires changes to proceed.Feb 28 2017, 4:16 PM
FeXoR updated this revision to Diff 668.Mar 1 2017, 11:02 PM
FeXoR edited edge metadata.

Adjusted to comments of elexis, thanks!

elexis accepted this revision.Mar 1 2017, 11:32 PM

still dont like those braces :p
no space before the \n in the string
integer does not exist in JS (doc), only number

Thanks for the fix!

This revision is now accepted and ready to land.Mar 1 2017, 11:32 PM
Vulcan added a comment.Mar 2 2017, 12:29 AM

Build has FAILED

Link to build: http://jw:8080/job/phabricator/445/
See console output for more information: http://jw:8080/job/phabricator/445/console

Harbormaster failed remote builds in B648: Diff 668!Mar 2 2017, 12:29 AM
FeXoR updated this revision to Diff 759.Mar 11 2017, 1:51 PM

Removed spaces
Changed "integer" to "number" for js doc for the changed function
Thanks elexis!

The "build failed" message does seem to be unrelated to this patch:

  • Raw diff applied cleanly
  • Nothing needed to be build
Vulcan added a comment.Mar 11 2017, 1:52 PM

Build has FAILED

Link to build: http://jw:8080/job/phabricator/497/
See console output for more information: http://jw:8080/job/phabricator/497/console

Harbormaster failed remote builds in B729: Diff 759!Mar 11 2017, 1:52 PM
FeXoR added inline comments.Mar 11 2017, 2:16 PM
binaries/data/mods/public/maps/random/heightmap/heightmap.js
123 ↗(On Diff #586)

That "checkpoint" is actually needed and pushed to the list of valid points at the end in this format.
I agree that the chosen format is not optimal - we have different kinds of "point" formats in use throughout rmgen only. That's indeed bad.

FeXoR requested review of this revision.Mar 11 2017, 2:57 PM
FeXoR edited edge metadata.
elexis accepted this revision.Mar 11 2017, 3:06 PM

Ok but replace the two spaces with one space in the two log() calls

This revision is now accepted and ready to land.Mar 11 2017, 3:06 PM
FeXoR updated this revision to Diff 763.Mar 11 2017, 3:27 PM

Removed the "two spaces".

Vulcan added a comment.Mar 11 2017, 3:28 PM

Build has FAILED

Link to build: http://jw:8080/job/phabricator/499/
See console output for more information: http://jw:8080/job/phabricator/499/console

Harbormaster failed remote builds in B731: Diff 763!Mar 11 2017, 3:28 PM
FeXoR requested review of this revision.Mar 11 2017, 3:28 PM
FeXoR edited edge metadata.
elexis accepted this revision.Mar 11 2017, 3:31 PM

And thats's where the diff of the diff function comes in handy

This revision is now accepted and ready to land.Mar 11 2017, 3:31 PM
Closed by commit rP19288: Fixes a bug that caused an error when distributeEntitiesByHeight() in hightmap. (authored by FeXoR). · Explain WhyMar 11 2017, 3:55 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathSize
ps/
trunk/
binaries/
data/
mods/
public/
maps/
random/
heightmap/
62 lines

Diff 764

View Options

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

Loading...
Wildfire Games · Phabricator