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.

Sign up for GitHub

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

Jump to bottom

fix animations lag on destroying #4234

Merged
merged 15 commits into from
May 25, 2018
Merged

fix animations lag on destroying #4234

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
2eea232
related to issue #4004
lucap86 May 22, 2017
4a602f4
related to issue #4004
lucap86 May 22, 2017
ad49330
Merge branch 'dev' into dev
GoodBoyDigital May 23, 2017
a09fcd8
Merge branch 'dev' of https://github.com/lucap86/pixi.js into dev
lucap86 Jun 20, 2017
05e7dfb
Merge remote-tracking branch 'upstream/dev' into dev
lucap86 Jul 28, 2017
ae6ecb8
fix graphics clear local bounds calc
lucap86 Jul 28, 2017
850fe45
fix graphics clear local bounds calc
lucap86 Jul 28, 2017
0f95065
Merge remote-tracking branch 'upstream/dev' into dev
lucap86 Aug 17, 2017
bf79eaf
revert
lucap86 Aug 17, 2017
7459ca7
fix animations lag on destroing
lucap86 Aug 17, 2017
da552b6
Merge remote-tracking branch 'upstream/dev' into dev
lucap86 Aug 20, 2017
c7a7907
fix ticker test
lucap86 Aug 20, 2017
b50ade6
fix ticker test
lucap86 Aug 20, 2017
afa34cb
fix ticker test
lucap86 Aug 20, 2017
7a57dcd
Merge branch 'dev' into dev
bigtimebuddy May 24, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/core/ticker/TickerListener.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ export default class TickerListener
}

// Redirect to the next item
const redirect = this.previous;
const redirect = this.next;

// Remove references
this.next = hard ? null : redirect;
Expand Down
32 changes: 32 additions & 0 deletions test/core/Ticker.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Copy link
Member

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.

Copy link
Contributor Author

@lucap86 lucap86 Sep 8, 2017 •
edited

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

    it('should remove itself before, still calling new item', function ()
    {
        const length = this.length();
        const listener2 = sinon.spy();
        const listener1 = sinon.spy(() =>
        {
            shared.remove(listener1);
            shared.add(listener2, null, PIXI.UPDATE_PRIORITY.LOW);

            // listener is removed right away
            expect(this.length()).to.equal(length + 1);
        });

        shared.add(listener1, null, PIXI.UPDATE_PRIORITY.NORMAL);

        shared.update();

        expect(this.length()).to.equal(length + 1);

        expect(listener2.calledOnce).to.be.true;
        expect(listener1.calledOnce).to.be.true;

        shared.remove(listener2);

        expect(this.length()).to.equal(length);
    });

suppose to have 3 listeners

    it('should remove itself before, still calling new item', function ()
    {
        const length = this.length();
        const listener3 = sinon.spy();
        const listener1 = sinon.spy();
        const listener2 = sinon.spy(() =>
        {
            shared.remove(listener2);
            shared.add(listener3, null, PIXI.UPDATE_PRIORITY.LOW);

            // listener is removed right away
            expect(this.length()).to.equal(length + 2);
        });

        shared.add(listener1, null, PIXI.UPDATE_PRIORITY.NORMAL);
        shared.add(listener2, null, PIXI.UPDATE_PRIORITY.NORMAL);

        shared.update();

        expect(this.length()).to.equal(length + 2);

        expect(listener2.calledOnce).to.be.true;
        expect(listener1.calledOnce).to.be.true;
        expect(listener3.calledOnce).to.be.true;

        shared.remove(listener2);

        expect(this.length()).to.equal(length+ 1);
    });

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

  • L3 remove itself and add L5 with priority low
  • L5 is added before L1.

after L3 emit what happen?

  • Before the fix L2 is called
  • After the fix L4 is called

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.


expect(listener2.calledOnce).to.be.true;
expect(listener1.calledOnce).to.be.true;

Expand Down Expand Up @@ -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();
});
Copy link
Member

Choose a reason for hiding this comment

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

However, I agree with this test. This should work.

});