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

Smooth Damp #2001

Closed
wants to merge 25 commits into from
Closed

Smooth Damp #2001

wants to merge 25 commits into from

Conversation

msklywenn
Copy link
Contributor

@msklywenn msklywenn commented Apr 24, 2021

This PR adds a smooth_damp function, useful to have a value gracefully reach a (moving) target over time.

It's from Game Programming Gems 4 and it is very similar to the one used in Unity. (They have an extra max speed parameter)
https://archive.org/details/game-programming-gems-4/page/95/mode/2up

Example:

#[derive(Default)]
struct PlayerMove {
    smoothed_input: Vec3,
    smoothed_input_velocity: Vec3,
};

// ...

        let (pos, vel) = Vec3::smooth_damp(
            player_move.smoothed_input,
            raw_input,
            player_move.smoothed_input_velocity,
            0.15,
            time.delta_seconds(),
        );
        player_move.smoothed_input = pos;
        player_move.smoothed_input_velocity = vel;

@msklywenn msklywenn marked this pull request as ready for review April 24, 2021 16:16
@Moxinilian Moxinilian added A-Core Common functionality for all bevy apps C-Enhancement A new feature S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Apr 24, 2021
@msklywenn
Copy link
Contributor Author

msklywenn commented Apr 27, 2021

I added a max speed version and fixed code duplication with private macros

impl_smooth_damp_max! {f32, f32, |change, max:f32| { f32::clamp(change, -max, max) }}
impl_smooth_damp_max! {f64, f64, |change, max:f64| { f64::clamp(change, -max, max) }}
impl_smooth_damp_max! {Vec2, f32, |change:Vec2, max| { change.clamp_length_max(max) }}
impl_smooth_damp_max! {Vec3, f32, |change:Vec3, max| { change.clamp_length_max(max) }}
Copy link
Contributor

Choose a reason for hiding this comment

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

You should probably write some test cases in a test module #[cfg(test)].
While I believe your implementation is correct, we should really start having tests for new features we add to bevy.

Copy link
Contributor Author

@msklywenn msklywenn Apr 27, 2021

Choose a reason for hiding this comment

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

Yup. Usually math functions are great candidates for tests. However, I've been scratching my head since the beginning and I have no idea of a good test for this. The function is usually used iteratively... Any suggestions?

velocity: Self,
smooth_time: f32,
delta_time: f32,
) -> (Self, Self)
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO returning structs > return tuples.
Having a struct makes it much clearer what each value is and prevent bugs where someone accidentally switches .0 and .1.

So having this return something like

pub struct SmoothDampResult<T> {
    smoothed: T,
    velocity: T,
}

would be much clearer and prevent some user-end bugs.

Copy link
Contributor Author

@msklywenn msklywenn Apr 27, 2021

Choose a reason for hiding this comment

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

At the moment, rust doesn't support tuple destructuring assignment (rust-lang/rust#71126).

But in the future, I expect users to call the function this way: (obj.pos, obj.vel) = smooth_damp(obj.pos, target, obj.vel, 0.1, dt); That's the only reason I wrote it this way.

If I were to use a SmoothDamp structure, I would use it as input too, but that's less friendly. It would prevent smoothing the translation of a Transform while keeping the velocity in another component, for example.

The last option would be to make the velocity input &mut, like C/C++/C# version do, but I fear needless lifetime issues and I like the function being pure...

Copy link
Contributor

Choose a reason for hiding this comment

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

Input parameters via &mut are not a good option IMO.

I would still argue for the struct return value since you can do something like this:

        let SmoothDampResult { smoothed, velocity } = Vec3::smooth_damp(
            player_move.smoothed_input,
            raw_input,
            player_move.smoothed_input_velocity,
            0.15,
            time.delta_seconds(),
        );
        player_move.smoothed_input = smoothed;
        player_move.smoothed_input_velocity = velocity ;

But I would leave this decision up to @cart 😄

@mockersf
Copy link
Member

Is Smooth Damp a special case of lerp / easing / tweening?

@msklywenn
Copy link
Contributor Author

Is Smooth Damp a special case of lerp / easing / tweening?

It's easing but it's iterative, ie. target value can change over time. You can even have several targets with different smoothing values sharing the velocity vector.

@msklywenn msklywenn closed this May 6, 2021
@msklywenn msklywenn deleted the branch bevyengine:main May 6, 2021 09:01
@msklywenn msklywenn deleted the main branch May 6, 2021 09:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Core Common functionality for all bevy apps C-Enhancement A new feature S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants