fix: tile renderer recovering the wrong camera position on globe - #8187
fix: tile renderer recovering the wrong camera position on globe#8187Alchez wants to merge 4 commits into
Conversation
… globe projection
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8187 +/- ##
==========================================
- Coverage 93.91% 93.91% -0.01%
==========================================
Files 290 290
Lines 24918 24918
Branches 6576 6576
==========================================
- Hits 23402 23401 -1
- Misses 1516 1517 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
coveringTiles recovering the wrong camera position|
Thanks for taking the time to open this PR! |
|
Can you update the initial post with a problem description and a proposed fix (solution explanation). Please make it short. |
|
@HarelM no worries, it's slightly hard to wrap my head around it too. I've updated the description, can you see if that helps? I couldn't shorten it too much, so I've atleast tried to use sections to separate the text. |
|
Thanks for the write-up, it helps to understand. |
| } | ||
|
|
||
| /** | ||
| * Returns the camera's own position in mercator coordinates. |
There was a problem hiding this comment.
I think should indicate what it does, but not in a negative form compared to a different method.
It's not clear what's the difference between this on and getCameraPoint and why not use the other one, or why not change the logic of the other one. The explanation should be part of the PR initial post those, not part of this comment.
| const frustum = transform.getCameraFrustum(); | ||
| const plane = transform.getClippingPlane(); | ||
| const cameraCoord = transform.screenPointToMercatorCoordinate(transform.getCameraPoint()); | ||
| const cameraCoord = cameraMercatorCoordinate(transform); |
There was a problem hiding this comment.
consider renaming this variable to better indicate what it holds.
| ]); | ||
| }); | ||
|
|
||
| test('far camera at high pitch: no tile is refined past the nominal zoom', () => { |
There was a problem hiding this comment.
how is "refined" related to covering tiles algorithm? I'm a bit confused by this test description.
There was a problem hiding this comment.
Are all the test here really needed? You changed a method "outside" the covering tiles method mostly, I would expect to see more tests there instead of here...?
Related Issues
createCalculateTileZoomFunctioninsteadProblem
On the globe projection, a zoom is decided per tile (after zoom level is > 4) based on the distance between the camera and tile. But sometimes the camera's location is incorrectly computed, which causes tiles for that camera to render instead.
Cause
coveringTiles) is trying to get the camera's(x, y)to decide which tiles to requestgetCameraPoint())screenPointToMercatorCoordinate())Proposed Solution
getCameraLngLatalready computes the camera this way, but instead of a long-lat, we want the coordinate instead.getCameraLngLathas been restructured to call the transform as well.Reproduction
JSFiddle: https://jsfiddle.net/auqdp6mk/5/
Benchmarks
The benchmarks are showing correctness instead of a perf gain.
Launch Checklist
CHANGELOG.mdunder the## mainsection.Assisted-By: Claude Opus 5