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

feat : add data parameter in Scene.switch #6876

Merged
merged 1 commit into from
Aug 6, 2024

Conversation

wooseok123
Copy link
Contributor

@wooseok123 wooseok123 commented Jul 26, 2024

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;
    },

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

Added a data argument to the Scene.switch method to allow passing data when switching scenes, enhancing flexibility in scene management.

  • Modified src/scene/SceneManager.js to include an optional data parameter in the switch method.
  • Updated queueOp function in src/plugins/ScenePlugin.js to accept a fourth parameter for data.
  • Ensured backward compatibility by handling cases where data is not provided.
  • Improved documentation and comments to clarify the new parameter usage.

1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings

@wooseok123
Copy link
Contributor Author

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

@photonstorm
Copy link
Collaborator

👍

@photonstorm photonstorm merged commit 8ef7545 into phaserjs:master Aug 6, 2024
@wooseok123
Copy link
Contributor Author

👍

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;
    },

@wooseok123 wooseok123 changed the title feat : add data argument in Scene.switch feat : add data parameter in Scene.switch Aug 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants