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

set an interim paint in style_layer #9658

Closed
wants to merge 2 commits into from

Conversation

mannnick24
Copy link
Contributor

@mannnick24 mannnick24 commented May 5, 2020

this is in place of pr #9574

when a StyleLayer is created its paint member is undefined untill recalculate is called. there are render scenarios where paint is accessed between construction and recalculate being called first time

see https://github.com/mapbox/ibm-cognos-collab/issues/154

Instead of checking whether paint is undefined in a layer when it is used, set an interim possiblyEvaluated value if there are any paint properties.

There is a very simple test for this

@ryanhamley ryanhamley requested a review from ansis May 5, 2020 18:56
@mannnick24
Copy link
Contributor Author

@asheemmamoowala there is a good use case and a fiddle on https://github.com/mapbox/ibm-cognos-collab/issues/153
this was closed when I added a check on the layer.paint member
this change seeks to prevent paint from ever being undefined, by setting a default value.
I'm adding a test that is really simple

@mannnick24
Copy link
Contributor Author

mannnick24 commented May 7, 2020

@asheemmamoowala I'm working on the idea that the race is between:

  1. style.js update
    which calls layer.recalculate - which in turn initialises the paint member with possibly evaluated properties
  2. map.js _render possibly stemming from a zoom event, although it must be skipping the style update - so style isnt dirty?

This really happens for us because we have a dynamic map... users can change the data slot mapping, essentially choosing different data to visualise, our code then removes layers and adds new ones and waits for the map to tell us its loaded
there is an ititial zoom level and then zoom levels are reapplied after the map is loaded

I have not found a reliable fiddle for this yet

Copy link
Member

@mourner mourner left a comment

Choose a reason for hiding this comment

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

This looks good apart from two nits.


if (properties.paint) {
// init default paint to cover until recalculate is called
// see https://github.com/mapbox/ibm-cognos-collab/issues/154
Copy link
Member

Choose a reason for hiding this comment

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

Nit: this is not a public issue so it shoudn't be included.

@@ -101,6 +102,13 @@ class StyleLayer extends Evented {

this._transitioningPaint = this._transitionablePaint.untransitioned();
}

if (properties.paint) {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: you can remove this if because there's the same condition above.

@karimnaaji
Copy link
Contributor

Merged as ef0884e, will be part of v1.10.1. Thanks @mannnick24 !

@karimnaaji karimnaaji closed this May 14, 2020
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