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

add support for parsing rOBJ and rCAM chunks #30

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mmckegg
Copy link

@mmckegg mmckegg commented Sep 7, 2022

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

mmckegg commented Sep 7, 2022

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

mmckegg commented Sep 7, 2022

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

Neo-Zhixing commented Sep 23, 2022

@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

@Neo-Zhixing Neo-Zhixing self-requested a review September 23, 2022 03:30
@@ -1,5 +1,9 @@
use crate::{Layer, Material, Model, SceneNode};
use std::io::{self, Write};
use crate::{parser::RenderCamera, Layer, Material, Model, SceneNode};
Copy link
Member

Choose a reason for hiding this comment

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

Can you re-export RenderCamera in the crate root and use it directly?

{
debug!("_weight observed outside of range of [0..1]: {}", w);
if let Some(w) = w {
if w < 0.0 || w > 1.0 {
Copy link
Member

Choose a reason for hiding this comment

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

If you're going to change unrelated code, please do it in a separate PR

Copy link
Author

Choose a reason for hiding this comment

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

Yep, not sure if this is still a problem in the latest main branch, will check and see. It was preventing me from compiling at all, but yeah I probably shouldn't have included in the PR :)

@mmckegg
Copy link
Author

mmckegg commented Sep 29, 2022

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.

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