Skip to content

Conversation

@pstaszek
Copy link
Contributor

@pstaszek pstaszek commented Jun 6, 2025

Fix image sources being clipped at -180 and 180 longitude when terrain is enabled.

TL;DR

Lack for wrapping support for terrain render to texture tiles.

Long description.

Terrain tiles are rendered to the texture, which then is rendered to the tile (images as well) so content can't overlap between tiles. There was no support for wrapping, so for every wrap image was rendered on the same position relative to the tile.

See the diagram:
wrapping_issue_illustration

The image on tile with wrap = 0 is rendered at desired position, however on tile with wrap = 1 it is rendered on the same position in relation to a tile as for previous one. It should be actually rendered with shift.

Before:

before

After:

after

I also added render test.

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
Copy link
Collaborator

HarelM commented Jun 8, 2025

Can you attach a screen shot of how the render test would look without the fix to make sure the render test covers this scenario?

@HarelM
Copy link
Collaborator

HarelM commented Jun 8, 2025

Is the code coverage report correct by highlighting that the changed code is not covered in tests?
image

@pstaszek
Copy link
Contributor Author

Render test screenshots
On main branch:
actual

On current branch (correct)
expected

@pstaszek
Copy link
Contributor Author

pstaszek commented Jun 12, 2025

For the coverage report, this is a bit more complex.

I assume the coverage report is based on unit tests. Unit tests check only logic responsible for adding and omitting tiles. They do not test matrix computation. This is because I believe tests should be easy to verify, it should be clear if they are correct, while in such tests we would compare some magic numbers in 3D matrices.

This logic is however tested in render test - but only a part of it. For the matrix there are 3 different scenarios, depending on a image and terrain tile zoom level (might be higher, lower or the same). I tested only the case which was relevant for the reported issue. My assumption was that I shouldn't add too many render tests, I didn't want to make them too granular covering very narrow edge cases.

Let me know if and how I can improve that. Should I add unit or render tests? Or current state is ok?

@HarelM
Copy link
Collaborator

HarelM commented Jun 13, 2025

Both render tests and unit tests are added to the coverage report, I think you might be able to find it in codecov site, unfortunately it doesn't get printed here due to a big on their side.
The requirement in general is that new code that was added will be covered (hopefully fully) with tests.

@HarelM
Copy link
Collaborator

HarelM commented Jul 2, 2025

As far as I can tell from the code coverage report, the last else was not covered before this PR and seems like it is still not covered after this PR.
I'm not sure I know how to get to this code path, but since you have changed it, I think it would be wise to try and get to it using either a render test or unit test.
https://github.com/pstaszek/maplibre-gl-js/blob/4884e54f27a509b3b6535f0b7623fee6023d586e/src/source/terrain_source_cache.ts#L239

Let me know if there's anything else I can help with.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Image source is clipped to -180,180 when terrain is enabled.

2 participants