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: Fixed morph attributes to match base geometry length #28397

Merged

Conversation

catalin-enache
Copy link
Contributor

@catalin-enache catalin-enache commented May 16, 2024

Fixed #28378

Description

Injected baseVertexPositions into geoInfo so that inside genFace we can generate triangles from real geometry,
enforcing faces that are not involved in morph to be pushed into morphAttributes.

genFace is used when generating base geometry as well as in generating morphs.
In genFace the vertices array is just a mean to generate triangles.
If we generate vertices from morphPositions, we wont have triangles for faces not involved in morph,
because ShapeUtils.triangulateShape returns an empty array of triangles when fed with for [0,0] elements
That's why we generate them from base geometry.
Later when buffers.vertex is being populated, the actual information is retrieved from morphPositions.

TLDR: we generate triangles from base geometry and based on these triangles we push data that we read from morphPositions into the final morph buffers.

@mrdoob mrdoob changed the title 28378 fixed morph attributes to match base geometry length FBXLoader: Fixed morph attributes to match base geometry length May 17, 2024
@mrdoob mrdoob requested a review from looeee May 17, 2024 08:07
Mugen87 added 2 commits May 17, 2024 19:44
Remove nurbs asset since it already has its own example.
The Stanford bunny is already used in three other examples. Let's only add assets in `webgl_loader_fbx` which are not used elsewhere.
'morph_test'
];

const transforms = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you think you can update morph_test.fbx such that no additional transforms are required in the example? Ideally the asset does not need be scaled and translated after the import. That would simplify the example a bit.

Copy link
Contributor Author

@catalin-enache catalin-enache May 17, 2024

Choose a reason for hiding this comment

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

Yes I can, however, the existing rabbit is already huge. Should we let it like that ?
I could, re-export the rabbit too to make it have a reasonable size

Copy link
Collaborator

@Mugen87 Mugen87 May 17, 2024

Choose a reason for hiding this comment

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

The rabbit is used in other examples and I would not like to touch them now. Notice that I have updated your branch and removed the nurbs and rabbit asset from the demo. They are already used in other demos so in webgl_loader_fbx I would only add assets which are not used elsewhere.

In any event, an optimized size for the morphed cube would be great!

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 see you removed the rabbit and nurbs.
Either we don't show it or I can re-export it (I'll do it in blender if so).
Meanwhile I'll just re-export the test example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pushed, changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The CI / E2E testing fail I think it was random. I tested manually that page (webgl_animation_skinning_ik) and looks fine (and it uses a gltf asset anyways).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sometimes we have false-positives. Rerunning the failed job often solves such issues. I'll give it a try.

@Mugen87 Mugen87 added this to the r165 milestone May 17, 2024
@Mugen87
Copy link
Collaborator

Mugen87 commented May 17, 2024

For testing: https://rawcdn.githack.com/catalin-enache/three.js_fork/df9db082f415b517de6051a9af51d1e0c5b53ab9/examples/webgl_loader_fbx.html

Copy link
Collaborator

@Mugen87 Mugen87 left a comment

Choose a reason for hiding this comment

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

The example looks good to me!

I'm not really familiar with FBX so I hope @looeee can have a look at the FBXLoader related changes. He knows the implementation best.

@mrdoob
Copy link
Owner

mrdoob commented May 20, 2024

The code looks good to me 👌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FBXLoader morphAttributes.position[n].count is less than attributes.position.count
3 participants