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.

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 morphAttributes.position[n].count is less than attributes.position.count #28378

Closed
catalin-enache opened this issue May 14, 2024 · 42 comments · Fixed by #28397
Closed
Labels
Milestone

Comments

@catalin-enache
Copy link
Contributor

catalin-enache commented May 14, 2024

Description

Hello,
I'm inspecting this model: https://raw.githubusercontent.com/catalin-enache/threejs-inspector/master/public/models/__free/fbx/Asuna/2/FreeTestAnimations_reexport.fbx and I observed that morphAttributes.position[n].count is less than attributes.position.count.

I have a fiddle here: https://codesandbox.io/p/sandbox/condescending-swanson-4fypcn?file=%2Fsrc%2Findex.js%3A81%2C4-81%2C142

I'm printing the loaded fbx. Please check the console for these 2 fields in the AsunaBodyMesh (the last skinned mesh of the root children).

image

It can be observed that attributes.position.count is 86082 and morphAttributes.position[0].count is 34902.

I'm trying to optimize the object by reconstructing new skinned meshes grouped by materialIndex from groups.
I'm extracting slices from attributes , push them into an array for that material then make a new geometry and skinned mesh for that material. I then replace the original mesh with as many meshes as materials the original mesh had.

The problem is that when I try to port the morphAttributes into new skinned meshes, since they have a different count,
I have no ideea how to slice them to match the attributes slices.

I asked here: https://discourse.threejs.org/t/how-are-related-morphattributes-position-arrays-to-attributes-position/65533
maybe my expectation (that the count should be the same between attributes and morphs) is wrong but I understand they should be equal.

Then I thought probably I should raise this issue here.

Reproduction steps

Just load the fbx asset and inspect the result: morphAttributes.position[n].count and attributes.position.count

Code

no code required

Live example

https://codesandbox.io/p/sandbox/condescending-swanson-4fypcn?file=%2Fsrc%2Findex.js%3A81%2C4-81%2C142

Screenshots

Provided in description

Version

0.164.1

Device

Desktop

Browser

Edge

OS

MacOS

@RemusMar
Copy link
Contributor

I'm inspecting this model: https://raw.githubusercontent.com/catalin-enache/threejs-inspector/master/public/models/__free/fbx/Asuna/2/FreeTestAnimations_reexport.fbx
It can be observed that attributes.position.count is 86082 and morphAttributes.position[0].count is 34902

Attributes and morphAttributes are two different things.
If you import the FBX file into a 3D modeler you'll see that the mesh is divided into 6 parts.
All of them with the Skin modifier, but only one of them (BodyMesh) with Morpher.
That's why the 86082 and 34902 results.

@catalin-enache
Copy link
Contributor Author

catalin-enache commented May 14, 2024

@RemusMar thanks for feedback.
Please correct me if I'm wrong.
In a SkinnedMesh the geometry.attributes.position is an array of vertices that make that mesh.
In the context of "Shape Keys" geometry.attributes.position represent also the base shape.
Now, other possible alternatives to the base shape are in geometry.morphAttributes.position array.
Each entry in this array is an alternative to the base shape.
The morphTargetInfluences is an array of weights (0..1) having the same number of entries as geometry.morphAttributes.position.
If I set morphTargetInfluences[0] = 0.5, that means: take the geometry.attributes.position and interpolate each vertex inside with each vertex from the variant 0 (geometry.morphAttributes.position[0]).
That would imply that they should be related 1 to 1, the first vertex in geometry.attributes.position relates to first vertex from geometry.morphAttributes.position[0] and the same for their last vertex.
But if their count don't match how would the last vertex from geometry.attributes.position be interpolated ?

For this mesh the count 86082 is for geometry.attributes.position and the count 34902 is for geometry.morphAttributes.position[0], where both of them are in the same GeometryBuffer for the same SkinnedMesh.

In Blender they are equal: (the base and the shoulder.L variant )
image
image
even if for some reason the vertices count are different than from the loaded with FBXLoader.

I mean it makes sense to be equal since they have to be interpolated vertex by vertex.

This one to one relation I'm also trying to apply when selecting vertices grouped by group.materialIndex from geometry.attributes.position and from geometry.morphAttributes.position[0]

Am i missing something ?

@RemusMar
Copy link
Contributor

I thought I was clear enough.
86082 vertices in total but only 34902 involved in Morphing.

Morpher

@catalin-enache
Copy link
Contributor Author

catalin-enache commented May 15, 2024

@RemusMar from your first answer my understanding of your explanation is that 86082 count represents the total of those 6 parts while 34902 count represents the morph part.

To be on the same page regarding what is what for this asset: "https://raw.githubusercontent.com/catalin-enache/threejs-inspector/master/public/models/__free/fbx/Asuna/2/FreeTestAnimations_reexport.fbx".
The root object is formed of 6 skinned meshes (not a mesh divided in 6 parts - of what ?).
One of these meshes (BodyMesh) has morph information. The other 5 meshes are not relevant.
I re-exported the model as fbx and gltf, only containing now the relevant BodyMesh.
These are the new assets:
https://raw.githubusercontent.com/catalin-enache/threejs-inspector/master/public/models/__free/fbx/Asuna/2/Only_BodyMesh.fbx

https://raw.githubusercontent.com/catalin-enache/threejs-inspector/master/public/models/__free/fbx/Asuna/2/Only_BodyMesh.glb

I updated the fiddle to load both of them (Note: it takes very long to load): https://codesandbox.io/p/sandbox/condescending-swanson-4fypcn?file=%2Fsrc%2Findex.js%3A68%2C3-68%2C129

If I load them both in Blender they have the same structure (fbx left / gltf right):
image
One mesh (just Mesh not SkinnedMesh, cos I got rid of skeleton too) with 3 materials and with morph information.

If I load them in ThreeJS I can observe that:

The GLTFLoader

is optimizing the one mesh with 3 materials by creating 3 meshes, one for each material.
image

Each extracted mesh has morph information.
For each mesh the geometry.attributes.position.count is equal with any morph variant count from geometry.morphAttributes.position.

1898 for the first extracted mesh:
image

This is as expected to me, a morph array should have the same length as the base from which it derives because they are interleaved one to one for each vertex in the base geometry.

The FBXLoader

maintains the original structure as one mesh with 3 materials.

image

but the geometry.attributes.position has different count than any morph alternative.

image

The same numbers as before 86082 (for base geometry) / 34902 (first morph variant).
Not all morphs have 34902 though (just the first few morphs). Some other morphs from those 62 have other lengths.

I believe that in translation from fbx format into Three format the length of morphs has been preserved since most likely the fbx format optimises and gets rid of redundant vertices (those that are not involved in morphing).

However, as I @donmccurdy confirmed my expectation here https://discourse.threejs.org/t/how-are-related-morphattributes-position-arrays-to-attributes-position/65533 the final Three format should get back the redundancy making the base shape and any derived morph arrays equal.

@donmccurdy can you please add your input here about the expectation that in final Three format, any morph variant array should have the same length as the base geometry from which it derives ?

If not, then there should be some mapper in the Three object that should tell us the correspondence between a morph vertex and the base position vertex (so that we can rebuild the morph array to be the same length as the base geometry - adding back redundancies) for scenarios when we want to split the base geometry by material carrying on the morph information along.
And not only for that I think. I wonder how does the shader succeeds in interpolating the vertices between 2 arrays of different lengths.
Probably it wont crash only if it takes as the reference the shorter array (the morph) but then the assumption that the mapping is one to one would be wrong and the result of the interpolation would be incorrect (or would be correct by coincidence).

@RemusMar
Copy link
Contributor

@catalin-enache
Because it seems you're confused:
Skinning and Morphing are different things.
Skinning ---> Bones animation
Morphing ---> Vertex animation
They are stored in different arrays, in many cases those arrays have different length and the logic involved for them is also different.
Now ...
Where is the "error" in your case?
It's either:

  1. the model was not designed properly (the Skinning/Morphing area)
    or
  2. the FBX exporter is buggy.

good luck

@catalin-enache
Copy link
Contributor Author

catalin-enache commented May 15, 2024

