bevyengine / bevy

A refreshingly simple data-driven game engine built in Rust
https://bevyengine.org
Apache License 2.0
35.56k stars 3.52k forks source link

Regression: gizmos panicking given bad output from `GlobalTransform::to_scale_rotation_translation` #14142

Closed aevyrie closed 2 months ago

aevyrie commented 3 months ago

Bevy version

0.14

What you did

Draw a big circle

What went wrong

Bevy panicked.

Error: The vector given to `Dir3::new_unchecked` is not normalized. The length is 0.25.
stack backtrace:
   0: _rust_begin_unwind
   1: core::panicking::panic_fmt
   2: bevy_math::direction::assert_is_normalized
   3: bevy_math::direction::Dir3::new_unchecked
   4: <bevy_gizmos::circles::SphereBuilder<Config,Clear> as core::ops::drop::Drop>::drop::{{closure}}

Additional information

This is a regression not present in 0.13.

rparrett commented 3 months ago

Draw a big circle

Could you elaborate on this?

I modified the 3d_gizmos example so that the spheres were very large, and also tried f32::max for a radius, but did not see this panic.

aevyrie commented 3 months ago

Repro is the big_space demo. I was trying to repro in a test in a PR to fix, but I can't figure out how to test this with the current setup, bevy_gizmo has no tests as far as I can tell.

aevyrie commented 3 months ago

Ah, found the issue. The quat from GlobalTransform::to_scale_rotation_translation is not normalized, and the sphere gizmo is exploding on that.

Almost certain I hit the same exact failure on the initial release of 0.13/12

[examples/demo.rs:155:13] scale.x * 0.505 = 5.05e18
[examples/demo.rs:153:13] translation = Vec3(
    8.431322e19,
    4.7312953e19,
    -7.1763914e19,
)
[examples/demo.rs:154:13] rotation = Quat(
    0.5,
    0.0,
    0.0,
    0.0,
)
aevyrie commented 3 months ago

Yup:

    // Ignore rotation due to panicking in gizmos, as of bevy 0.13
    let (scale, _, translation) = transform.to_scale_rotation_translation();
aevyrie commented 3 months ago

Deja vu: https://github.com/bevyengine/bevy/issues/12360

aevyrie commented 3 months ago

Looks like sphere was moved, and the normalize was removed. We really need tests.

Fixed in a patch to 0.13: https://github.com/bevyengine/bevy/pull/12375/files

Broken at some point: https://github.com/bevyengine/bevy/blob/c6a89c2187699ed9b8e9b358408c25ca347b9053/crates/bevy_gizmos/src/circles.rs#L215

mweatherley commented 3 months ago

Out of curiosity, why is the GlobalTransform conversion spitting out an invalid quaternion? Is it because the transform involves shearing?

aevyrie commented 3 months ago

It's very large and far from the origin. Converting the affine to trs is lossy.