Mindwerks / worldengine

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

support hdf5 serialization/unserialization of all the world data #153

Closed ftomassetti closed 8 years ago

ftomassetti commented 8 years ago

Fix #91

tcld commented 8 years ago

I have not checked every single line, it's a bit too late for that (here). But I love it anyway.^^

What are the major advantages of this format, by the way? I know that pickle isn't fully compatible between Python 2 and 3, and I read about you pointing out some problems with Protobuf (which I honestly don't like that much either, because I believe that Google is in charge of it - and Google tends to change course by 180 degrees regardless of how many million lifes are on the line xP; I hate relying on their work).

ftomassetti commented 8 years ago

Honestly is just that I want open worlds from Java and using protobuf I cannot do it right now. HDF5 seems a good format, with a great support for many languages and Bret suggested it for a while so I thought... why not?

ftomassetti commented 8 years ago

tox was not aware of h5py. Let's try again

ftomassetti commented 8 years ago

but sure, I have to install also the library... for both Travis and Appveyor. This is going to be a lot of fun, I am sure.

psi29a commented 8 years ago

Yeah... new libraries are hell.

I imagine that pyinstaller is going to be worse. ;)

tcld commented 8 years ago

I think this looks good - although it might be possible to get rid of almost all element-by-element copy-processes that are being done, since all participants are numpy-arrays anyway. Maybe I am overlooking something there?

Other than looking at it I think that a field test is the easiest way to see if all data is properly written and read.

tcld commented 8 years ago

What's up with the future replacement of pickle and protobuf?^^

psi29a commented 8 years ago

I don't see this replacing protobuf, since protobuf will eventually support java. HDF5 is just a stop-gap.

ftomassetti commented 8 years ago

Update status: the code is done and the round-trip works (serialization -> unserialization -> back to the original world). Then I started thinking about having to install another library on AppVeyor, having to explain to users how to installed. So I cried a little, opened a bottle of whiskey and not touched this PR for a while...

Now that I am sober again I am not sure... It is true that we could make hdf5 optional as GDAL is. What do you think guys?

tcld commented 8 years ago

As long as it isn't trivial to add another format, I would actually prefer seeing us focus on exactly one. Pickle is kind of worthless as long as Python 2 and 3 are supported since the change in Unicode-support breaks save-compatibility between the Python versions. Protobuf has been at the latest alpha for quite a while and I personally don't like relying on Googles code - they have scrapped or "reimagined" other software before, no matter how many people relied on it. (Of course, we could stick to the latest working version in that case. And even modify it ourselves: https://github.com/google/protobuf )

So while I don't know if we need a third way of saving data, we could really use one that is actually reliable and works for all purposes we can come up with. (And then hopefully get rid of the other formats.)

psi29a commented 8 years ago

I think hdf5 should be optional like gdal, it is there and useful for when the right libs are in place. :)

ftomassetti commented 8 years ago

rebased

ftomassetti commented 8 years ago

In the meantime Travis complains:

Command "python setup.py egg_info" failed with error code 1 in /tmp/pip-build-Lorf5r/pygdal
ERROR: could not install deps [coverage, numpy, pygdal==1.10.0.1, Pillow, PyPlatec, noise, nose, protobuf, six, h5py]; v = InvocationError('/home/travis/build/Mindwerks/worldengine/.tox/py27/bin/pip install coverage numpy pygdal==1.10.0.1 Pillow PyPlatec noise nose protobuf six h5py (see /home/travis/build/Mindwerks/worldengine/.tox/py27/log/py27-1.log)', 1)
psi29a commented 8 years ago

Numpy failed to compile... I'm going to assume that the wheel is not being installed here?

ftomassetti commented 8 years ago

That could be the case but I do not understand why this patch is affecting numpy

psi29a commented 8 years ago

It's not... likely a new release of numpy and we don't specify a specific version.

ftomassetti commented 8 years ago

Should we specify a min and max release?

tcld commented 8 years ago

The latest version is about a month old.

Why would it cause problems if there was a new version of a module? Does it maybe have to be recompiled?

ftomassetti commented 8 years ago

