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

Make geometry conversion optional for QueryRenderedFeatures #9815

Closed
wants to merge 3 commits into from

Conversation

tobrun
Copy link
Member

@tobrun tobrun commented Aug 21, 2017

WIP, Closes #9605,

This PR adds the option to not convert geometry when querying for rendered features. There are use-cases where you are only interested in feature properties and not the geometry.

Some temp results in ms querying for 18 features using the android binding:

Execution time with testQueryRenderedFeaturesExecutionTime: 126
Execution time with testQueryRenderedFeaturesWithoutGeometryExecutionTime: 56

Execution time with testQueryRenderedFeaturesExecutionTime: 136
Execution time with testQueryRenderedFeaturesWithoutGeometryExecutionTime: 49

Execution time with testQueryRenderedFeaturesExecutionTime: 118
Execution time with testQueryRenderedFeaturesWithoutGeometryExecutionTime: 50

Todo:

  • implement for ScreenBox query
  • optimize android occurrences of QueryRenderedFeatures

@tobrun tobrun added Android Mapbox Maps SDK for Android Core The cross-platform C++ core, aka mbgl labels Aug 21, 2017
@tobrun tobrun added this to the android-v5.2.0 milestone Aug 21, 2017
@tobrun tobrun self-assigned this Aug 21, 2017
@@ -180,4 +180,11 @@ Feature convertFeature(const GeometryTileFeature& geometryTileFeature, const Can
return feature;
}

Feature convertFeatureProperties(const GeometryTileFeature& geometryTileFeature) {
Feature feature { Point<double>() };
Copy link
Contributor

Choose a reason for hiding this comment

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

Forcing all features to be of type point means that the results will not be able to tell the geometry_type of a feature.
How about a switch statement to create the feature with the correct geometry type ?

@tobrun tobrun force-pushed the 9605-geometry-conversion-optional-query branch 2 times, most recently from 8c0ab02 to ce6496f Compare September 14, 2017 13:29
@tobrun tobrun force-pushed the 9605-geometry-conversion-optional-query branch from ce6496f to 7702e87 Compare September 14, 2017 13:51
@@ -14,14 +14,18 @@ namespace mbgl {
class RenderedQueryOptions {
public:
RenderedQueryOptions(optional<std::vector<std::string>> layerIDs_ = {},
optional<style::Filter> filter_ = {})
optional<style::Filter> filter_ = {},
optional<bool> geometryConversion_ = true)
Copy link
Contributor

Choose a reason for hiding this comment

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

Since geometryConversion defaults to true, it doesn't need to be optional

: layerIDs(std::move(layerIDs_)),
filter(std::move(filter_)) {}
filter(std::move(filter_)),
geometryConversion(std::move(geometryConversion_)) {}
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to use std::move here

Feature convertFeature(const GeometryTileFeature& geometryTileFeature, const CanonicalTileID& tileID) {
Feature feature { convertGeometry(geometryTileFeature, tileID) };
static Feature::geometry_type convertGeometryType(const GeometryTileFeature& geometryTileFeature) {
GeometryCollection geometries = geometryTileFeature.getGeometries();
Copy link
Contributor

Choose a reason for hiding this comment

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

getGeometries() ends up processing polygons - which is one of the hits you want to avoid. You might need to add a GeometryTileFeature.getGeometryType() method to get the type of the underlying geometry, as opposed to FeatureType

@tobrun tobrun removed this from the android-v5.2.0 milestone Oct 31, 2017
@tobrun
Copy link
Member Author

tobrun commented Nov 29, 2017

The issue I was trying to optimize for is now handled differently in #10267, don't see a need anymore for adding this. Closing.

@tobrun tobrun closed this Nov 29, 2017
@tobrun tobrun deleted the 9605-geometry-conversion-optional-query branch December 12, 2017 11:55
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Android Mapbox Maps SDK for Android Core The cross-platform C++ core, aka mbgl
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants