Skip to content

Change skinned mesh warning to only consider transformed parent nodes#247

Merged
lexaknyazev merged 1 commit intoKhronosGroup:mainfrom
aaronfranke:skinned-mesh-non-root
Dec 28, 2025
Merged

Change skinned mesh warning to only consider transformed parent nodes#247
lexaknyazev merged 1 commit intoKhronosGroup:mainfrom
aaronfranke:skinned-mesh-non-root

Conversation

@aaronfranke
Copy link
Contributor

This PR replaces NODE_SKINNED_MESH_NON_ROOT with NODE_SKINNED_MESH_PARENT_TRANSFORMS to avoid false positives for valid models. Fixes #133

I believe this check more accurately reflects the purpose of the original check. It's not that parent-child relationships entirely are considered invalid, rather that the transform aspect of skinned mesh nodes is ignored with respect to the visible mesh. The test model ignored_parent_transform.gltf is already set up this way, testing with a parent translated by [1, 2, 3]. There are valid use cases for inserting a skinned mesh into the hierarchy, for purposes like visibility, but we only need to validate for transforms.

@CLAassistant
Copy link

CLAassistant commented Dec 26, 2025

CLA assistant check
All committers have signed the CLA.

@lexaknyazev
Copy link
Member

The original warning also implicitly flags cases when the static transform is identity but the parent nodes are animated. This PR would reduce such coverage.

We could instead reduce the severity of the original message and add a new warning that only flags static transforms (as these are clearly problematic).

@aaronfranke aaronfranke force-pushed the skinned-mesh-non-root branch 2 times, most recently from 5a8896e to 43604cf Compare December 27, 2025 03:45
@aaronfranke
Copy link
Contributor Author

Done, the original message now has reduced severity, and a different message replaces it when the parent is transformed.

@lexaknyazev
Copy link
Member

Please don't replace the original message when the new message is added. Seeing both messages upfront would be a better UX than seeing the parent transforms warning, fixing it, and discovering that the asset still has issues.

@aaronfranke aaronfranke force-pushed the skinned-mesh-non-root branch from 43604cf to e9af317 Compare December 27, 2025 20:16
@aaronfranke aaronfranke force-pushed the skinned-mesh-non-root branch from e9af317 to d0bcf0a Compare December 28, 2025 20:26
@aaronfranke aaronfranke force-pushed the skinned-mesh-non-root branch from d0bcf0a to c6dca17 Compare December 28, 2025 20:27
@lexaknyazev lexaknyazev merged commit 2be17ea into KhronosGroup:main Dec 28, 2025
2 checks passed
@aaronfranke aaronfranke deleted the skinned-mesh-non-root branch December 28, 2025 22:34
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.

What does NODE_SKINNED_MESH_NON_ROOT mean exactly?

3 participants