Page MenuHomeWildfire Games
Differential D1635

Add DunePainter.
AcceptedPublic
Actions

Authored by nani on Sep 22 2018, 1:52 AM.

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
FeXoR added a reviewer: FeXoR.Nov 26 2019, 7:16 PM
FeXoR added a subscriber: FeXoR.

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...)

nani added a comment.Nov 26 2019, 7:20 PM
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

Stan added a subscriber: Stan.Nov 27 2019, 11:40 AM

@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..
Stan added a comment.EditedNov 30 2019, 10:21 AM

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?

nani updated this revision to Diff 10438.Nov 30 2019, 10:34 AM

prototype -> class

Owners added a subscriber: Restricted Owners Package.Nov 30 2019, 10:34 AM
nani updated this revision to 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 ?

nani updated this revision to Diff 10440.Nov 30 2019, 10:50 AM

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

nani updated this revision to Diff 10441.Nov 30 2019, 10:58 AM
nani marked 4 inline comments as done.

Fixes (again)

nani updated this revision to 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.
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 9*0.9 and 5 instead of 50*0.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
FeXoR added a comment.Nov 30 2019, 12:26 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

nani updated this revision to Diff 10444.Nov 30 2019, 12:58 PM
nani marked 2 inline comments as done.
nani marked 2 inline comments as done.Nov 30 2019, 12:59 PM
nani updated this revision to Diff 10445.Nov 30 2019, 1:07 PM
FeXoR accepted this revision.Nov 30 2019, 8:30 PM

After required D1630 and D1633 are added this is good to go ;)

This revision is now accepted and ready to land.Nov 30 2019, 8:30 PM
nani updated this revision to Diff 10489.EditedDec 6 2019, 4:19 PM

Made better dune generation, faster & better code, changed dependencies.

Revision Contents
Changeset List

Diff 10489

View Options

binaries/data/mods/public/maps/random/rmgen/painter/DunePainter.js

Loading...
Wildfire Games · Phabricator