-
Notifications
You must be signed in to change notification settings - Fork 1.3k
[core] Pre filter sources when querying by layer #6514
Conversation
@tmpsantos, thanks for your PR! By analyzing this pull request, we identified @jfirebaugh, @1ec5 and @incanus to be potential reviewers. |
/cc @brunoabinader |
@@ -199,6 +199,10 @@ std::unordered_map<std::string, std::vector<Feature>> Source::Impl::queryRendere | |||
return result; | |||
} | |||
|
|||
if (parameters.sourceIDs && parameters.sourceIDs->find(id) == parameters.sourceIDs->end()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer this check to be done in Style::queryRenderedFeatures
- this way we can avoid the creation of result
for sources not present in sourceIDs
.
0ecabc0
to
d0ceb18
Compare
d0ceb18
to
266ad02
Compare
@tmpsantos This is great. cc: @ivovandongen for adapting the Android API. |
For the record Annotations will get a perf bump for free. |
266ad02
to
34dc6de
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than adding new API surface area, can we optimize this case similarly to mapbox/mapbox-gl-js#2877?
Not sure about pre-filtering automatically. Seems like a valid optimization when we don't set the source filter, but I like the filter parameter specially for annotations and the use case that motivated me write this PR in the first place: query features on from a source I created using the dataset editor. I wanted these features to get in sync with native views, so they needed to be queried extremely fast. Looking at the code, the layers are stored in a |
Wouldn’t #5792 be a more direct API for this use case? |
The low level core API I'm proposing could be used by the platform bindings for implementing this API. |
Isn't this complimentary to explicitly specifying the sources to query? I wouldn't mind a little more api surface to have that flexibility here. |
This would be a departure from how GL JS implements feature querying: |
34dc6de
to
b178987
Compare
b178987
to
5e15919
Compare
@jfirebaugh I re-enabled the benchmarks tests on this PR and wrote a benchmark for querying features. Pre-filtering sources gives a 64x performance bump when the layer you are looking for is in a source not so dense (like annotations or a geojson source). Explicitly specifying the source makes no measurable difference compared to the pre-filter approach and exposes a new API surface. So let's do how you suggested. 👍 |
Pre-filtering sources:
Before the patch:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
5e15919
to
bd21249
Compare
Feature querying is extremelly slow because it goes through all the sources and these sources can be dense. This PR makes it possible to filter by source.
Annotations will benefit from it because they live in their own "virtual" source. Also if you have a low density source for say, road disruptions, you can make the query extremely responsive by querying only on this source.