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

dispatchTweenEvent overwrites one of the callback's parameters #5753

Closed
andrei-pmbcn opened this issue Jun 21, 2021 · 4 comments · Fixed by #5757
Closed

dispatchTweenEvent overwrites one of the callback's parameters #5753

andrei-pmbcn opened this issue Jun 21, 2021 · 4 comments · Fixed by #5757

Comments

@andrei-pmbcn
Copy link

andrei-pmbcn commented Jun 21, 2021

Where it says callback.params[1] = this.targets; in Phaser.Tweens.Tween#dispatchTweenEvent, the code overwrites your second parameter before calling the callback, which is clearly not something you want or are even warned about. This should be fixed, perhaps by appending this.targets to the start of callback.params instead of overwriting the second param. Calllbacks like these are created when calling myTween.setCallback().

/**
     * Internal method that will emit a Tween based Event and invoke the given callback.
     *
     * @method Phaser.Tweens.Tween#dispatchTweenEvent
     * @since 3.19.0
     *
     * @param {Phaser.Types.Tweens.Event} event - The Event to be dispatched.
     * @param {function} callback - The callback to be invoked. Can be `null` or `undefined` to skip invocation.
     */
dispatchTweenEvent: function (event, callback)
    {
        if (!this.isSeeking)
        {
            this.emit(event, this, this.targets);

            if (callback)
            {
                callback.params[1] = this.targets;

                callback.func.apply(callback.scope, callback.params);
            }
        }
    },

Also, in https://newdocs.phaser.io/docs/3.55.2/Phaser.Tweens.Tween#setCallback it says the params arrray is optional. The above code renders this documentation false, as an error is raised when the params argument is not included, because callback.params[1] returns undefined. Providing an empty params array fixes the error.

If you need more information, such as repro steps and a jsfiddle, I'll be happy to provide these.

@samme
Copy link
Contributor

samme commented Jun 22, 2021

What are the steps to reproduce?

@andrei-pmbcn
Copy link
Author

Repro steps:

  1. clone the phaser project template (https://github.com/photonstorm/phaser3-project-template.git)
  2. run 'npm install' and optionally 'npm audit fix'
  3. run 'npm list phaser' and check that the version is 3.55.2, otherwise run 'npm install phaser'
  4. edit the tween in the src/index.js file so that it says:
          let tween = this.tweens.add({
              targets: logo,
              y: 450,
              duration: 2000,
              ease: "Power2",
          });
          tween.setCallback('onComplete', function(param1, param2) {
              console.log(param1, param2);
          }, ['abcd', 'efgh']);
  1. run Webpack Dev Server from the project root using 'npm run start'
  2. Check localhost:8080 on a Firefox window
  3. Open the developer menu and switch to the console

Expected result: the console should display 'abcd' 'efgh'
Actual result: the console instead displays 'abcd' Array[ {...} ]

This is because the tween targets array always overwrites the callback's second argument.

repro rate 3/3, the bug always appears and it's obvious why once you look at the code

@samme
Copy link
Contributor

samme commented Jun 22, 2021

The problem is that Phaser uses setCallback() differently to the API. It stores the params with an offset, e.g., [this, null, 'abcd', 'efgh'], when setting the callback params from the tween config.

Probably setCallback() should offset the params itself.

@photonstorm
Copy link
Collaborator

Thank you for submitting this issue. We have merged the fix for this and it has been pushed to the main branch. It will be part of the next release. If you get time to build and test it for yourself we would appreciate that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants