-
Notifications
You must be signed in to change notification settings - Fork 1.3k
[core] Add queryFeatureExtension API #13382
Conversation
bed2c2e
to
847e1f7
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.
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/render_source.hpp
Outdated
@@ -76,6 +76,16 @@ class RenderSource : protected TileObserver { | |||
|
|||
void setObserver(RenderSourceObserver*); | |||
|
|||
class FeatureExtension { |
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.
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?
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 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.
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.
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.
@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. |
1ac2e2e
to
d163b76
Compare
@kkaefer could you please take a look? |
@tobrun has anyone considered using supercluster on the "client" side? Even with generic 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 |
@alexshalamov that proposed example would work for us, we can always build some facility methods around this to make integration for end users easier.
Not afaik |
@alexshalamov Just making a note that once this lands, I'll need to update my iOS PR: #12952 (thanks @tobrun) |
d163b76
to
3db3d9e
Compare
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.
3db3d9e
to
a5fba64
Compare
…) following feature extension API (#13382)
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.