The fbx exporter used is the one from Blender 4.1.
I am able to import the fbx asset Only_BodyMesh.fbx without any problem back in Blender (see prev screenshot) as well as in UE5.
image
I can see the morphs and play with them.

My problem is, how do I carry the morph information (which is less than the base shape) when I want to optimize an initial Three mesh having multiple materials by splitting it into multiple meshes (one for each material).

For the base shape it is straight forward.
I'm looping through materials for that mesh, then for each material index I'm looping through groups.
If the group has the same materialIndex I select the corresponding vertices (based on group.start and group.count) from geometry.position and push them into an array for that material.
In the end I create a geometry from the collected vertices and from it a new mesh with one single material.

But the same logic cannot be applied to morphs for fbx asset because morphs have less elements and for that matter we don't know how the vertices from the morph are related to the vertices from the base geometry. (For the gltf asset though, the same logic can be applied since the base geometry and the morphs arrays are equal from my observations so far).

@RemusMar
How do you suggest to approach the morph extraction for the fbx asset ?
Should I assume that with current FBXLoader implementation it is not supported ? and I should not try to optimize at import time fbx assets that have morph information, or any asset type if morph information is less than base information ?
Should I change current issue from a bug to a feature request ? asking for introducing redundant information into morph arrays to make them having the same length as the base geometry.position array ? or to introduce a mapper into the Three object that should help with filling in the blanks into morphs ?

@RemusMar
Copy link
Contributor

Your problem has nothing to do with three.js
If you read again my posted messages you'll see that I get that difference just importing your FBX file into 3DS Max.
And 3DS Max had always the best FBX importer ...

My advice to you here:
First of all, review the Skinning and Morphing.
Skinning requires all the vertices to be involved, but Morphing not!
Morphing is used for certain mesh areas (face animation, cloth parts, etc), so it's a bad idea to involve all the vertices if you don't need them.
Why to waste resources for nothing?
And why FBX if you're working with three.js ?
Why not GLB?
You have everything you need with this container.
Not to mention the best tools, exporters and importers.

good luck again

@donmccurdy
Copy link
Collaborator

donmccurdy commented May 15, 2024

Let's limit the scope of the issue to this:

FBXLoader produces a THREE.BufferGeometry instance in which a base attribute has 86,082 vertices, and a morph attribute has 34,902 vertices.

We understand why the FBX file format would contain a shorter morphed vertex list — not all vertices are morphed — but I'm not aware that varying attribute lengths is allowed or functional in THREE.BufferGeometry. Is it supported by three.js? If not, then FBXLoader should produce valid THREE.BufferGeometry instances, regardless of the FBX file's internal structure.

@catalin-enache
Copy link
Contributor Author

catalin-enache commented May 15, 2024

Why to waste resources for nothing? And why FBX if you're working with three.js ? Why not GLB? You have everything you need with this container. Not to mention the best tools, exporters and importers.

@RemusMar , I'm working an a threejs-inspector app (hobby for the moment, whishing to make it a reliable tool).
It is something similar with three editor but different in purpose.
One applicability of this app is to be able to drop it into an existing ThreeJS app, allowing you to double click any object that has been loaded in the scene and inspect and change every property that object has (materials, animations, morphs, children, or if it is a light, camera or something else, their specific properties).

I see that as a nice to have tool in development when you can play around with properties (by twisting a material value or or object position/rotation), find something you like then burn it in code.

It also allows you to import assets into the scene.
Now I observed that some assets when loaded into the scene cause thousands of draw calls even if they have few meshes and few materials, causing a very ugly experience when one just wants to load and visualize some random asset and play with its properties (the scene was almost frozen).
The reason was that those meshes, even having 2 materials had thousands of groups alternating materials at every few faces.
I could let it like that and mention that if assets are not optimized the app might get unresponsive, then go and optimize your asset before loading it, or I can try to optimize it on the fly after loading, to make as best user experience as possible,
I achieved very good results reducing draw calls from few thousands to 30,40.

So, fbx is not my choice, I just want to allow a smooth experience to inspect everything that ThreeJS can load (that includes fbx, gltf, obj, collada, stl, etc) - acting as an universal viewer in particular, at best possible experience even for un-optimised assets. Of course the asset optimisation function can be reused in specific Three app too.

