Skip to content

Conversation

@zeux
Copy link
Contributor

@zeux zeux commented Jan 29, 2021

If GLTFLoader is used to load a texture with no uri or bufferView, it
uses an empty uri which results in FileLoader loading the HTML document
from which the call originates. The resulting document is then fed into
an actual image loader such as KTX2Loader or BitmapLoader, which tends
to generate hard to understand errors.

This change detects the error early and throws a special message to make
debugging easier.

If GLTFLoader is used to load a texture with no uri or bufferView, it
uses an empty uri which results in FileLoader loading the HTML document
from which the call originates. The resulting document is then fed into
an actual image loader such as KTX2Loader or BitmapLoader, which tends
to generate hard to understand errors.

This change detects the error early and throws a special message to make
debugging easier.
@mrdoob mrdoob requested a review from donmccurdy January 30, 2021 00:20
@mrdoob mrdoob added this to the r126 milestone Jan 30, 2021
@donmccurdy
Copy link
Collaborator

What is a scenario where this error might occur? I don't think we'd want to go out of the way to include English error messages in the bundle to warn about malformed assets — there are an unending number of ways the asset might be invalid — but if this could indicate user error, like failing to register an extension handler, then I'm in favor of the change.

@zeux
Copy link
Contributor Author

zeux commented Feb 1, 2021

The scenario here is that either the source glTF file is invalid, or the image data comes from a non-optional image extension that three.js doesn't implement. (note that files with unknown required extensions aren't rejected)

I agree that we don't necessarily need to guard against every possible issue like that but I found the actual flow here (loading empty URI results in reinterpreting HTML data as image contents) very surprising, hence the change.

@mrdoob mrdoob merged commit 20d1f60 into mrdoob:dev Feb 1, 2021
@mrdoob
Copy link
Owner

mrdoob commented Feb 1, 2021

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.

4 participants