Account for property functions in query rendered features #8665

ivovandongen · 2017-04-06T15:09:04Z

ivovandongen · 2017-04-13T16:33:52Z

One last failing query test: "line-offset/property-function".

@kkaefer I'm thinking that FeatureIndex::query can use a little cleanup wrt the method arguments. Also, I'm not sure if it makes sense to move Layer::Impl::queryIntersectsFeature over to Bucket or not. Now, the layer gives back exact results, where the bucket uses the statistics to approach it. This might be a good split.

ivovandongen · 2017-04-14T14:00:05Z

I've tracked down the failure in "line-offset/property-function" to a separate bug. The test result is technically correct, but the resulting polygon is translated from:

[
  [
    [-10, 10],
    [10, 10],
    [10, -10],
    [-10, -10],
    [-10, 10]
  ]
]

To

[
  [
    [10, 10],
    [10, -10],
    [-10, -10],
    [-10, 10],
    [10, 10]
  ]
]

Which is functionally the same, but makes the test fail of course. The bug seems to be caused by a workaround in fixupPolygons called when loading the geometries in GeoJSONTileFeature::getGeometries. I'm filing a separate bug for this.

ivovandongen · 2017-04-18T20:40:43Z

Fixed up the query tests in mapbox/mapbox-gl-js#4603

jfirebaugh · 2017-04-18T21:42:35Z

src/mbgl/geometry/feature_index.hpp

@@ -1,6 +1,7 @@
 #pragma once

 #include <mbgl/style/types.hpp>
+#include <mbgl/tile/geometry_tile.hpp>


Prefer to forward-declare class GeometryTile; rather than #include.

jfirebaugh · 2017-04-18T21:43:30Z

src/mbgl/renderer/bucket.hpp

@@ -3,8 +3,11 @@
 #include <mbgl/renderer/render_pass.hpp>
 #include <mbgl/util/noncopyable.hpp>
 #include <mbgl/tile/geometry_tile_data.hpp>
+#include <mbgl/style/paint_property_statistics.hpp>


Are the added includes leftover from a prior version of this PR? They don't look like they're used.

jfirebaugh · 2017-04-18T21:50:20Z

src/mbgl/style/paint_property_binder.hpp

@@ -4,6 +4,7 @@
 #include <mbgl/gl/attribute.hpp>
 #include <mbgl/gl/uniform.hpp>
 #include <mbgl/util/type_list.hpp>
+#include "paint_property_statistics.hpp"


#include <mbgl/...>

jfirebaugh · 2017-04-18T22:07:39Z

src/mbgl/style/paint_property_binder.hpp

@@ -76,24 +77,24 @@ std::array<float, N*2> zoomInterpolatedAttributeValue(const std::array<float, N>
   `glVertexAttrib*` was unnacceptably slow. Additionally, in GL Native we have
   implemented binary shader caching, which works better if the shaders are constant.
 */
-template <class T, class A>
+template <class P, class T, class A, class Statistics>


Specializing on unique P and Statistics increases the binary size significantly:

~/Development/mapbox-gl-native $ ../bloaty/bloaty -d symbols build/macos/Release/Mapbox.framework/Versions/Current/Mapbox -- build-before/macos/Release/Mapbox.framework/Versions/Current/Mapbox VM SIZE FILE SIZE ++++++++++++++ GROWING ++++++++++++++ +95% +111Ki [Other] +111Ki +95% +2.0% +9.62Ki [None] +8.23Ki +1.7% [NEW] +1.06Ki mbgl::FeatureIndex::query(std::__1::unordered_map<std::__1::basic_string<char, s +1.06Ki [NEW] [NEW] +816 mbgl::style::LineLayer::Impl::queryIntersectsFeature(mbgl::GeometryCoordinates c +816 [NEW] [NEW] +632 mbgl::style::CompositeFunctionPaintPropertyBinder<mbgl::style::FillOutlineColor, +632 [NEW] [NEW] +620 mbgl::style::CompositeFunctionPaintPropertyBinder<mbgl::style::CircleColor, mbgl +620 [NEW] [NEW] +620 mbgl::style::CompositeFunctionPaintPropertyBinder<mbgl::style::IconHaloColor, mb +620 [NEW] [NEW] +620 mbgl::style::CompositeFunctionPaintPropertyBinder<mbgl::style::TextHaloColor, mb +620 [NEW] [NEW] +620 mbgl::style::CompositeFunctionPaintPropertyBinder<mbgl::style::CircleStrokeColor +620 [NEW] [NEW] +620 mbgl::style::CompositeFunctionPaintPropertyBinder<mbgl::style::FillColor, mbgl:: +620 [NEW] [NEW] +620 mbgl::style::CompositeFunctionPaintPropertyBinder<mbgl::style::IconColor, mbgl:: +620 [NEW] [NEW] +620 mbgl::style::CompositeFunctionPaintPropertyBinder<mbgl::style::LineColor, mbgl:: +620 [NEW] [NEW] +620 mbgl::style::CompositeFunctionPaintPropertyBinder<mbgl::style::TextColor, mbgl:: +620 [NEW] [NEW] +464 GCC_except_table681 +464 [NEW] [NEW] +434 mbgl::LineBucket::getQueryRadius(mbgl::style::Layer const&) const +434 [NEW] +46% +432 GCC_except_table37 +432 +46% [NEW] +426 mbgl::style::CircleLayer::Impl::queryIntersectsFeature(mbgl::GeometryCoordinates +426 [NEW] [NEW] +384 GCC_except_table717 +384 [NEW] [NEW] +352 mbgl::style::CompositeFunctionPaintPropertyBinder<mbgl::style::CircleBlur, float +352 [NEW] [NEW] +352 mbgl::style::CompositeFunctionPaintPropertyBinder<mbgl::style::LineOffset, float +352 [NEW] [NEW] +352 mbgl::style::CompositeFunctionPaintPropertyBinder<mbgl::style::FillOpacity, floa +352 [NEW] -------------- SHRINKING -------------- -12.8% -13.2Ki [Other] -13.2Ki -12.8% [DEL] -642 mbgl::FeatureIndex::query(std::__1::unordered_map<std::__1::basic_string<char, s -642 [DEL] [DEL] -630 mbgl::style::LineLayer::Impl::queryIntersectsGeometry(mbgl::GeometryCoordinates -630 [DEL] [DEL] -624 mbgl::style::CompositeFunctionPaintPropertyBinder<mbgl::Color, mbgl::gl::Attribu -624 [DEL] -65.3% -384 GCC_except_table718 -384 -65.3% [DEL] -358 mbgl::style::CircleLayer::Impl::queryIntersectsGeometry(mbgl::GeometryCoordinate -358 [DEL] [DEL] -336 mbgl::style::CompositeFunctionPaintPropertyBinder<float, mbgl::gl::Attribute<flo -336 [DEL] [DEL] -302 mbgl::style::FillLayer::Impl::queryIntersectsGeometry(mbgl::GeometryCoordinates -302 [DEL] [DEL] -292 mbgl::style::CompositeFunctionPaintPropertyBinder<mbgl::Color, mbgl::gl::Attribu -292 [DEL] [DEL] -286 mbgl::style::PaintPropertyBinder<mbgl::Color, mbgl::gl::Attribute<float, 2ul> >: -286 [DEL] -86.4% -280 GCC_except_table401 -280 -86.4% [DEL] -280 mbgl::style::CompositeFunctionPaintPropertyBinder<float, mbgl::gl::Attribute<flo -280 [DEL] -52.4% -260 GCC_except_table682 -260 -52.4% [DEL] -260 mbgl::style::PaintPropertyBinder<float, mbgl::gl::Attribute<float, 1ul> >::creat -260 [DEL] [DEL] -248 mbgl::style::PaintPropertyBinder<mbgl::Color, mbgl::gl::Attribute<float, 2ul> >: -248 [DEL] -40.1% -236 GCC_except_table180 -236 -40.1% [DEL] -228 mbgl::style::PaintPropertyBinder<float, mbgl::gl::Attribute<float, 1ul> >::creat -228 [DEL] [DEL] -224 mbgl::style::SourceFunctionPaintPropertyBinder<mbgl::Color, mbgl::gl::Attribute< -224 [DEL] -50.5% -220 GCC_except_table491 -220 -50.5% [DEL] -204 GCC_except_table683 -204 [DEL] [DEL] -200 mbgl::style::SourceFunctionPaintPropertyBinder<mbgl::Color, mbgl::gl::Attribute< -200 [DEL] +3.3% +112Ki TOTAL +110Ki +3.2%

Can we find a way to specialize only on the underlying type (e.g. float)?

Here's the approach I suggest: 2b83d39.

VM SIZE FILE SIZE ++++++++++++++ GROWING ++++++++++++++ +9.0% +8.72Ki [Other] +8.72Ki +9.0% +0.2% +1.21Ki [None] +1.28Ki +0.3% [NEW] +1.06Ki mbgl::FeatureIndex::query(std::__1::unordered_map<std::__1::basic_string<char, s +1.06Ki [NEW] [NEW] +816 mbgl::style::LineLayer::Impl::queryIntersectsFeature(mbgl::GeometryCoordinates c +816 [NEW] [NEW] +464 GCC_except_table681 +464 [NEW] +46% +432 GCC_except_table37 +432 +46% [NEW] +432 mbgl::LineBucket::getQueryRadius(mbgl::style::Layer const&) const +432 [NEW] [NEW] +426 mbgl::style::CircleLayer::Impl::queryIntersectsFeature(mbgl::GeometryCoordinates +426 [NEW] [NEW] +384 GCC_except_table717 +384 [NEW] [NEW] +348 mbgl::style::FillLayer::Impl::queryIntersectsFeature(mbgl::GeometryCoordinates c +348 [NEW] +116% +296 GCC_except_table179 +296 +116% +206% +280 GCC_except_table400 +280 +206% +182% +248 GCC_except_table231 +248 +182% +219% +228 GCC_except_table358 +228 +219% [NEW] +204 GCC_except_table412 +204 [NEW] [NEW] +184 std::__1::vector<mbgl::style::Layer const*, std::__1::allocator<mbgl::style::Lay +184 [NEW] [NEW] +178 std::__1::__tree_const_iterator<std::__1::__value_type<std::__1::basic_string<ch +178 [NEW] [NEW] +175 std::__1::__tree_const_iterator<std::__1::__value_type<std::__1::basic_string<ch +175 [NEW] [NEW] +164 std::__1::__tree_const_iterator<std::__1::__value_type<std::__1::basic_string<ch +164 [NEW] [NEW] +164 std::__1::__tree_const_iterator<std::__1::__value_type<std::__1::basic_string<ch +164 [NEW] [NEW] +162 mbgl::style::Style::getLayers() const +162 [NEW] -------------- SHRINKING -------------- -8.9% -7.34Ki [Other] -7.34Ki -8.9% [DEL] -642 mbgl::FeatureIndex::query(std::__1::unordered_map<std::__1::basic_string<char, s -642 [DEL] [DEL] -630 mbgl::style::LineLayer::Impl::queryIntersectsGeometry(mbgl::GeometryCoordinates -630 [DEL] -65.3% -384 GCC_except_table718 -384 -65.3% [DEL] -358 mbgl::style::CircleLayer::Impl::queryIntersectsGeometry(mbgl::GeometryCoordinate -358 [DEL] [DEL] -302 mbgl::style::FillLayer::Impl::queryIntersectsGeometry(mbgl::GeometryCoordinates -302 [DEL] -86.4% -280 GCC_except_table401 -280 -86.4% -64.2% -280 GCC_except_table491 -280 -64.2% -52.4% -260 GCC_except_table682 -260 -52.4% -40.1% -236 GCC_except_table180 -236 -40.1% [DEL] -204 GCC_except_table683 -204 [DEL] -80.7% -184 GCC_except_table359 -184 -80.7% -16.8% -184 GCC_except_table43 -184 -16.8% -13.9% -180 GCC_except_table38 -180 -13.9% -4.0% -180 GCC_except_table6 -180 -4.0% -14.2% -172 GCC_except_table41 -172 -14.2% [DEL] -162 mbgl::style::LineLayer::Impl::getQueryRadius() const -162 [DEL] -77.6% -152 GCC_except_table435 -152 -77.6% [DEL] -152 GCC_except_table723 -152 [DEL] -54.5% -144 GCC_except_table413 -144 -54.5% [DEL] -140 GCC_except_table499 -140 [DEL] +0.1% +4.00Ki TOTAL +4.06Ki +0.1%

ivovandongen · 2017-04-19T17:48:27Z

Thanks @jfirebaugh I've worked in the changes. Only blocked until #8760 is merged as the render tests are failing.

jfirebaugh · 2017-04-19T17:51:24Z

src/mbgl/style/paint_property_binder.hpp

@@ -136,6 +139,8 @@ class SourceFunctionPaintPropertyBinder : public PaintPropertyBinder<T, A> {
    }

    void populateVertexVector(const GeometryTileFeature& feature, std::size_t length) override {
+        auto evaluated = function.evaluate(feature, defaultValue);
+        this->statistics.add(evaluated);
        auto value = attributeValue(function.evaluate(feature, defaultValue));


I accidentally reverted this line in my changes, should be auto value = attributeValue(evaluated);

ivovandongen added Core The cross-platform C++ core, aka mbgl ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold labels Apr 6, 2017

ivovandongen self-assigned this Apr 6, 2017

anandthakker self-requested a review April 6, 2017 15:12

ivovandongen force-pushed the 8007-query-rendered-dds branch 3 times, most recently from 2b82df9 to 46f2d13 Compare April 13, 2017 16:28

ivovandongen requested a review from kkaefer April 13, 2017 16:29

ivovandongen force-pushed the 8007-query-rendered-dds branch from 46f2d13 to 0c8f1fc Compare April 18, 2017 20:39

ivovandongen removed the ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold label Apr 18, 2017

ivovandongen requested review from jfirebaugh and removed request for anandthakker April 18, 2017 20:40

ivovandongen force-pushed the 8007-query-rendered-dds branch from 0c8f1fc to 23134a7 Compare April 18, 2017 21:32

jfirebaugh suggested changes Apr 18, 2017

View reviewed changes

ivovandongen force-pushed the 8007-query-rendered-dds branch 2 times, most recently from 3833732 to 83d4618 Compare April 19, 2017 17:46

jfirebaugh approved these changes Apr 19, 2017

View reviewed changes

ivovandongen added 2 commits April 19, 2017 11:07

[core] property function statistics collection

46e7735

[core] make Tile::getBucket const

64f6073

ivovandongen force-pushed the 8007-query-rendered-dds branch 3 times, most recently from 87d4e57 to 33c7707 Compare April 19, 2017 21:29

[core] account for property functions in query rendered features

59a92db

ivovandongen force-pushed the 8007-query-rendered-dds branch from 33c7707 to 59a92db Compare April 19, 2017 23:24

ivovandongen merged commit 7ddca3b into master Apr 20, 2017

ivovandongen deleted the 8007-query-rendered-dds branch April 20, 2017 00:21

1ec5 mentioned this pull request May 1, 2017

Update changelogs #8853

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Account for property functions in query rendered features #8665

Account for property functions in query rendered features #8665

ivovandongen commented Apr 6, 2017

ivovandongen commented Apr 13, 2017

ivovandongen commented Apr 14, 2017 •

edited

Loading

ivovandongen commented Apr 18, 2017

jfirebaugh Apr 18, 2017

jfirebaugh Apr 18, 2017

jfirebaugh Apr 18, 2017

jfirebaugh Apr 18, 2017

jfirebaugh Apr 18, 2017

ivovandongen commented Apr 19, 2017

jfirebaugh Apr 19, 2017

Account for property functions in query rendered features #8665

Account for property functions in query rendered features #8665

Conversation

ivovandongen commented Apr 6, 2017

ivovandongen commented Apr 13, 2017

ivovandongen commented Apr 14, 2017 • edited Loading

ivovandongen commented Apr 18, 2017

jfirebaugh Apr 18, 2017

Choose a reason for hiding this comment

jfirebaugh Apr 18, 2017

Choose a reason for hiding this comment

jfirebaugh Apr 18, 2017

Choose a reason for hiding this comment

jfirebaugh Apr 18, 2017

Choose a reason for hiding this comment

jfirebaugh Apr 18, 2017

Choose a reason for hiding this comment

ivovandongen commented Apr 19, 2017

jfirebaugh Apr 19, 2017

Choose a reason for hiding this comment

ivovandongen commented Apr 14, 2017 •

edited

Loading