Modification to the routines for drawing colored elevation maps.

[esampson]

This raises the minimum altitude of the land to sea level and then normalizes the elevation values to the range of 1 to 12 for the colored elevation map. This prevents the need of repeating color bands to represent higher ranges. As elevation has no inherent value the normalization causes no real distortions. The raising of the minimum land value means there is no land with an altitude above sea level (something which can occur in the real world) but it makes the elevation map easier to import into other programs such as Fractal Terrains and such a solution is already used in the grayscale height maps.

This modification will affect the output of the colored elevation map to an extent where any blessed images of elevation maps will need to be recalculated.

[tcld]

You meant "no land below sea-level", I assume?

Since you mentioned the same being done for the grayscale-maps: When I worked on 16 Bit export, I removed that behaviour since I didn't see the reason for it. I was afraid the coasts would be made very unrealistic, but maybe I was wrong.

[esampson]

Sorry. Yes, I meant no land below sea level. This isn't something that occurs very often on Earth. No where near to the amount or the degree that is seems to be happening in world engine, so I thought it should be removed. Additionally it tends to make things kind of ugly when the maps get handed to other programs (since they tend to assume that anything below sea level is sea).

It doesn't appear to cause a significant change in the coastlines.

[ftomassetti]

I think this is a very, very, very good idea. Historically speaking we used to generate this simple elevation map BEFORE we calculated where there was ocean and where there was not. As a result we could not use the ocean matrix and depressions ended up being represented as sea. We did not have neither gray maps so having a map with a direct correlation between elevation and color was useful for usage in external programs. Right now having depressions represented as sea is just bad, confusing and make generated worlds look like crap.

So to conclude THANK YOU we really need this fix!

[ftomassetti]

About the tests failing, we probably need to regenerate some blessed images. Let me take a look at this.

[esampson]

We need to generate some new blessed images, but I'm getting better at understanding the error messages and they have helped me chase down some other issues. Mainly calls that I hadn't updated.

draw_simple_elevation and draw_simple_elevation_on_file were a little archaic. In the past we passed the elevation data as one array and the width and height as two separate variables. Because I also need to access the is_land function of the world I am now just passing the world object itself, which also happens to be how the newer routines work, so it's sort of a win/win.

[esampson]

Ok. If I'm reading things right there's an error being caused by the routine being called before the ocean array is created but that actual failure is being caused by the fact that the blessed image changes.

[ftomassetti]

yes, I think you are right

[ftomassetti]

When we call the method from generate_plates we do not have the ocean matrix calculated so we need some fallback. I think that in most cases we do the full generation so it definitely makes sense to use the ocean matrix in that case. However we need to be able to draw something when we stop at the plates phase.

[esampson]

I just handled that. generate_plates passes a sea_level of -1 and draw_simple_elevation now no longer checks for the ocean attribute in cases where the sea level is set to -1. Perhaps not the most elegant of solutions, but it works.

[esampson]

Ok. If I'm reading this correctly the only problem now is that the output doesn't agree with the blessed images.

[ftomassetti]

ok, we can solve that by regenerating the blessed images in the worldengine-data repository. Let's give the possibility to @psi29a to comment on this one. If he is ok with this we can:

1. regenerate the images using this code and commit them to worldengine-data
2. restart the tests on this branch manually on AppVeyor and Travis
3. merge this patch

[ftomassetti]

and the next release of WorldEngine is going to be great!

[psi29a]

Be sure to update the changelog with what you've changed and added. :)

If you believe it just needs new blessed images, then gen a few and send a PR to:
https://github.com/Mindwerks/worldengine-data

[tcld]

I had been working on most of the image-drawing methods in #141 . I won't be finished first, so I'll have to adapt to your changes. But: Please remember that now all arrays are numpy-arrays and this part of the code could be drastically shortened/sped up.
You can find some (deactivated) code here, should you need an example:
https://github.com/Mindwerks/worldengine/pull/141/files#diff-c209aa2ca5a09dd35a85ec7844837b6eR230

That does pretty much the same as you are doing here.

[tcld]

Here the relevant code:

mask = numpy.ma.array(world.elevation['data'], mask = world.ocean)
min_elev_land = mask.min()
max_elev_land = mask.max()

mask.mask = numpy.logical_not(mask.mask)
min_elev_sea = mask.min()
max_elev_sea = mask.max()

[tcld]

This replaces lines 170 till 191, inclusive.
mask is a masked array, every part of the map that corresponds to ocean is masked out. Therefore calling mask.min() etc. will give you the minimum of the non-masked elements.

Then the mask is inverted (now everything that is land is masked out) and the functionality of mask.min() changes accordingly.

[esampson]

Ok. sea_level now gets set to None and the comparison has been changed to sea_level is None. As I was generating the new blessed image I found that I needed to update the blessed image routine as well to accommodate the updated routine.

Also an updated blessed image has been submitted to worldengine-data.

[tcld]

If you added some numpy-code before this loop, you could get rid of the if-else-statements (and prepare the code for later changes where maybe the whole loop can be worked off in a single line of code):

