[core] Fix queryRenderedFeatures for colliding symbol features #6773

brunoabinader · 2016-10-20T16:16:48Z

This PR is a work-in-progress which contains a candidate solution for fixing queryRenderedFeatures for colliding symbol features.

My assumption for this is based on the fact we always calculate minimum placement scale for every symbol feature when populating the symbol buckets in SymbolLayout::place - so we can just reuse that placement scale value when querying - rationale is that if current scale trespasses the minimum placement scale for a given feature, it is then assumed it gets rendered.

/cc @ansis @jfirebaugh

mention-bot · 2016-10-20T16:16:50Z

@brunoabinader, thanks for your PR! By analyzing the history of the files in this pull request, we identified @jfirebaugh, @ansis and @kkaefer to be potential reviewers.

brunoabinader · 2016-10-20T20:47:32Z

@ansis I have a few questions in mind:

Do you recall the rationale for re-calculating the minimum placement scale when querying - and not using the previously calculated placementValue for each collision box on the tree?
I've noticed a tiny, but consistent de-sync between collision results stored in CollisionTile and actual rendered features. In the example below (1), queryRenderedFeatures returns 14, while all 100 features are still rendered. If I zoom in just a bit more (2), then we can clearly see the 14 rendered features returned by the query:

1	2

It is unclear to me if the current expectations for query tests symbol-features-in/fractional-outside and symbol/fractional-outside are correct. By outside do we mean the query boxes are outside of an area with rendered features?

ansis · 2016-10-20T21:49:41Z

Do you recall the rationale for re-calculating the minimum placement scale when querying - and not using the previously calculated placementValue for each collision box on the tree?

It's not recalculating the minimum placement scale. It's calculating a new value. findPlacementScale finds the scale at which two collision boxes could be shown without them overlapping.

When trying to avoid collisions, findPlacementScale is run for a box and all the boxes it might collide with. The end result is is the scale at which the new box can be shown without colliding with any box.

When querying, findPlacementScale is run for the query box and all boxes it may collide with. This is used to calculate whether it actually collides with those boxes. For example, you want to find out whether box A intersects with your query box Q. The minimumPlacementScale that is calculated with findPlacementScale for A and Q turns out to be 0.5. This means that:

when scale < 0.5 then A and Q overlap
when scale > 0.5 then A and Q do not overlap
when scale = 0.5 then A and Q touch at an edge

This line (scale <= minPlacementScale ) determines whether they intersect.

This step is necessary because symbols take up different amounts of space at different zoom levels. The initial rtree query only provides candidates. The second step is necessary to determine what actually intersects at that zoom level.

I think #6629 needs to be reverted.

This comparison is suspicious though. It's a float equality comparison. Some small differences could be causing significant problems.

I've noticed a tiny, but consistent de-sync between collision results stored in CollisionTile and actual rendered features. In the example below (1), queryRenderedFeatures returns 14, while all 100 features are still rendered. If I zoom in just a bit more (2), then we can clearly see the 14 rendered features returned by the query:

This seems like a great lead. It shouldn't be doing that. Looks like the rendering is treating the placementScale different than queryRenderedFeatures. The value used for rendering is rounded to the nearest tenth of a zoom level before being added to the buffers. Maybe the query logic needs to match this.

It is unclear to me if the current expectations for query tests symbol-features-in/fractional-outside and symbol/fractional-outside are correct.

They look right

By outside do we mean the query boxes are outside of an area with rendered features?

Yes. The query box in symbol/fractional-outside overlaps a collision box, but this collision box is "hidden" because it doesn't cover any text at the current zoom level. If you zoom out then this box starts "working".

Thanks for digging into this!

brunoabinader · 2016-10-20T22:27:35Z

Thank you for the detailed explanation @ansis!

This line (scale <= minPlacementScale ) determines whether they intersect.

Isn't this intersection done already here - when we intersect the query box with each feature on the tree?

When querying, findPlacementScale is run for the query box and all boxes it may collide with. This is used to calculate whether it actually collides with those boxes.

