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

Fix for animated sprite not stopping #7211

Merged
merged 4 commits into from
Feb 7, 2021

Conversation

Owlzy
Copy link
Contributor

@Owlzy Owlzy commented Jan 29, 2021

Description of change

Fixes issue where animated sprite continues to play even when _playing = false

Pre-Merge Checklist
  • Lint process passed (npm run lint)

@bigtimebuddy
Copy link
Member

Thanks for this @Owlzy. Can you provide an example of this not work so we can verify?

@Owlzy
Copy link
Contributor Author

Owlzy commented Jan 30, 2021

Thanks for this @Owlzy. Can you provide an example of this not work so we can verify?

Sure. Having made a playground I now see it is specific to autoUpdate = false.

In this playground can see that animation plays regardless of calling stop(). If loop is true it will continue looping, or if not looping it will play till the last frame and stop.

https://www.pixiplayground.com/#/edit/1EdyVzCi9_Djk1FbjhyGw

@bigtimebuddy
Copy link
Member

bigtimebuddy commented Jan 30, 2021

Got it, this example is helpful for context. At first, it seemed a little redundant to add this check, but now I see why because you are enabling autoUpdate.

I fine with this, but additional, I think it might be nice to add play and stop events. So the user could know when to remove from the ticker (which is what happens if autoUpdate is true). For example:

const sprite = new AnimatedSprite();
sprite.autoUpdate = false;
sprite.on('play', () => ticker.add(sprite.update, sprite) });
sprite.on('stop', () => ticker.remove(sprite.update, sprite) });

Play and stop would allow you to remove from the ticker or event loop and remove that call all together, which means you wouldn't need to defensively add the check in update. I think there's value in both, just a thought.

Copy link
Member

@bigtimebuddy bigtimebuddy left a comment

Choose a reason for hiding this comment

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

Can you please fix the linting? Otherwise, I'm fine with this.

@Owlzy
Copy link
Contributor Author

Owlzy commented Feb 1, 2021

Can you please fix the linting? Otherwise, I'm fine with this.

Sorry thought I fixed that already. Should be fixed in the latest commit.

@codecov
Copy link

codecov bot commented Feb 7, 2021

Codecov Report

Merging #7211 (99bfd39) into dev (851734e) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##               dev     #7211   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           17        17           
  Lines          699       699           
=========================================
  Hits           699       699           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 851734e...7e95c48. Read the comment docs.

@bigtimebuddy bigtimebuddy added the ✅ Ready To Merge Helpful when issues are in the queue waiting to get merged. This means the PR is completed and has t label Feb 7, 2021
@bigtimebuddy bigtimebuddy merged commit edb2672 into pixijs:dev Feb 7, 2021
@bigtimebuddy
Copy link
Member

Thank you @Owlzy 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✅ Ready To Merge Helpful when issues are in the queue waiting to get merged. This means the PR is completed and has t
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants