DataTextureLoader: fix promise never rejected on error

[ashconnell]

Related issue: None

Description

When using something that extends DataTextureLoader (eg RGBELoader) if it fails to parse the file then the .load() error callback is never called, and the .loadAsync() promise is never rejected.

This is especially important when preloading assets for games etc, if the promise never succeeds or fails it gets stuck loading forever.

[github-actions]

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize dev	Filesize PR	Diff
643.9 kB (159.7 kB)	643.9 kB (159.7 kB)	+12 B

🌳 Bundle size after tree-shaking

Minimal build including a renderer, camera, empty scene, and dependencies.

Filesize dev	Filesize PR	Diff
437 kB (105.8 kB)	437 kB (105.8 kB)	+0 B

[gkjohnson]

If we call "onError" we should be passing an error in. In my opinion the the parse function should be throwing an error that actually describes what failed instead of just passively returning null. While looking at RGBELoader - it feels like these lines where it just logs and returns an invalid data description should actually throw an error instead, as well.

[ashconnell]

It’s important that promises aren’t left completely unresolved, which this PR achieves.

To get correct error messages would require updating everything that extends DataTextureLoader, making it harder to review and reducing the chances that this PR gets merged.

[gkjohnson]

I'm suggesting that this PR is not using the onError callback correctly and is a band aid fix while recommending a different fix for the project. If this is merged there should be a separate effort to bubble the actual errors up through the callback instead of blindly calling onError. I definitely don't think all Loaders should be fixed in one PR - just that this alone is not a great long term solution.

[Mugen87]

Agreed. Ideally DataTextureLoader puts the parse() call into a try/catch statement. Loaders like RGBELoader should throw instances of Error when they encounter an error situation. The error can then be logged in the catch block as well as the onError() callback called. Next to RGBELoader other loaders to consider are:

• EXRLoader
• LogLuvLoader
• RGBMLoader
• TGALoader
• TIFFLoader

AFAICS, updating just DataTextureLoader and RGBELoader does not require to update the others as well. These can be done separately.

[Mugen87]

However, as a temporary solution this PR is fine imo. Maybe we can add a TODO comment that highlights the long-term solution. Something like

// TODO: Use try/catch and throw errors in derived loaders

[Mugen87]

Added a comment as suggested: b2fea48