Page MenuHomeWildfire Games

Add DunePainter.
AcceptedPublic

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

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
/ps/trunk
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 10158
Build 17226: 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 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
20

Missing @return

25

Any way to optimise this?

30

Jsdoc?

46

Loop over all the points. Not sure if comment is useful

47

++i

54

Better name ?

60

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
25

no

30

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
5

@ both scaleVertical and baseHeight: In what unit? State that. (e.g. tiles, meters, engine space units, art length units, ...)

19

Nice you added the expected value range but a discription is missing ;)

43

"wider than tall" once more.

55

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.

60

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
5

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)

55

0.9 and 0.1 are weights but yes, I prefer to leave the expression expanded for easy modification.

60

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.