implement queryRenderedFeatures

[ansis]

queryRenderedFeatures is mostly implemented. 48/50 query tests are passing. Two are skipped because different constrain behaviour is causing errors.

Questions

• What should queryRenderedFeatures argument signatures be? js api here
• What should the return type be? This will use geometry.hpp, right?
• Should we support filters? Can FilterExpression be made public?

If any of the above are not quick to decide or quick to implement, can we punt and merge this as is? The current external api is not something we want to have people use but it would be good to have this in master with tests running on ci. It's already a huge pull request.

querySourceFeatures is not implemented because it depends heavily on the return type question. It can be done in a separate pr once that's answered.

---

@jfirebaugh can you review or assign reviewing to someone else?

---

my other remaining tasks are:

• fix this build error. Any ideas? https://www.bitrise.io/build/108bf7df608dc23c
• track down this segfault https://www.bitrise.io/build/e63b43e20310d3e0
• implement querying for symbols with -ignore-placement: true

[mb12]

@ansis Can you please kindly answer the following?

1.) It looks like all geometries are being inserted in the index by default (irrespective of whether or not the application uses query feature or not)? Is that correct (or not) and intentional? Can tree construction (loadTree call ) for a tile be postponed until the application actually uses query feature?

2.) In 'Source:: Source::queryRenderedFeatures' , under what scenarios would 'tileQueries' have more than one element? We can reduce the number of rtree lookups by picking only one tile per Source.

3.) Is it possible to explain what geometries are actually inserted in the index for symbols(esp. with text)?

[ansis]

Thanks for digging in @mb12

1.) It looks like all geometries are being inserted in the index by default (irrespective of whether or not the application uses query feature or not)? Is that correct (or not) and intentional? Can tree construction (loadTree call ) for a tile be postponed until the application actually uses query feature?

Yes, all geometries that are rendered are inserted into the index. It is partially intentional. I need to profile to see if index creation time is significant. One of advantage of doing it at tile load time is that it doesn't block the main thread. On the mapbox-gl-js side we use a different kind of index where insertions are really cheap: https://github.com/mapbox/grid-index

2.) In 'Source:: Source::queryRenderedFeatures' , under what scenarios would 'tileQueries' have more than one element?

When the first argument of queryRenderedFeatures is a box instead of a point

We can reduce the number of rtree lookups by picking only one tile per Source.

This is already done here. Tiles that don't intersect the query box are skipped.

3.) Is it possible to explain what geometries are actually inserted in the index for symbols(esp. with text)?

Symbol querying uses the index that was already created during the symbol placement process. Nothing is inserted into the main FeatureIndex tree. The CollisionTile tree holds a bunch of axis-aligned boxes that are anchored around points. You can render these boxes with MapDebugOptions::Collision.

[jfirebaugh]

const std::vector<ScreenCoordinate>& ⇢ const std::array<ScreenCoordinate, 2>&

[jfirebaugh]

How about the following single-method interface:

    using QueryGeometry = mapbox::util::variant<ScreenCoordinate, std::array<ScreenCoordinate, 2>>;
    std::vector<std::string> queryRenderedFeatures(const QueryGeometry&, const optional<std::vector<std::string>>& layerIDs = {});

[jfirebaugh]

Nah, never mind, overloading is better.

[ansis]

done d13f7b9

[jfirebaugh]

From what I understand of the abstract algorithm so far, it seems like the simplest natural expression is (pseudocode):

    // do whatever computation is needed for dateline logic, tile cover, etc.
    auto queryRegion = computeQueryRegion(input.query);
    std::vector<result> results;

    for (layer in style.layers) {
        if (layer in input.layers) {
            results.concat(layer.source.queryRenderedFeatures(layer, queryRegion, ...));
        }
    }

    return results;

What fundamentals am I missing, or what would it take to be able to implement it this way?

[jfirebaugh]

As context, I would find it very appealing if this could be implemented as an online algorithm. Not so much for efficiency's sake, just that they tend in the end to be the simplest expression of a lot of algorithms.

[ansis]

What fundamentals am I missing, or what would it take to be able to implement it this way?

All features are currently indexed in a single index. For each layer's query you'd be querying the index with the same region again and again and would need to filter out features from all other layers. I haven't tried, but I'm guessing this would be slower. It might be fine though since reading features from the pbf and doing geometry intersections is the expensive part.

Why not index layers individually? They were indexed in a group index earlier and I just stuck with that. I guessed it could be cheaper for queries that cover all layers (the default and worse case) and it could have a lower memory footprint for the transferable grid index that was written for -js. Now that I think about it, it could be fine. Symbols use the existing collision index though, which would need to be split up into separate indexes once parsing is done.

I agree that an online algorithm would be simpler. Do you think it's worth the effort to try to switch?

[mb12]

Should this loop be made cancellable/interruptible (or not)?
See the comment here:

#2644 (comment)

[ansis]

@jfirebaugh I think all your notes are addressed except #4656 (comment) which I think we should leave for later. Can you review the fixes?

[jfirebaugh]

Can you push the rebased / squashed version?

[ansis]

squashed version pushed

[jfirebaugh]

👍 Let's merge once CI is passing.

[ansis]

CI is passing.

The rtree is really slow when built in Debug mode. Creating the index adds a couple hundred ms. In Release mode it takes 10-15ms. Is this ok, or is this something that needs to be optimized before merging?

[jfirebaugh]

That's somewhat worrisome. Besides being an annoyance for testing a debug build, it's often the case that slow debug performance on a desktop will translate to slow release performance on a mobile device. Can you test this and optimize accordingly?

[mb12]

@ansis and @jfirebaugh On mobile devices with use model similar to (Google Maps, Nokia Here, maps.me or Apple Maps) linear search through the geometries should be good enough( query is asynchronous and/or number of queries is too low because of long touch/tap use model).

This is what the use model looks likes:

• Long touch or tap (Drops a pin or draw a circle around point of query)
• Panel slides up from bottom or the side
• The panel gets asynchronously populated

[ansis]

@jfirebaugh I ported GridIndex from -js: 41e6e4b
This should be fast enough now. Can you review that commit?

@mb12 thanks for the suggestion