Skip to content

add more Curve adaptors#14794

Merged
alice-i-cecile merged 15 commits intobevyengine:mainfrom
JasmineLowen:more-curve-adaptors
Sep 30, 2024
Merged

add more Curve adaptors#14794
alice-i-cecile merged 15 commits intobevyengine:mainfrom
JasmineLowen:more-curve-adaptors

Conversation

@JasmineLowen
Copy link
Copy Markdown
Contributor

@JasmineLowen JasmineLowen commented Aug 17, 2024

Objective

This implements another item on the way to complete the Curves implementation initiative

Citing @mweatherley

Curve adaptors for making a curve repeat or ping-pong would be useful.

This adds three widely applicable adaptors:

  • ReverseCurve "plays" the curve backwards
  • RepeatCurve to repeat the curve for n times where n in [0,inf)
  • ForeverCurve which extends the curves domain to EVERYWHERE
  • PingPongCurve (name wip (?)) to chain the curve with it's reverse. This would be achievable with ReverseCurve and ChainCurve, but it would require the use of by_ref which can be restrictive in some scenarios where you'd rather just consume the curve. Users can still create the same effect by combination of the former two, but since this will be most likely a very typical adaptor we should also provide it on the library level. (Why it's typical: you can create a single period of common waves with it pretty easily, think square wave (= pingpong + step), triangle wave ( = pingpong + linear), etc.)
  • ContinuationCurve which chains two curves but also makes sure that the samples of the second curve are translated so that sample(first.end) == sample(second.start)

Solution

Implement the adaptors above. (More suggestions are welcome!)

Testing

  • add simple tests. One per adaptor

@JasmineLowen
Copy link
Copy Markdown
Contributor Author

I'm not surve about ping pong adaptors as that's basically just a combination of what we already have: Chain the curve with it's inverse.

@alice-i-cecile alice-i-cecile added C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Math Fundamental domain-agnostic mathematical operations S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Aug 17, 2024
@alice-i-cecile
Copy link
Copy Markdown
Member

Could the ping-pong adapter work as an example for how to make your own?

@IQuick143
Copy link
Copy Markdown
Contributor

I could see ping-pong being useful if it also did the repeating.
So it would do:
A -> A.chain(A.reverse()).repeat()

So it would for example produce a triangle-wave from a line segment.

@JasmineLowen
Copy link
Copy Markdown
Contributor Author

Tbh I think it's worth having a separate adaptor for ping pong after all. The issue with the "combining existing adaptors" is that it is kind of restricting since it's only possible to do this with by_ref which might not always be desirable.

@JasmineLowen JasmineLowen marked this pull request as ready for review August 18, 2024 10:39
@mweatherley
Copy link
Copy Markdown
Contributor

I could see ping-pong being useful if it also did the repeating. So it would do: A -> A.chain(A.reverse()).repeat()

So it would for example produce a triangle-wave from a line segment.

This is what I was imagining, yeah.

@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 Aug 19, 2024
Copy link
Copy Markdown
Contributor

@mweatherley mweatherley left a comment

Choose a reason for hiding this comment

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

I'll give this another look once the reversing issues are addressed. :)

@IQuick143 IQuick143 self-requested a review August 20, 2024 06:54
@JasmineLowen JasmineLowen changed the title add RepeatCurve and ReverseCurve Curve adaptors add more Curve adaptors Aug 21, 2024
Copy link
Copy Markdown
Contributor

@mweatherley mweatherley left a comment

Choose a reason for hiding this comment

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

Just a few more small things. Looks great otherwise!

@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Sep 27, 2024
@alice-i-cecile
Copy link
Copy Markdown
Member

@RobWalt merging once merge conflicts are resolved :)

@JasmineLowen
Copy link
Copy Markdown
Contributor Author

@RobWalt merging once merge conflicts are resolved :)

Ahhhh, thanks for the ping! Would've missed that for another few days probably 🙈

JasmineLowen and others added 6 commits September 27, 2024 16:17
Co-authored-by: eckz <567737+eckz@users.noreply.github.com>
Co-authored-by: Matty <2975848+mweatherley@users.noreply.github.com>
Co-authored-by: IQuick 143 <IQuick143cz@gmail.com>
Thanks for the continued support and reviews!

Co-authored-by: Matty <weatherleymatthew@gmail.com>
@alice-i-cecile
Copy link
Copy Markdown
Member

New lints are failing now :)

@alice-i-cecile
Copy link
Copy Markdown
Member

If you have time to format this in the next 15 hours or so that would be appreciated, otherwise I'll do it during my merge train.

@alice-i-cecile alice-i-cecile added this to the 0.15 milestone Sep 29, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Sep 30, 2024
Merged via the queue into bevyengine:main with commit ff30848 Sep 30, 2024
@JasmineLowen JasmineLowen deleted the more-curve-adaptors branch October 1, 2024 04:05
robtfm pushed a commit to robtfm/bevy that referenced this pull request Oct 4, 2024
# Objective

This implements another item on the way to complete the `Curves`
implementation initiative

Citing @mweatherley 

> Curve adaptors for making a curve repeat or ping-pong would be useful.

This adds three widely applicable adaptors:

- `ReverseCurve` "plays" the curve backwards
- `RepeatCurve` to repeat the curve for `n` times where `n` in `[0,inf)`
- `ForeverCurve` which extends the curves domain to `EVERYWHERE`
- `PingPongCurve` (name wip (?)) to chain the curve with it's reverse.
This would be achievable with `ReverseCurve` and `ChainCurve`, but it
would require the use of `by_ref` which can be restrictive in some
scenarios where you'd rather just consume the curve. Users can still
create the same effect by combination of the former two, but since this
will be most likely a very typical adaptor we should also provide it on
the library level. (Why it's typical: you can create a single period of
common waves with it pretty easily, think square wave (= pingpong +
step), triangle wave ( = pingpong + linear), etc.)
- `ContinuationCurve` which chains two curves but also makes sure that
the samples of the second curve are translated so that
`sample(first.end) == sample(second.start)`

## Solution

Implement the adaptors above. (More suggestions are welcome!)

## Testing

- [x] add simple tests. One per adaptor

---------

Co-authored-by: eckz <567737+eckz@users.noreply.github.com>
Co-authored-by: Matty <2975848+mweatherley@users.noreply.github.com>
Co-authored-by: IQuick 143 <IQuick143cz@gmail.com>
Co-authored-by: Matty <weatherleymatthew@gmail.com>
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-Math Fundamental domain-agnostic mathematical operations C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants