FBXLoader: Use getHandler() for custom texture loaders.

[tatsuya-ogawa]

#31031

Description
I’d like to submit a PR to three.js because the TextureLoader inside FbxLoader isn’t receiving any credentials—this causes errors when loading textures.
so add manager.addHandler to instantiate TextureLoader.

[donmccurdy]

I had a similar issue recently with KTX2Loader not passing 'path' or 'crossOrigin' options to its internal FileLoader, just haven't made a PR yet. I think probably we want to make sure that any options set on the parent loader are passed down to its internal loaders, rather than introducing a factory pattern? Perhaps don't change the PR yet, though – I'm curious if other maintainers agree.

[Mugen87]

How about adding FBXLoader.setTextureLoader(). This would match the set*Loader() pattern we are already using in other loaders.

By default, an instance of TextureLoader is used like in the existent code. This could be overwritten by a custom instance via setTextureLoader().

[tatsuya-ogawa]

@Mugen87
Thanks for the feedback. I've pushed the PR, and by default an instance of TextureLoader is still used. However, we've now added FBXLoader.setTextureLoader() so that we can override it with a custom instance for cases like authenticated texture fetching. Let me know what you think!

[gkjohnson]

I think probably we want to make sure that any options set on the parent loader are passed down to its internal loaders, rather than introducing a factory pattern? Perhaps don't change the PR yet, though – I'm curious if other maintainers agree.

I agree with this. I would prefer not to introduce a new pattern - my expectation would always be that the settings (or intent of the settings) on the parent loader should be passed to any other child loaders. Without knowing more this sounds like a bug. Is there a reason you wouldn't want this behavior?

How about adding FBXLoader.setTextureLoader(). This would match the set*Loader() pattern ...

Where else is set*Loader pattern being used? setKTX2Loader and setDRACOLoader are not comparable imo because they are not loading files - just interpreting buffers. I would expect LoadingManager.addHandler and getHandler to be used here, if anything.

[tatsuya-ogawa]

@gkjohnson Thank you.

I would expect LoadingManager.addHandler and getHandler to be used here,

Like this?
https://github.com/mrdoob/three.js/blob/dev/examples/jsm/loaders/MTLLoader.js#L573

[Mugen87]

I would expect LoadingManager.addHandler and getHandler to be used here, if anything.

That sounds even better!

@tatsuya-ogawa Do you mind updating the PR with the suggested approach? If you are not familiar with the addHandler() method, it allows to define custom loaders for loading specific file extensions. E.g.:

manager.addHandler( /\.tga$/i, new TGALoader() );

This would mean all TGA textures are now loaded with an instance of TGALoader and not TextureLoader. You can do the same for loading PNGs and JPGs with a custom texture loader.

However, LoadingManager.getHandler() must be used in FBXLoader otherwise the setting would be ignored.

[tatsuya-ogawa]

I'm checking e2e testcase webgl_loader_fbx

Bug found.

[tatsuya-ogawa]

@Mugen87

Bug found.

Thank you. fix it.
e400684

[gkjohnson]

I think allowing for getHandler here is a fine change but to be clear I think the issue Don raised should still be addressed - either here or in another PR:

I think probably we want to make sure that any options set on the parent loader are passed down to its internal loaders

The problem raised in the original post doesn't sound like something that should require custom texture loader handlers to fix.

[tatsuya-ogawa]

@Mugen87 @gkjohnson @donmccurdy
Thank you for your review.