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

GLTFLoader: Fix handling of GLTF files with optional KTX2 extensions #20518

Merged
merged 4 commits into from
Oct 23, 2020
Merged

GLTFLoader: Fix handling of GLTF files with optional KTX2 extensions #20518

merged 4 commits into from
Oct 23, 2020

Conversation

zeux
Copy link
Contributor

@zeux zeux commented Oct 16, 2020

If KTX2 loader wasn't registered, we would previously fail to load GLTF
files that used BasisU; this is a problem since files that don't require
BasisU support but specify it optionally would incorrectly fail.

The tradeoff here is that the error message that we previously emitted
for cases where the extension was required is now no more; we need some
mechanism for required extensions to actually be enforced to solve this.

If KTX2 loader wasn't registered, we would previously fail to load GLTF
files that used BasisU; this is a problem since files that don't require
BasisU support but specify it optionally would incorrectly fail.

The tradeoff here is that the error message that we previously emitted
for cases where the extension was required is now no more; we need some
mechanism for required extensions to actually be enforced to solve this.
@donmccurdy
Copy link
Collaborator

The tradeoff here is that the error message that we previously emitted
for cases where the extension was required is now no more; we need some
mechanism for required extensions to actually be enforced to solve this.

I'm not sure we can make this tradeoff safely – it's going to be much more common for a user to forget to set up KTX2Loader than to load a model with fallback uncompressed textures. For now I think we should leave it as it is, but I agree that some mechanism for enforcing required extensions should be added when possible.

@zeux
Copy link
Contributor Author

zeux commented Oct 16, 2020

When you say "as is", do you mean to say that until we get a cleaner mechanism to distinguish between required and optional uses of the extension, it's reasonable to only support the required case since it's more prominent? Asking because this came out of #20508 that has a similar tradeoff, and I want to be consistent - if we should keep the error handling for KTX2 as it is today for now, I'd rather change that PR to match as well.

Alternatively I could change this PR to be consistent with that PR by adding explicit error handling to the extension processing code :)

@donmccurdy
Copy link
Collaborator

If we need to make a (temporary) choice between (a) supporting fallback when a dependency is missing, and (b) showing useful errors when a dependency is missing, I'd take the latter. But if you see a clean way to add better error handling to the extension processing code and would be willing to add that, having both (a) and (b) sounds great!

@zeux
Copy link
Contributor Author

zeux commented Oct 16, 2020

@donmccurdy What do you think about this solution?

Copy link
Collaborator

@donmccurdy donmccurdy left a comment

Choose a reason for hiding this comment

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

Looks good, thanks! Just one change suggested.

examples/js/loaders/GLTFLoader.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@donmccurdy donmccurdy left a comment

Choose a reason for hiding this comment

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

Thank you!

@mrdoob mrdoob added this to the r122 milestone Oct 23, 2020
@mrdoob mrdoob merged commit 99771f6 into mrdoob:dev Oct 23, 2020
@mrdoob
Copy link
Owner

mrdoob commented Oct 23, 2020

Thanks!

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.

None yet

3 participants