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

Fix desynced duplicate GLTFNode transform properties #83231

Merged
merged 1 commit into from
Feb 9, 2024

Conversation

aaronfranke
Copy link
Member

@aaronfranke aaronfranke commented Oct 13, 2023

Draft because this PR requires #83229 to be merged first in order for exporting to function properly.

Before this PR, GLTFNode was storing multiple fields for the transformation matrix and the position/rotation/scale values. This is wrong, there should only be one data structure storing the transformation. I'm also adding the "needs testing" label because this PR may change behavior and should be tested before merging.

This PR fixes #82825, a bug where skeleton bone transforms were not set correctly.

Behavior in master (left is TRS, right is matrix):
Screenshot 2023-10-12 at 9 02 58 PM

Behavior in this PR:
Screenshot 2023-10-12 at 9 00 17 PM

Why is the model looking down? That's just how the model is, it's the same if I open it in Blender.

Copy link
Member

@fire fire left a comment

Choose a reason for hiding this comment

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

What's your opinion of instead of deleting TRS we delete transform? I'm curious why that would be worse.

@aaronfranke
Copy link
Member Author

aaronfranke commented Oct 15, 2023

@fire Transform has more degrees of freedom than TRS, so it's information loss. Also, Godot works with Transform3D more often than it works with TRS (it's pretty much just the skeleton code that needs TRS, and the glTF format generally prefers TRS but you can use matrix too).

@aaronfranke aaronfranke force-pushed the gltf-transform branch 2 times, most recently from da942c5 to f857b2b Compare December 8, 2023 19:35
@aaronfranke aaronfranke marked this pull request as ready for review December 8, 2023 19:35
@aaronfranke aaronfranke requested a review from a team as a code owner December 8, 2023 19:35
@fire
Copy link
Member

fire commented Dec 22, 2023

Note that non orthogonal matrices are out of spec for gltf, but we can still accept them.

When matrix is defined, it MUST be decomposable to TRS properties.

https://registry.khronos.org/glTF/specs/2.0/glTF-2.0.html#transformations

@aaronfranke
Copy link
Member Author

@fire Correct, and technically you can still perform decomposition on arbitrary matrices but it will just be lossy. The code in this PR ensures that we export data very close to what users have in Godot, so any data loss is not on Godot's end.


if (!gltf_node->position.is_zero_approx()) {
node["translation"] = _vec3_to_arr(gltf_node->position);
if (gltf_node->transform.basis.is_orthogonal()) {
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure this is necessarily a good idea, there are scenes (and common in skeletons) where you can have a non uniform scale and this is needed for proper importing, even if orthogonal.

Copy link
Member

@reduz reduz Dec 22, 2023

Choose a reason for hiding this comment

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

These may not import correctly if you convert to TRS (or they may, I am not sure). Either case, I would test this well.

Copy link
Member

Choose a reason for hiding this comment

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

Oh if this is just for exporting it may not matter so much.

Copy link
Member Author

Choose a reason for hiding this comment

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

The is_orthgonal check is to ensure there is no skew/shear. Non-uniform scale is still allowed.

Copy link
Member

Choose a reason for hiding this comment

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

A non uniform scale is de composable into TRS as mentioned above, polar decomposition and such for nested non uniform scale is probably not needed for sheer and skew, but I am not sure what happens when we export it in this failure case.

Copy link
Member Author

Choose a reason for hiding this comment

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

It will export the data Godot has in the best possible format. This code will prefer TRS if TRS can store all the data, otherwise it will use matrix.

@YuriSizov YuriSizov modified the milestones: 4.x, 4.3 Dec 22, 2023
@fire
Copy link
Member

fire commented Jan 19, 2024

3d Pipeline Meeting:

  1. We discussed how gltf spec requires that matrix is decomposible into and TRS
  2. What do we do if we can't decompose into TRS?
  3. Lyuma: recommends we use Matrix
  4. Arronfranke: Information loss for non compliant Matrix / TRS

Reasoning:

Godot uses basis so we wanted to match. Use one source of truth.

Copy link
Contributor

@lyuma lyuma left a comment

Choose a reason for hiding this comment

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

My original hesitation was that in the specific case of skeleton bones, we might be losing a tiny bit of precision in the conversion from quaternion -> basis -> quaternion. But to be honest, this factor is so miniscule it's not really worth worrying about.

Meanwhile, the advantage of this change is clear: the code is easier to verify when everything goes through a common code path of storing a Basis

@akien-mga akien-mga merged commit 1618946 into godotengine:master Feb 9, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@aaronfranke aaronfranke deleted the gltf-transform branch February 9, 2024 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

GLTF importer does not read the "matrix" property for nodes
7 participants