fix animations lag on destroying

[lucap86]

I think this is a typo
actually cause lags when stopping animations during ticker update

let A,B,C to be TickerListener

----- Before fix -----

A
B <-- emit + destroy
C <--- before destroy this is "next" candidate

after TickerListener.destroy

A <---NEXT
[B] <---destroy over B set next = A that will be updated twice
C

----- After fix -----

A
B <-- emit + destroy
C <--- before destroy this is "next" candidate

after destroy

A
[B] <---destroy over B set next = C that has never been updated in this tick cycle
C <---NEXT

    TickerListener.prototype.emit = function emit(deltaTime) {
        if (this.fn) {
            if (this.context) {
                this.fn.call(this.context, deltaTime);
            } else {
                this.fn(deltaTime);
            }
        }
        // NOTE: 
        //if this.context is destroyed in this.fn call
        // TickerListener.prototype.destroy is called too.
        // after destroy this.next actually is this.prev that will be called twice
        var redirect = this.next;

        if (this.once) {
            this.destroy(true);
        }

        // Soft-destroying should remove
        // the next reference
        if (this._destroyed) {
            this.next = null;
        }

        return redirect;
    };

this bug also cause this.previous to be called twice

[englercj]

Can you add a test that shows the failure case? The test should fail before your changes and succeed afterward. I think that will make it a lot clearer what problem you are fixing, and prevent regressions in the future.

[lucap86]

Here it is
focus on the club

(updated with pixi 4.5.4)
testAnim.zip

[englercj]

Sorry I think you misunderstood. I meant writing a unit test that fails before applying your fix, and succeeds after applying it. That way if we break it in the future, the test will fail and let us know.

[lucap86]

Oh, ok..

   it('Ticker should call destroyed "next" TickerListener 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();
    });

[englercj]

Commit the test so that it goes in with the merge and gets run by the CI. Also it looks like your change is failing another test, can you look into that?

[bigtimebuddy]

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.

[lucap86]

@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.

[bigtimebuddy]

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

[bigtimebuddy]

@lucap86 Can you make this work without changing the existing test?

[lucap86]

Coming back from vacation,
i will take a look this afternoon

Edit: @bigtimebuddy I have added a consideration to your comment of 24 aug

[ramarro123]

Hello, any update on this issue?

the fix proposed on tickerlistener work for me, so i integrate it locally, but it's another thing to remember when we want to upgrade pixi :D manually patch it :)

[ivanpopelyshev]

yep, that was a bug.

[bigtimebuddy]

Thank you @lucap86, sorry for the delay.

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.