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

Public easing #17711

Closed
wants to merge 3 commits into from
Closed

Public easing #17711

wants to merge 3 commits into from

Conversation

fjkorf
Copy link

@fjkorf fjkorf commented Feb 6, 2025

Objective

Make easing functions publicly accessible
Fixes #17665

Solution

add pub to mod

Testing

local build and run

@alice-i-cecile alice-i-cecile added D-Trivial Nice and easy! A great choice to get started with Bevy C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Animation Make things move and change over time A-Math Fundamental domain-agnostic mathematical operations S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Feb 6, 2025
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Is this useful if the functions within are pub(crate)?

@alice-i-cecile alice-i-cecile added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Feb 6, 2025
@alice-i-cecile alice-i-cecile added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Feb 6, 2025
@greeble-dev
Copy link
Contributor

greeble-dev commented Feb 7, 2025

I'm ambivalent about this change. These functions can already be used from the public EasingCurve API, although it's a little verbose:

let x = EasingCurve::new(0.0, 1.0, EaseFunction::SmoothStep).sample_clamped(t);

Also these low-level functions do not clamp their inputs, which might surprise people. Unity/Unreal/Godot and shader language smoothsteps all clamp.

On the other hand, having access to these low-level functions might be useful for optimisation. Maybe it's fine if mod easing_functions is renamed to mod unchecked_easing_functions?

Copy link
Member

@mockersf mockersf left a comment

Choose a reason for hiding this comment

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

I don't think those should be public, they're more of an implementation detail

@alice-i-cecile alice-i-cecile added X-Controversial There is active debate or serious implications around merging this PR S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Feb 8, 2025
@fjkorf fjkorf closed this Feb 8, 2025
github-merge-queue bot pushed a commit that referenced this pull request Feb 10, 2025
# Objective

- Expand the documentation for `EasingCurve`.
- I suspect this might have avoided the confusion in
#17711.
- Also add a shortcut for simple cases.

## Solution

- Added various examples and extra context.
- Implemented `Curve<T>` for `EaseFunction`.
- This means `EasingCurve::new(0.0, 1.0, EaseFunction::X)` can be
shortened to `EaseFunction::X`.
    - In some cases this will be a minor performance improvement.
    - Added test to confirm they're the same.
- ~~Added some benchmarks for bonus points.~~


## Side Notes

- I would have liked to rename `EaseFunction` to `EaseFn` for brevity,
but that would be a breaking change and maybe controversial.
    - Also suspect `EasingCurve` should be `EaseCurve`, but say la vee.
- Benchmarks show that calling `EaseFunction::Smoothstep` is still
slower than calling `smoothstep` directly.
- I think this is because the compiler refuses to inline
`EaseFunction::eval`.
- I don't see any good solution - might need a whole different
interface.

## Testing

```sh
cargo test --package bevy_math

cargo doc --package bevy_math
./target/doc/bevy_math/curve/easing/struct.EasingCurve.html

cargo bench --package benches --bench math -- easing
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Animation Make things move and change over time A-Math Fundamental domain-agnostic mathematical operations C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Trivial Nice and easy! A great choice to get started with Bevy S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expose bevy_math::curve::easing::easing_functions to public
4 participants