FBXLoader: Support more texture formats.

[catalin-enache]

Fixed #28514

Description

Improved the way FBXLoader calls for specific image loaders, by removing hardcodings and iterating through a set of predefined extensions.

[mrdoob]

With this new code... If a fbx file uses psd as textures, we no longer log a warning...? 🤔

[catalin-enache]

Correct.
I searched globally in the repo for psd and did not find another place where it is handled specifically.
I was thinking: do we need a dedicated handling for things that are not supported?
I mean, it does not need to be just .psd.
It could be a hardcoded black list (possibly quite large) of things that we should throw a warning for if encountered.
What if we apply the same approach everywhere ?
IMO, that adds complexity without adding value.
That's why a dropped that branch.

[mrdoob]

Ideally we could just do this:

const loader = this.manager.getHandler( `.${extension}` );

if ( loader !== null ) {

  texture = loader.load( fileName );

} else {

  console.warn(
	  `FBXLoader: ${extension.toUpperCase()} loader not found, creating placeholder texture for`,
	  textureNode.RelativeFilename
  );
  texture = new Texture();

}

But I guess for that to work we may need to add default handlers in LoadingManager?

https://github.com/mrdoob/three.js/blob/dev/src/loaders/LoadingManager.js#L140C1-L140C66

const DefaultTextureLoader = /*@__PURE__*/ new TextureLoader();

const DefaultLoadingManager = /*@__PURE__*/ new LoadingManager();
DefaultLoadingManager.setHandler( /\.png$/i, DefaultTextureLoader );
DefaultLoadingManager.setHandler( /\.jpeg$/i, DefaultTextureLoader );
DefaultLoadingManager.setHandler( /\.jpg$/i, DefaultTextureLoader );
DefaultLoadingManager.setHandler( /\.gif$/i, DefaultTextureLoader );

Or even populate this array directly?

@Mugen87 What do you think?

[Mugen87]

Um, I'm not sure I understand why it is required to add default handlers.

If a loader for an extension isn't found, an empty texture is returned and a warning is logged. That seems sufficient.

I was never a fan of the following branch in FBXLoader for processing non-supported images:

else {

    // TextureLoader#load() returns a texture in any case which can be used as placeholder if the image failed to load.
    texture = this.textureLoader.load( fileName );

}

Simply doing texture = new Texture(); is better, imo.

[catalin-enache]

Simply doing texture = new Texture(); is better, IMO.

const loader = this.manager.getHandler( .${extension} ); will return null for native images which are supposed to be handled without user adding default TextureLoader to the regex map for native extensions.
That's why there is this fallback :
texture = this.textureLoader.load( fileName );
@mrdoob would eventually like to not return null for native images, but that would require prepopulating the regex map with default TextureLoader. My only fear that I can think of with that is that the list of native extensions would not be comprehensive enough, preventing handling images that the browser could have handled.
If we'd like to set default TextureLoader for native images (which I'm afraid of possible other implications) we have to make sure to not override an eventual existing loader if it was setup by the user

[Mugen87]

Couldn't we have a second set with native extensions intended for TextureLoader (or ImageBitmapLoader). Meaning [ 'png', 'jpeg', ' jpg', 'gif', 'avif', 'webp' ]. If an extension is not in that set, we check the non-native one ( [ 'tga', 'tif', 'tiff', 'exr', 'dds', 'hdr', 'ktx2' ]). If the check is false, we return an empty texture.

My only fear that I can think of with that is that the list of native extensions would not be comprehensive enough, preventing handling images that the browser could have handled.

If we ever have an issue that something in the set is missing, we can add an entry.

[catalin-enache]

I see the will to simplify the logic especially those nested if branches. I pushed a new commit. The only reason for keeping around non native extensions is to be able to throw a warning in the console. If we won't want that warning, we don't need them around making the loader = specificLoader || defaultTextureLoader simplifying even more.
I believe though that we want that warning there, hence keeping around non native extensions set.

[Mugen87]

Um, now that I look at the code I'm not so happy that the FBX example loads all the modules by default. In production, that makes the example considerably slower to load.

I vote to remove the modifications to webgl_loader_fbx and just accept that we don't have a test asset for non-native textures.

[catalin-enache]

I'll try to make a checkbox in UI that when checked will asynchronously import the loaders. Let's see (if I manage to do that) how would you like it that way.

[Mugen87]

Wouldn't it be more simple to test for with_non_native_textures.fbx and then asynchronously load the texture loaders when the asset is selected?

[catalin-enache]

Better idea indeed. import the loaders when selecting the with_non_native_textures.fbx then when they're ready import the asset itself. Will try to do that. I have not started any changes yet. Will do soon.

[catalin-enache]

I pushed a new commit, now lazily importing loaders.
Note: I had to increase ESLint ECMA version in order to not complain about import() used as a function.

[Mugen87]

Note: I had to increase ESLint ECMA version in order to not complain about import() used as a function.

This is unfortunately not possible since we have to stick to ECMA 2018 and lint addon and src code respectively. Please revert the changes to .eslintrc.json.

Sorry, I think it's best to remove the asset from the example. It makes things just too complicated.

We want to keep the example as simple as possible so devs can quickly start coding. Using the examples to test every possible code path of an addon is not top-priority.

[catalin-enache]

I understand. I reverted it. I let there though some improvements on webgl_loader_fbx.html that happened during this PR (including some changes from mrdoob). The failing E2E tests seem random. The targets for the snapshots which I believe relate to HTML examples, I didn't found them - probably a re-trigger will work this time ?

Regarding testing, if there would be a possibility to unit-test the assets we won't need to pile up to HTML examples.
However that would only be possible if in unit-tests we could leverage a mock server (something like Sinon or MSW ) so that we could load assets from disk in tests.

Maybe I will come up with such PR at some point.
Do you think a feature like that in unit tests would be welcomed?

Related to ESLint ECMA version, should I make an issue about it to increase it to something more recent, to gain access to features like async import among others? Or there is some hard reason (which escapes me) for sticking with 2018 ?

Revert example changes.