CollisionTile::queryRenderedSymbols should receive the query geometry #6627
Conversation
|
@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. |
| std::unordered_map<std::string, std::unordered_set<std::size_t>> sourceLayerFeatures; | ||
|
|
||
| auto anchor = util::matrixMultiply(rotationMatrix, convertPoint<float>(box.min)); | ||
| mapbox::geometry::box<float> box { |
There was a problem hiding this comment.
This would be more clearly expressed as a rotating transform (using a for loop, std::transform, or a geometry.hpp specific version) followed by mapbox::geometry::envelope.
There was a problem hiding this comment.
Intuit was to avoid traversing through the geometries twice - first for transforming each point, and then later for obtaining the bounding box. However, I acknowledge the need for readability in complex code like this, so doing as suggested.
| @@ -153,12 +155,27 @@ Box CollisionTile::getTreeBox(const Point<float>& anchor, const CollisionBox& bo | |||
| }; | |||
| } | |||
|
|
|||
| std::vector<IndexedSubfeature> CollisionTile::queryRenderedSymbols(const mapbox::geometry::box<int16_t>& box, const float scale) { | |||
| std::vector<IndexedSubfeature> CollisionTile::queryRenderedSymbols(const GeometryCollection& queryGeometry, const float scale) { | |||
There was a problem hiding this comment.
I think we can simplify the parameter here, and various other places, to const GeometryCoordinates& queryGeometry. There's no need to support multipolygon query geometries: d7a816d.
There was a problem hiding this comment.
I perceived this too, though didn't wanted to change behavior - good to know 👍
brunoabinader
force-pushed
the
6627-collisiontile-queryrenderedsymbols-query-geometry
branch
2 times, most recently
from
cc1192a
to
5f524bc
Compare
|
Addressed @jfirebaugh review comments. |
brunoabinader
force-pushed
the
6627-collisiontile-queryrenderedsymbols-query-geometry
branch
2 times, most recently
from
83d8653
to
c956030
Compare
|
Unit test rendering check failure in Thus, I'm skipping rendering checks on |
This provides a means of testing cases where an updated geometry tile would return wrong results for `queryRenderedFeatures`.
This gives the ability to pan and/or rotate the map in a posterior step after initial render for testing purposes.
brunoabinader
force-pushed
the
6627-collisiontile-queryrenderedsymbols-query-geometry
branch
from
c956030
to
7dbc302
Compare
|
Render test failures were caused by rebasing the newly-added query tests |
brunoabinader
deleted the
6627-collisiontile-queryrenderedsymbols-query-geometry
branch
As per @ansis comments in #6055: