Page MenuHomeWildfire Games

Add DunePainter.
Needs ReviewPublic

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

Details

Reviewers
None
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
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 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 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

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.

Stan added subscribers: marder, lyv.Mar 12 2022, 9:05 PM

@smiley @marder any opinion on this?

marder added a comment.EditedMar 12 2022, 9:56 PM

Well, the map looks good, but I can't say that much about the code (Due to not having that much knowledge about random noise creation and due to not having looked at it that much).
As far as I can see this depends on perlin noise (or fractal noise?), which raises the question to me if perlin noise delivers that much better results that the noise that is already in rmgen ?
The last thing I heard about perlin noise is that there are copyright disputes, which is why OpenSimplex Noise was developed.

lyv added a comment.Mar 12 2022, 10:57 PM

Yes, already voiced at D2454.

Stan edited reviewers, added: Restricted Owners Package; removed: FeXoR.Oct 22 2022, 8:08 PM
This revision now requires review to proceed.Oct 22 2022, 8:08 PM