Why to waste resources for nothing?

It is not for nothing if it allows asset optimization on the fly.

@Mugen87
Copy link
Collaborator

Mugen87 commented May 15, 2024

but I'm not aware that varying attribute lengths is allowed or functional in THREE.BufferGeometry. Is it supported by three.js?

It is not. The count of the morph target buffer attribute must match the count of the target attribute that is going to be morphed.

If the morph target attribute is too small, the shader will still try to fetch data for the current gl_VertexID (the current vertex index) and will end up with no (undefined) data.

@Mugen87
Copy link
Collaborator

Mugen87 commented May 15, 2024

For testing: https://jsfiddle.net/8p9tyga0/

Noticed how the animation is broken if the morph target buffer is too small.

@RemusMar
Copy link
Contributor

For testing: https://jsfiddle.net/8p9tyga0/

I get the same result for

const morphAttribute = [
    	2, 1, 0,
      -2, 1, 0
];

and

const morphAttribute = [
    	2, 1, 0,
      -2, 1, 0,
      0, 0, 0,
      0, 0, 0
];

So the same result for different morphAttribute lengths.

@Mugen87
Copy link
Collaborator

Mugen87 commented May 15, 2024

The result is platform dependent. That's why I say the data are undefined. They can be evaluated as 0 but also as some sort of NaN.

@RemusMar
Copy link
Contributor

What do I see here (the latest Firefox version) is the same result for different morphAttribute lengths.
Where is the problem?

@RemusMar
Copy link
Contributor

If baseAttibute.length > morphAttribute.length morphAttribute is filled with zeroes and everything works as expected.

@catalin-enache
Copy link
Contributor Author

But is it correct to fill it with zeroes ? Shouldn't it be filled with the actual values (redundant values) from the base position ?

@Mugen87
Copy link
Collaborator

Mugen87 commented May 15, 2024

Correct. But the fix must be applied in FBXLoader.

@RemusMar Are you open for a PR?

@RemusMar
Copy link
Contributor

But is it correct to fill it with zeroes ?

Yes.
All the not involved vertices have ZERO influence in morphing.
Why should I increase the FBX file size for nothing, if it can be done at runtime?

@RemusMar Are you open for a PR?

LOL
I did never use the three.js FBX importer so far, but I can take a look over it if the initial author left us ...

@catalin-enache
Copy link
Contributor Author

catalin-enache commented May 15, 2024

If in the fiddle example the intended morph attributes would have been like this:

const morphAttribute = [
     2, 1, 0,
      // - 1, 1, 0,
     2, 1, 1,
     // 1, -1, 0
    ];

how would one know, at runtime, where should the blanks be filled in ?

@Mugen87
Copy link
Collaborator

Mugen87 commented May 15, 2024

If in the fiddle example the intended morph attributes would have been like this:

No, that isn't right. If you only want the first two vertices to animate, the subsequent two must have the same value like in the position attribute. They can't be left out and they can't be 0. So it must be:

const morphAttribute = [
    	2, 1, 0,
      - 2, 1, 0,
      - 1, - 1, 0,
      1, -1, 0
];

@catalin-enache
Copy link
Contributor Author

catalin-enache commented May 15, 2024

@RemusMar @donmccurdy @Mugen87 , guys, I appreciate a lot the time you spent looking into this and that we come up with a resolution.

@donmccurdy
Copy link
Collaborator

There's also the option of using geometry.morphTargetsRelative = true, and then the omitted values could be zero. GLTFLoader uses that option, but I don't know if it's helpful with FBX.

@RemusMar
Copy link
Contributor

They can't be left out and they can't be 0

Michael, that's not how Morphing works.
If a vertex is not involved in morphing, its weight is ZERO on all directions.
So (0,0,0) means no change for that vertex.

@RemusMar
Copy link
Contributor

There's also the option of using geometry.morphTargetsRelative = true, and then the omitted values could be zero.

That's the only correct option here.
Anything else is a waste of resources (memory) for nothing.

