Details

Reviewers

FeXoR

Summary

Procedural dune generation painter.

Test Plan

Used in map Dune: https://code.wildfiregames.com/D1636

test.js1 KBDownload

Diff Detail

Repository

rP 0 A.D. Public Repository

Branch

/ps/trunk

Lint

Lint OK

Unit

No Unit Test Coverage

Build Status

Buildable 10199
Build 17311: arc lint + arc unit

Event Timeline

nani created this revision.Sep 22 2018, 1:52 AM

nani created this object with visibility "nani".

nani edited the test plan for this revision. (Show Details)Sep 22 2018, 5:30 PM

nani changed the visibility from "nani" to "Public (No Login Required)".Sep 22 2018, 5:54 PM

elexis mentioned this in D1630: Add perlin noise generator..Mar 24 2019, 8:20 PM

Could you add a screenshot as a showcase, please?
There was the rectangularSmoothToHight function that took an influence map of surrounding tiles that could also be used to create dunes. (looks like it has been removed in a cleanup frenzy though...)

In D1635#102030, @FeXoR wrote:

Could you add a screenshot as a showcase, please?
There was the rectangularSmoothToHight function that took an influence map of surrounding tiles that could also be used to create dunes. (looks like it has been removed in a cleanup frenzy though...)

The map on https://wildfiregames.com/forum/index.php?/topic/24638-random-map-dune-desert/ is the showcase

@nani maybe a good occasion to use the new class syntax? VSCode can auto convert it.
Also missing newline at end of file and semicolons :)

nani edited the test plan for this revision. (Show Details)Nov 30 2019, 10:20 AM

nani added parent revisions: D1630: Add perlin noise generator., D1633: PerlinPainter add to rmgen..

nani added a child revision: D1636: Dune random map..

Some coding convention notes to keep you waiting for FeXoR.

Comments Start with caps a space and end with dots.
Boolean instead of bool I believe

I guess you could use let for that for loop.

Is it a rmgen convention to have lower case functions?

prototype -> class

Owners added a subscriber: Restricted Owners Package.Nov 30 2019, 10:34 AM

Harbormaster completed remote builds in B10154: Diff 10438.Nov 30 2019, 10:34 AM

nani updated this revision to Diff 10439.Nov 30 2019, 10:43 AM

Harbormaster completed remote builds in B10155: Diff 10439.Nov 30 2019, 10:43 AM

Stan added inline comments.Nov 30 2019, 10:49 AM

binaries/data/mods/public/maps/random/rmgen/painter/DunePainter.js
3	Missing space after star
18	I think we have '-' after the variable name everywhere else Missing final dot :)
38	Typo.
41	Any reason for this magic number ?

Seems I forgot add the possibility to customize the base height (was always 0 if ELEVATION_SET)

Harbormaster completed remote builds in B10156: Diff 10440.Nov 30 2019, 10:50 AM

Fixes (again)

Harbormaster completed remote builds in B10157: Diff 10441.Nov 30 2019, 10:58 AM

nani updated this revision to Diff 10442.Nov 30 2019, 11:18 AM

Harbormaster completed remote builds in B10158: Diff 10442.Nov 30 2019, 11:18 AM

Stan added inline comments.Nov 30 2019, 11:49 AM

binaries/data/mods/public/maps/random/rmgen/painter/DunePainter.js
21	Missing @return
26	Any way to optimise this?
31	Jsdoc?
47	Loop over all the points. Not sure if comment is useful
48	++i
55	Better name ?
61	Ternary?

nani marked 7 inline comments as done.Nov 30 2019, 12:07 PM

nani added inline comments.

binaries/data/mods/public/maps/random/rmgen/painter/DunePainter.js
26	no
31	part of rmgen internals, no need

nani updated this revision to Diff 10443.Nov 30 2019, 12:08 PM

nani marked 2 inline comments as done.

Harbormaster completed remote builds in B10159: Diff 10443.Nov 30 2019, 12:08 PM

FeXoR requested changes to this revision.Nov 30 2019, 12:23 PM

FeXoR added inline comments.

binaries/data/mods/public/maps/random/rmgen/painter/DunePainter.js
6	@ both scaleVertical and baseHeight: In what unit? State that. (e.g. tiles, meters, engine space units, art length units, ...)
20	Nice you added the expected value range but a discription is missing ;)
44	"wider than tall" once more.
56	Those are magic (optimized by fiddling around and testing aiming for the result you find most appealing I guess - that's OK of cause!) numbers anyway so would'nt it be appropriate to use 8.1 instead of 90.9 and 5 instead of 500.1? If this multiplication really helps wrapping your head around what's going on leave them as they are I guess.
61	Huh? When does the case "not this.type" happen???

This revision now requires changes to proceed.Nov 30 2019, 12:23 PM

Mostly good, please try to make the JSdoc phrases and comments more telling and check that last "type" check.

nani marked 3 inline comments as done.Nov 30 2019, 12:38 PM

nani added inline comments.

binaries/data/mods/public/maps/random/rmgen/painter/DunePainter.js
6	`scaleVertical` is a multiplier so its value is adimensional `baseHeight` has the same measure units as any other painter in painter/ implenetation -> world units (irrc 4 world units = 1 tile)
56	0.9 and 0.1 are weights but yes, I prefer to leave the expression expanded for easy modification.
61	this.type can be ELEVATION_SET = 1 or ELEVATION_MODIFY = 0