dust-engine / dot_vox

Rust parser for MagicaVoxel .vox files.
MIT License
53 stars 20 forks source link

add support for parsing rOBJ and rCAM chunks #30

Open mmckegg opened 2 years ago

mmckegg commented 2 years ago

This module currently supports parsing all chunks/data defined in https://github.com/ephtracy/voxel-model/blob/master/MagicaVoxel-file-format-vox-extension.txt except for rOBJ (render settings such as sun position, fog etc) and rCAM (camera settings).

This PR adds support for those missing chunks. Rather than exposing all the props as nice structs, I've followed the basic dict style that the scene graph PR used. I think this makes sense for consistency, and it's easy enough for the developer using this library to consume these dicts however they like.

With this change, the api includes .render_objects and .render_cameras accessors.

Please let me know if any changes are needed and I'm happy to keep working on this! Thanks :)


Notes

In encountered a few weird issues when trying to write tests for these modern features of MagicaVox. It looks like none of the test files include these chunks, so I added a new placeholder-with-render-objects.vox created with v0.99.6. However it exports the voxels in a different order to previous versions. Rather than updating all the old placeholder tests to new order, I just opted to only test the parsing of the new chunks in the new tests.

This PR also changes some debug code that was causing my compiler to complain:

if let Some(w) = w && (w < 0.0 || w > 1.0) {
  debug!("_weight observed outside of range of [0..1]: {}", w);
}
error[E0658]: `let` expressions in this position are unstable

I assume this is due to my use of an older version of rust, but it seems unnecessary to break compatibility with older versions just for a simple syntax cleanup for a debug feature, so I replaced the code with one compatible with older rust versions.

mmckegg commented 2 years ago

I have considered when using this API that it might be better if render_objects was grouped by _type as a HashMap instead of Vec. Also it possibly should be called render_settings, however the way it is in the PR right now is the closest to the spec doc.

I'm currently doing this inside the project I am using dot_vox:

    let render_objects: &Vec<HashMap<String, String>> = &vox.render_objects;
    let mut render_settings: HashMap<String, HashMap<String, String>> = HashMap::new();
    for obj in render_objects {
        if let Some(key) = obj.get("_type") {
            render_settings.insert(key.to_owned(), obj.to_owned());
        }
    }

But it kind of feels like doing this in the actual library could cause some weird issues later if the vox format doubles up on types or has items with type in the future. The spec says nothing about this.

mmckegg commented 2 years ago

Hmm, I see that this repo seems to be in the middle of a big refactor so I get if this isn't high priority for now.

I see that a lot of the scene graph stuff seems to be getting replaced with actual struct types. This makes sense, but kind of means that my implementation in this PR is now out of date.

Looks like i'll need to depend on my own fork for now. Awesome to see the changes that dust-engine is bringing to this module, but yeah, obviously not actually ready yet! Let me know if I can help at all, I am also heavily relying on this module for a project that fully imports MagicaVoxel scenes into a 3D engine (including sun position, hence my interest in rOBJ support).

Neo-Zhixing commented 2 years ago

@mmckegg Thank you for your contribution! You're correct that the library was undergoing a bit of work.

Regarding your comment on _type, I think this is one of those cases where "the spec is the implementation". I think it would make sense for this to be a dictionary as well.

You can insert some assertions here, and if in the future we have reports of MagicaVoxel producing multiple items for the same _type we can always change it later on.

Regarding whether an item should be a struct or a dictionary: For now, if the spec specifically enumerated the possible attributes, let's turn it into a struct. Otherwise, keep it as a dictionary. That means "rCAM" should be a struct, but "rOBJ" shouldn't.

Thanks again! I'm planning to cut a release soon - let's see if we can include this in the next release.

Also,

error[E0658]: `let` expressions in this position are unstable

this should be fixed now

mmckegg commented 2 years ago

Okay thanks @Neo-Zhixing, I'll take another look at this in the next few days. Not sure if it'll make the release in time, so let me know if you just want to take it over and clean it up yourself. Absolutely happy to do it though, just a bit short on time right at this minute.