@catalin-enache
Copy link
Contributor Author

@Mugen87 , you are saying that the morph array in its current form, even if shorter is continuous ? I can assume that all vertices in the morph match one to one with the vertices in the base position ? and what is remaining in the base position is not involved in morph ? If only the last vertex in the base position would need to be morphed then all the previous vertices would be pushed into the morph array even they are redundant ? leading to no possible memory optimisation ?

@Mugen87
Copy link
Collaborator

Mugen87 commented May 15, 2024

Using a different value for morphTargetsRelative could be interesting (depending on the input data). TBH, I don't care how it is implemented as long as the fix works.

Anything else is a waste of resources (memory) for nothing.

Using zero or the real values from the position attribute has no effect on memory since the underlying buffer will have the same size.

@catalin-enache
Copy link
Contributor Author

catalin-enache commented May 15, 2024

@donmccurdy , I observed in all imported assets geometry.morphTargetsRelative = true. But in the authoring 3d tool (I know Blender) that is an option (checked by default).
image

If the asset creator wants it to be absolute (unchecks it), and tweaks the morphs with it unchecked, export the asset like that, I believe it should be treated as such.
If it is absolute or relative at asset creation time, the values in the morph would be different.
Making it relative it in Three after the asset was exported with it absolute will miss-interpret those values.

I believe this option is inferred at asset parsing from the asset file itself, and it makes sense to not be changed after that or it will break how the asset is supposed to look when morphed. Controlling that value without breaking anything I think is only safe if we generate the asset programatically.

Of course it could be changed to relative at asset parsing in Three, even it was saved as absolute, but that would imply recalculating the values for all vertices to be indeed relative

@RemusMar
Copy link
Contributor

Using zero or the real values from the position attribute has no effect on memory since the underlying buffer will have the same size.

I have a character with 1,000,000 vertices involved in Bones animation and only 1000 of them involved in Morphing (facial animation).
Why should I export the FBX file with morphAttributes containing 999,000 not involved vertices?
Why should I build an array at runtime for something I don't use?
If I can pass (0,0,0) weight for all of them at runtime ...

@donmccurdy
Copy link
Collaborator

donmccurdy commented May 15, 2024

I think we're overcomplicating this: If FBXLoader produces a BufferGeometry in which attributes and morphAttributes have different lengths, that is a bug. The lengths of the attributes should match, and FBXLoader should do whatever it needs to do, as efficiently as possible, to produce a valid geometry from the .fbx file it is given.

PRs for that fix would be welcome, from anyone interested. 🙂

Controlling [morphTargetsRelative] without breaking anything I think is only safe if we generate the asset programatically.

Right, we certainly cannot just change morphTargetsRelative by itself. FBXLoader is constructing the geometry programmatically, right now it uses morphTargetsRelative = true every time. If a PR for the fix above would be better (simpler or faster?) with morphTargetsRelative=false, that would be fine, but I suppose it probably makes little difference.

@catalin-enache
Copy link
Contributor Author

I forked that nice fiddle, changing it so that only the last vertex is morphed:
https://jsfiddle.net/catalin_enache/6sf3uqyz/3/
The first 3 vertices in the morph array are obviously redundant (I can't see how not to specify them)
How would the optimised morph array look in the context of current FBXImporter behaviour ?
This is related to my prev question.
Can we assume that even if currently the morph array has less elements it is continuous ? matching one to one with base position array ? morph[0] => base[0], morph[n] => base[n] ?

@RemusMar
Copy link
Contributor

PRs for that fix would be welcome, from anyone interested.

I'm not very interested in the FBX Loader (I did never use it so far), but I can try to find a few hours this weekend in no one else ... ;)

For now this is all I know about this loader:
// Needs Support: Morph normals / blend shape normals
and
console.warn( 'THREE.FBXLoader: morph target attached to more than one geometry is not supported.' );

@catalin-enache
Copy link
Contributor Author

I can try but I'm a total outsider.
It might take long before figuring out something which might be fixing in the wrong place :).

@donmccurdy
Copy link
Collaborator

Can we assume that even if currently the morph array has less elements it is continuous ? matching one to one with base position array ? morph[0] => base[0], morph[n] => base[n] ?

