phaserjs / phaser

Phaser is a fun, free and fast 2D game framework for making HTML5 games for desktop and mobile web browsers, supporting Canvas and WebGL rendering.
https://phaser.io
MIT License
37k stars 7.09k forks source link

feat : add data parameter in Scene.switch #6876

Closed wooseok123 closed 1 month ago

wooseok123 commented 2 months ago

feat : add data parameter to Scene.switch

Thank you for creating an amazing framework. While developing with it, I've noticed a few areas that I think could benefit from some changes, and I'd like to make a suggestion.

Problem

When using theswitch method in Phaser to change scenes, we use the start method if the scene is being loaded for the first time.


// sceneManager.js
switch: function (from, to)
    {
        var sceneA = this.getScene(from);
        var sceneB = this.getScene(to);

        if (sceneA && sceneB && sceneA !== sceneB)
        {
            this.sleep(from);

            if (this.isSleeping(to))
            {
                this.wake(to);
            }
            else
            {
               // right here
                this.start(to);
            }
        }

        return this;
    },

And the start method allows us to pass data as a second parameter. However, the current implementation of the switch method only accepts a key, making it impossible to pass data.


// scenePlugin.js
switch: function (key)
    {
        if (key !== this.key)
        {
            this.manager.queueOp('switch', this.key, key);
        }

        return this;
    },

This limitation forces us to use the following pattern:

this.scene.sleep();
this.scene.start('key', data);

Solution

Thus, I would like to propose adding a data parameter to the switch method.


// sceneManager.js
switch: function (from, to, data)
    {
        var sceneA = this.getScene(from);
        var sceneB = this.getScene(to);

        if (sceneA && sceneB && sceneA !== sceneB)
        {
            this.sleep(from);

            if (this.isSleeping(to))
            {
                this.wake(to);
            }
            else
            {
                this.start(to, data);
            }
        }

        return this;
    },

However, this change would require modifications to the ScenePlugin pattern since a queueOp function in the scene currently has a maximum of three parameters.

// scenePlugin.js
queueOp: function (op, keyA, keyB)
    {
        this._queue.push({ op: op, keyA: keyA, keyB: keyB });

        return this;
    },

The switch method already uses these for the event name, the current scene, and the scene to be started, leaving no room for passing data.

// scenePlugin.js
switch: function (key)
    {
        if (key !== this.key)
        {
            this.manager.queueOp('switch', this.key, key);
        }

        return this;
    },

But the queueOp function's parameters are currently labeled keyA, keyB, etc., and in cases where only one key is needed, the third parameter is used to pass data. This could potentially confuse developers who want to contribute to the Phaser and use Phaser. ex) stop Method

stop: function (key, data)
    {
        if (key === undefined) { key = this.key; }
        // data argument is actually keyB 
        this.manager.queueOp('stop', key, data);

        return this;
    },

Therefore, I propose adding a fourth parameter to optionally receive data.

// to-be
queueOp: function (op, keyA, keyB, data)
    {
        this._queue.push({ op: op, keyA: keyA, keyB: keyB, data : data });

        return this;
    },

In cases where only one key is needed, we could pass null as the third parameter and use the fourth parameter for data, similar to the bringToTop method.

bringToTop: function (key)
    {
        if (this.isProcessing)
        {
           // if data argument is added, it could be like this._queue.push({ op: 'bringToTop', keyA: key, keyB: null, data : null });
            this._queue.push({ op: 'bringToTop', keyA: key, keyB: null });
        }
        else
        {
            var index = this.getIndex(key);

            if (index !== -1 && index < this.scenes.length)
            {
                var scene = this.getScene(key);

                this.scenes.splice(index, 1);
                this.scenes.push(scene);
            }
        }

        return this;
    },
wooseok123 commented 2 months ago

If this change seems reasonable, I'll go ahead and make the necessary modifications and submit a commit

photonstorm commented 1 month ago

👍

wooseok123 commented 1 month ago

👍

Hello thank you for considering my PR. But i think we need to modify scenePlugin.js as well. All i modified is just sceneManager.js

// scenePlugin.js  AS-IS
switch: function (key)
    {
        if (key !== this.key)
        {
            // need extra argument for data
            this.manager.queueOp('switch', this.key, key);
        }

        return this;
    },
// scenePlugin.js

// need extra argument for data
queueOp: function (op, keyA, keyB)
    {
        this._queue.push({ op: op, keyA: keyA, keyB: keyB });

        return this;
    },