Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FBXLoader: Support more texture formats. #28515

Merged
merged 9 commits into from
Jun 1, 2024
Merged

FBXLoader: Support more texture formats. #28515

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
7e6377a
#28514 extend FBXLoader textures type
catalin-enache May 28, 2024
5e72a2c
updated tiff image
catalin-enache May 28, 2024
b3bc369
Update FBXLoader.js
Mugen87 May 28, 2024
35322b0
Update webgl_loader_fbx.html
mrdoob May 29, 2024
236444b
simplified loadTexture logic
catalin-enache May 29, 2024
b19dfb8
lazy importing loaders
catalin-enache May 31, 2024
248856c
Revert "lazy importing loaders"
catalin-enache May 31, 2024
f756cd3
reverted non native textures example
catalin-enache May 31, 2024
fb50379
reverted non native textures example
catalin-enache May 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 9 additions & 24 deletions examples/jsm/loaders/FBXLoader.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -427,31 +427,20 @@ class FBXTreeParser {

let texture;

const extension = textureNode.FileName.slice( - 3 ).toLowerCase();
const nonNativeExtensions = new Set( [ 'tga', 'tif', 'tiff', 'exr', 'dds', 'hdr', 'ktx2' ] );

if ( extension === 'tga' ) {
const extension = textureNode.FileName.split( '.' ).pop().toLowerCase();

const loader = this.manager.getHandler( '.tga' );
if ( nonNativeExtensions.has( extension ) ) {

if ( loader === null ) {

console.warn( 'FBXLoader: TGA loader not found, creating placeholder texture for', textureNode.RelativeFilename );
texture = new Texture();

} else {

loader.setPath( this.textureLoader.path );
texture = loader.load( fileName );

}

} else if ( extension === 'dds' ) {

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

if ( loader === null ) {

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

} else {
Copy link
Owner

@mrdoob mrdoob May 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@catalin-enache catalin-enache May 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

@Mugen87 Mugen87 May 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@catalin-enache catalin-enache May 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@Mugen87 Mugen87 May 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@catalin-enache catalin-enache May 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Expand All @@ -461,13 +450,9 @@ class FBXTreeParser {

}

} else if ( extension === 'psd' ) {

console.warn( 'FBXLoader: PSD textures are not supported, creating placeholder texture for', textureNode.RelativeFilename );
texture = new Texture();

} 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 );

}
Expand Down
Binary file added examples/models/fbx/sample_512.dds
View file
Open in desktop
Binary file not shown.
Binary file added examples/models/fbx/sample_512.exr
View file
Open in desktop
Binary file not shown.
Binary file added examples/models/fbx/sample_512.hdr
View file
Open in desktop
Binary file not shown.
Binary file added examples/models/fbx/sample_512.tga
View file
Open in desktop
Binary file not shown.
Binary file added examples/models/fbx/sample_512.tiff
View file
Open in desktop
Binary file not shown.
Binary file added examples/models/fbx/with_non_native_textures.fbx
View file
Open in desktop
Binary file not shown.
36 changes: 30 additions & 6 deletions examples/webgl_loader_fbx.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,19 +31,33 @@
import { OrbitControls } from 'three/addons/controls/OrbitControls.js';
import { FBXLoader } from 'three/addons/loaders/FBXLoader.js';
import { GUI } from 'three/addons/libs/lil-gui.module.min.js';
import { EXRLoader } from 'three/addons/loaders/EXRLoader.js';
import { RGBELoader } from 'three/addons/loaders/RGBELoader.js';
import { TGALoader } from 'three/addons/loaders/TGALoader.js';
import { TIFFLoader } from 'three/addons/loaders/TIFFLoader.js';
import { DDSLoader } from 'three/addons/loaders/DDSLoader.js';

let camera, scene, renderer, stats, object, loader, guiMorphsFolder;
const clock = new THREE.Clock();
const manager = new THREE.LoadingManager();

manager.addHandler( /\.tga$/i, new TGALoader() );
manager.addHandler( /\.dds$/i, new DDSLoader() );
manager.addHandler( /\.exr$/i, new EXRLoader() );
manager.addHandler( /\.hdr$/i, new RGBELoader() );
manager.addHandler( /\.tiff$/i, new TIFFLoader() );
Copy link
Collaborator

@Mugen87 Mugen87 May 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@Mugen87 Mugen87 May 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@catalin-enache catalin-enache May 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@Mugen87 Mugen87 May 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@catalin-enache catalin-enache May 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?


let camera, scene, renderer, stats, object, loader, guiMorphsFolder;
let mixer;

const clock = new THREE.Clock();

const params = {
asset: 'Samba Dancing'
};

const assets = [
'Samba Dancing',
'morph_test'
'morph_test',
'with_non_native_textures'
];


Expand Down Expand Up @@ -87,7 +101,7 @@
grid.material.transparent = true;
scene.add( grid );

loader = new FBXLoader( );
loader = new FBXLoader( manager );
loadAsset( params.asset );

renderer = new THREE.WebGLRenderer( { antialias: true } );
Expand Down Expand Up @@ -126,8 +140,18 @@

object.traverse( function ( child ) {

if ( child.material ) child.material.dispose();
if ( child.material && child.material.map ) child.material.map.dispose();
if ( child.material ) {

const materials = Array.isArray( child.material ) ? child.material : [ child.material ];
materials.forEach( material => {

if ( material.map ) material.map.dispose();
material.dispose();

} );

}

if ( child.geometry ) child.geometry.dispose();

} );
Expand Down