Yes - this is exactly what I still don't quite understand. At this point we know that given the intersection check cited above (1) that all the features we are iterating intersects with the query box, and (2) minimum placement scale has already been done for that given zoom. Why calculate a new placement scale, then? Precision loss?

I think #6629 needs to be reverted.

Why - if for non-colliding symbols we iterate only over the features that intersects the query box and those features ignore placement?

ansis · 2016-10-20T23:05:22Z

The CollisionTile isn't 2D. It's 3D! The collision tile is a cube with (x, y, scale) as the three dimensions. The boxes in the index are actually truncated square pyramids. Queries are 2D rectangles that exist in 3d space. A query finds all the truncated square pyramids that intersect the rectangle. This 3D index uses a 2D index (the tree) to narrow things down and an extra step (findPlacementScale) to find the exact answer.

As you zoom, the size of the symbol changes relative to the tile. When you zoom in the symbol stays the same size and the tile gets a lot bigger. When you zoom in the symbol gets smaller relative to the tile. This size change is what needs to be accounted for.

The boxes inserted into the tree don't represent the exact bounds of the symbol at the current zoom level. They represent the bounds that the lowest zoom level (at which the symbol is biggest relative to the tile). Querying the tree gives you boxes for symbols that may collide with your query, but that may not collide at the current zoom level. This is why we need to do the extra step.

Yes - this is exactly what I still don't quite understand. At this point we know that given the intersection check cited above (1) that all the features we are iterating intersects with the query box, and (2) minimum placement scale has already been done for that given zoom. Why calculate a new placement scale, then? Precision loss?

No, we don't know (1). We know that we have all the features that do intersect the query box, but we also have some that may not (because of the size change that happens when zooming).

During querying we aren't calculating a new placement scale. We're just re-using the placement scale calculation code to determine whether the two boxes intersect at this scale. We do this by pretending we want to place the query box. If we placed the query box and it could be shown at the current scale then there is no intersection.

Why - if for non-colliding symbols we iterate only over the features that intersects the query box and those features ignore placement?

The tree boxes of all the features intersect the query box, but they actual collision box might not intersect the query box at the current zoom.

jfirebaugh · 2016-10-20T23:09:50Z

☝️ Super good information. Can we capture this in code comments or an architecture doc?

ansis · 2016-10-21T02:03:42Z

@jfirebaugh are you thinking somewhere in https://github.com/mapbox/mapbox-gl-native/blob/master/ARCHITECTURE.md ?

I wrote a bit more on the geometric interpretation of CollisionTile:

CollisionTile is responsible for preventing collisions among labels.

Labels are usually imagined as 2D rectangles on a 2D map. If that was it, we could just put all the rectangles in a 2D index and check if any new rectangles intersect with any existing ones. Unfortunately zooming makes it a whole lot more complicated. When you zoom, the possible collisions between labels change.

Why not just recalculate the 2D index whenever you zoom?

this might be too slow, resulting in visibly overlapping labels for a bit
without considering cohesiveness across zoom you may have labels popping in and out constantly
this could actually work ok. We've never tried it. (maybe we should?)

Instead of recalculating the 2D index we use a 3D index! The three dimensions are x, y and scale. Imagine the CollisionTile as a cube:

In a 3D index labels aren't be just rectangles. They are truncated rectangular pyramids.

These pyramids are thinner at higher scales. This is because at higher scales labels take up a smaller portion of the tile. As you zoom in on a map the tile gets bigger but the label stays the same size. Proportionally to the tile, the labels get smaller as you zoom in.

Since all the pyramids get narrower as scale is increases, if a label's pyramid fits at the bottom of a cube it will always fit at the top. If a label fits at a lower scale a label will always fit at a higher scale.

Some labels don't fit at the lower scales but do when you zoom in a bit. These labels are pyramids that float without touching the bottom of the cube. The scale value that defines the bottom of the pyramid is called the placementScale. This determines the lowest zoomlevel a label can be shown at.

CollisionTile.placeFeature determines the placementScale, the scale at which you need to crop the bottom of the pyramid so that it fits in the cube without a collision. CollisionTile.insertFeature adds the pyramid to the cube.

