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

Circle rotation results in invisible entity #5

Closed
Shatur opened this issue Mar 3, 2022 · 10 comments
Closed

Circle rotation results in invisible entity #5

Shatur opened this issue Mar 3, 2022 · 10 comments
Labels
external-bug A bug in some external dependency

Comments

@Shatur
Copy link

Shatur commented Mar 3, 2022

Here is the code:

use bevy::prelude::*;
use bevy_tweening::{lens::*, *};

fn main() {
    App::default()
        .add_plugins(DefaultPlugins)
        .add_plugin(TweeningPlugin)
        .add_startup_system(setup)
        .run();
}

fn setup(mut commands: Commands) {
    commands.spawn_bundle(OrthographicCameraBundle::new_2d());

    let size = 80.;
    let tween = Tween::new(
        EaseMethod::Linear,
        TweeningType::PingPong,
        std::time::Duration::from_secs(1),
        TransformRotationLens {
            start: Quat::IDENTITY,
            end: Quat::from_axis_angle(Vec3::Z, std::f32::consts::PI),
        },
    );

    commands
        .spawn_bundle((
            Transform::default(),
            GlobalTransform::default(),
        ))
        .with_children(|parent| {
            parent
                .spawn_bundle(SpriteBundle {
                    sprite: Sprite {
                        color: Color::RED,
                        custom_size: Some(Vec2::new(size, size * 0.5)),
                        ..Default::default()
                    },
                    ..Default::default()
                })
                .insert(Animator::new(tween));
        });
}

It works correctly, sprite rotates. But if you replace std::f32::consts::PI with std::f32::consts::TAU the sprite becomes invisible.

@djeedai
Copy link
Owner

djeedai commented Mar 4, 2022

I think this is because Quat::slerp() finds the shortest rotation between start and end, and with TAU your start and end points are the same. I'm not sure why the entity becomes invisible though, maybe q.slerp(q) has a bug or undefined behavior.

What I think would work in your case is a custom lens where you interpolate the angle first, then build the rotation from it second. Something like (untested):

struct RotationLens {
  start_angle: f32,
  end_angle: f32,
}

impl Lens<Transform> for RotationLens {
  fn lerp(&mut self, target: &mut Transform, ratio: f32) {
    let angle = self.start_angle.lerp(self.end_angle, ratio);
    target.rotation = Quat::from_rotation_z(angle);
}

then use as:

RotationLens {
    start_angle: 0.,
    end_angle: std::f32::consts::TAU,
}

@Shatur
Copy link
Author

Shatur commented Mar 4, 2022

I think this is because Quat::slerp() finds the shortest rotation between start and end, and with TAU your start and end points are the same. I'm not sure why the entity becomes invisible though, maybe q.slerp(q) has a bug or undefined behavior.

I tried using Quat::IDENTITY for end and it works as expected - sprite doesn't move.
But I think you are right about UB. In 3D I have not only invisible mode, but also strange glitches when I rotate camera.

What I think would work in your case is a custom lens where you interpolate the angle first, then build the rotation from it second. Something like (untested):

Yes, this works for me.

djeedai added a commit that referenced this issue Mar 5, 2022
Add more predefined rotation lenses interpolating the angle of rotation
instead of the `Quat` itself directly. Document at the `lens` module
level the difference between the shortest-path lens and the
angle-focused ones.

Bug: #5
djeedai added a commit that referenced this issue Mar 5, 2022
Add more predefined rotation lenses interpolating the angle of rotation
instead of the `Quat` itself directly. Document at the `lens` module
level the difference between the shortest-path lens and the
angle-focused ones.

Bug: #5
@djeedai
Copy link
Owner

djeedai commented Mar 5, 2022

@Shatur I've just merged some changes that add some new angle-based rotation lenses, and document in the lens module the difference with TransformRotationLens which is shortest-path based. This was a very good feedback, thanks for that! Hopefully things should be more clear now. Let me know if that suits and we can close this issue. Thanks!

@Shatur
Copy link
Author

Shatur commented Mar 5, 2022

Thanks! But isn't it strange that Quat::IDENTITY -> Quat::IDENTITY works, but Quat::IDENTITY -> Quat::from_axis_angle(Vec3::Z, TAU) results in such behavior?

Could you also draft a new release?

@djeedai
Copy link
Owner

djeedai commented Mar 5, 2022

Right, let's leave that issue open until we root caused that one. I didn't have time to look at it yet.

I missed to update the README too with the new lenses, I'll make a release after that.

@djeedai
Copy link
Owner

djeedai commented Mar 5, 2022

Actually just a note for self, I've been reminded looking at tests that Quat::from_rotation_z(TAU) produces (as it should) the quaternion (0,0,0,-1). So it could explain the difference if q.slerp(q, 1.0) works but q.slerp(q.conjugate(), 1.0) is undefined behavior or has a bug. The rotations q and q.conjugate() represent are the same, but the objects are different. And I bet it works with 2.0 * TAU for that same reason.

@djeedai
Copy link
Owner

djeedai commented Mar 5, 2022

Root-caused as expected to conjugate quaternions producing a divide-by-zero, which with the SSE2 implementation (default on Windows for example) yields all NaNs, so an invalid rotation that when applied to a vector probably moves the sprites to infinity and makes them invisible.

Filled a bug with glam : bitshifter/glam-rs#276

I propose we close this bug because there's not much else I can do here I think. We just need to wait for a fix to be written and trickle down to Bevy itself.

@djeedai djeedai added the external-bug A bug in some external dependency label Mar 5, 2022
@Shatur
Copy link
Author

Shatur commented Mar 5, 2022

Maybe glam-rs will fix slerp and using a separate lens for circle rotation won't be needed?

@djeedai
Copy link
Owner

djeedai commented Mar 5, 2022

The new lenses are still needed, because slerp() always do a shortest-path interpolation. And q and q.conjugate() represent the same rotation, so the shortest path is no movement at all. If you want to do a full turn, you have no choice and need to interpolate the angle instead. In fact, any rotation past a half turn will exhibit the issue, as slerp() will rotate "the other way around" since the angle in the opposite direction is less than a half-turn.

The glam bug is only to fix the sprite disappearing. But even fixed, you would see no rotation at all, which I don't think is very useful 😊

@Shatur
Copy link
Author

Shatur commented Mar 5, 2022

Oh, understand, thank you for looking into it!

@Shatur Shatur closed this as completed Mar 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
external-bug A bug in some external dependency
Projects
None yet
Development

No branches or pull requests

2 participants