c = numpy.array(world.elevation['data'].shape)
c[numpy.invert(world.ocean)] = ((world.elevation['data'] - min_elev_land) / elev_delta_land) + 1)
c[world.ocean] = ((world.elevation['data'] - min_elev_sea) / elev_delta_sea))

Then loop over c and use c[x, y].

[esampson]

I'll have to sit down and look at numpy a little bit. It's not clear to me how this is functioning. It looks like this should be for a later block of code than what you've marked and I'm guessing there's some additional code that needs to be added to convert c back to world.elevation.

[tcld]

The big thing you can do with numpy is operate on the whole array at once.
c is just an array filled with garbage data and the same shape as the elevation map (shape is a tuple (height, width) in this case)
If a numpy array is passed an "index" that is actually a bool array, you will operate on all elements of the array where the booleans are True. So c[numpy.invert(world.ocean)] operates on all areas of the array c that correspond to land (c[world.ocean] then operates on all that corresponds to ocean).

The things on the right side of the = are operations that are done on the whole array, as said initially.

The block of code I posted replaces this part:

+    for y in range(world.height):
+        for x in range(world.width):
+            e = world.elevation['data'][y, x]
+            if sea_level is None:
+                c = ((e - min_elev_land) / elev_delta_land) + 1
+            elif world.is_land((x, y)):
+                c = ((e - min_elev_land) / elev_delta_land) + 1
+            else:
+                c = ((e - min_elev_sea) / elev_delta_sea)

Although afterwards you will still have to iterate over the colour-array c to write out its values:

+    for y in range(world.height):
+        for x in range(world.width):
+            r, g, b = elevation_color(c[y, x], sea_level)
             target.set_pixel(x, y, (int(r * 255), int(g * 255),
                                     int(b * 255), 255))

[esampson]

Why don't I leave that to you, since you have more experience than I do with numpy?

This process has given me much better insight into the commands for coverage testing and I'm now able to test coverage locally. As a result I would like to spend a little time creating some new tests to expand coverage. Biome's only testing about 70% right now.

[ftomassetti]

Improving coverage would be EXTREMELY appreciated

[tcld]

Ok, improving coverage sounds fine. I'll go over the numpy-things afterwards, if you don't want to do it.

[esampson]

Ok. Looks like worldengine-data has been updated with the new blessed image but I don't seem to have a way to manually restart the checks.

[ftomassetti]

I restarted it. Perhaps you need the permissions on Travis and AppVeyor to do so. Any way the checks should be running now :)

[tcld]

I gave some explanations on the numpy code and hopefully clarified which lines of code it would replace.
Something about masked arrays: http://docs.scipy.org/doc/numpy/reference/maskedarray.html
Something about arrays in numpy: http://docs.scipy.org/doc/numpy/reference/arrays.html

numpy isn't too easy to use at first, but it gives a huge speed improvement due to many steps being done at once in native code (the code in irrigation.py was sped up by a factor of ~50 by switching to numpy, so it is really worth it). Getting the hang of at least some simple operations like min() and max() is really useful.

[ftomassetti]

Tests passing now. I am ready to merge if you are ok with it. I will just wait your ok, in case there is still some comment that you want to address.

Thank you @esampson and @tcld !

[tcld]

It would be nice to see the numpy-changes go in, but since I have been working on that file, too (and that work isn't finished yet), I could just as easily put it in myself in the very near future.

[psi29a]

@tcld any way that after this is merged that you can rebase your work on master?

[tcld]

It's going to be a bit of work, I think I might split my current PR #141 into two or three (GDAL, numpy and 16 Bit heightmaps) so the chance of them being touched by other work goes down and the code is easier to look over. In the end I hope it all works out.

[ftomassetti]

Ok then we can merge this one now and refactor it later given that anyway that area of code is going to be touched

[tcld]

Just noticed this. Even though there is technically no ocean yet, platec did run under the assumption that a certain percentage of the world was covered with ocean, that at the very least has a big influence on the resulting world. I am not sure if None is really the best choice here.

Also: You removed sea_levels purpose of existence but left the variable in - this makes the pyflakes test fail. It has to be removed.

[esampson]

I assume you are referring to line 106/105 and if you are then you're right, that line can be removed as it isn't doing anything. I just didn't think of it at the time.

While platec has made some assumptions concerning sea level at this point of production no ocean generation has been done. As such we need to pass a value that tells draw_simple_elevation_on_file not to try and reference the ocean mask.

I could examine the world and see if it has an ocean mask, at which point it will run as it currently runs, but at that point it would be ignoring the sea_level anyway. Since draw_simple_elevation_on_file doesn't pass the sea_level on to anything else (except for draw_simple_elevation) it would result in a change of code that doesn't really change anything.

[tcld]

Ok. I had been working on something to change the ocean-level but it turned out that passing a different value to platec (currently it always assumes 65% of the surface to be covered with ocean) resulted in ugly maps. So let's just do it your way and come back to this in the distant future. (But the dead variable should be taken care of by somebody to make pyflakes happy again.)