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

Supercluster.hpp 0.3.0 update #12726

Closed
wants to merge 7 commits into from
Closed

Supercluster.hpp 0.3.0 update #12726

wants to merge 7 commits into from

Conversation

tobrun
Copy link
Member

@tobrun tobrun commented Aug 23, 2018

This PR exposes the getChildren, getLeaves and getClusterExpansionZoom from v0.3.0 of supercluster.hpp (see mapbox/supercluster.hpp#16 for upstream PR).

Wasn't really sure about what the best approach is to expose these methods to the public API of geojson_source.cpp. For now, I introduced virtual methods that are no-op for GeoJSONVTData. Lmk if there is a cleaner way of handling this.

@tobrun tobrun added the Core The cross-platform C++ core, aka mbgl label Aug 23, 2018
@tobrun tobrun self-assigned this Aug 23, 2018
@tobrun
Copy link
Member Author

tobrun commented Aug 24, 2018

Going to refactor this using the visitor pattern as this would be more clean as the virtual function approach.

Copy link
Contributor

@kkaefer kkaefer left a comment

Choose a reason for hiding this comment

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

This PR adds a few functions, but they don't seem to be used or tested anywhere?

@mourner
Copy link
Member

mourner commented Oct 12, 2018

See also mapbox/mapbox-gl-js#6829 for discussion on unit tests for this in the JS version.

@tobrun
Copy link
Member Author

tobrun commented Oct 12, 2018

@kkaefer android integration is in #12727, I can work on updating that PR to current state and merging into here.

update: iOS in #12952

@alexshalamov
Copy link
Contributor

@kkaefer @mourner Added unit test for GeoJSONSource d4485c9 do you think it is sufficient?

Also, noticed that GeoJSONSource::Impl shares its private data as a raw pointer and GeoJSONSource doesn't check whether data is null. Fixed in fce4bff. Do you think it is required? If not, I will drop that commit.

@alexshalamov
Copy link
Contributor

@kkaefer @tobrun could you please take a look at this PR when you have time.

Copy link
Member Author

@tobrun tobrun left a comment

Choose a reason for hiding this comment

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

Thank you for fixing up the code and the tests look great! Since I added a part of the code, I don't feel that I'm the right person to sign off on this. @kkaefer can you take a look?

Copy link
Member

@mourner mourner left a comment

Choose a reason for hiding this comment

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

This looks great to me from the API standpoint. Can't comment on the C++ specifics though (pointer/type changes), maybe needs another pair of eyes.

ASSERT_NE(geojson_source, nullptr);
EXPECT_FALSE(geojson_source->getClusterChildren(1).empty());
EXPECT_FALSE(geojson_source->getClusterLeaves(1).empty());
EXPECT_NE(geojson_source->getClusterExpansionZoom(1), 0);
Copy link
Member

Choose a reason for hiding this comment

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

Can we test that the API returns specific values here instead of just making sure they're not default ones like for non-clustered sources?

Copy link
Contributor

Choose a reason for hiding this comment

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

@mourner Done. I noticed that default GeoJSONSource clustering options differ from supercluster's defaults. Should default options be synced or there was a specific reason for different default options?

Copy link
Member

Choose a reason for hiding this comment

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

Supercluster (the JS version) can be used in different contexts than GL Native (e.g. with Leaflet), so it's not a problem that we don't match default options.

GeoJSONSource::Impl shares its private data as a raw pointer, this
may lead to unwanted side-effects. Moreover GeoJSONSource, used to
access its Impl data without checking whether data is valid, this PR
fixes both issues.
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.

There is a synchronization issue exposed by the new methods on mbgl::style::GeoJSONSource. These methods return content from the current data available on the main thread. Cluster node information from queryRenderedFeatures is retrieved from the data available on the render thread.

Using cluster_ids from results of qRF data may not match the data on the main thread when already modified with a call to GeoJSONSource::setURL or GeoJSONSource::setGeoJSON.

One way to provide the results here is to use a ClusterFeature class for cluster nodes and implement that in the supercluster module or in mbgl::style::SuperclusterData. Features from qRF can then be cast to ClusteredFeature which would provide the getLeaf, getChildren, and getExpansionZoom methods. That said, each of these instances would need a shared_ptr<> back to to the root data.

Alternately, the new APIs can be provided in Renderer with a sourceId. That would move the implementation of the methods to RenderGeoJSONSource. Mobile SDKs already forward the qRF invocations from source peers to the renderer, so their implementation would remain on the source objects.

@@ -63,7 +63,7 @@ void RenderGeoJSONSource::update(Immutable<style::Source::Impl> baseImpl_,
util::tileSize,
impl().getZoomRange(),
optional<LatLngBounds>{},
[&] (const OverscaledTileID& tileID) {
[&, data = data_] (const OverscaledTileID& tileID) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: use data_ or rename so as to not shadow the member variable.


if (data_ != data) {
if (data.lock() != data_) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it necessary to create the shared_ptr<> here for the comparison or can you compare the internal pointers directly?

ASSERT_NE(geojson_source, nullptr);

auto children = geojson_source->getClusterChildren(1);
EXPECT_EQ(children.size(), 4U);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: use lower-case u to match style convention in this repo.

@alexshalamov
Copy link
Contributor

@asheemmamoowala Thanks for review.

Features from qRF can then be cast to ClusteredFeature which would provide the getLeaf, getChildren, and getExpansionZoom methods.

Would that require heap allocated Features, right? At the moment most of the code passes vectors of POD Features.

If I would move methods to Renderer, do you think that would acceptable?

@asheemmamoowala
Copy link
Contributor

Would that require heap allocated Features, right? At the moment most of the code passes vectors of POD Features

Ahhh, good catch @alexshalamov . Yes, they would not be POD-types anymore, and would have to hold shared_ptr<>s back to the mbgl::style::SuperclusterData they reference.
Thinking through this further, it would require the platform SDKs use wrapper types to hold on to the underlying core object (they use copy core data to POD types now), and be aware of it's lifetime. This is not how the SDKs return data from core at the moment, so it would be a change to how those APIs are written.

Using the Renderer to front the required methods seems awkward if they only apply to supercluster-based GeoJSON sources. This might be OK in core, because the platform SDKs can (as they do with qRF) expose the public facing methods from the Source peer wrapper.

cc @1ec5 @tobrun

@alexshalamov
Copy link
Contributor

@tobrun Do you have any suggestions?
@asheemmamoowala can querySourceFeatures be used to get more data about the Feature or it was meant to be used for different use-cases?

If I understand correctly, generic problem is that there should be an interface for querying additional data about the Feature provided by the source. For example, when features are generated at runtime.

@asheemmamoowala
Copy link
Contributor

can querySourceFeatures be used to get more data about the Feature or it was meant to be used for different use-cases?

querySourceFeatures is meant to query all available tiles of a source. It is similar to queryRenderedFeatures but not limited to visible features or the visible screen bounds. I don't think it is the appropriate way to query for additional information about specific features.

generic problem is that there should be an interface for querying additional data about the Feature provided by the source

This is a good way to think about this problem. How can different classes of features be queried for their particular unique datum. For the supercluster nodes though it does not address the need for identifying nodes using clusterId, as opposed to featureId, but maybe that can be made opaque to the interface by using a method like:

class Renderer {
...
mbgl::Value queryFeatureExtensions(
    std::string sourceId,
    optional<std::string> sourceLayer,
    const mbgl::Feature& feature,
    std::string extension,
    std::string extensionField );
}

//Usage:
auto zoomValue = queryFeatureExtensions("my_geosjon", {}, featureFromQRF, "supercluster", "expansionZoom" );

The "supercluster" extension would be registered with the Renderer::Impl and implemented on the RenderGeoJSONSource. Still need to come up with a way to statically register these, but I'll keep looking into it.

@alexshalamov
Copy link
Contributor

@asheemmamoowala could you please take a look at #13382 and tell what do you think?

@tobrun
Copy link
Member Author

tobrun commented Dec 20, 2018

This PR is now obsolete with #13631

@tobrun tobrun closed this Dec 20, 2018
@tobrun tobrun deleted the tvn-next-gen-cluster branch December 20, 2018 18:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Core The cross-platform C++ core, aka mbgl
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants