Mindwerks / worldengine

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

Release new version #139

Open ftomassetti opened 8 years ago

ftomassetti commented 8 years ago

I know there are many refactorings still going on but perhaps we could release a new version because there are already a lot of improvements (over 60 commits since v0.18.0 https://github.com/Mindwerks/worldengine/compare/v0.18.0...master).

What do you think @psi29a ?

tcld commented 8 years ago

Even though I wasn't explicitly asked: I think it would be good to get at least two more things out of the way first.

1. Removing pickle This might simplify installation and, if done at a later point, could break compatibility with old saves. Better get it done as soon as possible (or rule it out completely). I think that pickle-saves would not transfer well between Python 2 and 3, by the way.

2. More changes to seeding different algorithms As stated here https://github.com/Mindwerks/worldengine/pull/133 the current solution does not seem 100% deterministically stable. Therefore changes might come up at some point that make old worlds "unregeneratable". I don't know yet how, but if I could polish that branch so that those concerns will never crop up again, that would be really nice. (Help is appreciated since so far I have not come up with a clean solution.)

Aside from those things a new release would be great! Especially for users under Windows it is probably quite painful to get the latest version running. Giving them an easy-to-use release with some incredible speed-boosts (50-66% less time needed than before) on top would certainly be really good. :)

psi29a commented 8 years ago

We can re-write the pickle tests to only run based on python version. That way we can keep pickle and compare apples to apples as a result. Not that I like pickle, I'm just not 100% sold on protobuf yet. :)

tcld commented 8 years ago

protobuf is a Google-thing, isn't it? They tend to radically change things without hesitation, so I am kind of with you on that.^^

Do you know what would have to be changed to make pickle work properly?

psi29a commented 8 years ago

Nothing really, just that world files created with python2 should only be compared on python2. Same with python3, the resulting file should only be read by python3. I suggest in our current tests to add a check on python version:

    @unittest.skipIf(mylib.__version__ < (1, 3),
                     "not supported in this library version")
    def test_format(self):
        # Tests that work for only a certain version of the library.
        pass

https://docs.python.org/2/library/unittest.html#skipping-tests-and-expected-failures

That is an example of course, the @blahblah are decorators that we can use.

We can then write tests that are specific to a version.

We can nuke pickle and the tests when we are comfortable with our new 'stable' format.

In the next release, we can make protobuf the default and see what breaks for people. :)

The release after that, we can consider removing pickle.

tcld commented 8 years ago

