-
-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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 animations lag on destroying #4234
Merged
Merged
Changes from 14 commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
2eea232
related to issue #4004
lucap86 4a602f4
related to issue #4004
lucap86 ad49330
Merge branch 'dev' into dev
GoodBoyDigital a09fcd8
Merge branch 'dev' of https://github.com/lucap86/pixi.js into dev
lucap86 05e7dfb
Merge remote-tracking branch 'upstream/dev' into dev
lucap86 ae6ecb8
fix graphics clear local bounds calc
lucap86 850fe45
fix graphics clear local bounds calc
lucap86 0f95065
Merge remote-tracking branch 'upstream/dev' into dev
lucap86 bf79eaf
revert
lucap86 7459ca7
fix animations lag on destroing
lucap86 da552b6
Merge remote-tracking branch 'upstream/dev' into dev
lucap86 c7a7907
fix ticker test
lucap86 b50ade6
fix ticker test
lucap86 afa34cb
fix ticker test
lucap86 7a57dcd
Merge branch 'dev' into dev
bigtimebuddy File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -298,6 +298,11 @@ describe('PIXI.ticker.Ticker', function () | |
|
||
expect(this.length()).to.equal(length + 1); | ||
|
||
expect(listener2.called).to.be.false; | ||
expect(listener1.calledOnce).to.be.true; | ||
|
||
shared.update(); | ||
|
||
expect(listener2.calledOnce).to.be.true; | ||
expect(listener1.calledOnce).to.be.true; | ||
|
||
|
@@ -369,4 +374,31 @@ describe('PIXI.ticker.Ticker', function () | |
ticker.add(listener2, null, PIXI.UPDATE_PRIORITY.LOW); | ||
ticker.start(); | ||
}); | ||
|
||
it('should Ticker call destroyed listener "next" pointer after destroy', function (done) | ||
{ | ||
const ticker = new Ticker(); | ||
|
||
const listener1 = sinon.spy(); | ||
const listener2 = sinon.spy(() => | ||
{ | ||
ticker.remove(listener2); | ||
}); | ||
|
||
const listener3 = sinon.spy(() => | ||
{ | ||
ticker.stop(); | ||
|
||
expect(listener1.calledOnce).to.be.true; | ||
expect(listener2.calledOnce).to.be.true; | ||
expect(listener3.calledOnce).to.be.true; | ||
done(); | ||
}); | ||
|
||
ticker.add(listener1, null, PIXI.UPDATE_PRIORITY.HIGH); | ||
ticker.add(listener2, null, PIXI.UPDATE_PRIORITY.HIGH); | ||
ticker.add(listener3, null, PIXI.UPDATE_PRIORITY.HIGH); | ||
|
||
ticker.start(); | ||
}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. However, I agree with this test. This should work. |
||
}); |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you add a new listener of a lower priority the current listener, it should get called. Just want to point out that you have modified the existing test to change that functionality here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bigtimebuddy I think that this test was failing also before the fix if you use more than 2 listeners for the test
i have not tried yet because i'm not at home
this is the original test
suppose to have 3 listeners
does this worked before fix?
How to call a listener if is added in an already "parsed" position?
HEAD
L1 -> priority normal
L2 -> priority normal
L3 -> priority normal
L4 -> priority normal
suppose that
after L3 emit what happen?
L5 is not called neither before nor after the fix
I think that if we want that L5 is called in the current cycle we have to track whether or not a listener has been called and do another check at the end of the cycle in order to check if new uncalled listeners are been added.