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

Fix material changes within an object being ignored #11

Merged
merged 2 commits into from
Apr 2, 2019

Conversation

Skynoodle
Copy link
Contributor

Previously, a single object would always create a single model with the
last specified material. A usemtl statement in the middle of an object
will now emit a model at that point (with the previous material).

Previously, a single object would always create a single model with the
last specified material. A usemtl statement in the middle of an object
will now emit a model at that point (with the previous material).
@Twinklebear
Copy link
Owner

It looks like the cornell box tests are failing, my guess is this is due to the stray usemtl white which is repeated in the obj file https://github.com/Twinklebear/tobj/blob/master/cornell_box.obj#L122 . With this change we'd now output a separate model at that point when we probably shouldn't (since it's the same material), so the test gets 9 models instead of the expected 8 https://github.com/Twinklebear/tobj/blob/master/tests/lib.rs#L250.

I think in the case of this OBJ file (which is sort of not well formed) if we get a repeated tag of the currently active material we should just ignore it.

@Skynoodle
Copy link
Contributor Author

Whoops! Yeah, that sounds likely - it just naively emits a new model even if the usemtl is a repeat. Happy to fix that up, if the basic approach is acceptable (I noticed there was a discussion about this vs emitting materials per-primitive in the related issue).

@Twinklebear
Copy link
Owner

I think for the OBJ loader it's probably best to emit a model per-material, instead of per-primitive material IDs that a renderer would use, which is probably best left to the application.

Thanks @Skynoodle , this looks good!

@Twinklebear Twinklebear merged commit 17908e4 into Twinklebear:master Apr 2, 2019
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