That is a great solution. Once Google releases a proper version 3.0 of protobuf (not an alpha) we will at least have that as a fallback, should they ever decide to completely change directions on it. But until then supporting both formats sounds like a very good idea. (How about a custom format, by the way? I don't even know what things are really saved, but I loved to come up with custom file-formats in C++.^^)

ftomassetti commented 8 years ago

Probably next version should be 0.19.0: there are a lot of improvements!

tcld commented 8 years ago

To come back to this:

 @unittest.skipIf(mylib.__version__ < (1, 3),
                     "not supported in this library version")
    def test_format(self):
        # Tests that work for only a certain version of the library.
        pass

Would this not be equally good, maybe even better? This way the functionality that is the same under Python 2 and 3 can easily be reused.

diff --git a/tests/blessed_images/generated_blessed_images.py b/tests/blessed_images/generated_blessed_images.py
index 913ccbe..0d070b2 100644
--- a/tests/blessed_images/generated_blessed_images.py
+++ b/tests/blessed_images/generated_blessed_images.py
@@ -8,6 +8,7 @@ A script, generate_blessed_images, can be used to regenerate blessed images
 """

 import os
+import platform
 from worldengine.world import *
 from worldengine.draw import *

@@ -26,7 +27,10 @@ def main(blessed_images_dir, tests_data_dir):
     draw_temperature_levels_on_file(w, "%s/temperature_28070.png" % blessed_images_dir)
     draw_biome_on_file(w, "%s/biome_28070.png" % blessed_images_dir)

-    w_large = World.from_pickle_file("%s/seed_48956.world" % tests_data_dir)
+    if platform.python_version_tuple()[0] < 3:
+        w_large = World.from_pickle_file("%s/py2_seed_48956.world" % tests_data_dir)
+    else:
+        w_large = World.from_pickle_file("%s/py3_seed_48956.world" % tests_data_dir)
     draw_ancientmap_on_file(w, "%s/ancientmap_28070_factor3.png" % blessed_images_dir, resize_factor=3)
     draw_ancientmap_on_file(w_large, "%s/ancientmap_48956.png" % blessed_images_dir, resize_factor=1)

diff --git a/tests/data/data_generator.py b/tests/data/data_generator.py
index a47dcee..5793ad0 100644
--- a/tests/data/data_generator.py
+++ b/tests/data/data_generator.py
@@ -8,12 +8,16 @@ because the plate simulation steps do not provide the same results on all the pl
 """

 import os
+import platform
 from worldengine.plates import _plates_simulation

 def main(tests_data_dir):
     w = _plates_simulation("Foo", 300, 200, 279)
-    w.to_pickle_file("%s/plates_279.world" % tests_data_dir)
+    if platform.python_version_tuple()[0] < 3:
+        w.to_pickle_file("%s/py2_plates_279.world" % tests_data_dir)
+    else:
+        w.to_pickle_file("%s/py3_plates_279.world" % tests_data_dir)

 if __name__ == '__main__':
diff --git a/tests/drawing_functions_test.py b/tests/drawing_functions_test.py
index 1a6775d..a18e676 100644
--- a/tests/drawing_functions_test.py
+++ b/tests/drawing_functions_test.py
@@ -1,4 +1,5 @@
 import unittest
+import platform

 from worldengine.drawing_functions import draw_ancientmap, gradient, draw_rivers_on_image
 from worldengine.world import World
@@ -12,7 +13,10 @@ class TestDrawingFunctions(TestBase):
         self.w = World.open_protobuf("%s/seed_28070.world" % self.tests_data_dir)

     def test_draw_ancient_map_factor1(self):
-        w_large = World.from_pickle_file("%s/seed_48956.world" % self.tests_data_dir)
+        if platform.python_version_tuple()[0] < 3:
+            w_large = World.from_pickle_file("%s/py2_seed_48956.world" % self.tests_data_dir)
+        else:
+            w_large = World.from_pickle_file("%s/py3_seed_48956.world" % self.tests_data_dir)
         target = PixelCollector(w_large.width, w_large.height)
         draw_ancientmap(w_large, target, resize_factor=1)
         self._assert_img_equal("ancientmap_48956", target)
diff --git a/tests/generation_test.py b/tests/generation_test.py
index 8ad27d3..2142f9c 100644
--- a/tests/generation_test.py
+++ b/tests/generation_test.py
@@ -1,4 +1,5 @@
 import unittest
+import platform
 from worldengine.plates import Step, center_land, world_gen
 from worldengine.world import World

@@ -29,7 +30,10 @@ class TestGeneration(TestBase):
         return borders_total_elevation / n_cells_on_border

     def test_center_land(self):
-        w = World.from_pickle_file("%s/plates_279.world" % self.tests_data_dir)
+        if platform.python_version_tuple()[0] < 3:
+            w = World.from_pickle_file("%s/py2_plates_279.world" % self.tests_data_dir)
+        else:
+            w = World.from_pickle_file("%s/py3_plates_279.world" % self.tests_data_dir)

         # We want to have less land than before at the borders
         el_before = TestGeneration._mean_elevation_at_borders(w)

If that was ok, I would immediately generated the Python 3 file, rename the Python 2 file, create a PR for this patch and all pickle-related problems might be delayed for a bit. :P

tcld commented 8 years ago

Here is a different version, prettier in my opinion.

diff --git a/tests/blessed_images/generated_blessed_images.py b/tests/blessed_images/generated_blessed_images.py
index 913ccbe..d3d03b6 100644
--- a/tests/blessed_images/generated_blessed_images.py
+++ b/tests/blessed_images/generated_blessed_images.py
@@ -8,6 +8,7 @@ A script, generate_blessed_images, can be used to regenerate blessed images
 """

 import os
+import platform
 from worldengine.world import *
 from worldengine.draw import *

@@ -26,7 +27,7 @@ def main(blessed_images_dir, tests_data_dir):
     draw_temperature_levels_on_file(w, "%s/temperature_28070.png" % blessed_images_dir)
     draw_biome_on_file(w, "%s/biome_28070.png" % blessed_images_dir)

-    w_large = World.from_pickle_file("%s/seed_48956.world" % tests_data_dir)
+    w_large = World.from_pickle_file("%s/py%s_seed_48956.world" % (tests_data_dir, platform.python_version_tuple()[0]))
     draw_ancientmap_on_file(w, "%s/ancientmap_28070_factor3.png" % blessed_images_dir, resize_factor=3)
     draw_ancientmap_on_file(w_large, "%s/ancientmap_48956.png" % blessed_images_dir, resize_factor=1)

diff --git a/tests/data/data_generator.py b/tests/data/data_generator.py
index a47dcee..2d627c3 100644
--- a/tests/data/data_generator.py
+++ b/tests/data/data_generator.py
@@ -8,12 +8,13 @@ because the plate simulation steps do not provide the same results on all the pl
 """

 import os
+import platform
 from worldengine.plates import _plates_simulation

 def main(tests_data_dir):
     w = _plates_simulation("Foo", 300, 200, 279)
-    w.to_pickle_file("%s/plates_279.world" % tests_data_dir)
+    w.to_pickle_file("%s/py%s_plates_279.world" % (tests_data_dir, platform.python_version_tuple()[0]))

 if __name__ == '__main__':
diff --git a/tests/drawing_functions_test.py b/tests/drawing_functions_test.py
index 1a6775d..91600a5 100644
--- a/tests/drawing_functions_test.py
+++ b/tests/drawing_functions_test.py
@@ -1,4 +1,5 @@
 import unittest
+import platform

 from worldengine.drawing_functions import draw_ancientmap, gradient, draw_rivers_on_image
 from worldengine.world import World
@@ -12,7 +13,7 @@ class TestDrawingFunctions(TestBase):
         self.w = World.open_protobuf("%s/seed_28070.world" % self.tests_data_dir)

     def test_draw_ancient_map_factor1(self):
-        w_large = World.from_pickle_file("%s/seed_48956.world" % self.tests_data_dir)
+        w_large = World.from_pickle_file("%s/py%s_seed_48956.world" % (self.tests_data_dir, platform.python_version_tuple()[0]))
         target = PixelCollector(w_large.width, w_large.height)
         draw_ancientmap(w_large, target, resize_factor=1)
         self._assert_img_equal("ancientmap_48956", target)
diff --git a/tests/generation_test.py b/tests/generation_test.py
index 8ad27d3..0a16b4a 100644
--- a/tests/generation_test.py
+++ b/tests/generation_test.py
@@ -1,4 +1,5 @@
 import unittest
+import platform
 from worldengine.plates import Step, center_land, world_gen
 from worldengine.world import World

@@ -29,7 +30,7 @@ class TestGeneration(TestBase):
         return borders_total_elevation / n_cells_on_border

     def test_center_land(self):
-        w = World.from_pickle_file("%s/plates_279.world" % self.tests_data_dir)
+        w = World.from_pickle_file("%s/py%s_plates_279.world" % (self.tests_data_dir, platform.python_version_tuple()[0]))

         # We want to have less land than before at the borders
         el_before = TestGeneration._mean_elevation_at_borders(w)
psi29a commented 8 years ago

If the pickle can be written and read by both Py2 and Py3, then that is also great IMHO.

@ftomassetti we ok with replacing the blessed images? Did you want to eyeball them first?

tcld commented 8 years ago

I hope you noticed this:

"%s/py%s_plates_279.world" % (self.tests_data_dir, platform.python_version_tuple()[0])

Regarding the "new" blessed images: They are probably so different because I fed different parameters to platec.

psi29a commented 8 years ago

The blessed images are just the output created from the world files... 'blessed' is all data that goes into worldengine-data. ;)

Ah, now that you mentioned it... building it into the name is rather handy. :)

ftomassetti commented 8 years ago

The idea of the "blessed" images is that any time you change them you look at them and approve them to be used in tests after "visual inspection". In other words you have to check they looks good and give to them your blessing.

ftomassetti commented 8 years ago

86 commits and 43 files changed so far. I would definitely go for v 0.19.0: this is a huge improvement over v 0.18.0! I would love to release what has been done throught these great contributions (thanks @tcld and @esampson !) and share it.

tcld commented 8 years ago

What kind of timeframe are you thinking about?

ftomassetti commented 8 years ago

Nothing specific, but it would be nice to have a new release soon. Bear in mind that nothing prevents us to release v0.19.1 the day after we released v0.19.0

tcld commented 8 years ago

One of my PRs (at the very least the change to the RNG-handling https://github.com/Mindwerks/worldengine/pull/146) will break seed-compatibility, meaning that old seeds will produce different worlds. At the same time, if done right, this might guarantee seed-compatibility for the foreseeable future. So I think this should be done before a new release.

Then there is the non-working parameter ocean_level, which I so far failed to properly handle (the output was ugly). Should something be done about that before a release? Maybe the parameter should be "muted" for now so users aren't confused by it? (Solving the problem would be a lot better, of course.)

And, finally, I was working on changing the standard heightmap-output to 16 Bit grayscale PNGs - which I think could be very useful if worldengine was used to create 3D-worlds. If the quality of the 16 Bit heightmaps was satisfying, this, too, would be a nice thing to include in a release.

And just to be sure: Are there any major bugs to take care of at the moment? I saw the negative masses crop up once or twice (that's platec, right?), but that seems rare and (strangely) still leads to valid worlds, as far as I can tell.

ftomassetti commented 8 years ago

I think that the compatibility breakage are the only things which necessarily goes in: bug fixes and backward-compatible improvements can be inserted in a successive version without any problem I think. In my opinion release early, release often is a very valid principle for Open-Source (and for most kinds of software).

If we start to produce a new release the user could benefit from the great improvements we already have and start reporting possibly new bugs we introduced :) In the meantime we can work on the next release with even more cool stuff.

So we can definitely wait for #146 (no hurry!) and also for the 16 Bit grayscale PNGs. That is just my opinion, we should also listen to @psi29a of course

tcld commented 8 years ago

146 is a very simple one, although there is one thing I need some input about before finishing it:

https://github.com/Mindwerks/worldengine/pull/146/files#diff-80eed145c1070172ffba1f642a13b731R185

The 16 Bit-stuff...well, I continued working on that for probably too long (I fully replaced Pillow in my local branch) and now I am not sure if I have gone too far for the sake of getting rid of a single dependency. So that needs some thinking and refining by me before I can even update the corresponding PR.

ftomassetti commented 8 years ago

If you are asking an opinion about the sub_seeds thing it seems fine to me. However could be a bit painful to specify the right index (sub_seeds[0] for precipitation, sub_seeds[1] for erosion, etc. What if I add a step in between?). Perhaps you can use constants or a wrapper class for that which exposes the method next_seed.

tcld commented 8 years ago

I wanted things to be deterministic - if one were to add a step in between, one could use sub_seeds[2] (or any other one as there are 100 of them). Some kind of mapping or similar might be prettier, though. I will think about how to do it. (Maybe just use a dictionary that maps strings to numbers?)

ftomassetti commented 8 years ago

111 commits since v0.18.0 :)

