Property refactor #5682

jfirebaugh · 2017-11-15T16:56:27Z

Ports PropertyValue, TransitionablePropertyValue, PossiblyEvaluatedPropertyValue, etc. from native.

Fixes #2739
Fixes #3044
Fixes #2769
Fixes #5627
Fixes not #3425 -- exception no longer occurs, but the layer turns blank. Probably a variant of #4012.

Fix CI
Fix Paint benchmark performance
Write some architecture docs
Test transitions
~~[ ] Include fix for Synchronize StyleLayer and Bucket/Buffer state #4012?~~ Nah, followup PR.
~~[ ] Include fix for Sharing StyleLayers between WorkerTiles is dangerous #3479?~~ Nah, wait until the style/render split.

jfirebaugh · 2017-11-16T00:36:01Z

The Paint benchmark shows reduced performance, and it looks difficult to maintain the same performance while still fixing #2769. StyleLayer::recalculate obtains its current performance by essentially ignoring default values -- i.e. it's fast, but incorrect.

anandthakker

@jfirebaugh I haven't looked beyond properties.js yet, but reading through that, I think the architecture looks great

anandthakker · 2017-11-16T14:21:03Z

src/style/properties.js

+            return finalValue;
+        } else if (now > this.end) {
+            // Transition from prior value is now complete.
+            this.prior = null;


It looks like the reason for clearing prior is that we want it to be destroyed once we don't need it anymore -- is that right?

That makes sense internally, but it does feel a bit weird to me that possiblyEvaluate({now, ...}) has a side effect. I can't think of a good way to make this class immutable, but if it is going to have temporal state, I think it might be worthwhile to make it more explicit: hold a _lastEvaluated and disallow going "backwards in time", i.e. assert(parameters.now >= _lastEvaluated)

anandthakker · 2017-11-16T14:24:39Z

src/style/properties.js

+            // Transitions to data-driven properties are not supported.
+            // We snap immediately to the data-driven value so that, when we perform layout,
+            // we see the data-driven function and can use it to populate vertex buffers.
+            this.prior = null;


Could this happen in the constructor? Or else, if value could be modified, would it be better to encapsulate behind a setValue()?

anandthakker · 2017-11-16T14:35:07Z

src/style/properties.js

+ * `PossiblyEvaluatedPropertyValue` is used for data-driven paint and layout property values. It holds a
+ * `PossiblyEvaluatedValue` and the `GlobalProperties` that were used to generate it. You're not allowed to supply
+ * a different set of `GlobalProperties` when performing the final evaluation because they would be ignored in the
+ * case where the input value was a constant or camera function.


👍 good call.

anandthakker · 2017-11-16T14:50:34Z

src/style/properties.js

+export type PossiblyEvaluatedValue<T> =
+    | {kind: 'constant', value: T}
+    | SourceExpression
+    | CompositeExpression;


Do we need to export this type? Having both PossiblyEvaluatedValue<T> and PossiblyEvaluatedPropertyValue<T> as public types from this module might be confusing (or at least add conceptual overhead), and anyhow I think that any time you have a PossiblyEvaluatedValue<T>, you might need to evaluate it and thus would need the owning PossiblyEvaluatedPropertyValue<T>.

It's used only in query_utils.js, to quarantine an any cast. We could probably remove this with some additional refactoring.

It's not really a layout property -- it can't be an expression or function. Thus we can make it a top-level property on StyleLayer and give it a strong type. This matches GL Native.

anandthakker · 2017-11-16T18:59:54Z

src/data/bucket/symbol_bucket.js

-            this.textSizeData = getSizeData(this.zoom, layer, 'text-size');
-            this.iconSizeData = getSizeData(this.zoom, layer, 'icon-size');
+            const layer: SymbolStyleLayer = this.layers[0];
+            const unevaluatedLayoutValues = layer._unevaluatedLayout._values;


Why not use

const textSize = layer.getLayoutProperty('text-size'); const iconSize = layer.getLayoutProperty('icon-size');

instead of accessing _unevaluatedValues directly?

Oh, is this because getLayoutProperty's return type isn't parameterized on the property name?

It's because layer.getLayoutProperty returns the raw style spec value (as needed for the public runtime styling API), and we need the PropertyValue.

Gotcha. I guess we could add SymbolStyleLayer#get{Text,Icon}Size()... but treating *_bucket as a 'friend' of *_style_layer is probably reasonable, too.

anandthakker · 2017-11-16T19:13:39Z

src/data/program_configuration.js

                       start: number,
                       length: number,
                       feature: Feature) {
-        const value = layer.getPaintValue(this.property, {zoom: 0}, feature);
+        const value: any = this.expression.evaluate({zoom: 0}, feature);


The return type of SourceExpression.evaluate() is already any -- do we need the annotation here?

Nope, this was just copypasta.

anandthakker · 2017-11-16T20:02:35Z

src/render/draw_symbol.js

@@ -168,8 +166,7 @@ function drawTileSymbols(program, programConfiguration, painter, layer, tile, bu
    const tr = painter.transform;

    if (isSDF) {
-        const haloWidthProperty = `${isText ? 'text' : 'icon'}-halo-width`;
-        const hasHalo = !layer.isPaintValueFeatureConstant(haloWidthProperty) || layer.paint[haloWidthProperty];
+        const hasHalo = layer.paint.get(`${isText ? 'text' : 'icon'}-halo-width`).constantOr(1) !== 0;


I was gonna be pretty impressed that Flow could handle this, but it turns out it's just bailing -- adding typo to the property name expression here doesn't produce a flow error. It does work if the conditional logic is used to select a string literal; i.e., isText ? 'text-halo-width' : 'icon-halooooo-width' produces an error, whereas ${isText ? 'text' : 'icon'}-haloooo-width does not.

👍 , will switch to isText ? 'text-halo-width' : 'icon-halo-width'.

anandthakker · 2017-11-16T20:12:05Z

src/shaders/describe_shaders

@@ -49,7 +48,7 @@ style.layers.forEach((layer) => {

 function addProgramInfo(layer, programInterface, shaderDefinition) {
    const styleLayer = new StyleLayer(layer);
-    styleLayer.updatePaintTransitions([], {}, { zoom: zoom }, new AnimationLoop(), {});
+    styleLayer.updatePaintTransitions([], {}, { zoom: zoom }, {});
    const configuration = ProgramConfiguration.createDynamic(
        programInterface, styleLayer, zoom);
    const key = `${layer.type}${configuration.cacheKey || ''}`;


I'd be okay with just removing describe_shaders -- it's not part of any regular workflows, and that makes it very likely to go stale

anandthakker · 2017-11-16T20:19:38Z

src/style/properties.js

+            }
+        }
+        return false;
+    }


Along the lines of the point above re: the temporal state in TransitioningPropertyValue, would it make sense for hasTransition() to accept a now parameter (here and elsewhere)?

* heatmap-color default is no longer a special case

Instead, at the end of each frame, calculate whether another one is needed.

Instead make it a member variable that gets set when the PossiblyEvaluated is created as a result of (possibly) evaluating properties at a certain zoom level. This makes it clear that you can't supply a different zoom level at the final evaluation step. The SymbolBucket constructor now creates individual PossiblyEvaluatedPropertyValues for all the necessary zoom levels. See code comment for details.

jfirebaugh · 2017-11-16T21:31:01Z

I figured out how to restore StyleLayer::recalculate performance using prototypal inheritance -- see followup commit. This puts benchmarks in good territory: no significant performance regressions, improvement in LayerCircle and QueryPoint (probably due to the possibly evaluated mechanism) and StyleLayerCreate.

https://bl.ocks.org/anonymous/raw/aad0ba1349e552599828bc7e5feadae4/

jfirebaugh · 2017-11-17T00:21:24Z

src/style/style.js

+            }
+        }
+
+        // if (this.placement.hasTransition()) {


Need to uncomment and fix this. @ChrisLoer Is this.placement.stillFading() the right thing to check? It looks a bit different than native's hasTransitions().

Err.... hopefully we can improve this with a back-port from native. I think right now, the answer may be that you should ignore the placement state in this hasTransitions. They key logic for keeping animation running for placement/collision fading is the needsRerender = !this.placement.isDone() || this.placement.stillFading() part that sets this._placementDirty in Map#_render. That accounts for both collision fade animations (that's what stillFading() checks) but also for the map being in a state where it's waiting for the next placement to occur or in the middle of an incremental placement (kind of an analog of stale on the native side although it's not quite the same).

For now, if you just leave this out of style.hasTransitions, it should be fine because the render logic checks this state separately. /cc @ansis

mollymerp

I did the best I could with my limited familiarity with this area of the codebase. I really appreciate the time you took to write these detailed explanatory comments!

mollymerp · 2017-11-16T00:11:24Z

src/data/bucket/symbol_bucket.js

+    //   2. `icon-size` at the zoom level of the bucket. Used to calculate a per-feature size for source `icon-size`
+    //       expressions.
+    //   3. `text-size` and `icon-size` at the zoom level of the bucket, plus one. Used to calculate collision boxes.
+    //   4. `text-size` at zoom level 18. Used for something line-symbol-placement-related.


Should we add more detail to why we need the max-size for line-symbol placement?

We should, but nobody seems remember the details! Tracking this in #5683.

mollymerp · 2017-11-16T22:09:55Z

src/style/properties.js

+ *
+ * `PossiblyEvaluatedValue` represents the three possible outcomes of this step: if the input value was a constant or
+ * camera expression, then the "possibly evaluated" result is a constant value. Otherwise, the input value was either
+ * a source expression or a camera expression, and we must defer final evaluation until supplied a feature. We separate


do you mean:

Otherwise, the input value was either a source expression or a composite expression

if not, otherwise I'm confused bc it seems the previous line contradicts this one.

Yes, good catch, will fix.

mollymerp · 2017-11-16T23:10:25Z

src/data/program_configuration.js

@@ -43,12 +46,11 @@ function packColor(color: Color): [number, number] {
    ];
 }



a brief explanatory comment on the Binder interface might be useful here.

👍, will port the docs from native.

mollymerp · 2017-11-17T01:24:17Z

src/style/properties.js

+ * on the default value: using `for (const property of Object.keys(this._values))`, they can iterate over
+ * only the _own_ properties of `_values`, skipping repeated calculation of transitions and possible/final
+ * evaluations for defaults, the result of which will always be the same.
+ *


This is a very cool solution 🥇

Introduces a `Properties` class which holds objects containing default values for the layout or paint property set of a given layer type. These objects are immutable, and they are used as the prototypes for the `_values` members of `Transitionable`, `Transitioning`, `Layout`, and `PossiblyEvaluated`. This allows these classes to avoid doing work in the common case where a property has no explicit value set and should be considered to take on the default value: using `for (const property of Object.keys(this._values))`, they can iterate over only the _own_ properties of `_values`, skipping repeated calculation of transitions and possible/final evaluations for defaults, the result of which will always be the same.

jfirebaugh · 2017-11-17T18:50:47Z

The remaining unaddressed review comments were @anandthakker's observations about the interior mutability of TransitioningPropertyValue's prior member. On the one hand, I agree this is an oddity in what is otherwise an immutable class. On the other, it's currently pretty much a verbatim port from native, and I'm hesitant to start introducing differences to what has been working well on native. On balance, I think I'd like to keep this as-is for now. @anandthakker does that sound ok?

jfirebaugh force-pushed the property-refactor branch from 3dce1db to 0ed1c7a Compare November 15, 2017 17:09

jfirebaugh requested a review from mollymerp November 15, 2017 19:48

jfirebaugh force-pushed the property-refactor branch 4 times, most recently from d20c81b to 1349285 Compare November 15, 2017 23:34

jfirebaugh force-pushed the property-refactor branch from 1455dd5 to 1ca6949 Compare November 16, 2017 00:36

anandthakker reviewed Nov 16, 2017

View reviewed changes

jfirebaugh added 3 commits November 16, 2017 10:59

Simplify paint property statistics bookkeeping

86dbf1b

Inline isOpacityZero

3893bcf

Special-case visibility

a6f3ba0

It's not really a layout property -- it can't be an expression or function. Thus we can make it a top-level property on StyleLayer and give it a strong type. This matches GL Native.

jfirebaugh force-pushed the property-refactor branch from 1ca6949 to 51aace0 Compare November 16, 2017 18:59

anandthakker reviewed Nov 16, 2017

View reviewed changes

jfirebaugh added 3 commits November 16, 2017 13:20

Refactor style properties

d941a2f

* heatmap-color default is no longer a special case

Eliminate AnimationLoop

69485f6

Instead, at the end of each frame, calculate whether another one is needed.

jfirebaugh force-pushed the property-refactor branch from 588b1d0 to 9588222 Compare November 16, 2017 21:22

jfirebaugh force-pushed the property-refactor branch from 9588222 to 6be367e Compare November 16, 2017 21:51

jfirebaugh commented Nov 17, 2017

View reviewed changes

mollymerp reviewed Nov 17, 2017

View reviewed changes

jfirebaugh added 6 commits November 17, 2017 10:26

Add internal API docs

fb7139d

Fix heatmap layer performance regression

b344e2b

Remove describe_shaders

e491e69

Avoid exporting PossiblyEvaluatedValue

80d0e48

Remove commented-out code pending backport from native

7315489

jfirebaugh force-pushed the property-refactor branch from 97d579c to 7315489 Compare November 17, 2017 18:26

Add internal API docs for Binder

7c03ef8

anandthakker approved these changes Nov 17, 2017

View reviewed changes

jfirebaugh merged commit 485ff9f into master Nov 17, 2017

jfirebaugh deleted the property-refactor branch November 17, 2017 19:03

mourner mentioned this pull request Nov 20, 2017

Big rendering issues in Safari #5712

Closed

renovate bot mentioned this pull request Feb 2, 2018

Update dependency mapbox-gl to ^0.44.0 - autoclosed oSoc17/rideaway-frontend#72

Closed

mourner mentioned this pull request Jul 25, 2018

Call stack size exceeded when calling setPaintProperty in a loop #5179

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Property refactor #5682

Property refactor #5682

jfirebaugh commented Nov 15, 2017 •

edited

Loading

jfirebaugh commented Nov 16, 2017 •

edited

Loading

anandthakker left a comment

anandthakker Nov 16, 2017

anandthakker Nov 16, 2017

anandthakker Nov 16, 2017

anandthakker Nov 16, 2017

jfirebaugh Nov 16, 2017

anandthakker Nov 16, 2017

anandthakker Nov 16, 2017

jfirebaugh Nov 16, 2017

anandthakker Nov 16, 2017 •

edited

Loading

anandthakker Nov 16, 2017

jfirebaugh Nov 16, 2017

anandthakker Nov 16, 2017

jfirebaugh Nov 16, 2017

anandthakker Nov 16, 2017

jfirebaugh Nov 16, 2017

anandthakker Nov 16, 2017

jfirebaugh commented Nov 16, 2017

jfirebaugh Nov 17, 2017

ChrisLoer Nov 17, 2017

mollymerp left a comment

mollymerp Nov 16, 2017

jfirebaugh Nov 17, 2017

mollymerp Nov 16, 2017

jfirebaugh Nov 17, 2017

mollymerp Nov 16, 2017

jfirebaugh Nov 17, 2017

mollymerp Nov 17, 2017

jfirebaugh commented Nov 17, 2017

		@@ -43,12 +46,11 @@ function packColor(color: Color): [number, number] {
		];
		}

Property refactor #5682

Property refactor #5682

Conversation

jfirebaugh commented Nov 15, 2017 • edited Loading

jfirebaugh commented Nov 16, 2017 • edited Loading

anandthakker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anandthakker Nov 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jfirebaugh commented Nov 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mollymerp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jfirebaugh commented Nov 17, 2017

jfirebaugh commented Nov 15, 2017 •

edited

Loading

jfirebaugh commented Nov 16, 2017 •

edited

Loading

anandthakker Nov 16, 2017 •

edited

Loading