Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Release new version #139

Open
ftomassetti opened this issue Oct 17, 2015 · 32 comments
Open

Release new version #139

ftomassetti opened this issue Oct 17, 2015 · 32 comments

Comments

@ftomassetti
Copy link
Member

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 v0.18.0...master).

What do you think @psi29a ?

@tcld
Copy link
Contributor

tcld commented Oct 17, 2015

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 #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
Copy link
Member

psi29a commented Oct 17, 2015

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
Copy link
Contributor

tcld commented Oct 17, 2015

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
Copy link
Member

psi29a commented Oct 17, 2015

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
Copy link
Contributor

tcld commented Oct 17, 2015

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
Copy link
Member Author

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

@tcld
Copy link
Contributor

tcld commented Oct 20, 2015

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
Copy link
Contributor

tcld commented Oct 20, 2015

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
Copy link
Member

psi29a commented Oct 20, 2015

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
Copy link
Contributor

tcld commented Oct 20, 2015

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
Copy link
Member

psi29a commented Oct 20, 2015

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Contributor

tcld commented Oct 26, 2015

What kind of timeframe are you thinking about?

@ftomassetti
Copy link
Member Author

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
Copy link
Contributor

tcld commented Oct 26, 2015

One of my PRs (at the very least the change to the RNG-handling #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
Copy link
Member Author

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
Copy link
Contributor

tcld commented Oct 26, 2015

#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
Copy link
Member Author

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
Copy link
Contributor

tcld commented Oct 26, 2015

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
Copy link
Member Author

111 commits since v0.18.0 :)

@tcld
Copy link
Contributor

tcld commented Oct 30, 2015

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
Copy link
Member Author

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

@tcld
Copy link
Contributor

tcld commented Oct 31, 2015

How would I even do that?

@ftomassetti
Copy link
Member Author

You could list yourself in the README file

@ftomassetti
Copy link
Member Author

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
Copy link
Contributor

tcld commented Nov 3, 2015

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
Copy link
Member Author

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

@psi29a
Copy link
Member

psi29a commented Nov 4, 2015

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
Copy link
Contributor

tcld commented Nov 10, 2015

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
Copy link
Member Author

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
Copy link
Contributor

tcld commented Nov 11, 2015

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants