[core] data-driven styling support for *-pattern properties #12284

mollymerp · 2018-07-03T04:41:28Z

corresponding PR to mapbox/mapbox-gl-js#6289

TODO:

fix compile errors on non-macos platforms
fix strange rendering behavior when using dds
add pattern features to feature index
clean up code + commit history
dynamicly bind cross-faded vertex buffers based on current Faded values
extend line-pattern support to other -pattern properties
add tests and update gl-js commit hash

thanks to @kkaefer, @anandthakker, and @jfirebaugh for all the help wrangling the gnarly templating challenges in this one!

mollymerp · 2018-08-08T05:08:01Z

still a few things to do but this is working for the most part and ready for review

diagnose and fix seg fault/bad access when zooming in/out with a fill-extrusion-pattern layer
figure out if geometry_tile_worker state changing with needsPatternLayout makes sense (possibly related to above)
determine if failing render test is an existing issue or related to this PR (Investigate extruded fill-pattern pixel mismatch in render tests mapbox-gl-js#3327)
fix windows tuple compile errors
try to reduce binary size growth
add unit tests?
merge gl-js PR to get nitpick to pass

tmpsantos · 2018-08-09T07:41:12Z

include/mbgl/util/tuple.hpp

@@ -2,18 +2,20 @@

 // Polyfill needed by Windows because MSVC STL
 // is not compatible with our IndexedTuple code
-#if defined(_WINDOWS)
+#if 0


We should remove the polyfill from the code and tao from cmake if it is no longer needed.

Mean't to request a change, not approve.

kkaefer

Wow, what a pull request! This already looks fairly polished to me; I didn't notice any changes that still need to happen on a code level; all my comments are about code formatting.

@jfirebaugh, do you have any ideas for how we could reduce the code size increase of this PR?

kkaefer · 2018-08-10T13:01:52Z

include/mbgl/util/indexed_tuple.hpp

@@ -48,6 +49,18 @@ class IndexedTuple<TypeList<Is...>, TypeList<Ts...>> : public tuple_polyfill<Ts.
            other.template get<Js>()...
        };
    }
