Fixes null @ currentBatch due flush() calls @ LightPipeline.js

[urueda]

This PR: Fixes a bug

Test scenario:

1. Mesh gameobject -> .setPipeline('Light2D')
2. Applying a rotation to the mesh
   The test might reveal an exception/crash due null values at currentBatch property of LightPipeline.
   Crash flow: MeshWebGLRenderer -> setGameObject -> setNormalMapRotation -> flush

Rationale: setGameObject @ MeshWebGLRenderer invoking a flush @ setNormalMapRotation and making currentBatch=null.
The crash happens @ MeshWebGLRenderer once reading a property on currentBatch (null).

[photonstorm]

@greptileai

[greptile-apps]

PR Summary

• Fixed bug in src/renderer/webgl/pipelines/LightPipeline.js where currentBatch could be null after flush() call
• Ensured new batch creation if currentBatch is null to prevent crashes
• Watch for potential side effects in setGameObject and setNormalMapRotation methods

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

[zekeatchan]

@urueda I am not able to reproduce the bug mentioned. Here is a link to the test code:
https://labs.phaser.io/view.html?src=src%5Cbugs%5C6822%20mesh%20null%20currentbatch.js

It would be great if you could provide a link that demonstrates the bug you mentioned so that we can investigate further. Thanks.

[urueda]

@zekeatchan I suggest:

1. a test code over Phaser version 3.70.0 (bug was found there)
2. increasing the rendering complexity of the scene

For the bug to reveal you would need a batch flush to happen when rendering the mesh.
Perhaps, you could repeat the track-race mesh many times to increase the chances for the crash to happen (reading a property on null batch).

[photonstorm]

We don't test against 3.70, as 3.80 is the current version.

The Mesh we tested with has 10,136 faces, which is about the limit of the number of faces a Mesh should have in Phaser 3. The batch size is 4096 by default. I forced it down to 1024 in the config it still doesn't cause a crash, even though you can clearly see it flushing multiple times during the single draw in Spector.gl.

As it stands, unless you can provide us with a reproducible test case against v3.80 then we can't move ahead with this issue.

[urueda]

Please, find next a test code revealing the bug: https://phaser.io/sandbox/xv7EdA2P

[photonstorm]

The sandbox is what should have been provided at the very start, really. You don't need to call setInterval. You don't even need to create a complex batch. All you need is a single object on the display list, followed by a mesh (of any size) and then immediately call setRotation on it. Like this:

class Example extends Phaser.Scene
{
    constructor ()
    {
        super();
    }

    preload ()
    {
        this.load.setPath('https://labs.phaser.io/assets/obj/racing/');
        this.load.obj('roadStart', 'roadStartPositions.obj', 'roadStartPositions.mtl');
    }

    create ()
    {
        this.add.rectangle(50, 50, 50, 50, 0xff0000, 0.25).setPipeline('Light2D');

        const track = this.add.mesh(400, 300);

        track.setPipeline('Light2D');

        const rot90 = Phaser.Math.DegToRad(90);
        const rot180 = Phaser.Math.DegToRad(180);

        track.addVerticesFromObj('roadStart', 1, 0, 0, 0, rot90, rot180);

        track.setRotation(Math.random());
    }
}

const config = {
    type: Phaser.AUTO,
    width: 800,
    height: 600,
    backgroundColor: '#0a440a',
    parent: 'phaser-example',
    scene: Example
};

let game = new Phaser.Game(config);

setRotation is important. Using one of the rotate properties, like rotateX doesn't cause the crash.

Comment out the rectangle before it, and it doesn't crash. Remove the Light2D pipeline from the rectangle and it doesn't crash. Add a sprite or text or anything not using Light2D before the Mesh, and it doesn't crash.

The combination is very specific: Something using Light2D immediately before a Mesh that uses Light2D, and you have to then immediately invalidate the Mesh to trip the bug.

[greptile-apps]

PR Summary

• Fixes null currentBatch after flush() in src/renderer/webgl/pipelines/LightPipeline.js
• Adds check for currentBatch null and creates new batch if necessary
• Prevents crashes in setGameObject and setNormalMapRotation methods

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

[photonstorm]

Thanks for opening this issue, and for submitting a PR to fix it. We have soft-merged your PR into the master branch and attributed the work to you in the Change Log. If you need to tweak the code for whatever reason please submit a new PR.