Repository navigation
port circle querying fixes - #11571
Conversation
|
Can you retarget this at |
da2e451 to
785fd40
Compare
|
I started taking a look at this but wanted to get it building first. The rebase got a little ugly because of some changes I had made and because of a conflict on the gl-js pin. I made a new branch and cherry picked everything but the pin change -- the branch is |
| auto stroke = evaluated.evaluate<style::CircleStrokeWidth>(zoom, feature); | ||
| auto size = radius + stroke; | ||
|
|
||
| // For pitch-alignment: map, compare feature geometry to query geometry in the plane of the tile |
There was a problem hiding this comment.
Nit: tab -> spaces for leading white space
1578373 to
e29cea3
Compare
|
@ChrisLoer thanks for the rebase. I've updated the branch and the query tests are now passing. Did you want to review this further? |
ChrisLoer
left a comment
ChrisLoer
left a comment
There was a problem hiding this comment.
Looks good! I left one question about testing overscaled tiles, but I didn't actually see a problem, it's just an area where it's easy to get confused since native and js store overscaling info differently.
| auto maxPitchScaleFactor = transformState.maxPitchScaleFactor(); | ||
|
|
||
| for (const RenderTile& renderTile : sortedTiles) { | ||
| const float scale = std::pow(2, transformState.getZoom() - renderTile.id.canonical.z); |
There was a problem hiding this comment.
Using renderTile.id.canonical.z here instead of this.tileID.overscaledZ as in GL JS caught my eye. I think the reason is that here we're using the hardwired util::tileSize, while the equivalent tile.tileSize in JS has the power-of-two overscaleFactor built into it.
Which just leaves me wondering -- did you do any manual tests with overscaled tiles, or would it make sense to add an explicit overscaled test here?
There was a problem hiding this comment.
I didn't test with overscaled tiles. What do you see as a better way to test this? a query test or a unit test somehow? I'm leaning towards query test but I think it could be tricky to set it up so that it fails just when the overscaling isn't accounted for.
There was a problem hiding this comment.
For a manual test I'd just zoom into z20 or something with some pitched circles and see that the queries hit where we expect. For the query tests, we could just set them to use overscaled tiles, so that the functionality was getting exercised. Outside of regression tests, I don't think there's a specific "test for overscaling" since of course it can break in lots of different ways.
I am 👍 on merging either way.
This fixes circle querying for cases where either
circle-pitch-alignment=map
or
circle-pitch-scaling=viewport
e29cea3 to
89069ee
Compare
Previously we relied on tile buffers for querying features who's rendered representations cross tile boundaries. Now we query multiple tiles making it unnecessary to index features that are completely outside a tile's boundaries.
89069ee to
c7dcdb9
Compare
4 participants
Fix #10615
port mapbox/mapbox-gl-js#6036 to -native
The fixes include:
circle-pitch-alignment=mapcircle-pitch-scaling=viewport