+
+    // Help out MSVC++
+    bool operator==(const IndexedTuple<TypeList<Is...>, TypeList<Ts...>>& other) const {


why is the second parameter not const here?

kkaefer · 2018-08-10T13:09:18Z

src/mbgl/gl/uniform.hpp

@@ -42,18 +35,19 @@ ActiveUniforms activeUniforms(ProgramID);
 template <class Tag, class T>
 class Uniform {
 public:
-    using Value = UniformValue<Tag, T>;
+    // TODO should maybe remove this altogether bc it is now redundant?
+    using Value = T;


kkaefer · 2018-08-10T13:24:38Z

src/mbgl/renderer/buckets/circle_bucket.cpp

@@ -39,7 +39,8 @@ bool CircleBucket::hasData() const {
 }

 void CircleBucket::addFeature(const GeometryTileFeature& feature,
-                              const GeometryCollection& geometry) {
+                                     const GeometryCollection& geometry,
+                                     const ImagePositions&) {


Nit: indentation

kkaefer · 2018-08-10T13:24:57Z

src/mbgl/renderer/buckets/circle_bucket.hpp

@@ -18,7 +18,8 @@ class CircleBucket : public Bucket {
    CircleBucket(const BucketParameters&, const std::vector<const RenderLayer*>&);

    void addFeature(const GeometryTileFeature&,
-                    const GeometryCollection&) override;
+                            const GeometryCollection&,
+                            const mbgl::ImagePositions&) override;


Nit: indentation

kkaefer · 2018-08-10T13:32:32Z

src/mbgl/renderer/paint_property_binder.hpp

+template <class T, class A1, class A2>
+class CompositeCrossFadedPaintPropertyBinder : public PaintPropertyBinder<T, std::array<uint16_t, 4>, PossiblyEvaluatedPropertyValue<Faded<T>>, A1, A2> {
+public:
+    // to fix -- we will want to use both attributes


Does this still need addressing?

kkaefer · 2018-08-10T13:35:39Z

src/mbgl/tile/geometry_tile_worker.cpp

@@ -328,7 +338,16 @@ void GeometryTileWorker::parse() {
        }
    }

+    std::vector<std::string> patternOrder;
+    for (auto it = layers->rbegin(); it != layers->rend(); it++) {


Why are we iterating backwards here?

jfirebaugh · 2018-08-11T00:09:08Z

Here's bloaty size diff output for the macOS dynamic framework: https://gist.github.com/jfirebaugh/40eeb26948d13076990952441401de39

mollymerp · 2018-08-22T21:54:37Z

@jfirebaugh would you mind taking a look specifically at the changes in GeometryTileWorker to make sure the changes to state management make sense wrt the PatternLayout step? Mainly I'm curious if patternNeedsLayout is being used in a way that makes sense in the existing state machine and that the changes to symbolDependenciesChanged() look ok.

jfirebaugh · 2018-08-28T22:47:59Z

src/mbgl/tile/geometry_tile_worker.hpp

+    using FillPatternLayout = PatternLayout<FillBucket>;
+    using FillExtrusionPatternLayout = PatternLayout<FillExtrusionBucket>;
+
+    std::vector<variant<std::unique_ptr<LinePatternLayout>, std::unique_ptr<FillPatternLayout>, std::unique_ptr<FillExtrusionPatternLayout>>> patternLayouts;


This is an instance where we'd be better off with an abstract Layout base class rather than using variant. Both SymbolLayout and PatternLayout should inherit from it. The goal should be that GeometryTileWorker can treat layouts uniformly:

Replace symbolLayouts and patternLayouts with a single std::vector<std::unique_ptr<Layout>> layouts member, and a single loop over all layouts in performSymbolLayout.

GeometryTileWorker has two steps, currently named parse and performSymbolLayout. Let's come up with better names. Perhaps prepare and finalize? While we're at it, we should also take the Symbol out of hasPendingSymbolDependencies. Symbols aren't so special anymore.

Make createLayout a virtual method in RenderLayer. It can return a null unique_ptr<Layout>, indicating that createBucket should be used instead. (Or should we make things completely uniform, and have a simple Layout subclass for every layer type, even those that don't have dependencies? Then we could call the two worker steps createLayouts and createBuckets, and have only a single RenderLayer::createLayout method. This approach would be more work, but sounds cleaner.)

You'll need to do some design work to figure out what the Layout interface should look like. It looks like it might be a single method createBucket (name open for discussion):

For the concrete subclass SymbolLayout, this would be a combination of the methods prepare and place.

For pattern layouts, there's already a createBucket method. Is it possible to remove the conditional in checkPatternLayout, always create the layout, and have it do the work that checkPatternLayout is doing now in the case that a pattern isn't being used?

To avoid GeometryTileWorker needing to iterate over concrete layerPaintProperties types, it's fine if createBucket takes as a mutable input parameter the buckets map and emplaces into it in a loop itself.

createBucket will need a lot of input parameters, not all of which are used by every subclass. That's okay. One pattern that's been effective in situations like this is to package all the parameters into a struct (BucketParameters, PropertyEvaluationParameters, etc.). This allows us to add or remove arguments without needing to modify the parameter list in every subclass.

I think these changes will make GeometryTileWorker easier to understand and modify, and hopefully it will help reduce the binary size impact as well.

…onstructors

…ure expressions in `*-pattern` properties

* The worker no longer needs to maintain symbol layer order. * No need for separate symbolLayoutsNeedPreparation state. That dates back to when we had "two phase" symbol layout. Now we can just check symbolLayouts.empty(). (Similarly for pattern layouts.) * No need to loop over symbol layouts twice in performSymbolLayout. Same reason as above. * Simplify iconAtlas initialization. It initialized via every possible branch, so just do it up front.

…support

mollymerp added feature GL JS parity For feature parity with Mapbox GL JS ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold Core The cross-platform C++ core, aka mbgl labels Jul 3, 2018

mollymerp force-pushed the crossfaded-dds branch 3 times, most recently from 22b5a65 to 03986b9 Compare July 4, 2018 00:02

jfirebaugh mentioned this pull request Jul 16, 2018

[core] Replace {Source,Camera,Composite}Function with PropertyExpression #12400

Merged

mollymerp force-pushed the crossfaded-dds branch 3 times, most recently from d31d3c2 to 0fa5cfd Compare August 3, 2018 22:42

mollymerp requested review from anandthakker and ansis August 8, 2018 00:09

mollymerp force-pushed the crossfaded-dds branch 3 times, most recently from 1b307e2 to e064984 Compare August 8, 2018 04:51

mollymerp force-pushed the crossfaded-dds branch 6 times, most recently from 3e45622 to 0ab9b7b Compare August 8, 2018 21:32

mollymerp requested a review from kkaefer August 9, 2018 00:29

tmpsantos previously approved these changes Aug 9, 2018

View reviewed changes

tmpsantos reviewed Aug 9, 2018

View reviewed changes

kkaefer reviewed Aug 10, 2018

View reviewed changes

mollymerp mentioned this pull request Aug 14, 2018

Data-driven styling support for *-pattern properties mapbox/mapbox-gl-js#6289

Merged

11 tasks

mollymerp force-pushed the crossfaded-dds branch from 06d0fc6 to b16e36c Compare August 22, 2018 21:41

mollymerp removed the request for review from anandthakker August 22, 2018 21:55

kkaefer approved these changes Aug 23, 2018

View reviewed changes

mollymerp force-pushed the crossfaded-dds branch 2 times, most recently from 9594235 to 5d0d582 Compare August 27, 2018 20:21

mollymerp removed the ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold label Aug 28, 2018

jfirebaugh reviewed Aug 28, 2018

View reviewed changes

mollymerp force-pushed the crossfaded-dds branch 2 times, most recently from 59566b9 to d26d7c0 Compare August 30, 2018 18:56

mollymerp mentioned this pull request Aug 31, 2018

Support GL face culling #12725

Merged

mollymerp force-pushed the crossfaded-dds branch from e21b240 to 3292677 Compare August 31, 2018 18:09

kkaefer and others added 12 commits August 31, 2018 12:03

[core] simplify attribute/location types

863dcd5

[core] rename gl::Attribute to gl::Attribute to remove overloaded naming

fa9348c

[core] allow passing multiple AttributeTypes to a PaintPropertyBinder

9ffc7cb

[core] introduce TypeListConcat and TypeList::ExpandInto

1c97afd

[core] move PaintProperty::Attribute to TypeList and pass to Binder c…

4d44f84

…onstructors

[core] Implement CrossFadedDataDrivenProperty to add support for feat…

34b3497

…ure expressions in `*-pattern` properties

[windows, qt] fix windows compiler errors, remove tao + tuple polyfill

a6bf7c5

[test] ignore legacy expression tests

8afc8e2

[core] remove redundant Uniform::Type alias

4de775c

[core] simplify dependency layout process and refactor data structures

4bea281

[ios, node, macos, android] update changelogs for feature expression …

4e48ca1

…support

mollymerp force-pushed the crossfaded-dds branch from 3292677 to 4e48ca1 Compare August 31, 2018 19:24

mollymerp merged commit 95b89e6 into master Aug 31, 2018

mollymerp mentioned this pull request Aug 31, 2018

[core] replace createBucket with createLayout for remaining layers #12789

Closed

1ec5 mentioned this pull request Sep 1, 2018

Copyedit iOS, macOS changelogs #12792

Merged

tmpsantos deleted the crossfaded-dds branch September 3, 2018 10:23

jfirebaugh mentioned this pull request Sep 11, 2018

Master ticket for remaining data-driven style properties mapbox/mapbox-gl-js#4434

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core] data-driven styling support for *-pattern properties #12284

[core] data-driven styling support for *-pattern properties #12284

mollymerp commented Jul 3, 2018 •

edited

Loading

mollymerp commented Aug 8, 2018 •

edited

Loading

tmpsantos Aug 9, 2018

kkaefer left a comment

kkaefer Aug 10, 2018

kkaefer Aug 10, 2018

kkaefer Aug 10, 2018

kkaefer Aug 10, 2018

kkaefer Aug 10, 2018

kkaefer Aug 10, 2018

jfirebaugh commented Aug 11, 2018

mollymerp commented Aug 22, 2018

jfirebaugh Aug 28, 2018 •

edited

Loading

[core] data-driven styling support for *-pattern properties #12284

[core] data-driven styling support for *-pattern properties #12284

Conversation

mollymerp commented Jul 3, 2018 • edited Loading

mollymerp commented Aug 8, 2018 • edited Loading

tmpsantos Aug 9, 2018

Choose a reason for hiding this comment

kkaefer left a comment

Choose a reason for hiding this comment

kkaefer Aug 10, 2018

Choose a reason for hiding this comment

kkaefer Aug 10, 2018

Choose a reason for hiding this comment

kkaefer Aug 10, 2018

Choose a reason for hiding this comment

kkaefer Aug 10, 2018

Choose a reason for hiding this comment

kkaefer Aug 10, 2018

Choose a reason for hiding this comment

kkaefer Aug 10, 2018

Choose a reason for hiding this comment

jfirebaugh commented Aug 11, 2018

mollymerp commented Aug 22, 2018

jfirebaugh Aug 28, 2018 • edited Loading

Choose a reason for hiding this comment

mollymerp commented Jul 3, 2018 •

edited

Loading

mollymerp commented Aug 8, 2018 •

edited

Loading

jfirebaugh Aug 28, 2018 •

edited

Loading