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

Be tolerant when encountering emissiveFactor with array length 4 #445

Merged
merged 2 commits into from
Sep 1, 2023

Conversation

nyalldawson
Copy link
Contributor

Raise a warning when encountering emissiveFactor with array length of 4 and truncate to the first 3 elements, instead of aborting the model loading entirely.

nyalldawson added a commit to nyalldawson/QGIS that referenced this pull request Aug 24, 2023
of 4 instead of aborting the model loading

Submitted upstream as syoyo/tinygltf#445
wonder-sk pushed a commit to qgis/QGIS that referenced this pull request Aug 24, 2023
of 4 instead of aborting the model loading

Submitted upstream as syoyo/tinygltf#445
@syoyo
Copy link
Owner

syoyo commented Aug 24, 2023

The spec says emissiveFactor is number[3], so I think we should report it as error, not warning.

https://registry.khronos.org/glTF/specs/2.0/glTF-2.0.html#reference-material

What is gltf-validator's result to emissiveFactor with number[4]?

@nyalldawson
Copy link
Contributor Author

nyalldawson commented Aug 24, 2023

How about if I add an optional "flags" argument to model loading, with a "permissive" flag which enables workarounds like this?

@syoyo
Copy link
Owner

syoyo commented Aug 25, 2023

Adding a flag to control the parsing behavior would be a nice idea. For example...

TinyGLTF::SetParseStrictness(enum)

@nyalldawson
Copy link
Contributor Author

@syoyo how's that now?

@syoyo
Copy link
Owner

syoyo commented Aug 28, 2023

@syoyo how's that now?

Thanks! Let me give some time to review it.

@syoyo syoyo merged commit acf1e8a into syoyo:release Sep 1, 2023
6 checks passed
@syoyo
Copy link
Owner

syoyo commented Sep 1, 2023

Merged!

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