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

Fixed PACK implementation for read/write #38

Merged
merged 1 commit into from
Mar 7, 2023

Conversation

AideTechBot
Copy link
Contributor

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

AideTechBot commented Jan 1, 2023

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

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

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!

src/parser.rs Outdated
@@ -25,7 +25,7 @@ pub enum Chunk {
Main(Vec<Chunk>),
Size(Size),
Voxels(Vec<Voxel>),
Pack(Model),
Pack(Vec<Chunk>),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding of the MagicaVoxel spec is that the PACK chunk only encodes "how many SIZE and XYZI chunks do we have in this file?". We don't need actually need this number because we just stop the parser whenever we run out of bytes. It doesn't make much sense to have the Pack chunk owning a list of Chunks, since the XYZI chunks aren't exactly childrens of PACK.

Let's just remove the PACK chunk and have the deserializer ignore all PACK chunks. Old users who have PACK chunks wouldn't be impacted at all - again, we don't need this number because we stop the parser whenever we run out of bytes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding of the MagicaVoxel spec is that the PACK chunk only encodes "how many SIZE and XYZI chunks do we have in this file?".

This is correct.

We don't need actually need this number because we just stop the parser whenever we run out of bytes. It doesn't make much sense to have the Pack chunk owning a list of Chunks, since the XYZI chunks aren't exactly childrens of PACK.

This is also right, I was misled by the implementation of the PACK chunk writer that the models where children of the PACK.

In my most recent changes, I have completely removed PACK chunk writing as well as PACK chunk parsing (they are ignored).

@Neo-Zhixing Neo-Zhixing merged commit 2851ece into dust-engine:master Mar 7, 2023
@AideTechBot AideTechBot deleted the pack-fix branch March 7, 2023 22:05
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

Successfully merging this pull request may close these issues.

2 participants