Parameter ocean_level was not passed to platec.

[tcld]

I took a look into the problem of the inability to create oceans with differing depths.
According to the following comment from https://github.com/Mindwerks/plate-tectonics/blob/master/src/lithosphere.cpp
// Find the actual value in height map that produces the continent-sea ratio defined be "sea_level".

I assume that said sea_level is indeed what is responsible for the depth of the ocean and hence isomorphic to the variable ocean_level used in https://github.com/Mindwerks/worldengine/blob/master/worldengine/plates.py

That variable, however, was not passed to generate_plates_simulation() and thus not to platec (line 42 of the commit).

I am still unable to run the worldengines source-code, therefore I cannot guarantee that this fixes the problem described here:
#114
#116

At the very least this looks like a sensible change and one that is simple enough to not cause problems.

This is my very first pull request. Feedback on its style is appreciated.

[tcld]

One thing left to mention:
I renamed the parameter "sea_level" in the Python code to make it consistent. That might be considered bad style, though.

[psi29a]

Looks like the smoke-test went up in flames.

======================================================================
ERROR: test_smoke_full (tests.cli_test.TestCLI)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/build/Mindwerks/worldengine/tests/cli_test.py", line 61, in test_smoke_full
    raise e
RuntimeError: maximum recursion depth exceeded in cmp

Why did you rename the sea_level variable to ocean_level?
Why did you set the default value to 1.0?

[tcld]

I renamed the variable for the sake of consistency. The same goes for the default value - ocean_level is handed down through half a dozen functions, all of which default to 1.0, and as long as it is set anywhere along that chain the final default value will not have an influence anyway.

@psi29a I noticed that your commit didn't pass that final check at first - is there any trick to apply?

EDIT: Before the fix the ocean level was fixed at 0.65, now the parameter handed to worldengine is actually used (and it is usually set to 1.0). As far as I can see, this can be the only source of the failed tests, then. I will take a look into platec later.

[tcld]

I now understand. sea_level refers to the ratio of the surface-areas of land and sea. Terribly named variable.

What exactly does ocean_level refer to and, if it is something different, how to translate it to sea_level?

EDIT: It seems that ocean_level refers to "elevation cut off for sea level (default = 1.0)" - so sea_level and ocean_level would be fundamentally different. (And ocean_level probably never had any effect on the generation.)
Should I look into replacing ocean_level by sea_level or would that be too intrusive?

PS: Sorry for the misinterpretation of sea_level.

[psi29a]

What do you mean, my commit? This is your commit. :)

I suggest running the tests on your machine locally until you find out what the problem is and fix it. It is what we do and why we have tests to begin with. Unless of course, the test is faulty somehow. ;)

[psi29a]

@ftomassetti this is your field, what does ocean_level mean and how does it relate to sea_level?

[tcld]

I think I know what the problem is:

So:
ocean_level as used in the worldengine-code refers to some height relative to the geometry of the world. (Which is a bit problematic since the meaning of a value of "1.0" is seems mysterious.)
sea_level refers to the amount of surface covered by oceans, and that is how platec uses it.

The two are very similar, and there actually is a method in simulations/basic.py that could probably be used for mapping one to the other.

Which course of action would be the better one now: Completely eliminating ocean_levels meaning in favour of the sea_level one (new name: e.g. ocean_coverage), or mapping ocean_level to sea_level when communicating with platec? The latter way seems really difficult to implement, the first one should come with renaming a variable, which might be bad.

[psi29a]

Normally, 0.0 means the lowest possible float point (when dealing with positive numbers) and 1.0 is the highest possible value. So basically the heightmap is made of floating points between 0.0 and 1.0
so a sea-level of 1.0 would mean that everything is sea! :)

[tcld]

I would have expected something like "1.0 refers to an ocean level that is 1 meter above the lowest point of geometry". That would allow for an ocean that covers the whole planet (and even goes above the highest mountains).

I am now looking into replacing ocean_level with sea_level. (And into virtualenv, thank you. :) )

[psi29a]

Sadly we can't deal in infinity (positive and negative), so we deal with 0.0 to 1.0 which is actually common practice in random number generation (and by extension security/cryptography).

Here (and in most other terrain generation applications) would 0.0 be the equivalent to Mariana Trench and 1.0 would be Everest's summit.

https://en.wikipedia.org/wiki/Diamond-square_algorithm <-- easy to implement using positive real numbers 0.0 >= X >= 1.1

[tcld]

I will remove this pull request as soon as possible. For now I think it is useful as a reference for me (and maybe for others to read).

[tcld]

The CI build failed again due to a problem comparing floats. Maybe this is a case for assertAlmostEqual() ?

EDIT: It is not. Somehow the ocean_level float is involved in the randomization. This commit breaks deterministic reproduction of generated worlds using fixed seeds. I'll click the close-button and come back to it tomorrow.

[ftomassetti]

From what I recall there is one value passed to plate-tectonics and it is the value it attributes to sea-level while performing the simulation. I think that the sea-level affects the erosion calculated during plate-tectonics and how the clashes between plates are handled.
Then there is the value we used during normalization: we cover with water the required percentage of the world. Note that we start from the lowest point in the world and we grow the oceans until we have covered enough space. This approach permits to have depressions (land with elevation inferior to the sea level). During the plate-tectonics simulation instead what is below the sea level is just ocean.
To get a more precise answer I think we should dig into the plate-tectonics code.

About breaking the deterministic result: that is ok, some changes affect the final result. In these case I look at the result, decide if it is good enough and use it as a new reference. Depending on the tests that is broken you could have to generate new "blessed images" (see https://github.com/Mindwerks/worldengine/blob/master/tests/blessed_images/generated_blessed_images.py)

[tcld]

The deterministicness is broken in a bad way: When using the same seed twice, the results differ wildly. I am currently trying to trace the seed-value and see where this might happen. Should I figure out the problem, I will update this patch and reopen it.

[psi29a]

I would love to see 'blessed images' be smaller in size... because it takes so long on Travis to test. If that is undesirable, we can just not run that particular test during CI and only when manually running the test-suite.

[ftomassetti]

We could reduce a bit the blessed images but the larger they are the most probable it is that we cover all the possible weird situations that can happen. Of course we could write more unit tests and reduce the need for this sort of functional test.

[tcld]

I think this branch is almost finished. Here some images - 95% ocean, 65% ocean and 30% ocean, in that order:

The worlds all used the seed 7049 but they seem quite different - as far as I understand things this should be platecs doing (otherwise the geometry should be the same and the following steps would differ, right?).

The only thing that is missing is a small fix to one of the test-files (comparing serialized and original ocean_level float currently gives an error due to floating point imprecisions). Since I for sure modified that file in the perf_opt-branch I don't know if I can make changes to it here (I think git would consider the changes a conflict).

EDIT: Since I deleted and recreated the branch that this pull request was based on I will have to open a new one. #134

[psi29a]

Why did you delete and recreate the branch?

You can just do what I suggested:

git rebase -i master

[tcld]

At some point I did git fetch upstream to get the latest changes to master and I wasn't able to push the ocean-branch afterwards. At that point I thought that it might not be useful anyway and... well, I think it is too late for a rebase.

I did use your suggestion for the accidentally committed World_pb2.py, though. It worked and I will remember it for next time. Sorry if my commits are a bit messy at the moment, I only ever used git for private projects so there were a lot less cables to stumble over.

[psi29a]

You're doing fine! :)

Great work!