GLTFLoader: fix exception when a texture can't be fetched/created

[hybridherbst]

Description

When loading user-provided models, we ran into the issue that when any texture inside the model is corrupt / not reachable, the entire model doesn't load. This PR simply prevents a null reference that caused that. The result is that models load, but the affected textures aren't there, which I think is preferrable to an exception.

Here's a simple case to reproduce:

new GLTFLoader().load("https://dl.polyhaven.org/file/ph-assets/Models/gltf/4k/hand_plane_no4/hand_plane_no4_4k.gltf", gltf => {
	scene.add(gltf.scene);
});

[takahirox]

The result is that models load, but the affected textures aren't there, which I think is preferrable to an exception.

Perhaps better to first clarify the loader's policy, what the loader should do if part of data is broken/invalid.

I thought the loader expects the input data is valid and we ask users to validate the data, if needed, with validation tools beforehand. (Please correct me if I'm wrong.) The loader doesn't really validate the data.

So, based on them, I feel that invoking onError callback if part of data is corrupt may be a more natural behavior...?

[hybridherbst]

GLTFLoader is used in basically all viewers and runtimes based on three.js, which means it deals with users, not just developers 😅.

I understand that from a technical perspective a stance would be that users are simply required to ensure correctness, but that doesn't match reality on what data is out there...

For context, I stumbled upon this issue with models that have textures but those textures are (accidentally) protected by CORS or have wrong URLs or some other software has written corrupt image data; that's really hard to check beforehand and impossible for a user to fix.

[takahirox]

I found that we have already discussed whether to show the model if texture loading is failed at #21977 (I was in the discussion, I know I have bad memory...)

We decided to show the model without textures. So the change in this PR should be good.

I feel there might be a better fallback than returning null from loadTexture(). For example return a dummy texture. With it texture null check can be removed and the code can be simpler. But it is out of scope from this PR. We may be able to revisit the fallback later if needed.

[donmccurdy]

The changes here look good to me – thanks!

For example return a dummy texture. With it texture null check can be removed and the code can be simpler

I'd prefer that GLTFLoader not create materials containing empty textures. What was originally a practical simplification for users (#21977) feels like it will leak complexity into other parts of the library; there are already enough edge cases to think about with textures that haven't finished loading, ImageData textures, KTX2 textures, etc., without adding invalid ones.

GLTFLoader is used in basically all viewers and runtimes based on three.js

Just FYI, gltf.report no longer uses THREE.GLTFLoader. Made necessary by supporting lossless roundtrip import+export.

[takahirox]

I'd prefer that GLTFLoader not create materials containing empty textures.

I don't think of empty textures. A dummy texture that I mentioned may be like a texture having one pixel (white = all RGBA 1.0?) image with image data or base64 image. It should match the standard flow that texture is ready to upload when onLoad is fired. Anyways I haven't deeply thought yet.

[donmccurdy]

Ok, I see. Blender does something like that with a violet texture I think. That could be Texture.DEFAULT_IMAGE in our case? No strong opinion but I would probably be OK with that too.

[LeviPesin]

I think Texture.DEFAULT_IMAGE is exactly the method for setting dummy texture if a texture image is missing.

Maybe we can return new Texture() instead of null?

[hybridherbst]

Just to add to the above discussion regarding default textures, in Unity all texture shader properties (material slots in three.js language) have a fallback specified - there's basically "white" (1), "black" (0), "grey" (0.5) and "bump" (normal map that does nothing) with "black" as default when a shader doesn't specify it.

Want to note that I like three's usual approach of having defines for everything and keeping shaders fast much more than fallback textures - in Unity it's done that way because by default one shader is one shader and doesn't bring defines for particular map slots or not (with some exceptions).

[drcmda]

I was just made aware of this. Would have voiced the same discomfort. Is it really threes responsibility to just gloss over errors? In my opinion this makes three more unreliable. A missing texture is a catastrophic failure in production, and now there can be no error response.

Not to mention unit testing models. Can't imagine not being able to do that ever to be a good thing.

[donmccurdy]

The decision to log an error rather than throwing an exception when textures do not load was made much earlier; the PR here was simply fixing an unintended bug in that code path. See discussion in:

• GLTFLoader: Return null when failing to load textures #21977

[drcmda]

even a log would normally be considered out of place for a library. libraries should not catch errors, or log them. that is 100% the responsibility of the end user. if any other library did that, and they don't because it seems like widely agreed upon, it would be considered a bug. it takes a critical feature away: error response and unit testing are vital for anything that can claim to be production ready. i have never worked in a professional setting where testing was absent, that's why im puzzled how easy going three is about that. 🤷‍♂️

[hybridherbst]

Common unit testing frameworks do have methods to specify whether warnings or errors are expected from a run... it's a very typical thing that warnings are considered blockers for unit tests if that strictness is wanted. Logging an error instead of throwing is not, as you kind of put it there, a blocker for unit tests or an "easy going" approach.

I do agree that choices are nice to have (e.g. an option to choose if a method throws or not, a callback to react to errors and decide there what to do, ...), but in the absent of that I personally favor the user experience over throwing about every detail.

[drcmda]

const loader = new GLTFLoader()
loader.ignoreErrors = true

would have been a more sensible direction imo. same goes for #21977

it's a very typical thing that warnings are considered blockers

can't catch a log or warning. no testing or live environment should survive missing assets.

and just wondering, the 2nd loader breaking error response. i've tried to explain why this is a critical feature, what else can i do.

but in the absent of that I personally favor the user experience

UX shouldn't be accidental, nobody should run into something like that. now they will, without the ability to respond. nor do i think whoops, texture bad, model white is good UX. every js library or browser api works like this, imagine fetch(url) silently failing, and this is exactly what this is, a smothered fetch. 🤷‍♂️

[CodyJasonBennett]

Common unit testing frameworks do have methods to specify whether warnings or errors are expected from a run... it's a very typical thing that warnings are considered blockers for unit tests if that strictness is wanted. Logging an error instead of throwing is not, as you kind of put it there, a blocker for unit tests or an "easy going" approach.

I do agree that choices are nice to have (e.g. an option to choose if a method throws or not, a callback to react to errors and decide there what to do, ...), but in the absent of that I personally favor the user experience over throwing about every detail.

That has been very much regarded in the earlier discussion at #21977.

UX shouldn't be accidental, nobody should run into something like that. now they will, without the ability to respond. nor do i think whoops, texture bad, model white is good UX. every js library or browser api works like this, imagine fetch(url) silently failing, and this is exactly what this is, a smothered fetch. 🤷‍♂️

Since this maybe wasn't clear enough after multiple PRs, there's zero agency for this in user-land, and unlike #21977, zero observability also. I don't see how this is anything, but problematic to hide at the library level.

[hybridherbst]

Could you clarify what you mean with zero observability @CodyJasonBennett? All this poor PR here did was remove an exception (which as discussed was unintended), and now the actual errors are logged properly again.

Behaviour before this PR:

(note no "Loading Complete" log since the exception prevents loading from completing without a way to react to that on app level)

Behaviour after this PR:

[CodyJasonBennett]

I mean no offense, but is this a serious question? Should I create an issue instead?

[hybridherbst]

Sorry, it's late here, maybe I just don't understand the points you're both trying to make. Would be great if you could elaborate. From my (limited) perspective, to a user dropping something in a viewer "my file loads without texture and the app can show me an error about that" is better than "my file doesn't load".

[donmccurdy]

It's valid that some use cases benefit from being able to load a file incompletely (without textures) rather than failing outright. Blender does that too — showing a violet color as a placeholder. It's also valid that other use cases benefit from a programmatically-receivable error, which the current logs do not provide, and we can discuss ways to offer that.

Please, all, let's keep this thread respectful of others' goals and intentions.

[LeviPesin]

By the way, is the texture shown instead of the fail-to-be-loaded just simply white or Texture.DEFAULT_IMAGE? In the second case the user can configure their own placeholder that is easily noticeable (e.g. magenta).

[drcmda]

a user dropping something in a viewer

i think that's the main differentiator. i totally understand how this makes sense in a viewer. but i hope you see that this makes little sense in anything else.

in a normal application that relies on assets, how is loader.load(url) anything different from fetch(url) and why does the latter throw while the former is silent. i think we're breaking very basic and widely agreed upon programming principles here. if loader.ignoreErrors = true is still a possibility for both this and the texture thing @donmccurdy then i would suggest we revert both and add it. and it would allow both the viewer to plow on and everything else to be reliable through testing and error response.

[hybridherbst]

I think one difference is that loaders in three are basically "multiple chained fetches" and there's nuance between "first fetch fails" and "any subsequent fetch fails"; ideally apps can decide which behaviour they want (e.g. your suggestion of ignoreErrors or a callback for handleErrors).

For me at least, a loader is something different then a validator – the job of a loader is to load something, sanitize the data if it must, circumvent errors, just make it work, and provide a log of what was incorrect/changed/etc – someone wants to load something, give them the best that you can.
The job of a validator is to check that the data is correct – either before or after loading, depends on the usecase. In CI and pipelines I'd use a validator, not a loader, to verify validity of files to a developer-configurable degree.

[drcmda]

as i've stated before, it isn't the responsibility of a library to silence fetch no matter if first or consecutive load, at least not without deliberate opt in (the flag).

it would make much more sense to discuss a revert, and a replacement with a flag (as well as the pr that did this to textures). as you said, this would solve all use cases. it would allow error response, testing, and viewers can have white textures. would anyone be against this? @donmccurdy @takahirox ?

[donmccurdy]

I'm open to adding a flag of some kind, or perhaps a simple console-agnostic logging API. Let's start an issue to discuss. But I don't think reverting this specific PR beforehand would be a helpful step toward that.

[donmccurdy]

As illustration, here's the result of loading a model with missing textures in https://gltf-viewer.donmccurdy.com/ —

I'm using the official glTF Validator to detect problems in the file, including missing textures, and display that as a flag at the bottom of the page. So I don't currently need that information from GLTFLoader, though I do support being able to get the information from GLTFLoader somehow or put it into a stricter mode. I do think people appreciate being able to see both an attempt at rendering the model, and the fact that there are errors, in this situation within an online viewer.