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

Allow overriding SymbolLayer's paint properties using format expression options #8068

Merged
merged 11 commits into from
Aug 13, 2019

Conversation

alexshalamov
Copy link
Contributor

@alexshalamov alexshalamov commented Mar 22, 2019

Port of mapbox/mapbox-gl-native#14062

Launch Checklist

@alexshalamov alexshalamov changed the title Allow overriding SymbolLayer paint properties using format expression's options Allow overriding SymbolLayer's paint properties using format expression options Mar 22, 2019
@alexshalamov alexshalamov force-pushed the alexshalamov_format_expr_text_color branch 2 times, most recently from a98d394 to 760a137 Compare March 26, 2019 13:24
@alexshalamov alexshalamov force-pushed the alexshalamov_format_expr_text_color branch 3 times, most recently from 07e5dae to 4958eac Compare April 2, 2019 14:11
@alexshalamov
Copy link
Contributor Author

Benchmark results:

Screen Shot 2019-04-02 at 20 49 43

Screen Shot 2019-04-02 at 20 50 32

this.hasPaintOverrides = SymbolStyleLayer.hasPaintOverrides(layout);
if (this.hasPaintOverrides) {
for (const layer of this.layers) {
layer.setPaintOverrides();
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 a SymbolStyleLayer can exist either with overrides applied or not. And since the state depends on this method call it is hard to tell elsewhere what the state of the style layer is. Your current implementation looks correct but I think someone less familiar with this could easily make changes that either result in setPaintOverrides() being called more or less than once, or before recalculate().

Would triggering it in styleLayer.recalculate(...) make sense?

}
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I think at this point sections is set to the sections of a single formatted expression. In the case where there is a ZoomConstantExpression with multiple possible formatted expressions it will only hold the last one. If the last expression has no overrides but an earlier one does, does this return the wrong value?

For example:

["case",
    ["get", "colourful"], ["format", "red text", { "text-color": "red" }],
    ["format", "default text", {}]]


constructor(specification: StylePropertySpecification) {
constructor(specification: StylePropertySpecification, overrides?: Object) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be possible to make the type of overrides more specific?

@chloekraw chloekraw added this to the release-queso milestone Jul 30, 2019
@ansis ansis force-pushed the alexshalamov_format_expr_text_color branch from 02b9db3 to a35dd5c Compare August 7, 2019 19:35
alexshalamov and others added 11 commits August 13, 2019 18:21
The previous way of checking for paint overrides didn't properly handle
the case where there might be multiple cases with some not having
overrides.
This seems more robust because it changes the calculated values
consistently after they are calculated instead of just in some cases.
This makes it a bit easier to understand what state the style layer is
in.
@ansis ansis force-pushed the alexshalamov_format_expr_text_color branch from 9bd3be4 to b4386e3 Compare August 13, 2019 22:22
@ansis ansis merged commit b4fd6ee into master Aug 13, 2019
@ansis ansis deleted the alexshalamov_format_expr_text_color branch August 13, 2019 22:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants