Skip to content

Remove TransformCurve #15598

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

Merged
merged 1 commit into from
Oct 2, 2024

Conversation

mweatherley
Copy link
Contributor

@mweatherley mweatherley commented Oct 2, 2024

Objective

It is somewhat unlikely we will actually be able to support TransformCurve (introduced in #15434) after the AnimationGraph evaluation order changes in the immediate future. This is because correctly blending overlapping animation properties is nontrivial, and Transform overlaps with all of its own fields. We could still potentially create something like this in the future, but it's likely to require significant design and implementation work. By way of contrast, the single-property wrappers TranslationCurve, ScaleCurve, and RotationCurve should work perfectly fine, since they are non-overlapping.

In this version release, creating TransformCurve in userspace is also quite easy if desired (see the deletions from this PR).

Solution

Delete TransformCurve.

Migration Guide

There is no released version that contains this, but we should make sure that TransformCurve is excluded from the release notes for #15434 if we merge this pull request.

@mweatherley mweatherley added C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Animation Make things move and change over time D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Oct 2, 2024
@mweatherley mweatherley added this to the 0.15 milestone Oct 2, 2024
@mweatherley
Copy link
Contributor Author

I'm open to being persuaded off of this, but I figured that if we should remove this, we have to do it soon, which is why I submitted this. :)

@mweatherley mweatherley 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 Oct 2, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Oct 2, 2024
Merged via the queue into bevyengine:main with commit 587a508 Oct 2, 2024
32 checks passed
robtfm pushed a commit to robtfm/bevy that referenced this pull request Oct 4, 2024
# Objective

It is somewhat unlikely we will actually be able to support
`TransformCurve` (introduced in bevyengine#15434) after the `AnimationGraph`
evaluation order changes in the immediate future. This is because
correctly blending overlapping animation properties is nontrivial, and
`Transform` overlaps with all of its own fields. We could still
potentially create something like this in the future, but it's likely to
require significant design and implementation work. By way of contrast,
the single-property wrappers `TranslationCurve`, `ScaleCurve`, and
`RotationCurve` should work perfectly fine, since they are
non-overlapping.

In this version release, creating `TransformCurve` in userspace is also
quite easy if desired (see the deletions from this PR).

## Solution

Delete `TransformCurve`. 

## Migration Guide

There is no released version that contains this, but we should make sure
that `TransformCurve` is excluded from the release notes for bevyengine#15434 if
we merge this pull request.
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 C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Straightforward Simple bug fixes and API improvements, docs, test and examples 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.

3 participants