Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[core] #7319: Add support for Filters in queryRenderedFeatures #1

Closed
wants to merge 5 commits into from

Conversation

asheemmamoowala
Copy link
Owner

Change map.queryRenderedFeatures() to accept a query options struct similar to mapbox-gl-js. This change should satisfy mapbox#7319, although it does not add the 'layer' property as requested mapbox#7162.

This changes the core API and requires updating the mobile SDK bindings

The new signature is std::vector<Feature> queryRenderedFeatures(const ScreenCoordinate&, const QueryOptions& options = {});

QueryOptions struct accepts layerIds and filter parameters, but is expandable for more query options.

Usage:

    mbgl::style::EqualsFilter buildingTypeFilter = { "type" , std::string("building") };
    mbgl::style::GreaterThanEqualsFilter tallFilter = { "height" , 6.0 };
    mbgl::style::AllFilter  tallBuildingFilter = { {buildingTypeFilter, tallFilter} };
    std::vector<mbgl::Feature> features = map->queryRenderedFeatures(
        screenCoordinate, {
            {},
            { tallBuildingFilter } 
        }
    );

Development Notes:

I chose to rework the existing QueryParameters struct as that internal struct mixed external and internal types and could not be reused for an external facing API such as map.queryRenderedFeatures().

To minimize future changes and to ease adding additional query parameters I chose to add QueryOptions and use it for all external facing parameters. QueryParameters was then flattened out and removed.

Testing:

This PR includes unit tests in core, and updated node bindings that can be used with new query tests added to the mapbox-gl-js integration test suite with this PR.

I also validated with some changes to the macosapp but stopped short of updating the bindings. The macosapp has minimal changes to compile and run but does not yet support filters in [map visibleFeautresInRect:] and [map visibileFeaturesAtPoint:].

While I have played with some local changes to the macos app, I haven't done a thorough benchmark test to gauge the performance of compound filters such as All,Any.

@jfirebaugh
Copy link

Thanks @asheemmamoowala, this is great. I opened a PR for this in the main repo to run CI and prepare for merge: mapbox#8246.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants