Added changeScene API

[yaustar]

Updated: Tue 20 Sep 2022

This is to address the many topics for switching scene in PlayCanvas. The page and helper script I wrote doesn't seem to be enough https://developer.playcanvas.com/en/user-manual/packs/loading-scenes/

So I'm adding the most common use case to the API.

Changing scenes becomes a one liner with this change where it loads the scene data, destroys all entities under the app.root and adds the scene settings and hierarchy:

this.app.scenes.changeScene('name of scene');

This PR also adds support for loading scene settings/hierarchy by name, again reducing the extra boilerplate that the user needs to write.

I confirm I have read the contributing guidelines and signed the Contributor License Agreement.

[yaustar]

Project to test this PR with : https://playcanvas.com/project/979499

[kungfooman]

Right now in SceneRegistry#_loadSceneData there is this code:

        let url = sceneItem;

        if (sceneItem instanceof SceneRegistryItem) {
            url = sceneItem.url;
        } else {
            sceneItem = this.findByUrl(url);
            if (!sceneItem) {
                sceneItem = new SceneRegistryItem('Untitled', url);
            }
        }

It could be shortened into this code:

        let url = sceneItem;
        if (typeof sceneItem === 'string') {
            sceneItem = this.findByUrl(url) || this.find(url) || new SceneRegistryItem('Untitled', url);
        }
        url = sceneItem.url;

Then you can delete this code in SceneRegistry#changeCode aswell:

if (!(sceneItem instanceof SceneRegistryItem)) {
    sceneItem = this.find(sceneItem);
}

Overall it removes lines of code and SceneRegistry#changeCode would also accept URL's (and SceneRegistry#loadSceneData would also accept scene names).

I don't know if scene names and scene URL's could conflict though (in my little testing everything worked as it should and the unit tests are also happy).

[yaustar]

Right now in SceneRegistry#_loadSceneData there is this code:

        let url = sceneItem;

        if (sceneItem instanceof SceneRegistryItem) {
            url = sceneItem.url;
        } else {
            sceneItem = this.findByUrl(url);
            if (!sceneItem) {
                sceneItem = new SceneRegistryItem('Untitled', url);
            }
        }

It could be shortened into this code:

        let url = sceneItem;
        if (typeof sceneItem === 'string') {
            sceneItem = this.findByUrl(url) || this.find(url) || new SceneRegistryItem('Untitled', url);
        }
        url = sceneItem.url;

Then you can delete this code in SceneRegistry#changeCode aswell:

if (!(sceneItem instanceof SceneRegistryItem)) {
    sceneItem = this.find(sceneItem);
}

Overall it removes lines of code and SceneRegistry#changeCode would also accept URL's (and SceneRegistry#loadSceneData would also accept scene names).

I don't know if scene names and scene URL's could conflict though (in my little testing everything worked as it should and the unit tests are also happy).

I like the idea that loadScene* and changeScene can all take a name, URL or SceneRegistryItem and would make the loadScene* functions more flexible.

Any opinions from the rest of the engine team?

[yaustar]

I've added @kungfooman's suggestion on being able to use the scene name and added a few more tests

[kungfooman]

Thank you for the changes @yaustar

Since SceneRegistry#changeScene is basically SceneRegistry#loadSceneHierarchy with a preface, it can be rewritten as exactly that to reduce quite some lines of code (and makes future rewrites easier I think too):

    changeScene(sceneItem, callback) {
        /**
         * This function is called before the new scene hierarchy is added.
         *
         * @param {SceneRegistryItem} sceneItem - The scene item
         */
        const preface = (sceneItem) => {
            const app = this._app;
            // Destroy/Remove all nodes on the app.root
            const rootChildren = app.root.children;
            while (rootChildren.length > 0) {
                const child = rootChildren[0];
                child.reparent(null);
                child.destroy?.();
            }
            app.applySceneSettings(sceneItem.data.settings);
        };
        this.loadSceneHierarchy(sceneItem, callback, preface);
    }

SceneRegistry#loadSceneHierarchy just needs to handle the preface:

    loadSceneHierarchy(sceneItem, callback, preface) {
        this._loadSceneData(sceneItem, false, (err, sceneItem) => {
            if (err) {
                if (callback) {
                    callback(err);
                }
                return;
            }
            if (preface) {
                preface(sceneItem);
            }
            this._addHierarchyToScene(sceneItem.url, sceneItem.data, callback);
        });
    }

Using arrow functions is reducing code size aswell, you don't need to worry about self (arrow functions inherit this and its fixed/unchangeable). Babel takes care to rewrite it into ES5:

	  _proto.changeScene = function changeScene(sceneItem, callback) {
	    var _this2 = this;

	    var preface = function preface(sceneItem) {
	      var app = _this2._app;
	      var rootChildren = app.root.children;

	      while (rootChildren.length > 0) {
	        var child = rootChildren[0];
	        child.reparent(null);
	        child.destroy == null ? void 0 : child.destroy();
	      }

	      app.applySceneSettings(sceneItem.data.settings);
	    };

	    this.loadSceneHierarchy(sceneItem, callback, preface);
	  };

[yaustar]

I'm not keen on use the preface as loadSceneHierarchy is existing public API and traditionally, the callback is the last param

[kungfooman]

This app closure variable also has too much context and maybe add JSDoc?

[yaustar]

Is JSDoc needed here for a private function that is only used in the scope of the current class?

[kungfooman]

I guess its not needed, I just use them as a mind crutch and when the types are checked, it can offer some insight about what is going wrong in which position

[kungfooman]

Since you are using arrow functions now, you can just use this and move that closure variable into the arrow function itself. And since app is only used once, it doesn't really need to be declared at all

[kungfooman]

I am running into an issue loading SeeMore as non-trivial scene and for some reason it doesn't destroy the old camera:

(one camera rendering over the other)

I can fix it by adding this to your onBeforeAddHierarchy():

app.scene?._activeCamera?.node?.destroy();

But I don't understand it, shouldn't the camera be in app.root anyway or what else is required to completely destroy a scene?

[yaustar]

I am running into an issue loading SeeMore as non-trivial scene and for some reason it doesn't destroy the old camera:

(one camera rendering over the other)

I can fix it by adding this to your onBeforeAddHierarchy():

app.scene?._activeCamera?.node?.destroy();

But I don't understand it, shouldn't the camera be in app.root anyway or what else is required to completely destroy a scene?

That's odd, there's no more than 1 scene in that project

Edit: I just tried it with my branch and can't reproduce your issue?

[yaustar]

Testing with a Seemore fork with changing scenes with the existing stable engine and the double render is there too: https://playcanvas.com/project/983477/overview/f-seemore

[kungfooman]

Edit: I just tried it with my branch and can't reproduce your issue?

Thank you for testing, I downloaded SeeMore in 2019 but I also just tested the last version and it has the same issue.

1. Open https://playcanv.as/p/MflWvdTW/
2. Get both cameras: cams = pc.app.root.find("name", "Camera");
3. Delete as you do in this PR:

const rootChildren = pc.root.children;
while (rootChildren.length > 0) {
    const child = rootChildren[0];
    child.reparent(null);
    child.destroy?.();
}

4. It still renders skybox for you too?

The first cam seems to "survive" 🧟

[yaustar]

Edit: I just tried it with my branch and can't reproduce your issue?

Thank you for testing, I downloaded SeeMore in 2019 but I also just tested the last version and it has the same issue.

1. Open https://playcanv.as/p/MflWvdTW/
2. Get both cameras: cams = pc.app.root.find("name", "Camera");
3. Delete as you do in this PR:

const rootChildren = pc.root.children;
while (rootChildren.length > 0) {
    const child = rootChildren[0];
    child.reparent(null);
    child.destroy?.();
}

4. It still renders skybox for you too?

The first cam seems to "survive" 🧟

I tried this with engine 1.56.0 (current stable) and loadSceneSettings + Hierarchy methods and the issue is there too.

This means it's not a new issue that this PR introduces

[kungfooman]

I am also not quite happy with onBeforeAddHierarchy and I realized that we basically just need an event...

The first step would be to move your scene clearing code into class Scene (where it imo belongs). Since Scene already extends EventHandler, we have events without any changes.

New method for src/scene/scene.js:

    clear() {
        this.fire("clear");
        const app = getApplication();
        // Destroy/Remove all nodes on the app.root
        const rootChildren = app.root.children;
        while (rootChildren.length > 0) {
            const child = rootChildren[0];
            child.reparent(null);
            child.destroy?.();
        }
        this?._activeCamera?.node?.destroy();
    }

(the last line with _activeCamera fixes the SeeMore issue/bug, until someone comes up with a better solution)

The SceneRegistry#changeScene turns into:

    changeScene(sceneItem, callback) {
        const app = this._app;
        app.scene.once("beforeAddHierarchy", (sceneItem) => {
            app.scene.clear();
            app.applySceneSettings(sceneItem.data.settings);
        });
        this.loadSceneHierarchy(sceneItem, callback);
    }

onBeforeAddHierarchy is completely removed and you can remove that extra private method.

Finally, in loadSceneHierarchy, instead of:

            if (onBeforeAddHierarchy) {
                onBeforeAddHierarchy(sceneItem);
            }

            const app = this._app;

We simply have to fire the event while passing sceneItem:

            const app = this._app;
            app.scene.fire("beforeAddHierarchy", sceneItem);

[mvaligursky]

I would not use _activeCamera at all .. I want to remove it from code base, as it's just a hack added by particle system rendering. I'm not entirely what this is trying to solve, but it should be done differently.

[kungfooman]

I would not use _activeCamera at all ..

Yes, I can imagine that, for testing I just needed a way to clear the entire scene, since deleting only app.root.children still retains one camera. I don't know why, just that it can be deleted by that _activeCamera line. Do you have an idea how to fix this?

[yaustar]

I'm less keen to add an event into a system that hasn't used them before. It feels a bit overkill and would like to put a pin in it here for team review.

In regards to Seemore, I will create a new ticket to investigate as that's odd but not introduced by this PR. Maybe due to the use of zones or portals?

[mvaligursky]

It's really strange the Camera component survives the scene hierarchy getting deleted. There must be some code re-adding it or similar?

If you have a repro, it'd be interesting to trace layer composition:

pc.Tracing.set(pc.TRACEID_RENDER_ACTION, true);

at the point the hierarchy is deleted, I'd expect this to print no render actions - meaning no cameras are in the scene.

[yaustar]

@mvaligursky Ready to be reviewed :)

[mvaligursky]

approving with small comments