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

Fix mapbox layout layer updates #2734

Merged
merged 1 commit into from
Jun 13, 2018
Merged

Fix mapbox layout layer updates #2734

merged 1 commit into from
Jun 13, 2018

Conversation

etpinard
Copy link
Contributor

  • when a new source is given, remove layer, update source and add new layer in that order to avoid mapbox-gl errors.

Some more info about how mapbox layout layers are updated:

These are the available update pathways:

proto.update = function update(opts) {
if(!this.visible) {
// IMPORTANT: must create source before layer to not cause errors
this.updateSource(opts);
this.updateLayer(opts);
} else if(this.needsNewSource(opts)) {
// IMPORTANT: must delete layer before source to not cause errors
this.updateLayer(opts);
this.updateSource(opts);
} else if(this.needsNewLayer(opts)) {
this.updateLayer(opts);
} else {
this.updateStyle(opts);
}
this.visible = isVisible(opts);
};

where needsNewSource is fairly dumb (but fast):

proto.needsNewSource = function(opts) {
// for some reason changing layer to 'fill' or 'symbol'
// w/o changing the source throws an exception in mapbox-gl 0.18 ;
// stay safe and make new source on type changes
return (
this.sourceType !== opts.sourcetype ||
this.source !== opts.source ||
this.layerType !== opts.type
);
};

which explains why

// new source object on each react
function makeFigure() {
  return {
    // ...
   layout: {
      mapbox: {
        layers: [{source: {/**/}}]
      }
   }
  }
}

Plotly.react(gd, makeFigure())
Plotly.react(gd, makeFigure())

can behave differently than

// same source object
var source = {/* */}

function makeFigure() {
  return {
    // ...
   layout: {
      mapbox: {
        layers: [{source: source}]
      }
   }
  }
}

Plotly.react(gd, makeFigure())
Plotly.react(gd, makeFigure())

So perhaps we could add uid attribute in each mapbox layer so that users that create a new source object on each react call can see their map update faster. Thoughts?

cc @alexcjohnson

- when a new source is given, remove layer, update source and
  add new layer in that order to avoid mapbox-gl errors.
@etpinard etpinard added bug something broken status: reviewable labels Jun 13, 2018
@etpinard
Copy link
Contributor Author

This PR may help with #2345 (comment) but not the original #2345 problem.

@alexcjohnson
Copy link
Collaborator

So perhaps we could add uid attribute in each mapbox layer so that users that create a new source object on each react call can see their map update faster. Thoughts?

Perhaps we should compare source as (JSON) strings if they fail strict equality (and use _compareAsJSON so Plotly.react does this too)?

if(valObject._compareAsJSON && JSON.stringify(oldVal) === JSON.stringify(newVal)) continue;

Could be a bit slow if source is large, but I feel like uid is asking users to get a bit too involved in our low-level optimizations.

@etpinard
Copy link
Contributor Author

Could be a bit slow if source is large,

That was my main concern.

@alexcjohnson
Copy link
Collaborator

Anyway my comment ^^ is just a 🐎 question, could be addressed now or not, up to you. The bugfix here looks great, so if you want to leave it at that 💃 !

@etpinard
Copy link
Contributor Author

Anyway my comment ^^ is just a question, could be addressed now or not, up to you.

Great. I'll defer it if you don't mind. I'd like to investigate _compareAsJSON on some real-life geojsons before implementing it.

@etpinard etpinard merged commit b931e4f into master Jun 13, 2018
@etpinard etpinard deleted the mapbox-update-layer-fix branch June 13, 2018 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something broken
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants