Fixed PACK implementation for read/write

dust-engine / dot_vox

Rust parser for MagicaVoxel .vox files.

MIT License

53 stars 20 forks source link

Fixed PACK implementation for read/write #38

Closed AideTechBot closed 1 year ago

AideTechBot commented 1 year ago

While I was playing around with this module I came across (what I assume to be) a bug:

I wasn't able to write_vox to a file then immediately read it with load. To be a bit more specific, I was writing multiple models to the .vox file and when I would read it, the models array would be empty.

Turns out this is because write_vox uses PACK chunks and load is incorrectly loading the PACK chunks according to the vox spec.

After I reimplemented that, I noticed that num_vox_bytes is incorrectly calculating the PACK chunk children length because it's missing 28 bytes per model. (Mostly because of headers detailed in the spec)

Anyways, this PR is intended to make the module parse/write PACK headers correctly. Feel free to critique the code as harshly as you want, I'm not that proficient in rust so there may be shorter ways to do things.

AideTechBot commented 1 year ago

Also, there is some debate to be had as to using a PACK chunk at all when writing multiple models. The spec says they are entirely optional (which is what I account for in the code), but according to the author of the spec: PACK is for old animations and is deprecated.

It makes sense to be able to read them for backwards compatibility, but does it make sense to write them?

Neo-Zhixing commented 1 year ago

Thank you for your contribution! I believe the right thing to do here is to deprecate the PACK chunk and ignore it entirely. It's only used by one version, and if the user have trouble importing it, they can just open it up with MagicaVoxel and save it again. We don't need the PACK chunk to tell us how many chunks to read.

The deserializer works correctly when importing a file with multiple models, so the bug is most likely on the serializer. It would be great if you can help me to debug this. Should the serializer emit the PACK chunk? Let's keep it consistent with the current version of MagicaVoxel.

Again, lots of thanks!

AideTechBot commented 1 year ago

The deserializer works correctly when importing a file with multiple models, so the bug is most likely on the serializer. It would be great if you can help me to debug this. Should the serializer emit the PACK chunk? Let's keep it consistent with the current version of MagicaVoxel.

Sure, here is the updated behavior of this PR:

I've disabled PACK chunk generation for serializing. This allows it to be up to date with the current MagicVoxel behavior.
I've added PACK chunk deserialization so that the users can still load old files with PACK chunks. It's not a lot of code and I feel like its good to be backwards compatible without forcing users to re-save their file in MagicVoxel.

Feel free to tell me specific parts I should add/remove/modify!