Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

[core] Add queryFeatureExtension API #13382

Merged
merged 4 commits into from
Dec 12, 2018

Conversation

alexshalamov
Copy link
Contributor

@alexshalamov alexshalamov commented Nov 15, 2018

New API allows to query additional information about rendered features.
Implementation of #12726 (comment)
Based on PR #12726 made by @tobrun

This PR adds supercluster extension that is implemented in geojson render source and provides access to getLeaves, getChildren and getClusterExpansion zoom methods.

  • Expose new supercluster methods for geojson data
  • Expose new queryFeatureExtension API
  • Add unit tests

@alexshalamov alexshalamov force-pushed the alexshalamov_queryFeatureExtensions branch 2 times, most recently from bed2c2e to 847e1f7 Compare November 15, 2018 16:08
Copy link
Contributor

@asheemmamoowala asheemmamoowala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great start.

Continuing from RenderGeoJSONSource(comment), I'm curious if there is a real need for the FeatureExtension class. What can be enabled by allowing multiple feature query extensions on sources? Maybe use this to join features across tiles or return the original GeoJSON geometry.

src/mbgl/renderer/sources/render_geojson_source.cpp Outdated Show resolved Hide resolved
src/mbgl/renderer/sources/render_geojson_source.cpp Outdated Show resolved Hide resolved
src/mbgl/renderer/renderer_impl.cpp Outdated Show resolved Hide resolved
@@ -76,6 +76,16 @@ class RenderSource : protected TileObserver {

void setObserver(RenderSourceObserver*);

class FeatureExtension {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this extension be used for more than just a single query method? If not, could this be renamed to FeatureQueryExtension or removed entirely and the multiple extensions be managed in the subclasses?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed intermediate object for simplicity. I have a question about proposal:
#12726 (comment)

mbgl::Value queryFeatureExtensions(
    std::string sourceId, <-- Source  / RenderSource
    optional<std::string> sourceLayer,
    const mbgl::Feature& feature,
    std::string extension, <-- Extension
    std::string extensionField );
}

It implies that one source might have multiple extensions, right? That made me thinking that it might be easier to manage separate extension for particular source in a separate implementation. So that render can do something like:

if (FeatureExtension* ext = renderSource->getExtension(extensionID)) {
   return extension->queryFeatureExtension(...);
}

Yet, that is an implementation detail, it can be changed if needed. Public facing interface is not affected in any way.

Copy link
Contributor

@asheemmamoowala asheemmamoowala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even though there is only one extension for now, the supercluster extension should be named and used. I'm guessing that it is still a //TODO, but just wanted to make sure that it gets used.

src/mbgl/renderer/render_source.hpp Outdated Show resolved Hide resolved
@alexshalamov
Copy link
Contributor Author

@tobrun whenever you have a time, could you please check if new API is compatible with jni wrappers. I quickly checked and imo, types should be easily convertible.

@alexshalamov alexshalamov force-pushed the alexshalamov_queryFeatureExtensions branch 3 times, most recently from 1ac2e2e to d163b76 Compare November 19, 2018 09:38
@alexshalamov
Copy link
Contributor Author

@kkaefer could you please take a look?

@alexshalamov
Copy link
Contributor Author

@tobrun has anyone considered using supercluster on the "client" side? Even with generic queryFeatureExtensions, it feels that the new API would be used exclusively for supercluster.

For example, what if on Android, similar functionality would be implemented like:

...
this.supercluster = new Supercluster(map.querySourceFeatures("supercluster-source"));
...
boolean onMapClick(...) {
    Features features = map.qRF(screenCoord);
    Features children = supercluster.getChildren(features[0].getNumberProperty("cluster_id"));
}
...

Are there other types of sources that might benefit from generic queryFeatureExtensions?

@tobrun
Copy link
Member

tobrun commented Nov 29, 2018

@alexshalamov that proposed example would work for us, we can always build some facility methods around this to make integration for end users easier.

Are there other types of sources that might benefit from generic queryFeatureExtensions?

Not afaik

@julianrex
Copy link
Contributor

@alexshalamov Just making a note that once this lands, I'll need to update my iOS PR: #12952 (thanks @tobrun)

@alexshalamov alexshalamov force-pushed the alexshalamov_queryFeatureExtensions branch from d163b76 to 3db3d9e Compare December 10, 2018 14:14
@alexshalamov alexshalamov changed the title [core][wip] Add queryFeatureExtension [core] Add queryFeatureExtension API Dec 10, 2018
test/api/query.test.cpp Outdated Show resolved Hide resolved
New interface allows it's users to query additional information about
feature that was provided by qRF interface. This is particularly
useful for clustered features.
@alexshalamov alexshalamov force-pushed the alexshalamov_queryFeatureExtensions branch from 3db3d9e to a5fba64 Compare December 12, 2018 12:01
@alexshalamov alexshalamov merged commit 191d47b into master Dec 12, 2018
@alexshalamov alexshalamov deleted the alexshalamov_queryFeatureExtensions branch December 12, 2018 13:08
julianrex added a commit that referenced this pull request Jan 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants