Remove unnecessary sorting when getting rendered tiles before symbol placement

[zmiao]

Launch Checklist

Two times of sorting are performed when getting the rendered tiles, however, the first sorting result will anyway be overridden by the second sorting, thus remove it from the code.

The second sorting is done right after the first sorting:

mapbox-gl-js/src/style/style.js

Lines 1630 to 1636 in 63bd294

	if (!layerTiles[styleLayer.source]) {
	const sourceCache = this._getLayerSourceCache(styleLayer);
	if (!sourceCache) continue;
	layerTiles[styleLayer.source] = sourceCache.getRenderableIds(true)
	.map((id) => sourceCache.getTileByID(id))
	.sort((a, b) => (b.tileID.overscaledZ - a.tileID.overscaledZ) || (a.tileID.isLessThan(b.tileID) ? -1 : 1));
	}

• briefly describe the changes in this PR
• apply changelog label ('bug', 'feature', 'docs', etc) or use the label 'skip changelog'
• add an entry inside this element for inclusion in the mapbox-gl-js changelog: <changelog></changelog>

[astojilj]

@zmiao
getRenderableIds(true) seems to be called also from getVisibleCoordinates() .. and further depending on that order in tile clipping masks.

mapbox-gl-js/src/render/painter.js

Line 448 in 63bd294

coordsDescendingSymbol[id] = sourceCache.getVisibleCoordinates(true).reverse();

[zmiao]

@astojilj nice catch! I will update my approach then

[mourner]

👍 apart from a nit. Looks like the rotated sorting was originally added in #5601 to match native, but is apparently no longer necessary.

[mourner]

symbolLayer argument is no longer used so we should remove it from signature / calls.

[mourner]

getRenderableIds(true) seems to be called also from getVisibleCoordinates() .. and further depending on that order in tile clipping masks.

Yeah, but removing the sorting doesn't affect any render tests, which made me think it's no longer necessary for matching native, but maybe @ansis can chime in.

[zmiao]

Yeah, but removing the sorting doesn't affect any render tests, which made me think it's no longer necessary for matching native, but maybe @ansis can chime in.

@mourner @ansis , after checking the code, I am assuming that tile order will only matter for symbol placement, the other places that using sourceCache.getVisibleCoordinates(true).reverse() may just only want to include all of the tiles, including tiles that are hold for fade. Maybe the reverse() call also be removed.

I just tested with my above assumption in the latest commit, only following two tests are failed:

[rreusser]

It's interesting to me that we no longer sort by z, instead sorting by zoom then ascending x/y tile coordinate. I almost wonder if what we really want is a hybrid of the two that integrates bearing into compareTileId.

One place where I think this is visible (maybe; not 100% certain) is for circle layers which use read-only depth and are affected by two orderings: 1) tile order, and 2) vertex order. If it weren't overridden, the removed sort would seem to affect the ordering of tiles of circles (though not circles within a single tile). The result is circle layering that, for two reasons (within a tile and across tiles), doesn't always make sense, e.g.:

This PR is okay with me as removal of this redundant sort does seem to check out, though I don't completely follow the logic of compareTileId sorting by canonical.x and canonical.y without bearing.

Marking this review as a "comment" since it falls somewhere between not blocking the merging as this seems valid, but also not fully understanding the slightly larger goal of sorting tiles.

[zmiao]

It's interesting to me that we no longer sort by z, instead sorting by zoom then ascending x/y tile coordinate.

This PR is okay with me as removal of this redundant sort does seem to check out, though I don't completely follow the logic of compareTileId sorting by canonical.x and canonical.y without bearing.

@rreusser to be honest, I have the same question when I cross-check the tile sorting algorithms between gl-js and gl-native.
The reason why I remove the first sorting part is I don't have an answer which one is correct but apparently two consecutive sortings are not necessary, so I removed the first one and try to adapt code in gl-native.

In fact, gl-native was using a similar sorting algorithm that is very similar to the removed one, only with one difference, that is sorting tiles in descending zoom order instead of ascending order. And that's the only sorting in gl-native, which in my view is actually the correct sorting approach. (due to the diverged sorting approaches, several render tests are failing in gl-native and currently, we have to skip them)

So in another way, if we use gl-native's approach, which means we could actually keep using the removed sorting algorithm but with following changes:

  return renderables.sort((a_: Tile, b_: Tile) => {
                const a = a_.tileID;
                const b = b_.tileID;
                const rotatedA = (new Point(a.canonical.x, a.canonical.y))._rotate(this.transform.angle);
                const rotatedB = (new Point(b.canonical.x, b.canonical.y))._rotate(this.transform.angle);
--                return a.overscaledZ - b.overscaledZ || rotatedB.y - rotatedA.y || rotatedB.x - rotatedA.x;
++               // Sort from child tile to parent tile, from bigger y corrdinates to smaller ones(near viewer tiles to further ones in case map is pitched), from smaller x corrdiantes to bigger ones(from left to right)
++               return b.overscaledZ - a.overscaledZ || rotatedB.y - rotatedA.y || rotatedA.x - rotatedB.x;
            }).map(tile => tile.tileID.key);

This will lead to several render tests be updated in gl-js.

cc: @ansis @mourner @asheemmamoowala