When placing a feature we determine whether a pyramid intersects with any other pyramid and then crop it so that it doesn't. A 2D index (RTree in -native, grid-index in -js) is used to accelerate this. All previously placed pyramids are added to the 2D index using their bottom rectangle. This bottom rectangle is the widest part of the pyramid. Querying the 2D index gives you all the pyramids that may intersect. We then need to loop over all these possible intersections and run findPlacementScale to find the scale at which you would need to crop the pyramid to make it fit. The maximum of all of these is pyramids placementScale.

CollisionTile also gets used to query what symbols are being rendered within a box. It works like this: Pretend your query box is actually a label. Find a pretend placementScale for query box. If the placementScale is larger than the current scale, then the pyramid of the pretend query label has been cropped to avoid a collision with a label. This means the other label intersects the query at the current zoom.

mb12 · 2016-10-21T04:24:31Z

@ansis Can you please also kindly clarify the following?

1.) How is pitch handled for symbol placement? Some symbols disappear(correctly) as pitch is increased. How is this modeled for placement?

2.) Also is it possible to explain why increasing pitch adds rendering artifacts even on retina screens (lines get aliased esp the ones near the top of the viewport, same for symbols etc)? This also happens for built in mapbox styles.

ansis · 2016-10-21T16:31:13Z

1.) How is pitch handled for symbol placement? Some symbols disappear(correctly) as pitch is increased. How is this modeled for placement?

We stretch all the boxes in the y direction by this factor. It's a very rough solution, but it's simple and it works pretty well as long as all features are in the same plane.

2.) Also is it possible to explain why increasing pitch adds rendering artifacts even on retina screens (lines get aliased esp the ones near the top of the viewport, same for symbols etc)? This also happens for built in mapbox styles.

The approximations used to handle perspective antialiasing aren't perfect.

brunoabinader · 2016-10-21T19:49:59Z

It turns out what we were missing was accounting for scaling when generating the colliding query box. For this to work, we also pass scale when generating the tree box so the query intersects with the right candidates.

The fix mentioned above fixes the issues mentioned by @bounds in #6055 - where only features located on the top-left were queried. Notice this doesn't fix the small de-sync between rendered vs. queried symbols.

ansis · 2016-10-21T20:59:27Z

src/mbgl/text/collision_tile.cpp

+    // Account for scaling when generating the query box.
+    const float x2 = (box.max.x - box.min.x) * scale;
+    const float y2 = (box.max.y - box.min.y) * scale;
+    const auto queryBox = CollisionBox { box.min, 0, 0, x2, y2, scale };


I still don't understand why this is the right fix

The query gets converted to the tile's units here.

It gets passed to FeatureIndex.query here without a conversion.

It gets passed to CollisionTile.queryRenderedSymbols here without a conversion.

They get rotated here.

At this point I think the query should be in tile units, but rotated to match CollisionTile's frame of reference.

Why do these units need to be multiplied by scale?
Also, why would the width/height be multiplied but not the point box.min?

Why do these units need to be multiplied by scale?

My suspicion is that we must follow the same approach done when populating the symbol buckets:

https://github.com/mapbox/mapbox-gl-native/blob/master/src/mbgl/layout/symbol_layout.cpp#L422-L434

You can see that placementZoom is actually zoom - which is an integer zoom value multiplied the scale difference.

Also, why would the width/height be multiplied but not the point box.min?

Anchor (box.min) doesn't get multiplied by scale when generating the tree box:
https://github.com/mapbox/mapbox-gl-native/blob/master/src/mbgl/text/collision_tile.cpp#L149-L154

Because (x1, y1) is always 0, we don't need them to scale.

I think I'm starting to understand.

The units of query box are correct. But findPlacementScale treats the query as a pyramid instead of a plain rectangular prism. What we want to make is a pyramid that covers the query box at scale. This means that if scale != 1 then the base of the pyramid will be bigger. The base of the pyramid is the value we use to define the CollisionBox. So we need to calculate the size of the pyramid at scale=1 so that at the current scale the pyramid is as big as the query box. The * scale calculates this correctly.

jfirebaugh · 2016-10-22T00:27:19Z

Great stuff! Let's convert @ansis's overview comments to class documentation for CollisionTile, and the discussion above to a code comment at the appropriate place.

brunoabinader · 2016-10-22T18:05:19Z

Updated PR: as per conversation with @ansis on avoiding the query box as a collision box heuristic, I believe we've achieved a good status now:

To obtain precise results, we:

Round scale value to obtain same results from symbol shader.
- This fixes the de-sync between currently rendered vs. queried symbols
Generate a boost geometry polygon out of the (rotated) query geometry to check if it intersects() against all feature boxes.
- This fixes perspective, as the query geometry has already been projected into tile coordinate space.
Check if current scale is within each feature's minimum and maximum placement scales.
De-scale feature boxes when intersecting to account for the fractional zoom scaling.
- On my first attempts with this approach, the query tests were failing because (1) I forgot to check if blocking.maxScale <= scale and (2) to de-scale the feature boxes when intersecting.

Fixes #6055 and #6630.

brunoabinader · 2016-10-22T18:09:20Z

Qt5 build bot failures are unrelated to the changes made in this PR - see #6794.

ansis · 2016-10-24T17:00:45Z

src/mbgl/text/collision_tile.cpp

+    // Account for the rounding done when updating symbol shader variables.
+    float zoom = util::log2(scale);
+    zoom = std::ceil(zoom * 10.0f) / 10.0f;
+    scale = std::pow(2.0f, zoom);


I think this rounded scale needs to be used for the visibleAtScale but the unrounded scale needs to be used when scaling the box in intersectsAtScale. The rounded value is only used in the shaders to figure out when a label is visible. The actual size and position uses the more exact value.

ansis · 2016-10-24T17:07:31Z

src/mbgl/text/collision_tile.cpp

+    // Check if query polygon intersects with the feature box at current scale.
+    auto intersectsAtScale = [&] (const CollisionTreeBox& treeBox) -> bool {
+        const Box& box = std::get<0>(treeBox);
+        const auto anchor = bg::return_centroid<CollisionPoint>(box);


I think this should use the CollisionBox values (get<1>(treeBox)) instead of the tree box values to calculated the scaled box. In some cases the anchor might not be the same as the centroid.

Good point - at some point I attempted using CollisionBox but I was obtaining wrong values because I forgot to account for the yStretch multiplication. I'll modify the code to use CollisionBox.

ansis · 2016-10-24T17:15:56Z

src/mbgl/text/collision_tile.cpp

+                anchor.get<1>() + ((box.max_corner().get<1>() - anchor.get<1>()) / scale),
+            }
+        };
+        return bg::intersects(polygon, unscaledBox);


We don't have boost in -js so we'll need to write the intersection test ourselves there. It might be worth using our own version in -native as well just so that it matches -js exactly. I think it's already mostly written. See polygonIntersectsMultipolygon. If queryGeometry is just a polygon we might want to add a non-multi-polygon version.

ansis · 2016-10-24T17:19:18Z

src/mbgl/text/collision_tile.cpp

+    // Account for the rounding done when updating symbol shader variables.
+    float zoom = util::log2(scale);
+    zoom = std::ceil(zoom * 10.0f) / 10.0f;
+    scale = std::pow(2.0f, zoom);


I think this rounded scale needs to be used for the visibleAtScale but the unrounded scale needs to be used when scaling the box in intersectsAtScale. The rounded value is only used in the shaders to figure out when a label is visible. The actual size and position uses the more exact value.