tcld commented 8 years ago

I have only one "new" thing left in mind that I might start working on, aside from that I will focus on finishing the things I started (that includes 16 Bit heightmaps, for which there is no PR at the moment).

I realized that I never updated the changelog, maybe I should do that at the end. Although almost none of my changes will be visible at the surface, so sped things up and totally broke seed-compatibility might be enough to add.^^

ftomassetti commented 8 years ago

You should definitely add yourself as a contributor. We are now at 135 commits since v0.18.0 :)

tcld commented 8 years ago

How would I even do that?

ftomassetti commented 8 years ago

You could list yourself in the README file

ftomassetti commented 8 years ago

170 commits since version 0.18.0

This is a lot :)

I would like just to get the satellite map because it comes from a new contributor and we want his code to be part of a new release. After that I think we could make new binary packages and make it available for people who do not want to know what a compiler is.

And promote it, and asks for more feedback and keep working on really cool stuff.

And also discuss an agenda for version 1.0.0

tcld commented 8 years ago

It's probably a good time for a release. The only open issue I remember is the one with non-working ocean-levels, but maybe that is fine for a release? Aside from that we should hope for a time with less PRs.^^

And yes, the satellite-PR should go in. And be non-optional in my opinion. I don't really see any use for it aside from eye-candy, but at that it is a tremendously great addition I wouldn't want to see left out.

ftomassetti commented 8 years ago

I love PRs, I hope to keep having a lot of those good PRs you Evan and others keep throwing at us :D

psi29a commented 8 years ago

We should hammer out the open PRs first.

I was hoping that documentation will make it in, but it isn't necessary for a release.

tcld commented 8 years ago

Probably not the best place to mention this, but I noticed that there are quite a lot of branches to choose from in the WorldEngine-repo at the moment. I assume that at least some of them stem from merged PRs - could they maybe deleted?

ftomassetti commented 8 years ago

I closed a few merged PRs, others are connected to open PRs or require some investigation to understand if they still have valuable code.

tcld commented 8 years ago

It's already a lot better, thank you very much! :)