-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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 following via generalized interpolation #13613
Conversation
Great: my first question was going to be "how does this relate to |
Nice work! I support this direction and I quite like how following is implemented.
In my view, this generalizes part of
Actually, I'd like to hear from any of the people who participated in the (very extensive) Regarding |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm perplex about this change. See the comment about the trait assumptions and guarantees. The mention about using for colors is the future is also concerning, because color mixing is fairly complex when color spaces are involved, and there's more than one way to do it so wouldn't fit a trait which doesn't really guarantee anything.
I'd be fine if we provided stronger guarantees, like saying the interpolation is always linear (including for rotations and directions, so the rotation angle is linear, so slerp
). But as it stands I don't think it's useable outside the animation library without assuming a particular implementation, which defeats the abstraction.
@@ -161,3 +161,133 @@ impl NormedVectorSpace for f32 { | |||
self * self | |||
} | |||
} | |||
|
|||
/// A type that can be intermediately interpolated between two given values | |||
/// using an auxiliary linear parameter. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your comment below shows an example with a non linear t
. In general I don't see any reason why we should restrict this to be linear. Unless you mean "linear interpolation", but again, no reason for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parameter itself is "linear" in the sense that it is one-dimensional, and the second trait requirement treats the parameter itself linearly, since it involves interpolating f32
s with lerp
via interpolate
, but this doesn't mean at all that the interpolation has to be linear.
That assumption is more like a constant-velocity requirement; I'm not sure if I actually like it, but its raison d'être is that it guarantees that smooth_nudge
's decay parameter actually means something relatively precise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parameter itself is "linear" in the sense that it is one-dimensional,
That's "scalar" not "linear".
and the second trait requirement treats the parameter itself linearly, since it involves interpolating f32s with lerp via interpolate, but this doesn't mean at all that the interpolation has to be linear.
I don't understand this sentence, you sem to contradict yourself. If the implementation is lerp
then the only source of non linearity is t
so it's not linear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not contradicting myself. The condition stated in the trait documentation involves linear reparametrization: if interpolate
, then for
(Another natural condition that is more lax is that the former merely be some reparametrization of the latter.)
@@ -161,3 +161,133 @@ impl NormedVectorSpace for f32 { | |||
self * self | |||
} | |||
} | |||
|
|||
/// A type that can be intermediately interpolated between two given values |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm slightly concerned about the fact we're adding to a foundation library a trait which is very vague, and whose implementation is relied upon by another library (animation) so is making implicit assumptions in its implementation for math types (directions and rotations use slerp
, other types use lerp
). I understand the drive to uniformize and remove duplicates code, but what happens when someone wants to use that trait in another library? There's almost zero guarantee on the produced value other than some vague "interpolation" mention, so if I were a crate author I'd avoid that trait because I can't reason about it. While it's located in the animation library at least there's an assumption it's used for animation, and therefore that the interpolation is linear. But here, nothing prevents me from implementing this trait as some weird random value provided I satisfy the constraints at t=0 and t=1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To me, using lerp
/slerp
for linear and rotational types has anything to do with animation per se — those are the modes of interpolation that are mathematically natural; my expectation is that if bevy_animation
wanted something else specific to their use-cases, they would newtype them to customize the interpolation. In my opinion, the only concession here is for bool
, but perhaps the better way forward would be to just force bevy_animation
to newtype it.
As to the matter of vagueness: this is at least partially by design; for some additional context, this trait's real reason for existence is to allow for quite varied notions of interpolation, especially in light of the future Curve RFC, which presents a general framework wherein this trait allows curves to be consistently defined by discrete data. That was designed primarily with the needs of future work in bevy_math
in mind (i.e. curve geometry, RMFs, reparametrizing motion, and so on), but the idea is that bevy_animation
would also use it for curves in animation.
I'm not sure exactly what the concern about API consumers is; I would expect that calling interpolate
on vectors or directions should produce an intuitive notion of interpolation between them, and it does. If third-party libraries want to implement weird implementations of interpolation, there is nothing anyone can do to stop them regardless, but that's true of all public trait interfaces.
On some level, I do think this is almost lower-level than bevy_math
(i.e. bevy_math
is kind of more of an API consumer than a producer), but I don't think there's really any better place for it right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if bevy_animation wanted something else specific to their use-cases, they would newtype them to customize the interpolation
Probably impossible, Bevy Animation works on the types it's asked to work on by the user, not the types it wants to.
has anything to do with animation per se
Maybe, but animation relies on linear interpolation. If we changed it, everything would break. And this trait doesn't provide any assurance about linearity.
I'm not sure exactly what the concern about API consumers is; I would expect that calling interpolate on vectors or directions should produce an intuitive notion of interpolation between them, and it does.
This explanation highlights very precisely what I'm worried about, the only definition here is "intuitive" and that's subjective. There's millions of values between 0.
and 1.
, all of them can be defined as an interpretation of 0.
and 1.
via some function. If we don't specify the function, and just say "intuitive", then the trait is unusable because what's intuitive depends on the usage context, and it's not intrinsic such that we can have a single implementation.
this trait's real reason for existence is to allow for quite varied notions of interpolation
I don't understand how a trait with a single important per type can allow varied notions. The traditional way of doing non linear interpolation, like Bevy Animation, is to likely interpolate values using a non linear parameter t
. But then your other comment states that it's linear, which prevents this pattern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably impossible, Bevy Animation works on the types it's asked to work on by the user, not the types it wants to.
Actually quite easy, considering that Curves are mappable: the underlying data that is being interpolated does not need to have the same type as its output.
Maybe, but animation relies on linear interpolation. If we changed it, everything would break. And this trait doesn't provide any assurance about linearity.
No, but it makes very strong demands on interpolation nonetheless.
This explanation highlights very precisely what I'm worried about, the only definition here is "intuitive" and that's subjective. There's millions of values between
0.
and1.
, all of them can be defined as an interpretation of0.
and1.
via some function. If we don't specify the function, and just say "intuitive", then the trait is unusable because what's intuitive depends on the usage context, and it's not intrinsic such that we can have a single implementation.
"Some function" is not likely to satisfy the requirements of the trait implementation, which are quite strict.
I am referring specifically to the /// Linear interpolation of two colors within a given color space.
pub trait Mix: Sized {
/// Linearly interpolate between this and another color, by factor.
/// Factor should be between 0.0 and 1.0.
fn mix(&self, other: &Self, factor: f32) -> Self;
/// Linearly interpolate between this and another color, by factor, storing the result
/// in this color. Factor should be between 0.0 and 1.0.
fn mix_assign(&mut self, other: Self, factor: f32) {
*self = self.mix(&other, factor);
}
} |
I think we should set colorspaces and mix aside, they are out of scope of the current PR and deserve a discussion all to themselves. |
Let me clarify something about what's going on with the trait requirements here (the documentation pretty brief and not all that clear). The main idea is that interpolation should satisfy some version of "self-similarity". To make this precise, I'm going to be a little mathy. Let
The latter is the condition that is currently stated on the trait. Note that:
One reason to care about this is that the strong condition guarantees a kind of subdivision stability, while the weak one does not: if you split the interval into two parts and interpolate each separately and then join them back together in the obvious way, then the strong condition implies that you get the same thing as just interpolating the whole thing at once, while the weak condition does not. |
This is not the clarification which is need for reviewers. The one piece missing is that this change is based on the Curve RFC proposal, and most notably on a fundamental change to how Bevy Animation works, which is only proposed in a Draft PR #13105 which has only been reviewed by @nzhao95, and is missing approval from the folks involved in the current design of Bevy Animation, notably @pcwalton, as well as probably @james7132, @rparrett, @NthTensor, and @mockersf. The proposed change as I understand it flips the interpolation scheme on its head by making all interpolations go through the curve trait, moving the non-linear sampling from the parameter From: let t: f32 = sample_curve(t: f32);
let out: T = lerp(a: T, b: T, t: f32); into: let out: T = sample_curve(a: T, b: T, t: f32); This change is I think flexible for e.g. an Editor scenario where you want to have maximum control over curves and editing capabilities, including allowing things like constant velocity (since the parameter varies linearly), but has a good chance of being a bottleneck performance-wise. There's also no indication that this scheme can allow batching / SIMD implementations, in the same way that the current Bevy Animation was designed for (there were extensive discussions about current and future plans for performance-oriented improvements during the initial PR #4482). So, reiterating, as currently designed, this change breaks Bevy Animation by failing to guarantee that |
To be fair, I originally suggested this line of inquiry when reviewing the curves RFC. I'm going to upgrade this to controversial, though I'm hopeful we will still find a path forward on this through coordination between the math and animation domains. I suspect there may be some "right hand doesn't know what the left is doing" communication issues here as well. For example, let t: f32 = sample_curve(t: f32);
let out: T = lerp(a: T, b: T, t: f32); then
Some of this motivation has gotten a little lost in the formal specification, I fear. It might be easier to chat about some of this stuff in a less structured setting (the discord). |
Hello ! I did review the draft a little and came up with, I think, similar conclusions than @djeedai . I've been using bevy for a very short while so I don't want to overstep but I think there's definitely a way for math and animation to work together. |
I sat with this a while and I think that even this might be misguided (having a direct relationship with Here is a concrete proposal in the direction of ameliorating this:
This also allows us to get rid of the awkward implementation for |
I'm confident enough that the redesign is a better idea that I'm just going to close this and file a new PR with the new design later. |
# Objective Partially address #13408 Rework of #13613 Unify the very nice forms of interpolation specifically present in `bevy_math` under a shared trait upon which further behavior can be based. The ideas in this PR were prompted by [Lerp smoothing is broken by Freya Holmer](https://www.youtube.com/watch?v=LSNQuFEDOyQ). ## Solution There is a new trait `StableInterpolate` in `bevy_math::common_traits` which enshrines a quite-specific notion of interpolation with a lot of guarantees: ```rust /// A type with a natural interpolation that provides strong subdivision guarantees. /// /// Although the only required method is `interpolate_stable`, many things are expected of it: /// /// 1. The notion of interpolation should follow naturally from the semantics of the type, so /// that inferring the interpolation mode from the type alone is sensible. /// /// 2. The interpolation recovers something equivalent to the starting value at `t = 0.0` /// and likewise with the ending value at `t = 1.0`. /// /// 3. Importantly, the interpolation must be *subdivision-stable*: for any interpolation curve /// between two (unnamed) values and any parameter-value pairs `(t0, p)` and `(t1, q)`, the /// interpolation curve between `p` and `q` must be the *linear* reparametrization of the original /// interpolation curve restricted to the interval `[t0, t1]`. /// /// The last of these conditions is very strong and indicates something like constant speed. It /// is called "subdivision stability" because it guarantees that breaking up the interpolation /// into segments and joining them back together has no effect. /// /// Here is a diagram depicting it: /// ```text /// top curve = u.interpolate_stable(v, t) /// /// t0 => p t1 => q /// |-------------|---------|-------------| /// 0 => u / \ 1 => v /// / \ /// / \ /// / linear \ /// / reparametrization \ /// / t = t0 * (1 - s) + t1 * s \ /// / \ /// |-------------------------------------| /// 0 => p 1 => q /// /// bottom curve = p.interpolate_stable(q, s) /// ``` /// /// Note that some common forms of interpolation do not satisfy this criterion. For example, /// [`Quat::lerp`] and [`Rot2::nlerp`] are not subdivision-stable. /// /// Furthermore, this is not to be used as a general trait for abstract interpolation. /// Consumers rely on the strong guarantees in order for behavior based on this trait to be /// well-behaved. /// /// [`Quat::lerp`]: crate::Quat::lerp /// [`Rot2::nlerp`]: crate::Rot2::nlerp pub trait StableInterpolate: Clone { /// Interpolate between this value and the `other` given value using the parameter `t`. /// Note that the parameter `t` is not necessarily clamped to lie between `0` and `1`. /// When `t = 0.0`, `self` is recovered, while `other` is recovered at `t = 1.0`, /// with intermediate values lying between the two. fn interpolate_stable(&self, other: &Self, t: f32) -> Self; } ``` This trait has a blanket implementation over `NormedVectorSpace`, where `lerp` is used, along with implementations for `Rot2`, `Quat`, and the direction types using variants of `slerp`. Other areas may choose to implement this trait in order to hook into its functionality, but the stringent requirements must actually be met. This trait bears no direct relationship with `bevy_animation`'s `Animatable` trait, although they may choose to use `interpolate_stable` in their trait implementations if they wish, as both traits involve type-inferred interpolations of the same kind. `StableInterpolate` is not a supertrait of `Animatable` for a couple reasons: 1. Notions of interpolation in animation are generally going to be much more general than those allowed under these constraints. 2. Laying out these generalized interpolation notions is the domain of `bevy_animation` rather than of `bevy_math`. (Consider also that inferring interpolation from types is not universally desirable.) Similarly, this is not implemented on `bevy_color`'s color types, although their current mixing behavior does meet the conditions of the trait. As an aside, the subdivision-stability condition is of interest specifically for the [Curve RFC](bevyengine/rfcs#80), where it also ensures a kind of stability for subsampling. Importantly, this trait ensures that the "smooth following" behavior defined in this PR behaves predictably: ```rust /// Smoothly nudge this value towards the `target` at a given decay rate. The `decay_rate` /// parameter controls how fast the distance between `self` and `target` decays relative to /// the units of `delta`; the intended usage is for `decay_rate` to generally remain fixed, /// while `delta` is something like `delta_time` from an updating system. This produces a /// smooth following of the target that is independent of framerate. /// /// More specifically, when this is called repeatedly, the result is that the distance between /// `self` and a fixed `target` attenuates exponentially, with the rate of this exponential /// decay given by `decay_rate`. /// /// For example, at `decay_rate = 0.0`, this has no effect. /// At `decay_rate = f32::INFINITY`, `self` immediately snaps to `target`. /// In general, higher rates mean that `self` moves more quickly towards `target`. /// /// # Example /// ``` /// # use bevy_math::{Vec3, StableInterpolate}; /// # let delta_time: f32 = 1.0 / 60.0; /// let mut object_position: Vec3 = Vec3::ZERO; /// let target_position: Vec3 = Vec3::new(2.0, 3.0, 5.0); /// // Decay rate of ln(10) => after 1 second, remaining distance is 1/10th /// let decay_rate = f32::ln(10.0); /// // Calling this repeatedly will move `object_position` towards `target_position`: /// object_position.smooth_nudge(&target_position, decay_rate, delta_time); /// ``` fn smooth_nudge(&mut self, target: &Self, decay_rate: f32, delta: f32) { self.interpolate_stable_assign(target, 1.0 - f32::exp(-decay_rate * delta)); } ``` As the documentation indicates, the intention is for this to be called in game update systems, and `delta` would be something like `Time::delta_seconds` in Bevy, allowing positions, orientations, and so on to smoothly follow a target. A new example, `smooth_follow`, demonstrates a basic implementation of this, with a sphere smoothly following a sharply moving target: https://github.com/bevyengine/bevy/assets/2975848/7124b28b-6361-47e3-acf7-d1578ebd0347 ## Testing Tested by running the example with various parameters.
Objective
Partially addresses #13408.
Unify interpolation between various math types under a shared trait, allowing for common operations utilizing interpolation to be shared between them. In particular, implement a version of "smoothed following" wherein the distance between an entity and its target shrinks exponentially over time, framerate-independently.
Solution
A new trait,
Interpolate
, has been added tobevy::math::common_traits
. Its essential part looks like this:This has a blanket implementation over
V: VectorSpace
usinglerp
, and it is also implemented forQuat
and the direction types usingslerp
.Furthermore, the interpolation functionality has been extracted from
bevy_animation
'sAnimatable
trait, which instead has anAnimatable: Interpolate
bound. The remaining implementations required to make this work are thef64
vectorial types —f64
and theDVec
types — together withbool
andTransform
, all of which now haveInterpolate
implementations inbevy_math
, except forTransform
, which lives inbevy_transform
. There is no functional difference between the oldAnimatable
implementations and the new ones.Now for the "following" functionality that this facilitates:
For any type implementing
Interpolate
, this allows a value to be smoothly nudged in the direction of another, with the "speed" of this nudging determined by thedecay_rate
parameter. In particular, this can be used positionally to make an entity follow another in space, as demonstrated by thesmooth_follow
example:Screen.Recording.2024-05-31.at.3.58.07.PM.mov
However, since this is abstracted over
Interpolate
, this can similarly be applied to, e.g., make directions, rotations, and Transforms also smoothly follow a target (which is perhaps moving). In a system, this looks something like this (taken from the example):Testing
Recommend testing by running/tweaking the included example.
Changelog
Interpolate
created inbevy_math::common_traits
.interpolate
moves frombevy_animation
'sAnimatable
toInterpolate
.smooth_follow
demonstrates usage ofsmooth_nudge
.Migration Guide
Anyone creating their own implementations of
Animatable
will have to move theinterpolate
part of such implementations into an implementation ofbevy_math
'sInterpolate
.Discussion
Design decisions
Originally
smooth_nudge
was more likeinterpolate
in that it just returned the value and users had to assign it themselves or use asmooth_nudge_assign
method for that. I figured that the would-besmooth_nudge_assign
would be what users would actually want the vast majority of the time, so I made that the default behavior instead. We could probably bikeshed about this a little.Notably, there are also other ways of specifying the exponential decay rate: for example, the half-life related to the units of
delta
is another natural choice, as is the fraction of distance that remains after a single one of thedelta
units. The present version was chosen becausef32::exp
is more efficient than alternatives, but there may be reason to include the others in the future for ease of use (see future directions).Unrelatedly, a slightly unsatisfying (to me) thing about the present suite of implementation is that it has to contain
bool
in order to makebool: Animatable
continue to be possible, and for types with stepped interpolation,smooth_nudge
is basically useless. I have mixed feeling about this kind of thing being inbevy_math
in general, but I don't really see any alternative that doesn't alterbool: Animatable
.Finally, I wanted to say something about consistency with notions of distance. Naïvely, of course, there is no real notion of distance between two interpolable values; however, the second assumed constraint on
Interpolate
implies that the "interpolation distance" is kept consistent between successive calls. For example, if adecay_rate
ofln(2)
is used, then every unit of "time", the interpolation parameter will progress by half: first from0
to0.5
, then to0.75
,0.875
, and so on. In normed vector spaces, this can be interpreted in terms of actual distance, but for things like directions, this coincides with the intuition about a shrinking angle between the source and the target.(Note that, in general, this interpolation constraint is automatically satisfied by any form of interpolation which is shadowed by linear interpolation in the associated group's Lie algebra.)
Future directions
bevy_color
'sMix
should probably also be absorbed intoInterpolate
, since it is exactly the same thing, but this was intentionally excluded from this PR to limit the scope of its impact.There are probably other ergonomic ways of specifying similar behavior to
smooth_nudge
, but I decided to leave it at just the one for the sake of simplicity. Another important thing in this direction is that in the special case ofV: NormedVectorSpace
, it makes sense to allow for limitations on instantaneous velocity. This could be done by exposing a non-self
-modifying version ofsmooth_nudge
inInterpolate
and looking at the norm of the difference relative todelta
.I think it would also be good to demonstrate non-positional smooth following behavior somehow in an example (perhaps just expanding this one); e.g. following a rotation or a full
Transform
.