-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Implement TryStableInterpolate.
#21633
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
base: main
Are you sure you want to change the base?
Conversation
|
@LikeLakers2 Tried to add you as a reviewer but could not for some reason. |
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.
No worries. I hope I didn't frustrate you too much in the issue you made!
| impl<T: StableInterpolate> TryStableInterpolate for T { | ||
| fn try_interpolate_stable(&self, other: &Self, t: f32) -> Result<Self, InterpolationError> { | ||
| Ok(self.interpolate_stable(other, t)) | ||
| } | ||
| } |
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.
Huh, so this ended up working? I'm curious what was different about how you initially tested it, that caused the error you were reporting.
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 think it has to do with the fact that the impls are in the same module as the target types instead of being in the same module as the trait definition. But to be honest, I'm not really sure why it works.
|
Your PR caused a change in the graphical output of an example or rendering test. This might be intentional, but it could also mean that something broke! If it's expected, please add the M-Deliberate-Rendering-Change label. If this change seems unrelated to your PR, you can consider updating your PR to target the latest main branch, either by rebasing or merging main into it. |
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.
Just a couple more nitpicks. These ones could reasonably be skipped if desired.
|
|
||
| /// Error produced when the values to be interpolated are not in the same units. | ||
| #[derive(Clone, Debug)] | ||
| pub struct MismatchedUnitsError; |
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.
You'll probably want to impl Error for MismatchedUnitsError. Error requires Display also be implemented, so I'll also include such an impl here:
impl core::error::Error for MismatchedUnitsError {}
impl core::fmt::Display for MismatchedUnitsError {
fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
write!(f, "cannot interpolate between two values of different units")
}
}| /// A trait that indicates that a value _may_ be interpolable via [`StableInterpolate`]. An | ||
| /// interpolation may fail if the values have different units - for example, attempting to | ||
| /// interpolate between [`Val::Px`] and [`Val::Percent`] will fail, | ||
| /// even though they are the same Rust type. | ||
| /// | ||
| /// The motivating case for this trait is animating UI entities with [`Val`] | ||
| /// properties, which, because they are enums, cannot be interpolated in the normal way. This same | ||
| /// concept can be extended to other types as well. | ||
| /// | ||
| /// Fallible interpolation can be used for animated transitions, which can be set up to fail | ||
| /// gracefully if there's a mismatch of units. For example, the a transition could smoothly | ||
| /// go from `Val::Px(10)` to `Val::Px(20)`, but if the user attempts to go from `Val::Px(10)` to | ||
| /// `Val::Percent(10)`, the animation player can detect the failure and simply snap to the new | ||
| /// value without interpolating. | ||
| /// | ||
| /// An animation clip system can incorporate fallible interpolation to support a broad set of | ||
| /// sequenced parameter values. This can include numeric types, which always interpolate, | ||
| /// enum types, which may or may not interpolate depending on the units, and non-interpolable | ||
| /// types, which always jump immediately to the new value without interpolation. This meaas, for | ||
| /// example, that you can have an animation track whose value type is a boolean or a string. | ||
| /// | ||
| /// Interpolation for simple number and coordinate types will always succeed, as will any type | ||
| /// that implements [`StableInterpolate`]. Types which have different variants such as | ||
| /// [`Val`] and [`Color`] will only fail if the units are different. | ||
| /// Note that [`Color`] has its own, non-fallible mixing methods, but those entail | ||
| /// automatically converting between different color spaces, and is both expensive and complex. | ||
| /// [`TryStableInterpolate`] is more conservative, and doesn't automatically convert between | ||
| /// color spaces. This produces a color interpolation that has more predictable performance. | ||
| /// | ||
| /// [`Val::Px`]: https://docs.rs/bevy/latest/bevy/ui/enum.Val.html | ||
| /// [`Val::Percent`]: https://docs.rs/bevy/latest/bevy/ui/enum.Val.html | ||
| /// [`Val`]: https://docs.rs/bevy/latest/bevy/ui/struct.enum.html | ||
| /// [`Color`]: https://docs.rs/bevy/latest/bevy/color/enum.Color.html |
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 really appreciate the extensive documentation here, but now that the error type can be customized by the user, I do think it focuses a bit too much on mismatched units.
Here's how I might rewrite this documentation:
/// A type with a natural interpolation that provides strong subdivision guarantees, similar to
/// [`StableInterpolation`], but which cannot be guaranteed to interpolate correctly between two
/// arbitrary values.
///
/// The motivating case for this trait is animating UI entities with [`Val`] properties. Because
/// different `Val` instances may be of different incompatible variants, there is no guarantee that
/// we can be interpolate between two `Val`s. The same concept can be extended to other types as
/// well.
///
/// Bevy's default animation system handles erroring interpolations by snapping to the new value
/// without interpolating.
///
/// [`Val`]: https://docs.rs/bevy/latest/bevy/ui/enum.Val.html
It's not a perfect rewrite, and could probably use some work, but I hope it helps.
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.
However, putting aside those nitpicks, I think this PR looks good. So this gets my approval.
Co-authored-by: MichiRecRoom <1008889+LikeLakers2@users.noreply.github.com>
Co-authored-by: MichiRecRoom <1008889+LikeLakers2@users.noreply.github.com>
Co-authored-by: MichiRecRoom <1008889+LikeLakers2@users.noreply.github.com>
Co-authored-by: François Mockers <francois.mockers@vleue.com>
Fixes: #20579
Testing
Note: Because docs.rs was down, I could not validate the doc link urls.