(I copied this comment from #6773 (comment) which was getting hidden for being on an outdated diff).

- Prefer pass-by-value for primitive types. - Use floating point precision for yStretch to match receiving type.

Make 'edges' static to avoid every instance of CollisionTile having its own copy of it.

Improve findPlacementScale semantics by moving the check if the placement scale result is bigger than minimum scale out of the function scope.

To obtain precise results, we: 1. Round scale value to obtain same results from symbol shader. 2. Generate a boost geometry polygon to check if it intersects() against all feature boxes. 3. Check if current scale is within each feature's minimum and maximum placement scales. 4. De-scale feature boxes when intersecting to account for the fractional zoom scaling.

Now testing 'query-tests/symbol-features-in/pitched-screen'.

ansis · 2016-10-24T22:53:57Z

The changes look good to me!

Port of mapbox/mapbox-gl-native#6773.

brunoabinader added bug ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold Core The cross-platform C++ core, aka mbgl labels Oct 20, 2016

brunoabinader force-pushed the collision-tile-fixes branch from b7143f3 to 54a3b9f Compare October 20, 2016 19:41

brunoabinader force-pushed the collision-tile-fixes branch from 54a3b9f to 600af3e Compare October 21, 2016 19:39

brunoabinader added ✓ ready for review and removed ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold labels Oct 21, 2016

ansis reviewed Oct 21, 2016

View reviewed changes

brunoabinader force-pushed the collision-tile-fixes branch from 600af3e to abad80a Compare October 21, 2016 22:10

brunoabinader force-pushed the collision-tile-fixes branch from abad80a to 295efed Compare October 22, 2016 17:46

brunoabinader force-pushed the collision-tile-fixes branch 4 times, most recently from defc47d to 5f64dc9 Compare October 24, 2016 12:43

ansis suggested changes Oct 24, 2016

View reviewed changes

ansis reviewed Oct 24, 2016

View reviewed changes

[core] Cleanup function signatures in CollisionTile

cc8d32c

- Prefer pass-by-value for primitive types. - Use floating point precision for yStretch to match receiving type.

brunoabinader added 3 commits October 25, 2016 01:40

[core] Avoid duplicating edges for CollisionTile

25faac2

Make 'edges' static to avoid every instance of CollisionTile having its own copy of it.

[core] Simplify CollisionTile::findPlacementScale

d0b4914

Improve findPlacementScale semantics by moving the check if the placement scale result is bigger than minimum scale out of the function scope.

[core] Added util::polygonIntersectsPolygon

97c5181

brunoabinader force-pushed the collision-tile-fixes branch from 5f64dc9 to 0254968 Compare October 24, 2016 22:40

brunoabinader added 3 commits October 25, 2016 01:45

[glfw] Added queryPointAnnotations shortcut

2dae720

[test] Update mapbox-gl-test-suite hash

94c02c4

Now testing 'query-tests/symbol-features-in/pitched-screen'.

brunoabinader force-pushed the collision-tile-fixes branch from 0254968 to 94c02c4 Compare October 24, 2016 22:45

ansis approved these changes Oct 24, 2016

View reviewed changes

brunoabinader merged commit fb9fbef into master Oct 25, 2016

brunoabinader deleted the collision-tile-fixes branch October 25, 2016 05:12

This was referenced Oct 25, 2016

Querying rendered features returns wrong values in perspective view #6630

Closed

Map::queryPointAnnotations() -> Map::queryRenderedFeatures() returns unexpected result as map is moved #6055

Closed

This was referenced Oct 25, 2016

[Android] Dispatch touch event from MarkerView to MapView #5388

Closed

[android] #5388 - moves touch handling of MarkerViews to core #5639

Merged

brunoabinader mentioned this pull request Oct 25, 2016

Fix bug preventing "queryRenderedFeatures" from returning all features mapbox/mapbox-gl-js#2647

Closed

brunoabinader added a commit to mapbox/mapbox-gl-js that referenced this pull request Oct 25, 2016

Reuse last placement data in CollisionTile.queryRenderedSymbols()

af691b4

Port of mapbox/mapbox-gl-native#6773.

brunoabinader mentioned this pull request Oct 25, 2016

Fix queryRenderedFeatures for symbol features mapbox/mapbox-gl-js#3459

Merged

brunoabinader added a commit to mapbox/mapbox-gl-js that referenced this pull request Oct 25, 2016

Reuse last placement data in CollisionTile.queryRenderedSymbols()

b1cb5a0

Port of mapbox/mapbox-gl-native#6773.

brunoabinader added a commit to mapbox/mapbox-gl-js that referenced this pull request Oct 25, 2016

Reuse last placement data in CollisionTile.queryRenderedSymbols()

b876b1b

Port of mapbox/mapbox-gl-native#6773.

brunoabinader added a commit to mapbox/mapbox-gl-js that referenced this pull request Oct 25, 2016

Reuse last placement data in CollisionTile.queryRenderedSymbols()

c8d04a6

Port of mapbox/mapbox-gl-native#6773.

brunoabinader added a commit to mapbox/mapbox-gl-js that referenced this pull request Oct 25, 2016

Reuse last placement data in CollisionTile.queryRenderedSymbols()

b68f607

Port of mapbox/mapbox-gl-native#6773.

brunoabinader added a commit to mapbox/mapbox-gl-js that referenced this pull request Oct 25, 2016

Reuse last placement data in CollisionTile.queryRenderedSymbols()

683fbaf

Port of mapbox/mapbox-gl-native#6773.

boundsj mentioned this pull request Nov 2, 2016

Crash on launch after updating to ios-v3.4.0-beta.2 from ios-v3.4.0-beta.1 #6883

Closed

1ec5 removed the ✓ ready for review label Nov 29, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core] Fix queryRenderedFeatures for colliding symbol features #6773

