Skip to content

Adding Lerp trait and common implementations#8189

Closed
Kjolnyr wants to merge 15 commits intobevyengine:mainfrom
Kjolnyr:lerp-trait
Closed

Adding Lerp trait and common implementations#8189
Kjolnyr wants to merge 15 commits intobevyengine:mainfrom
Kjolnyr:lerp-trait

Conversation

@Kjolnyr
Copy link
Contributor

@Kjolnyr Kjolnyr commented Mar 24, 2023

Objective

  • Being able to use linear interpolation with common types like f32 / f64 but also common bevy related types like Color
  • Being able to easily implement linear interpolation for custom types

Solution

  • add a new trait Lerp to bevy_math which define a linear interpolation function as show below
  • Implement the Lerp trait to f32 and f64
  • Implement the Lerp trait to Color

@github-actions
Copy link
Contributor

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

@Kjolnyr
Copy link
Contributor Author

Kjolnyr commented Mar 24, 2023

I've put my example in the transforms/ folder, though I'm not sure this is where it belongs, since it shows how to lerp with colors as well.

@alice-i-cecile alice-i-cecile added C-Feature A new feature, making something new possible A-Math Fundamental domain-agnostic mathematical operations labels Mar 24, 2023
@alice-i-cecile
Copy link
Member

I've put my example in the transforms/ folder, though I'm not sure this is where it belongs, since it shows how to lerp with colors as well.

This probably belongs with animation :)

@james7132
Copy link
Member

james7132 commented Mar 24, 2023

Wanted to quickly make a few notes:

  • There's an active PR looking to add an Animatable trait that includes linear interpolation as a part of its definition.
  • Color interpolation is only well defined between colors of the same color space. Color conversions are inevitably lossy (and computationally expensive compared to the actual interpolation). There has been multiple attempts at interpolation of color, and most of them do not work well with the current type definition.

With these in mind, we may want to carefully approach how we structure the Lerp trait, as it will inevitably interact with both.

@james7132 james7132 added A-Rendering Drawing game state to the screen A-Animation Make things move and change over time labels Mar 24, 2023
@james7132 james7132 added the X-Needs-SME This type of work requires an SME to approve it. label Mar 24, 2023
@Kjolnyr
Copy link
Contributor Author

Kjolnyr commented Mar 24, 2023

Wanted to quickly make a few notes:

* There's an active PR looking to add an Animatable trait that includes linear interpolation as a part of its definition.

* Color interpolation is only well defined between colors of the same color space. Color conversions are inevitably lossy (and computationally expensive compared to the actual interpolation). There has been multiple attempts at interpolation of color, and most of them do not work well with the current type definition.

With these in mind, we may want to carefully approach how we structure the Lerp trait, as it will inevitably interact with both.

My initial idea before lerping colors was to have a lerp() fonction for f32. A trait like Animatable would be relevant in that case.

@james7132 Should I therefore remove my Color interpolation since it can't be accurate between color spaces? We can still keep the Lerp trait to let the user implement custom types.

@james7132
Copy link
Member

Removing it would make this PR significantly less controversial, and allow us to address it completely at a later time.

I still want to give this a full review and ensure it can work well enough with the Animatable trait down the line though.

@Kjolnyr
Copy link
Contributor Author

Kjolnyr commented Mar 24, 2023

The Lerp impl for Color has been removed, and the example adapted.

Also, @harudagondi, I've made the example less contrived

@github-actions
Copy link
Contributor

You added a new example but didn't add metadata for it. Please update the root Cargo.toml file.

@Kjolnyr Kjolnyr requested review from harudagondi and removed request for aevyrie and james7132 March 24, 2023 14:53
@Kjolnyr
Copy link
Contributor Author

Kjolnyr commented Mar 24, 2023

@james7132 @aevyrie Sorry my github PR skills are lacking. I've removed you as reviewers by accident

@alice-i-cecile alice-i-cecile requested a review from aevyrie March 24, 2023 15:38
@komadori
Copy link
Contributor

This definition appears similar to the widely used interpolation crate. Has the trade-off between a Bevy-specific Lerp trait and one shared by the broader ecosystem been discussed?

I don't feel strongly either way, but it seems like an important decision point.

@Kjolnyr
Copy link
Contributor Author

Kjolnyr commented Mar 26, 2023

It's indeed close to the interpolation crate, however, The crate uses references for its lerp() function whereas I opted for Copy. Since we are talking about stdlib types, it's just more convenient and is in my opinion a bit more leaning to Bevy's philosophy.

We could still have some kind of LerpRef trait, or integrate the interpolation crate to have a ref based lerp() function.

Copy link
Member

@aevyrie aevyrie left a comment

Choose a reason for hiding this comment

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

Is it worth discussing the overlap of this with CubicSegment<Vec2>::ease()? You can technically define a linear shaping function and use it to lerp between two values. Obviously this is limited to f32 and is less efficient than this lerp implementation, but does any consideration need to be made to ensure we don't have multiple APIs that work slightly differently but accomplish the same thing?

@Kjolnyr
Copy link
Contributor Author

Kjolnyr commented Mar 29, 2023

Is it worth discussing the overlap of this with CubicSegment<Vec2>::ease()? You can technically define a linear shaping function and use it to lerp between two values. Obviously this is limited to f32 and is less efficient than this lerp implementation, but does any consideration need to be made to ensure we don't have multiple APIs that work slightly differently but accomplish the same thing?

I reckon one of bevy's core value is to be ergonomic, and I believe this lerp implementation is much more ergonomic than lerping using CubicSegment<P>.

Even though they accomplish the same thing, I think we have to take in account ergonomics and efficiency. It might be noticeable when lerping in a query over a few 1000s entries.

@aevyrie
Copy link
Member

aevyrie commented Apr 1, 2023

Yes, I agree with you, but my point is that this is not ergonomic. Providing multiple ways of interpolation might seem odd or confusing to users. The question I'm asking is: "Is this potential confusion enough of an issue to warrant changing the public API?"

The answer might be "No", and this is a non-issue. I'm not certain I know what the answer is though.

@Kjolnyr
Copy link
Contributor Author

Kjolnyr commented Apr 18, 2023

Where are we on this? Should this PR be closed in favor of #4482?

Copy link
Contributor Author

@Kjolnyr Kjolnyr left a comment

Choose a reason for hiding this comment

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

I've updated it already

@Kjolnyr Kjolnyr requested a review from james7132 April 18, 2023 10:10
@alice-i-cecile
Copy link
Member

I think my preference is to close, but I'll defer to @james7132 on how this fits together.

@tim-blackbird
Copy link
Contributor

The next version of https://github.com/bitshifter/glam-rs will include a FloatExt trait that provides lerp, inverse_lerp and remap methods for f32 and f64.

@alice-i-cecile
Copy link
Member

Closing in favor of just using glam's extension methods here :)

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-Needs-SME This type of work requires an SME to approve it.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants