GLTFLoader: Return null when failing to load textures

[mrdoob]

Description

GLTFLoader currently fails to load a model when it's unable to load its textures.
I think it's friendlier to load the model whenever possible.

[WestLangley]

Nice. It renders the model -- just without the texture. (Metalness/roughness in this case).

[takahirox]

It may be a good fall back. I'm wondering if we should console warning. Otherwise, users may be confused by seeing a model looking different from what they expect.

And curious to know, how did you fail to load textures? (I assume you made this PR because you faced a situation you failed to load.) CORS error?

[donmccurdy]

+1 for logging something, but returning null and continuing to load sounds fine to me. Should also file bugs on tools creating models with invalid textures references, if applicable, although this isn't an issue I've seen before. 🙂

[mrdoob]

@takahirox

And curious to know, how did you fail to load textures? (I assume you made this PR because you faced a situation you failed to load.) CORS error?

I was basically trying to figure out how many triangles this model had.
I managed to download the gltf and bin and was hoping to be able to drag it into https://threejs.org/editor/.

[mrdoob]

Added console.error() 👍

[drcmda]

@mrdoob wouldn't this make it harder to respond to errors though, or is it still catchable? error handling normally is user land because a missing asset is a critical problem with the app infrastructure, that is not something a library should interfere with, or else it breaks testing or error response (catching errors and sending them to a rest backend etc). for larger scale applications this becomes critical.

as an example, something big like zillow, they probably test their models. if this pr breaks it, then if something goes wrong the end user just bumps into a kitchen with a missing texture.

[mrdoob]

I would think someone big like zillow would have a more robust test suite? Ie. e2e pixel testing.
I'm sure they'd also look for errors in the console.

[drcmda]

what is more robust than catching fetch exceptions? that is what exceptions are for. pixel testing is complex and error prone, gives false alerts on varying platforms.

maybe you remember, there were similar problems with threejs being a little overzealous with jsm/controls and preventDefault, imo this is the same thing, there has to be a principled divide between user land and internals. error handling is never something that a library should "do for us". it looks friendly here, but it will blow up error response, which is a critical part of how applications function. i'm sure that if you ask anyone that is in charge of a bigger infrastructure they will tell you same.

[mrdoob]

Understood. However, I prefer to revert changes after I hear real consequences rather than hypotheticals.

This is one of those changes where for one to win someone else has to lose... And, as you know, I tend to prefer making things easier for creative developers that are starting out rather than for multi billion dollar corporations.

[gsimone]

You can't really read the console or do any kind of pixel checking in the client - without going crazy -, so if a user were to upload a gltf that fails to load a texture we wouldn't have a way to catch that and it would just look wrong to them.

An alternative could be having an onWarning callback to be called where you catch, so that developers interested in handling the "error" could use it.

You don't have to sacrifice one user for another, just make tiny escape hatches for those developers that do need the additional info

[mrdoob]

@gsimone That sounds like a good middle ground 👍

[mrdoob]

Actually, maybe another option would be to throw an error but still set null.

Then in the editor I should be able to still load the model without textures by adding a try/catch around the loader.parse() call, right?

three.js/editor/js/Loader.js

Lines 284 to 292 in de80e88

	loader.parse( contents, '', function ( result ) {
	var scene = result.scene;
	scene.name = filename;
	scene.animations.push( ...result.animations );
	editor.execute( new AddObjectCommand( editor, scene ) );
	} );

[mrdoob]

Would change the Promise's .catch() to .finally() do the trick already?

[drcmda]

i think try/catch around the loaders parse should work. promise finally imo triggers no matter if the promise has been resolved or rejected (?) never used it before 😋

[makc]

try/catch around the loaders parse will not work since it does not return the value immediately. it has to wait for buffers and textures, you know

[mrdoob]

Indeed. It doesn't work 😕

[makc]

@mrdoob what about amending TextureLoader in the editor so that any failed request would be replaced with 1 pixel data url or some 2x2 checkerboard, Idk

[makc]

@mrdoob same issue is now reported for dae