phaser icon indicating copy to clipboard operation
phaser copied to clipboard

Logic for `TilemapLayer.skipCull` seems to be backwards.

Open veleek opened this issue 3 years ago • 3 comments

Version

  • Phaser Version: Phaser v3.52.0 (WebGL | Web Audio)
  • Operating system: Windows

Description

I have an issue where tiles in a TilemapLayer are culled unexpectedly. In order to verify this I want to ignore all culling for the TilemapLayer (e.g. render all tiles). I would expect that setting skipCull = true would result in no culling occurring so all tiles will be rendered.

For consistency with the culling done by the camera system (see logic in BaseCamera here) the Tilemap cull functions like IsometricCullTiles should return all tiles when skipCull is true, but they currently return no tiles.

Tiles being culled is a separate issue related to putting a TileMapLayer in a container. See https://github.com/photonstorm/phaser/issues/5494#issuecomment-766275655 for a bit more detail on that problem. I believe that's unrelated to this specific skipCull issue.

Example Test Code

    this.map = scene.make.tilemap({ key: name });

    this.tilesetName = tilesetName;
    const tileset = this.map.addTilesetImage(tilesetName);

    this.floorLayer = this.map.createLayer("floor", tileset).setSkipCull(true);

No tiles are rendered. Change to setSkipCull(false) renders tiles (and culls some of them).

Additional Information

Rename skipCull to disableCull. Update cull function in TilemapLayer to return all tiles if disableCull is true.

veleek avatar Jan 24 '21 04:01 veleek

Tried out an implementation of TileMapLayer.cull that looks like this:

cull: function (camera)
    {
        if(this.skipCull)
        {
            return this.layer.data.flat();
        }

        return this.cullCallback(this.layer, camera, this.culledTiles, this._renderOrder);
    }

And it seems to work. This is using the ES10 Array.flat() method. But a more widely available option is also fine. I just picked the shortest option from this list: https://www.jstips.co/en/javascript/flattening-multidimensional-arrays-in-javascript/. If you think is is reasonable let me know and I'll create a PR with the change (along with removing the skipCull logic from the individual cull methods, and renaming the property.

veleek avatar Jan 24 '21 05:01 veleek

@gammafp - Are there any guidelines or requirements around which JS APIs we need to build phaser against? If I get an answer to this then I can go ahead and submit a PR.

veleek avatar Feb 08 '21 05:02 veleek

I think IE 10 is the minimum.

samme avatar Sep 08 '21 02:09 samme

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

photonstorm avatar Mar 22 '23 18:03 photonstorm