[core] Fix queryRenderedFeatures for colliding symbol features #6773

brunoabinader commented Oct 20, 2016

mention-bot commented Oct 20, 2016

brunoabinader commented Oct 20, 2016

ansis commented Oct 20, 2016

brunoabinader commented Oct 20, 2016

ansis commented Oct 20, 2016 •

edited

Loading

jfirebaugh commented Oct 20, 2016

ansis commented Oct 21, 2016

mb12 commented Oct 21, 2016

ansis commented Oct 21, 2016

brunoabinader commented Oct 21, 2016

ansis Oct 21, 2016

brunoabinader Oct 21, 2016

ansis Oct 21, 2016

jfirebaugh commented Oct 22, 2016

brunoabinader commented Oct 22, 2016 •

edited

Loading

brunoabinader commented Oct 22, 2016

ansis Oct 24, 2016

ansis Oct 24, 2016

brunoabinader Oct 24, 2016

ansis Oct 24, 2016

ansis Oct 24, 2016 •

edited

Loading

ansis commented Oct 24, 2016

[core] Fix queryRenderedFeatures for colliding symbol features #6773

[core] Fix queryRenderedFeatures for colliding symbol features #6773

Conversation

brunoabinader commented Oct 20, 2016

mention-bot commented Oct 20, 2016

brunoabinader commented Oct 20, 2016

ansis commented Oct 20, 2016

brunoabinader commented Oct 20, 2016

ansis commented Oct 20, 2016 • edited Loading

jfirebaugh commented Oct 20, 2016

ansis commented Oct 21, 2016

mb12 commented Oct 21, 2016

ansis commented Oct 21, 2016

brunoabinader commented Oct 21, 2016

ansis Oct 21, 2016

Choose a reason for hiding this comment

brunoabinader Oct 21, 2016

Choose a reason for hiding this comment

ansis Oct 21, 2016

Choose a reason for hiding this comment

jfirebaugh commented Oct 22, 2016

brunoabinader commented Oct 22, 2016 • edited Loading

brunoabinader commented Oct 22, 2016

ansis Oct 24, 2016

Choose a reason for hiding this comment

ansis Oct 24, 2016

Choose a reason for hiding this comment

brunoabinader Oct 24, 2016

Choose a reason for hiding this comment

ansis Oct 24, 2016

Choose a reason for hiding this comment

ansis Oct 24, 2016 • edited Loading

Choose a reason for hiding this comment

ansis commented Oct 24, 2016

ansis commented Oct 20, 2016 •

edited

Loading

brunoabinader commented Oct 22, 2016 •

edited

Loading

ansis Oct 24, 2016 •

edited

Loading