In three.js, these lengths must match. The ith base vertex corresponds to the ith morph vertex. The morph attribute must contain an entry for every vertex in the base geometry, redundant or not.

@catalin-enache
Copy link
Contributor Author

Thanks Don, this is how they should be which I totally agree.
About how are they now it seems we cannot be sure.
I wanted to know if I can rely on them as they are, so I can slice them in the same way I'm slicing the geometry.position based on materialIndex, even if they are shorter, if at least they are continuous.

@RemusMar
Copy link
Contributor

I'm inspecting this model: https://raw.githubusercontent.com/catalin-enache/threejs-inspector/master/public/models/__free/fbx/Asuna/2/FreeTestAnimations_reexport.fbx and I observed that morphAttributes.position[n].count is less than attributes.position.count.

To conclude:
that model imported into 3DS Max (the best FBX importer) and exported as GLB (Babylon GLTF Exporter is one of the best) looks close to PERFECT into three.js.
Except the right eye (some wrong animation), everything else (including all the animations) is just fine.
I can upload the GLB file somewhere if you really need that.

@catalin-enache
Copy link
Contributor Author

@RemusMar, thanks for the hints.
I had no prob with how it looks but with how can I carry on the morph information into newly generated meshes based on materialId (for runtime optimisation purpose).
I managed to link the new meshes with the old skeleton, to update the animation tracks (when they targeted meshes instead of bones) to match the new meshes names.
But, I got blocked into the morph array length being shorter than the base position and with no clue in how morph entries are related to base position entries (are morph entries continuous so that I can extract them in the same way as I extract main position ? - to me it was and still is hard to assume that).

@RemusMar
Copy link
Contributor

I had no prob with how it looks but with how can I carry on the morph information into newly generated meshes based on materialId (for runtime optimisation purpose).

I know, but you can optimize something at runtime ONLY if that something is properly designed.
And it's not this case!
Here is the design error (fixed by the Babylon exporter via GLTF):
Asuna

@catalin-enache
Copy link
Contributor Author

catalin-enache commented May 16, 2024

Guys, are there already some unit tests for FBX Loader ? I created this PR and I thought maybe to add a test for it but I could't find a place where to add it eventually. The intention for the test would be to commit a small FBX file with partial morphs and inside the test, load it from the disk and check if the base geometry buffer and morph buffer have the same length.
In the test folder I see only core stuff is tested.

I made this fbx asset in Blender
morph_test.fbx.zip
https://raw.githubusercontent.com/catalin-enache/threejs-inspector/master/public/models/__free/fbx/Asuna/2/morph_test.fbx to create a test for this issue and without the fix proposed in the PR it thrown an error in console:
image
because the morph array was empty:
image

The asset was loaded but the morph was not doing anything (as expected since there was nothing in morph array).

With the fix it doesn't throw any error and the morph works fine.

@Mugen87
Copy link
Collaborator

Mugen87 commented May 16, 2024

How about enhancing the default FBX example (https://threejs.org/examples/webgl_loader_fbx) with a GUI that allow to select assets for testing? We use this approach in other loaders as well, e.g.

https://threejs.org/examples/webgl_loader_3mf
https://threejs.org/examples/webgl_loader_vrml
https://threejs.org/examples/webgl_loader_svg

You can then add your test file to the repository. That makes it indeed easier to test the changes.

@catalin-enache
Copy link
Contributor Author

I pushed a change to https://threejs.org/examples/webgl_loader_fbx along with uploading my morph example asset.
I was working on a unit test too but I was blocked in not being able to load the asset without a server.
Sinon lib would do that AFAIK but there is no Sinon in package.json and is beyond me to add a new dependency.

@donmccurdy
Copy link
Collaborator

At this time we don't maintain unit tests for the examples/jsm/* codepaths, only for src/*.

@Mugen87
Copy link
Collaborator

Mugen87 commented May 21, 2024

Fixed via #28397.

@Mugen87 Mugen87 closed this as completed May 21, 2024
@Mugen87 Mugen87 added this to the r165 milestone May 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants