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

Math for Curves and Interpolations (Lerp and Callmul-Rom) #1837

Closed
wants to merge 52 commits into from

Conversation

lassade
Copy link
Contributor

@lassade lassade commented Apr 6, 2021

Required by #1429, adds curves and a Lerp trait used by the animation system in the bevy_math crate;

Notes

  • I didn't know what lerp algorithm was used by Vec2, Vec3 and Vec4 so I used one with precise t == 0 and t == 1
  • lerp is by default clamped (but there is a lerp_unclampled)

Missing bits, (non block)

  • Handle and HandleUntyped should implement lerp using a step function
  • Test CurveVariable behaviour

The new Color definition make lerping and a color curve really complex and impractical (please join the discussion here), It's is some what related but doesn't block this issue;

@lassade lassade changed the title Math for Curves and Lerp and interpolations Math for Curves and Interpolations (Lerp and Callmul-Rom) Apr 6, 2021
@alice-i-cecile alice-i-cecile added C-Feature A new feature, making something new possible A-Rendering Drawing game state to the screen help wanted labels Apr 6, 2021
@alice-i-cecile alice-i-cecile added the A-Animation Make things move and change over time label Apr 7, 2021
lassade added a commit to lassade/bevy that referenced this pull request Apr 11, 2021
@inodentry
Copy link
Contributor

inodentry commented Apr 11, 2021

Have you looked at the bevy_easings crate as prior art? Would anything from there be an inspiration / to be included into bevy?

https://github.com/mockersf/bevy_extra/tree/master/bevy_easings

Also, would your animations work obsolete bevy_easings? It would be great if the use cases that crate offers could be provided by bevy internally.

Note that bevy_easings provides quite a rich selection of interpolation functions. In particular, I think having a "quadratic in/out" interpolation function would be useful, it's a common one.

@mockersf
Copy link
Member

In the scope of #1429, this is much more general than bevy_easings.
For interpolation functions, they all at their core use the lerp method, so once it's available it gets trivial to add all the functions you want

There is commented code in this PR, should it be removed?

About the TODOs, do they need to be fixed in this PR or leaved for later?

Also, some of the doc comments don't sound completely right to my non-english native ear, could someone check?

@lassade
Copy link
Contributor Author

lassade commented Apr 12, 2021

What comented code? I wan't to keep the Color stuff around because I know we are going to need them;

Not sure what TODOs, but I want to keep them for later, otherwise they will be forgoten;

Can you point out the comments that sound strange?

Note also that CurveVariableLinear and Interpolation are still a bit rough

@mockersf
Copy link
Member

mockersf commented Apr 12, 2021

I added a few comments on commented code and doc

Not sure what TODOs, but I want to keep them for later, otherwise they will be forgoten;

There are 8 TODOs, will you address them in a next PR? Or need some help with them?

@lassade
Copy link
Contributor Author

lassade commented Jul 10, 2021

Added 2 more functions in the Curve api to move common functions from curve specific impl to the trait impl;
Added a resampling function that I wrote a while back

///
/// Implementation borrowed from Piston under the MIT License: [https://github.com/PistonDevelopers/skeletal_animation]
#[inline]
pub fn fast_inv_sqrt(x: f32) -> f32 {
Copy link
Member

Choose a reason for hiding this comment

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

Alright, I did some more digging: Rust's native solution for this is faster than the approximate solution because inverse square root has a dedicated instruction these days. See: https://stackoverflow.com/a/59082210/3234149

IMO this function should be cut completely and its usages should be replaced with sqrt().recip().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch

Copy link
Contributor Author

@lassade lassade Jul 14, 2021

Choose a reason for hiding this comment

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

Do you know how the intrinsic rsqrt + a single Newton-Raphson iteration compares with the 'carmack' method?

In that same thread I found this: https://stackoverflow.com/questions/31555260, so I compiled a few possible methods here https://play.rust-lang.org/?version=stable&mode=release&edition=2018&gist=ddc10be842400c95b81a0c5fba218da6;

If my memory is not failing me, in a old bench I found that sqrtss as a very hot instruction and that made me use the carmak method, I will have to double check that ...

Copy link
Member

@james7132 james7132 Jul 14, 2021

Choose a reason for hiding this comment

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

Small note: While sqrt and 1.0/x are guaranteed under IEEE-754 2008, implementations at the hardware level may or may not be compliant. My particular game I'm working on requires animation to be in the networking simulation hotpath, and cross-platform determinism is mandatory, something that may not be uncommon to see across other networked games too. If we are going down the route of sqrt().recip(), could we use libm::sqrtf here under a feature flag? glam already has a feature flag for it, so we could pass the feature flag through for ease of use.

The 'carmack' method here does not have this issue and should be consistent across platforms.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think we're going to need to do our own modern benchmarks here to get a firm answer on what's correct for this context.

The note on determinism is important; the approach James suggests seems very reasonable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think in doing _mm_rsqrt_ss with a fallback to quake3 method for non x86 or libm / no_std but I'm not sure if the aligment requiriment will be a problem, expecially when called by the consumer code, on the nlerp side I can make things rigth I think ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@james7132 did you checked the lib impl for sqrtf here? It don't look like it's cross-plataform deterministic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I submitted a revision for this function, by the looks of it the compiler can optimize the re-alignment instructions so it should not be a problem

Copy link
Member

Choose a reason for hiding this comment

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

It doesn't look like it at a first glance and it's difficult to tell if it actually produces the same output as the intrinsic with sse disabled. If it does, we may need to reevaluate this approach to achieving cross-platform FP determinism.

The approx_rsqrt implementation here, however, we for a fact know does not produce the same bit-for-bit results between the two implementations given the error difference seen from the benchmarks.

Copy link
Contributor Author

@lassade lassade Jul 15, 2021

Choose a reason for hiding this comment

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

I suggest you to resample the curve animations, that way you can mitigate any differences between cross plataform hardware

@cart cart added the S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work label Jul 23, 2021
@DJMcNab DJMcNab removed the S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work label Aug 7, 2021
Copy link
Contributor

@Weibye Weibye left a comment

Choose a reason for hiding this comment

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

The examples/animations/smooth_curves.rs needs an entry in the examples/README.md

@alice-i-cecile alice-i-cecile added S-Needs-Review A-Math Fundamental domain-agnostic mathematical operations labels Sep 22, 2021
@james7132 james7132 mentioned this pull request Dec 31, 2021
7 tasks
@alice-i-cecile
Copy link
Member

@lassade, can you rebase this?

@james7132 is this still useful for your animation plans?

@alice-i-cecile alice-i-cecile added the X-Controversial There is active debate or serious implications around merging this PR label Apr 25, 2022
@james7132
Copy link
Member

The curves are definitely still needed. However, I think it would be better to use the Animatable trait over Lerp as it also addresses blending values a more clean way.

@lassade
Copy link
Contributor Author

lassade commented Apr 29, 2022

@lassade, can you rebase this?

I can't :(

@alice-i-cecile
Copy link
Member

Fair enough. I'll close this out; we should reuse (and credit) this code later when attempting to tackle this down the road.

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 A-Rendering Drawing game state to the screen C-Feature A new feature, making something new possible 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.