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

Adds safety checks for bad animation data #19

Conversation

timokorkalainen
Copy link

Some optimized GLB files can contain animation data that causes a null reference exception here. This PR adds a safety check for that that causing issues.

@unity-cla-assistant
Copy link

unity-cla-assistant commented May 16, 2024

CLA assistant check
All committers have signed the CLA.

@atteneder
Copy link
Collaborator

@timokorkalainen Thanks for the contribution.

Could you provide examples/glTFs that trigger the error?

While defensive coding (with check like yours) is good in general, I want to make sure we're not hiding potential problems and make them hard to find. If there's a problem with our code, we should fix it. If there's a problem with the input/glTF, we should report/log it.

@atteneder
Copy link
Collaborator

I've done my own attempt of achieving more resilience in this particular part of the code base, therefore closing this pull request.

The upside is that now there's either a proper error message/log when the glTF is invalid or a runtime error (assertion) when there are internal inconsistencies.

Again, thanks for the work!

@atteneder atteneder closed this Sep 2, 2024
@atteneder
Copy link
Collaborator

@timokorkalainen Thanks for sending me a test file on another channel.

TIL that if an accessor's bufferView is not set, it is still valid and its values should get initialized with zeroes.

Therefor the initial PR would indeed have masked the actual problem, which is now tracked in #24.

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.

3 participants