Skip to content
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 by Bors] - Rename play to start and add new play method that won't overwrite the existing animation if it's already playing #6350

Closed

Conversation

anchpop
Copy link
Contributor

@anchpop anchpop commented Oct 23, 2022

Objective

  • You usually want to say that a given animation should be playing, doing nothing if it's already playing.

Solution


Changelog

Changed

AnimationPlayer::play will now not restart the animation if it's already playing

Added

An AnimationPlayer ::start method, which has the old behavior of play

Migration guide

  • If you were using play to restart an animation that was already playing, that functionality has been moved to start. Now, play won't have any effect if the requested animation is already playing.

@anchpop anchpop force-pushed the @anchpop/play_unless_already_playing branch from 249734a to f525419 Compare October 23, 2022 17:17
@alice-i-cecile
Copy link
Member

Bikeshed: can we call this method start_playing?

@alice-i-cecile alice-i-cecile added C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Animation Make things move and change over time labels Oct 23, 2022
@MrGVSV
Copy link
Member

MrGVSV commented Oct 23, 2022

Here's my bikeshed: maybe call this play and change the current play method to start?

@anchpop
Copy link
Contributor Author

anchpop commented Oct 23, 2022

@MrGVSV I really like that, @alice-i-cecile what do you think?

@alice-i-cecile
Copy link
Member

Yep, I'm on board with that change.

@anchpop anchpop force-pushed the @anchpop/play_unless_already_playing branch from f525419 to 005508e Compare October 23, 2022 23:47
@anchpop
Copy link
Contributor Author

anchpop commented Oct 23, 2022

@alice-i-cecile ok, done!

@@ -115,14 +115,22 @@ impl Default for AnimationPlayer {

impl AnimationPlayer {
/// Start playing an animation, resetting state of the player
pub fn play(&mut self, handle: Handle<AnimationClip>) -> &mut Self {
pub fn start(&mut self, handle: Handle<AnimationClip>) -> &mut Self {
Copy link
Member

Choose a reason for hiding this comment

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

Does this change affect other code, such as any examples?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tested the examples and they work the same as before. AFAICT none of them call play on an animation that's already playing.

crates/bevy_animation/src/lib.rs Outdated Show resolved Hide resolved
@anchpop anchpop force-pushed the @anchpop/play_unless_already_playing branch from 005508e to e0274dc Compare October 24, 2022 04:06
Copy link
Contributor

@Pietrek14 Pietrek14 left a comment

Choose a reason for hiding this comment

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

Except for the small thing with the docs, it looks good to me.

*self = Self {
animation_clip: handle,
..Default::default()
};
self
}

/// Like `start`, but does nothing if the requested animation is already playing.
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't start be wrapped in a square brackets to link the method?

Copy link
Member

Choose a reason for hiding this comment

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

Could be wrong but it might even need a path to the method (i.e. [`start`](Self::start))

Copy link
Member

Choose a reason for hiding this comment

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

Correct :)

Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Once you add that doc link, update the PR title and add a migration guide, this LGTM.

@alice-i-cecile alice-i-cecile added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Oct 24, 2022
@anchpop anchpop force-pushed the @anchpop/play_unless_already_playing branch from e0274dc to 2d63bf4 Compare October 24, 2022 16:04
@anchpop anchpop changed the title Add play_unless_already_playing method to AnimationPlayer Rename play to start and add new play method that won't overwrite the existing animation if it's already playing Oct 24, 2022
@anchpop
Copy link
Contributor Author

anchpop commented Oct 24, 2022

@alice-i-cecile done!

@anchpop anchpop force-pushed the @anchpop/play_unless_already_playing branch from 2d63bf4 to 4205190 Compare October 24, 2022 16:31
@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Oct 24, 2022
*self = Self {
animation_clip: handle,
..Default::default()
};
self
}

/// Like [`start`](Self::start), but does nothing if the requested animation is already playing.
Copy link
Member

Choose a reason for hiding this comment

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

this doc is not self-explaining, could you reformulate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you think of the new doc?

Copy link
Member

Choose a reason for hiding this comment

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

Much better IMO.

Copy link
Member

Choose a reason for hiding this comment

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

good for me, thanks 👍

@anchpop anchpop force-pushed the @anchpop/play_unless_already_playing branch from 4205190 to 924544e Compare October 24, 2022 17:41
@anchpop anchpop force-pushed the @anchpop/play_unless_already_playing branch from 924544e to 9549e46 Compare October 24, 2022 17:53
Copy link
Contributor

@Pietrek14 Pietrek14 left a comment

Choose a reason for hiding this comment

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

I like the new doc :)

@alice-i-cecile
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Oct 24, 2022
…te the existing animation if it's already playing (#6350)

# Objective

- You usually want to say that a given animation *should* be playing, doing nothing if it's already playing.

## Solution

- Rename play to start and add new play method that won't overwrite the existing animation if it's already playing #6350

---

## Changelog

### Changed

`AnimationPlayer::play` will now not restart the animation if it's already playing

### Added

An `AnimationPlayer ::start` method, which has the old behavior of `play`

## Migration guide

- If you were using `play` to restart an animation that was already playing, that functionality has been moved to `start`. Now, `play` won't have any effect if the requested animation is already playing.
@bors bors bot changed the title Rename play to start and add new play method that won't overwrite the existing animation if it's already playing [Merged by Bors] - Rename play to start and add new play method that won't overwrite the existing animation if it's already playing Oct 24, 2022
@bors bors bot closed this Oct 24, 2022
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 28, 2022
…te the existing animation if it's already playing (bevyengine#6350)

# Objective

- You usually want to say that a given animation *should* be playing, doing nothing if it's already playing.

## Solution

- Rename play to start and add new play method that won't overwrite the existing animation if it's already playing bevyengine#6350

---

## Changelog

### Changed

`AnimationPlayer::play` will now not restart the animation if it's already playing

### Added

An `AnimationPlayer ::start` method, which has the old behavior of `play`

## Migration guide

- If you were using `play` to restart an animation that was already playing, that functionality has been moved to `start`. Now, `play` won't have any effect if the requested animation is already playing.
Pietrek14 pushed a commit to Pietrek14/bevy that referenced this pull request Dec 17, 2022
…te the existing animation if it's already playing (bevyengine#6350)

# Objective

- You usually want to say that a given animation *should* be playing, doing nothing if it's already playing.

## Solution

- Rename play to start and add new play method that won't overwrite the existing animation if it's already playing bevyengine#6350

---

## Changelog

### Changed

`AnimationPlayer::play` will now not restart the animation if it's already playing

### Added

An `AnimationPlayer ::start` method, which has the old behavior of `play`

## Migration guide

- If you were using `play` to restart an animation that was already playing, that functionality has been moved to `start`. Now, `play` won't have any effect if the requested animation is already playing.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
…te the existing animation if it's already playing (bevyengine#6350)

# Objective

- You usually want to say that a given animation *should* be playing, doing nothing if it's already playing.

## Solution

- Rename play to start and add new play method that won't overwrite the existing animation if it's already playing bevyengine#6350

---

## Changelog

### Changed

`AnimationPlayer::play` will now not restart the animation if it's already playing

### Added

An `AnimationPlayer ::start` method, which has the old behavior of `play`

## Migration guide

- If you were using `play` to restart an animation that was already playing, that functionality has been moved to `start`. Now, `play` won't have any effect if the requested animation is already playing.
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 M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide 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