MM1nd commented 6 years ago

Hi again,

this is at best the first step on a long journey to make ancient_map faster. However, this is huge already, so I'm making it a PR before continuing. Note that while there is one more PR to come for generate_world this is not related to those changes I made a year ago. This is new.

It does a lot of things so let's explain:

Biomes

ancient_map groups several biomes together. For example BorealMoistForest, BorealWetForest and BorealRainForest are grouped together as BorealForest. The information, how this grouping is done was hardcoded in the implementation of some functions in world.py e. g. is_boreal_forest.
I argue that this is not a good idea. Because a) I need this information to make ancient_map fast and there is nowhere to read it from other than the code itself and b) it's inconsistent. The place to put all information regarding biomes, and that includes their grouping, should be in biome.py
The changes to biome.py make good on the concept established earlier therein to model biomes through the class hierarchy. These changes are straightforward and consistant with how biomes always worked.
We can provide functions in world.py who use this and work exactly like the old is_*functions. There is one example left and that is is_iceland. The other functions of that sort can be written in the same style, at some point during the refactoring I had them all there, but eventually nobody calls them, there is no testing code, and they were terribly slow. So I decided to remove them. I know that might be controversial, but I really think it's for the better. I'm pretty sure they were only added for the one use case that's no longer there, anyway.
This also means that world.py needs to import far fewer things.
The end game here is to get rid of tiles_around and the like entirely. I can see the beauty of the concept but these functions are excruciatingly slow and they are not easier to use than manipulating the data directly with numpy. No offense, but I think this was overengineering, it did not work out, and it should no longer be there.

Bugfix

draw_ancient_map was called with verbose=False disregarding the command line. I fixed that in main.py

Quality of Life Improvement

I added a function count_neighbours to common.py that given an array of binary values can count how many neighbours of each coordinate are set to one. It is fast.

Refactorings

There was a lot of code in drawing_functions.py that screamed "copy and paste". The github diff is horrible at showing this to the point of being useless, but there are only three different pixel patters that where used in several functions so I extracted them to be reused (_draw_forest_pattern1, _draw_forest_pattern2, _draw_desert_pattern).
In the same vein, the shading method for most of the other biomes was the same fundamentally so I factored that out too (_draw_shaded_pixel).
Note that this PR comes with 237 additions and 465 deletions while being functionally equivalent and faster. This is not the result of trying to make the code short at the cost of becoming obscure, I think the readability went up.

Speedup

Calculation of masks for the individual biomes was extremely slow and used a lot of code. With the new biome groups it is all done generically in _build_biome_group_masks. All other mask related code is simply gone. This makes it super easy to add new biome groups.
The rest of the changes in drawing_functions.py make it so that the generic masks are used. These changes are straightforward.
As for the effect, this brings test_ancient_map to unter a minute for the first time on my machine. It takes a full minute off the clock for drawing a 1024*512 map at scaling factor 1 (from ~4 minutes to ~3 minutes)

Next steps

There is another anti_alias function here, distinct from the one in common.py and it's very slow. With this PR everything is prepared to tackle that issue next.

Cheers Alex

ftomassetti commented 6 years ago

The code definitely looks nicer, if we are sure it also produces the same result... it is quite an achievement!

coveralls commented 6 years ago

Coverage increased (+0.04%) to 85.331% when pulling a6c3b96fd6a261d69dc17fc4f344e11b7abb3d6c on MM1nd:refactor_drawing_functions into b5db0c6db7b99636a7490049db4610beb14d5a23 on Mindwerks:master.

coveralls commented 6 years ago

Coverage increased (+0.04%) to 85.331% when pulling a6c3b96fd6a261d69dc17fc4f344e11b7abb3d6c on MM1nd:refactor_drawing_functions into b5db0c6db7b99636a7490049db4610beb14d5a23 on Mindwerks:master.

MM1nd commented 6 years ago

"if we are sure it also produces the same result... it is quite an achievement!"

I triple checked at all stages. Not saying its impossible that I missed something, but it seems unlikely. Ideally you make a reference image and see for yourself. The tests are good (currently travis fails for a pyflakes annoyance, I'm about to fix that)

codecov-io commented 6 years ago

Codecov Report

Merging #248 into master will decrease coverage by 0.67%. The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #248      +/-   ##
==========================================
- Coverage    80.4%   79.72%   -0.68%     
==========================================
  Files          28       28              
  Lines        3888     3625     -263     
  Branches      768      726      -42     
==========================================
- Hits         3126     2890     -236     
+ Misses        572      546      -26     
+ Partials      190      189       -1

Impacted Files	Coverage Δ
worldengine/biome.py	`100% <100%> (ø)`	:arrow_up:
worldengine/common.py	`84.88% <100%> (+1.98%)`	:arrow_up:
worldengine/drawing_functions.py	`80.8% <100%> (-3.55%)`	:arrow_down:
worldengine/cli/main.py	`79% <100%> (ø)`	:arrow_up:
worldengine/model/world.py	`74.5% <100%> (-1.9%)`	:arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update b5db0c6...7fbe66b. Read the comment docs.

coveralls commented 6 years ago

Coverage decreased (-0.8%) to 84.455% when pulling a58344d4ea57ea6f52021b8802c2d8ecd524f137 on MM1nd:refactor_drawing_functions into b5db0c6db7b99636a7490049db4610beb14d5a23 on Mindwerks:master.

coveralls commented 6 years ago

Coverage decreased (-0.8%) to 84.455% when pulling a58344d4ea57ea6f52021b8802c2d8ecd524f137 on MM1nd:refactor_drawing_functions into b5db0c6db7b99636a7490049db4610beb14d5a23 on Mindwerks:master.

coveralls commented 6 years ago

Coverage decreased (-0.6%) to 84.725% when pulling 4e15c106a212f6b04b7f117ebf4ba484c7b8792f on MM1nd:refactor_drawing_functions into b5db0c6db7b99636a7490049db4610beb14d5a23 on Mindwerks:master.

coveralls commented 6 years ago

Coverage decreased (-0.6%) to 84.725% when pulling 4e15c106a212f6b04b7f117ebf4ba484c7b8792f on MM1nd:refactor_drawing_functions into b5db0c6db7b99636a7490049db4610beb14d5a23 on Mindwerks:master.

coveralls commented 6 years ago

Coverage decreased (-0.6%) to 84.725% when pulling 4e15c106a212f6b04b7f117ebf4ba484c7b8792f on MM1nd:refactor_drawing_functions into b5db0c6db7b99636a7490049db4610beb14d5a23 on Mindwerks:master.

coveralls commented 6 years ago

Coverage decreased (-0.4%) to 84.938% when pulling 7fbe66b336036f0af494979bf391e651a6284cde on MM1nd:refactor_drawing_functions into b5db0c6db7b99636a7490049db4610beb14d5a23 on Mindwerks:master.

coveralls commented 6 years ago

Coverage decreased (-0.4%) to 84.938% when pulling 7fbe66b336036f0af494979bf391e651a6284cde on MM1nd:refactor_drawing_functions into b5db0c6db7b99636a7490049db4610beb14d5a23 on Mindwerks:master.

psi29a commented 6 years ago

I'm overwhelmed by how professional your posts/PRs are, from documentation to code itself. Great job!

MM1nd commented 6 years ago

Well, thank you!

psi29a commented 6 years ago

time python worldengine -x 1024 -y 1024 went from real 5m43.636s to real 2m5.086s

Great work!

Mindwerks / worldengine

Refactor drawing functions #248

Codecov Report