Added perlin noise feature for topography#922
Open
TilmanMay wants to merge 9 commits into
Open
Conversation
Pull Request Test Coverage Report for Build 23892686894Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
|
Contributor
Author
|
Contributor
|
@TilmanMay Very cool figure!! I can imagine that this will be super useful for landscape evolution modeling to have more control over the initial evolution of a model while still providing randomness. I'll review the code in a few minutes |
Contributor
danieldouglas92
left a comment
danieldouglas92
left a comment
There was a problem hiding this comment.
Thanks for another cool extension of the Perlin Noise functionality! Once again only small comments about documentation and consistency between different features, but once these are addressed I think this is good to go.
In a follow up PR (after your cookbook gets merged), I think it would be great to add another section that documents how you would use this feature in that same cookbook as well.
Comment on lines
+40
to
+47
| * Perlin noise based topography model for oceanic plates. | ||
| * | ||
| * This model generates topography using layered Perlin noise (octaves). | ||
| * Parameters exposed to input files are similar to the continental | ||
| * implementation: min/max depth, min/max topography, frequency, | ||
| * octaves, persistence, lacunarity and operation. | ||
| */ | ||
| class PerlinNoise final: public Interface |
Contributor
There was a problem hiding this comment.
This doc string is much better than the one you have for the continental .h file. I would copy this one and put it there too
Comment on lines
+27
to
+39
| class PerlinNoise final: public Interface | ||
| { | ||
| public: | ||
| PerlinNoise(WorldBuilder::World *world); | ||
| ~PerlinNoise() override final; | ||
|
|
||
| static void declare_entries(Parameters &prm, const std::string &parent_name = ""); | ||
| void parse_entries(Parameters &prm, const std::vector<Point<2>> &coordinates) override final; | ||
|
|
||
| double get_topography(const Point<3> &position, | ||
| const Objects::NaturalCoordinate &position_in_natural_coordinates, | ||
| double topography) const override final; | ||
|
|
Contributor
There was a problem hiding this comment.
Missing doc strings for these functions
Comment on lines
+50
to
+64
| /** | ||
| * Constructor | ||
| * @param world pointer to the containing World instance | ||
| */ | ||
| PerlinNoise(WorldBuilder::World *world); | ||
|
|
||
| /** | ||
| * Destructor | ||
| */ | ||
| ~PerlinNoise() override final; | ||
|
|
||
| /** | ||
| * Declare parameter entries used by this model to the Parameters system. | ||
| */ | ||
| static void declare_entries(Parameters &prm, const std::string &parent_name = ""); |
Contributor
There was a problem hiding this comment.
And you have all the function documentation here that should be copied to continental_plate as well 😄
| void | ||
| PerlinNoise::declare_entries(Parameters &prm, const std::string &) | ||
| { | ||
| // Document plugin. Unlike some topography plugins (e.g. DepthSurface) |
Contributor
There was a problem hiding this comment.
Suggested change
| // Document plugin. Unlike some topography plugins (e.g. DepthSurface) | |
| // Unlike some topography plugins (e.g. DepthSurface) |
| void | ||
| PerlinNoise::declare_entries(Parameters &prm, const std::string &) | ||
| { | ||
| // Document plugin. This model exposes min/max topography and Perlin |
Contributor
There was a problem hiding this comment.
Suggested change
| // Document plugin. This model exposes min/max topography and Perlin | |
| // This model exposes min/max topography and Perlin |
Contributor
|
Also please add a changelog entry! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add the option to include perlin noise for the topography