Fix create physical bone when up vector is collinear to child origin

[Blackiris]

Fix issue #39757 but for Godot 4.0

When generating physical bone, set_look_at is used to change the bone direction.

body_transform.set_look_at(Vector3(0, 0, 0), child_rest.origin);

However, if the vector child_rest.origin is collinear with the default up vector =Vector3(0, 1, 0), the cross product cannot generate a third vector (with length > 0) and set_look_at fails.
In order to avoid this issue, we first compute an up vector not collinear:

/// Get an up vector not collinear with child rest origin
Vector3 up = Vector3(0, 1, 0);
if (up.cross(child_rest.origin).length() == 0) {
	up = Vector3(0, 0, 1);
}

And then use it

body_transform.set_look_at(Vector3(0, 0, 0), child_rest.origin, up);

Please find a reproduction project for Godot 4.0 at #39757 (comment)

Bugsquad edit:

• Fixes Import GLTF curves physical skeleton not exact place #39757

[fire]

What happens if the first one succeeds and the second test fails?

[Blackiris]

Good question, I was asking myself this question too yesterday night before sleeping...

In Transform::set_look_at...

	v_z = p_eye - p_target;
	v_z.normalize();
	v_y = p_up;

	v_x = v_y.cross(v_z);
#ifdef MATH_CHECKS
	ERR_FAIL_COND(v_x.length() == 0);
#endif

... the check is done only if MATH_CHECKS is defined. Then, in case of failure, the error is thrown and the basis and origin change, which happen in the next lines, won't happen.
So if the first set_look_at succeed and the second one failed, only the first will change the direction.

However, I am wondering if MATH_CHECKS is always defined in the editor. Currently it is defined in math_defs.h:

#ifdef DEBUG_ENABLED
#define MATH_CHECKS
#endif

But if DEBUG_ENABLED is not enabled in the editor, then the vectors check must be done before using set_look_at...
After looking it seems it is enabled only if target is release_debug or debug.

Thus, I need to change the PR...

[EDIT] : wondering also if the join transform is correct, since it was unchanged for some time...

[Blackiris]

I have changed the solution: up is now checked before being used in set_look_at. The first top comment is also changed to show exactly that.
The joint transform also looks correct.

[fire]

Can you rebase? I'll try get some review on this pr.

[Blackiris]

Thank you! Just done rebasing this morning

[fire]

@pouleyKetchoupp Is this something in your area?

[pouleyKetchoupp]

The code itself looks fine, I've just tested this PR and the 3.x PR (#48327):

• The 3.x version works fine, and also fixes other issues like PhysicalBones are not alligned with skeleton when created #52974 and "Generate physical skeleton" creates wrong bone positions #53466.

• But the 4.0 version doesn't work well, bone offsets and rotations are wrong in some cases. I wonder if there's another bug in the skeleton itself, or something that has changed recently that breaks this PR. Would be good for someone more familiar with skeleton to check.

Also there's another PR #53515 that addresses issues around the same area. Might be worth checking what the best way to handle all these cases is.
CC @MrMinimal @lyuma

[lyuma]

I think this bug is caused by Basis::looking_at being non-robust, which is really bad for a game engine. (Imagine bugs in a game where you can trick the AI to walk the wrong direction if you go directly above it)

The best fix would be for Basis::looking_at to be changed to handle this corner case internally rather than force all callers to do this work. (Though I'd understand if such a breaking change would only be done in 4.x)

Anyway, given this flaw in Basis::looking_at, I think this fix looks great.

[lyuma]

Side note: #53515 appears to address a different bug: namely the fact that the bone is offset along the wrong axis in 3.x, I think because the basis is not aligned to the bone. we can test this if this is still a problem once #48327 is merged, but #48327 is a more complete fix by backporting the looking_at instead of assuming that the bones are aligned to their children.

[lyuma]

Oh sorry, I meant to say please use (up.cross(...).is_equal_approx(Vector3()) instead of .length == 0

conceptually it seems fine

[Blackiris]

Thanks a lot for the feedback! I have applied the requested change and rebase the branch to master.
However, it seems the bones in the test project are indeed broken in 4.0. I will do a git bisect later today.

[Blackiris]

I have noticed only DAE bones are broken, but not GLTF ones.
Bisecting shows the issue appears after 5ffed49, which is quite a big commit...

[lyuma]

@Blackiris Is DAE broken without this PR applied? or does it only break in combination with this PR.

Can you please upload the DAE model you are testing. Reproduction project? Is an issue filed?
I'd be curious to see why that happens.

[Blackiris]

@lyuma Sorry I wasn't clear. Here are the results I get with the following project (1 model saved as .dae and .glb)
DAE_skeleton_Test.zip

Before this commit:
DAE and GLTF bones are good
For physical skeleton generation: direction and length are broken
=> this patch fix only physical bones directions for both

After this commit
GLTF bones looks correct,
DAE bones are broken (weird direction)
=> This patch fix physical bones generation direction only, but their length are still wrong (sometimes too long in my tests)

To conclude,

1. the DAE bones seems broken from this commit
2. Bones length are broken even before, and this patch don't fix it

I think it still needs some work to understand and fix the issues before validating this PR

[Blackiris]

I have created the specific issue for the DAE bones #56242 as it is a different issue compared to this one.
This PR still looks good to me, as it respect the bones after import (good bones for GLTF, wrong for DAE)

[lyuma]

Yes, I remember reviewing this. I think it's fine for this purpose.

Long term, I'd think this type of fix ought to belong inside Basis::looking_at itself. For example, I could imagine a game developer using looking_at in an AI script, and having hard-to-reproduce bugs caused by these pole vectors)

[akien-mga]

Thanks!