Perhaps it is trying to use a version for which there are no pre-compiled packages. Damn how much easier are these things on the JVM....

ftomassetti commented 8 years ago

rebased after #187 has been merged

ftomassetti commented 8 years ago

I cannot see wheel packages for Numpy 1.10.1 https://pypi.python.org/pypi/numpy

ftomassetti commented 8 years ago

Appveyor is now happy but Travis keeps failing and I am out of ideas. Google did not help so far. I tried setting several numpy versions in tox.ini

ftomassetti commented 8 years ago
Command "python setup.py egg_info" failed with error code 1 in /tmp/pip-build-yIncyD/pygdal
ftomassetti commented 8 years ago

Ok, that it is fixed but tests are still failing

psi29a commented 8 years ago

"AttributeError: 'Namespace' object has no attribute 'protobuf'"

time to rebase from master?

ftomassetti commented 8 years ago

I wish was that but I have already rebased from master

ftomassetti commented 8 years ago

Ok, this is solved now but still have 4 tests failing. 3 are failing because they refer to file in worldengine-data that were delete (e.g., biome_test.world which is referred by tests/biome_test.py).

tcld commented 8 years ago

That's due to the data for https://github.com/Mindwerks/worldengine/pull/192 having already been merged while the PR itself isn't merged yet.

ftomassetti commented 8 years ago

Ok, it seems good to go from a tests point of view. Feel free to point out corrections and improvements!

ftomassetti commented 8 years ago

Most comments addressed and all tests passing. Time to merge and move forward with the release?

tcld commented 8 years ago

Do you want to release immediately after this PR? I wouldn't mind adding the tests (although...not too important for a release, I guess) and maybe the frozen oceans (which are done here, I just need to update the data). Also...you really didn't want to put the serialization-stuff into its own function?^^

ftomassetti commented 8 years ago

about the serialization stuff, like this it is much easier to debug and to implement: when I support a new serialization method I start serializating the different elements one by one and as I figure things out I add another layer. So for me it is much simpler like this, until we introduce classes for the single layers and we reorder the World class.

About other features, I guess we will be always working on cool stuff we want but a certain point we need to release stuff so that users can benefit from it. I would suggest we do a release very soon and after that we start doing releases more and more often.

tcld commented 8 years ago

I think the frozen oceans are something that at least provides a visual benefit to users. And that's always good. ;P (Also: It is practically done, but nobody commented the latest version yet. I would have to adapt the HDF5-code a bit to make it work, though.)

The serialization-code should be put into its own function. I think it is very ugly that it appears twice in the same file (and we aren't talking about five lines of code) and even differs slightly due to personal preference of the person who wrote the respective lines of code. Of course that's not a huge problem, especially since it's only tests we are talking about. I wouldn't object a merge at all, just wanted to put some more salt into the wound.^^

ftomassetti commented 8 years ago

Well, there are other tasks to do before releasing the new version. Whatever it is merged before we tag release 0.19.0 can go in. Otherwise it could end up in 0.19.1.

This repository has ~700 commits. 200 commits have been added since we released version 0.18.0 (and we have not yet merged this PR). It is really time to share it with the world :)

tcld commented 8 years ago

I would really like to see the satellite-map as a standard output for the next release, since it is such great work by @pangal. Including polar caps. Of course it's not a necessity but it might leave a good impression to have some more eye-candy.

ftomassetti commented 8 years ago

I definitely agree with that: I love the satellite view!

tcld commented 8 years ago

I suggest merging this PR and then https://github.com/Mindwerks/worldengine/pull/185 (which also fixes several bugs of the satellite-view - it's actually not just about tests). After that WorldEngine should be in a good state to release. If there is another day we can wait, let's at least make the satellite-view a default. (Polar caps...well, I would prefer adding them now, since they also introduce a new variable that will be saved to the world-file. I don't know if that could cause any problems when done later.)

ftomassetti commented 8 years ago

Let's start by merging this one then. At this point we should move to the next point of the schedule for releasing a new version which is getting the documentation ready. While we work on that we could merge more stuff but it is important to understand that it is not the end of the world if some feature ends up in the next release instead of this one.