Mindwerks / worldengine

World generator using simulation of plates, rain shadow, erosion, etc.
MIT License
982 stars 128 forks source link

Modification to the routines for drawing colored elevation maps. #152

Closed esampson closed 8 years ago

esampson commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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

esampson commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

yes, I think you are right

ftomassetti commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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

ftomassetti commented 8 years ago

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 commented 8 years ago

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

psi29a commented 8 years ago

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

esampson commented 8 years ago

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.

esampson commented 8 years ago

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 commented 8 years ago

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

tcld commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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

tcld commented 8 years ago

It's going to be a bit of work, I think I might split my current PR https://github.com/Mindwerks/worldengine/pull/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 commented 8 years ago

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