Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Data-driven styling support for *-pattern properties #6289

Merged
merged 2 commits into from
Aug 27, 2018
Merged

Conversation

mollymerp
Copy link
Contributor

@mollymerp mollymerp commented Mar 7, 2018

todos:

  • figure out flow typing for CrossFadedDataDrivenProperty or potentially refactor CrossFaded out
  • preserve precision when passing pattern texture coordinates to shaders
  • add support for composite expressions
  • identify bug causing flashing/incorrect texture sampling
  • figure out how to deal w layers that share a bucket but have different line-pattern properties
  • decide on how to pack the attributes / manage the attribute buffers

Launch Checklist

  • briefly describe the changes in this PR
  • write tests for all new functionality
  • document any changes to public APIs
  • post benchmark scores
  • manually test the debug page

@mollymerp mollymerp added breaking change ⚠️ Requires a backwards-incompatible change to the API api 📝 labels Mar 7, 2018
@mollymerp mollymerp added under development cross-platform 📺 Requires coordination with Mapbox GL Native (style specification, rendering tests, etc.) and removed api 📝 breaking change ⚠️ Requires a backwards-incompatible change to the API labels Mar 9, 2018
@mollymerp
Copy link
Contributor Author

mollymerp commented Mar 10, 2018

this is ready for an initial review, with the caveat that there are two open questions:

  • because line-pattern is a paint property, it is possible that there would be more than one layer with different line-pattern expressions on the same bucket. not sure how we want to handle this case, because it will come up again with the other *-pattern properties.
  • right now I'm not doing any attribute packing on the line-pattern PaintArrays. There are a couple of options I see:
    • we could get down to only needing a single vec4 attribute for line-pattern, but it would require creating two paint arrays, one with the packed coordinates for the min and mid CrossFaded pattern locations, and the other with mid and max pattern locations, and then manually binding the correct paint buffer depending on if we're zooming in or out.
    • we could have 2 attributes, one 4 component holding the packed coordinates of min and max (we only need one for rendering, but don't know which at layout time) and one 2 component with mid coordinates which are always used.

luckily, cross-faded properties are already interpolated along zoom-levels and thus require the same data to be passed for Source and Composite functions, so we don't need to worry about packing in double the data for the Composite case.

cc @ansis @jfirebaugh @kkaefer

return z > p.zoomHistory.lastIntegerZoom ?
{ fromScale: 2, toScale: 1, t: fraction + (1 - fraction) * t } :
{ fromScale: 0.5, toScale: 1, t: 1 - (1 - t) * fraction };
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I implement fill-pattern and fill-extrusion-pattern dds in a follow-up PR, I will probably move this function to the parent StyleLayer class.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 along with the CrossfadeParameters type

@@ -3,6 +3,9 @@
export type CrossFaded<T> = {
from: T,
to: T,
min: T,
mid: T,
max: T,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

when I implement the remaining *-pattern properties, I will be able to remove the from and to components from this type in favor of the StyleLayer getter for cross fade parameters.

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like it should be possible now for CrossFaded<T> to be pared down to {min: T, mid: T, max: T}. Maybe adding from: 'min' | 'max' to CrossfadeParameters so that in places where we're doing pattern.from, we can do pattern[crossfade.from] instead?

@jfirebaugh
Copy link
Contributor

because line-pattern is a paint property, it is possible that there would be more than one layer with different line-pattern expressions on the same bucket.

Yep, this is something that's true of all data-driven paint properties. We handle it in ProgramConfigurationSet, which maintains a map of layer ID to ProgramConfiguration, and ProgramConfiguration contains all the per-layer data. Will that strategy work for line-pattern or is there additional nuance?

@mollymerp
Copy link
Contributor Author

🤦‍♀️ well, that's embarrassing. I think I confused myself about this step where we determine which glyphs to add to the tile's iconAtlas and how it relates to the same step in SymbolBucket where all properties that reference a sprite are layout specific.

https://github.com/mapbox/mapbox-gl-js/pull/6289/files#diff-b1205e64508e847d415d42243c10212bR158

it shouldn't be an issue to loop through each layer with data-driven line-pattern here and add all relevant glyphs to the atlas 👍

@jfirebaugh
Copy link
Contributor

I think we should aim to handle non-data-driven *-pattern properties via ProgramConfiguration + ConstantBinder + Tile#iconAtlasImage, rather than keeping a separate atlas in ImageManager for them and manually setting the uniforms.

@mollymerp
Copy link
Contributor Author

Getting some diffs after moving uniform bindings to the Pattern*Binders

image

they're tiny and I haven't looked too much into them – if they're acceptable diffs I will update the tests, and if not I will spend more time trying to fix.

I also have a version of this PR that reduces the # attributes to 1, but requires dynamic checking and possibly binding of the vertex buffer on each frame/tile (there is a separate vertex buffer per tile depending on if the user is zooming in or zooming out for crossfading to work correctly) – should I pursue that route or stick with the current approach?

@jfirebaugh
Copy link
Contributor

Updating the tests for those diffs sounds good. Or, if you want to, minimize the tests so that they don't show those minor differences (use inline GeoJSON with simple geometries, 64x64 size, etc.).

Reducing the number of attributes to one sounds great! It should be fairly cheap to switch vertex buffers on the fly, and it'll get cheaper once we have state tracking that allows us to rebind only the attributes that have changed.

@mollymerp
Copy link
Contributor Author

mollymerp commented Mar 22, 2018

Thanks @anandthakker for helping me better understand floating point precision 🔢
I haven't come up with a way to pack two larger-than 8bit integers into a single Float32 attribute, but I have reduced the number of attributes to two and implemented a method to update the vertex buffer that is used for data-driven line-patterns at draw time.

This is ready for review, with the caveat that I still need to fix this PR for non-integer zoom stops because that is currently broken for both data-driven styling and camera functions. DIdn't notice this was broken until late this afternoon, so I will aim to address that tomorrow.

@@ -224,9 +225,8 @@ class Tile {

const gl = context.gl;

if (this.iconAtlasImage) {
this.iconAtlasTexture = new Texture(context, this.iconAtlasImage, gl.RGBA);
this.iconAtlasImage = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like we were previously clearing this.iconAtlasImage to avoid creating the Texture every time. Should we do something analogous?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes – I had meant to come back to this. setting iconAtlas.image to null was upsetting flow but I will revisit! 👍

} else {
self.binders[property] = new CompositeExpressionBinder(value.value, name, type, useIntegerZoom, zoom);
keys.push(`/z_${name}`);
const structArrayLayout = layoutType(property, type, 'composite');
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: since structArrayLayout is actually a class, capitalize the var

* @private
*/

export class CrossFadedDataDrivenProperty<T> extends DataDrivenProperty<?CrossFaded<T>> {

This comment was marked as resolved.

This comment was marked as resolved.

{ from: min, to: mid, fromScale: 2, toScale: 1, t: fraction + (1 - fraction) * t } :
{ from: max, to: mid, fromScale: 0.5, toScale: 1, t: 1 - (1 - t) * fraction };
{ from: min, to: mid, min: min, mid: mid, max: max, fromScale: 2, toScale: 1, t: fraction + (1 - fraction) * t } :
{ from: max, to: mid, min: min, mid: mid, max: max, fromScale: 0.5, toScale: 1, t: 1 - (1 - t) * fraction };
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: this can just be min, mid, max instead of min: min, mid: mid, max: max.

return z > p.zoomHistory.lastIntegerZoom ?
{ fromScale: 2, toScale: 1, t: fraction + (1 - fraction) * t } :
{ fromScale: 0.5, toScale: 1, t: 1 - (1 - t) * fraction };
}
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 along with the CrossfadeParameters type

]);

export default lineLayoutAttributes;
export const {members, size, alignment} = lineLayoutAttributes;
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this module now exports more than one set of attributes, let's drop the default export in favor of two named exports (See #6336)

if (programConfiguration.binders && programConfiguration.binders['line-pattern'] && programConfiguration.binders['line-pattern'].isDataDriven()) {
this.dataDrivenPatternLayers.push(i);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than storing this on this, could we compute it locally in populate()? (Also: why is this an array of indices into this.layers rather than an array of layers themselves?)

Copy link
Contributor

Choose a reason for hiding this comment

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

One more question: why use binder.isDataDriven() rather than checking layer.paint.get('line-pattern')?

}
}

// used if line-pattern is data-driven
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't addFeatures() also used if line-pattern is constant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, sorry – an artifact from an earlier iteration 🙈


}

class PatternConstantBinder<T> extends ConstantBinder<T> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm still inclined to say PatternConstantBinder should be a top-level class rather than a subclass of ConstantBinder (and likewise for PatternCompositeExpressionBinder).

@mollymerp mollymerp force-pushed the linepattern-dds branch 3 times, most recently from ca66d46 to 40c68f5 Compare March 23, 2018 23:06
const programId =
layer.paint.get('line-dasharray') ? 'lineSDF' :
layer.paint.get('line-pattern') ? 'linePattern' : 'line';
linePattern && linePattern.constantOr((1: any)) ? 'linePattern' : 'line';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

open to better ways of doing this check – basically if line-pattern is undefined, constantOr(1) will return undefined, but linePattern itself is no longer falsy if line-pattern is undefined because it is now a PossiblyEvaluatedPropertyValue

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm okay with this as-is, since it's analogous to how we do other such checks.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way to eliminate the any cast?

@mollymerp
Copy link
Contributor Author

@charandas sorry for the delay. we have to wait to merge until the implementation in mapbox-gl-native is complete because the two libraries share shaders. Its hopefully almost done 🙏⏳mapbox/mapbox-gl-native#12284

thanks for the bug report – working on a fix for that now!

@mollymerp mollymerp force-pushed the linepattern-dds branch 2 times, most recently from 2f8d0b4 to fb233d7 Compare August 14, 2018 23:53
@mollymerp
Copy link
Contributor Author

notable benchmarks

image image image image image image

@mourner
Copy link
Member

mourner commented Aug 16, 2018

@mollymerp looks like a pretty huge regression for Layout — is that spurious or reproducible with repeated runs?

@mollymerp
Copy link
Contributor Author

@mourner 😞 yep its reproducable and kind of expected because there is an additional worker-main thread round trip for *-pattern layers to fetch the icons needed to layout. I'm trying something quickly to avoid this if the pattern property is not used, and hopefully that will reduce the regression.

@@ -162,6 +186,7 @@ class StyleLayer extends Evented {
}

recalculate(parameters: EvaluationParameters) {
this._parameters = parameters;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we avoid adding this state?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we can get rid of it if we pass the evaluation parameters from Map#_render to painter#render and through to the draw calls. Is that preferable?

Copy link
Contributor

Choose a reason for hiding this comment

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

I took a deeper look and understand things a lot better now: CrossFadedDataDrivenProperty wants access to the same state -- fromScale, toScale, and t -- that CrossFadedProperty returns via CrossFaded. So here's what you could do support them both in a common way:

  • Rename this._parameters to this._crossfadeParameters and change the type to CrossfadeParameters.
  • Move getCrossfadeParameters() to EvaluationParameters, so the above line in the PR becomes this._crossfadeParameters = parameters.getCrossfadeParameters(). (This isn't strictly necessary, but getCrossfadeParameters() seems like it should live on EvaluationParameters per Feature Envy.)
  • Remove fromScale, toScale, and t from CrossFaded<T> and the duplicate calculation of these values from CrossFadedProperty<T>::_calculate. Instead, have layers that use either CrossFadedProperty or CrossFadedDataDrivenProperty obtain the calculated values from the same source: this._crossfadeParameters.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like CrossFaded<T> should become {min: T, mid: T, max: T}. This would allow it to be used in more places, and maybe DRY up some of the duplication in the bucket classes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jfirebaugh {min, mid, max} are only needed for data-driven property evaluation, and to, from is still useful for constant and camera expressions, and since the expression evaluation in the binders returns T, not CrossFaded<T> I've kept the to, from values for now.

I DRYd up the bucket code in f1a06a3 and took your suggestions for reworking getCrossfadeParameters in 9f6bac9. Thank you!!

fillFeature.id = feature.id;
}

this.features.push(fillFeature);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason that calculating the needed pattern images and iterating over features need to happen at the same time? If not, let's split that into separate steps, so that buckets don't need to create a temporary array of BucketFeatures.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this follows the pattern that is also used in symbol layout. we could split it into separate steps, but this way avoids repeating the work of parsing features out of the vector layers here:

for (const sourceLayerId in layerFamilies) {
const sourceLayer = data.layers[sourceLayerId];
if (!sourceLayer) {
continue;
}
if (sourceLayer.version === 1) {
warnOnce(`Vector tile source "${this.source}" layer "${sourceLayerId}" ` +
`does not use vector tile spec v2 and therefore may have some rendering errors.`);
}
const sourceLayerIndex = sourceLayerCoder.encode(sourceLayerId);
const features = [];
for (let index = 0; index < sourceLayer.length; index++) {
const feature = sourceLayer.feature(index);
features.push({ feature, index, sourceLayerIndex });
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I was going to say, since we're restricting *-pattern properties to values where the possible outputs are contained as literals within the expression, we can take advantage of that to calculate the needed pattern images without reference to the actual features. But it looks like you've lifted that restriction in the latest revision. Do you remember the original rationale for it? On the one hand, it's nice that it would allow this optimization, but on the other, I can definitely envision someone wanting to use e.g. "fill-pattern": ["get", "pattern-name"], since that's been so useful for icon-image.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the original reason for the restriction was to avoid the per-feature evaluation before the layout step, but yes it has been lifted and I agree, the ["get", "pattern-name"] use-case is probably going to be one users will appreciate and use.

*/

export class CrossFadedDataDrivenProperty<T> extends DataDrivenProperty<?CrossFaded<T>> {
possibleOutputs: Array<T>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's figure out how to avoid adding this state. These classes are already tough to understand and reason about even though their methods avoid stateful side-effects. I fear that adding statefulness to them will add another level of complexity.

@mollymerp
Copy link
Contributor Author

layout benchmark looking much better after 3cd1985
image

@mollymerp mollymerp force-pushed the linepattern-dds branch 3 times, most recently from 59d62d7 to ad748a0 Compare August 22, 2018 17:30
Copy link
Contributor

@jfirebaugh jfirebaugh left a comment

Choose a reason for hiding this comment

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

Great work here @mollymerp! You took an on an ambitious feature without an obvious solution, drove it to completion without compromising on performance or code quality, and in the process deepened your knowledge of key parts of the codebase and C++ template programming. 👏

I agree, the ["get", "pattern-name"] use-case is probably going to be one users will appreciate and use.

👍 Is a test for this case included?

@@ -25,6 +25,12 @@ import type {
FilterSpecification
} from '../style-spec/types';

export type CrossfadeParameters = {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is duplicated in evaluation_parameters.js -- let's import it from there.

@mollymerp mollymerp force-pushed the linepattern-dds branch 2 times, most recently from 37d0687 to 8b723b4 Compare August 24, 2018 03:35
Copy link
Contributor

@ansis ansis left a comment

Choose a reason for hiding this comment

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

I just noted a couple things that jumped out at me but I don't think any of them are clear or important enough to block this pr. Don't let them stop you from merging unless you feel like they are things that should be done before merging.

setConstantPatternPositions(posTo: ImagePosition, posFrom: ImagePosition) {
this.patternPositions.patternTo = posTo.tlbr;
this.patternPositions.patternFrom = posFrom.tlbr;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if this is something we need to address in this pr, but:

I think it would be good to get rid of this state and indirect way of setting this values. It would be good if they could be calculated from the values passed to program.draw(...). Maybe by passing patternPositions or imageAtlas through? Another way to think about this could be to evaluate a PossiblyEvaluatedPropertyValue<T> for a specific tile (turn (icon_name, atlas) -> coords).

addFeature(patternFeature, geometry, index, {});
}

options.featureIndex.insert(feature, geometry, index, sourceLayerIndex, bucketIndex);
Copy link
Contributor

Choose a reason for hiding this comment

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

This method handles a couple non-pattern related things, like loading the geometry and indexing features. Is there anything here that can be separated and shared with buckets that don't have patterns (like circles?). It's very possible that doing this wouldn't make sense. Either way I don't see this as something that needs to block this pr.

Molly Lloyd added 2 commits August 24, 2018 12:35
…ll-extrusion-pattern properties

allow multiple attributes per style-spec property

add CrossFadedDataDrivenProp for line-pattern DDS"

convert line_pattern shaders to use pragmas

create layouts for data-driven line-pattern vertex buffers

add source function support for line-pattern to line bucket population and draw code

use min, mid, max images for cross-fading data-driven patterns

also use tile's IconAtlas for data constant line-pattern

extend Binders to support line-pattern properties

add initial render test

nit fix

ensure all possible icons for line-pattern camera funcs are added to the icon atlas

make arguments needed for ddpattern required

set binder type on property

make pattern attributes independent of line layer

implement data-driven styling for fill-pattern

add dds render test for fill-pattern

eliminate black flash on setPaintProperty with a pattern value

extend integer-only evaluation to CrossFadedDataDrivenProps

address review comments

remove getPossibleOutputs and fix rendering

extend feature state updating to CrossFadedCompositeBinder

use getPossibleOutputs instead of iterating over all features

add 1px padding wrap to sprites

separate icon and pattern sprites in ImageAtlas to fix wrapping in -pattern properties

rename imageAtlas --> iconAtlas now that it holds both icons and pattern images

update to use new style-spec expression schema

implement fill-extrusion-pattern dds

address review comments

simplify imageAtlas check

remove redundant CrossFaded properties

backport #6665

remove unpack function for pattern attrs

backport #6745 and fix rebase flubs

update with uniform binding state management

expose possibleOutputs() at the StylePropertyExpression level

add some query tests

rebase fix

Don't wait for pattern images to layout layers no -pattern property set

bonus: remove limitation on non-deterministic expression outputs for pattern properties and reliance on `possibleOutputs()` state

remove getPossibleOutputs from CrossFadedDataDrivenProperty

refactor CrossFaded and CrossfadeParameters

DRY bucket code with util function

refactor pattern bucket functions
@charandas
Copy link

Needed @mapbox/whoot-js@^3.1.0 to get this building on my end.

[!] Error: 'getTileBBox' is not exported by node_modules/@mapbox/whoots-js/index.umd.js
https://github.com/rollup/rollup/wiki/Troubleshooting#name-is-not-exported-by-module
src/source/tile_id.js (3:8)
1: // @flow
2:
3: import {getTileBBox} from '@mapbox/whoots-js';

Otherwise, this import wasn't working for me. Just FYI @mollymerp

@jfirebaugh
Copy link
Contributor

Thanks for the heads up @charandas -- should be addressed by #7205.

@mollymerp mollymerp merged commit 53e622b into master Aug 27, 2018
@mollymerp mollymerp deleted the linepattern-dds branch August 27, 2018 18:12
@charandas
Copy link

Thanks @jfirebaugh @mollymerp. Did something significant change as to the interface for this using feature-state in the commits that came after this message?

I was able to get fill-pattern DDS work using feature-state prior to that day using code from this PR, and now it just silently fails, when I use the code from master?

I am doing something as simple as:

map.setPaintProperty('ctracts', 'fill-pattern', ["step", ["number", ["feature-state", "lmi"], 0], "pattern-1", 1, "pattern-2", 3, "pattern-3"])

I can also confirm that the feature-state is loaded with the lmi property. I can make a different issue with a JSBin once master is released to npm.

@mollymerp
Copy link
Contributor Author

yep @charandas – thanks for the flag. this was a bug I introduced as part of a performance related refactor. working on a fix now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cross-platform 📺 Requires coordination with Mapbox GL Native (style specification, rendering tests, etc.) feature 🍏
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants