Fix image rendered partially on terrain

[pstaszek]

Fixes issue #1559 - image was rendered partially on terrain.

Description of the cause.
Image is always in always assigned to one tile, but the content could exceed that tile. This approach doesn't work for terrain. Content of each terrain tile is rendered separately, and for each terrain tile MapLibre renders only overlapping tiles (children, parents and same tile). This is why sometimes image is not rendered - because it is on the tile that does not belong to some terrain tile.

As the solution I added optional information to the OverscaledTileID what tiles on various zoom levels are overlapping with this tile. This way while computing tiles to render for each terrain tile, we are able to render image tile as well, even if canonical tile itself are not overlapping (but the content is).

For fixing animated image it was neccesary to clear all terrain tiles textures (changes in src/ui/map.ts).

I also found another way to fix this problem - however I'm not sure if it is doable that way. As the image is applied to single tile, we could do it to tile 0/0/0. However, this creates issues with unsufficient precision of transformation matrix, also there is another issue with terrain tiles, which I didn't figure out wih that approach.

Before:

After:

For testing you can use scripts provided in issue thread:

• static image: https://jsbin.com/yowixah/1/edit?html,output
• animated image: https://jsbin.com/juwikuq/4/edit?html,output

Launch Checklist

• Confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask!
• Briefly describe the changes in this PR.
• Link to related issues.
• Include before/after visuals or gifs if this PR includes visual changes.
• Write tests for all new functionality.
• Document any changes to public APIs.
• Post benchmark scores.
• Add an entry to CHANGELOG.md under the ## main section.

[HarelM]

This looks like a bbox type, what's the difference?

[pstaszek]

Values there reference tile X and Y number, while bbox I could find in the repo were abould LngLat. Unless I missed some other type.

[HarelM]

I think there's a geojson.bbox type that is simply an array of 4 numbers, but it might not be a great fit here, IDK...

[pstaszek]

Right, I missed that.

Hmm... I still think separate type here is a better fit. GeoJSON BBox implies that it contains geographic coordinates - longitude and latitude, while here we store canonical tile numbers. Also having values in an array is a bit less robust, that having them explicitly named.

Let me know if this rationale sounds good for you.

[HarelM]

This feels a bit odd to me as this is not a "regular" property of a single tile, can this be stored somewhere else perhaps?

[pstaszek]

Yeah, it was difficult to find such logical place. I chose OverscaledTileID for following reasons:

• it already contains specific information for terrain tile, it seems logical to me to keep them in the same place
• TileID-kind type seems valid for such information, as it describes placement of the tile. It is not about the data as an example.
• This is the type we provide to TerrainSourceCache.getTerrainCoords in RenderToTexture.prepareForRender method - which seems logical, and we don't have to change that.

[HarelM]

Is it possible to move this logic so that map doesn't need to do something special for the case of image source?

[pstaszek]

Tbh this entire fragment of code (_terrainDataCallback) seems a bit special - I will take a closer look on that, I would try to move it somewhere.

[pstaszek]

I wasn't able to find better placement for this logic.

We have to clear all terrain tiles for image source because we don't know which terrain tiles were affected, for two reasons:

• image can exceed its tile (however we can use new range object introduced in this PR),
• image could be moved from one tile to another, but we only get information about new tile.
  I don't see the way to fix that without some more major refactoring.

What I tried:

• add hasCustomTerrainRange method to OverscaledTileID, then we can ready from tileId in TerrainSourceCache.freeRtt that this tile is in fact an image tile - but it is very implicit, the logic won't be clear in my opinion
• pass information about the source to freeRtt but it is also bad, as freeRtt method probably also should not be aware of source

This place seems the best, as it contains already logic that decides when to clear terrain tiles based on event-related circumstances - eg. when style changes, clear all.