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

Support reusing Layer instances #257

Closed
austinhyde opened this issue Dec 8, 2016 · 4 comments
Closed

Support reusing Layer instances #257

austinhyde opened this issue Dec 8, 2016 · 4 comments
Labels

Comments

@austinhyde
Copy link
Contributor

In my application, I have a Map component that tracks viewport state and sets some common properties, and other components simply provide the deck.gl layers to display on the map:

class Map extends React.Component {
  ...
  render() {
    return (
      <MapGL {...this.state.vp} {...this.state.size} onViewportChange={vp => this.setState({vp})} ...>
        <DeckGL {...this.state.vp} {...this.state.size} layers={this.props.layers}/>
      </MapGL>
    );
  }
}

class Page extends React.Component {
  ...
  render() {
    const layer = new ScatterplotLayer({ id: 'xyz', ... });
    return <Map layers={[layer]}/>;
  }
}

However, this arrangement hits the "deck.gl sanity check - Matching layer is same" assertion every time the Map component is re-rendered without the parent component being re-rendered, as is the case when you pan or zoom the map. This is not exactly expected behavior, although the docs do hint at it.

Instead of requiring new layer instances, I believe a better approach would be to either clone the layer when this condition is detected, or change the underlying code to not rely on old/new layers being separate instances.

@ibgreen
Copy link
Collaborator

ibgreen commented Dec 8, 2016

@austinhyde
Correct. deck.gl currently requires freshly minted layers each render. But as you say, it would not be terribly hard to support "reusing" layers.

And we could optimize deck.gl pretty heavily when reusing layers, essentially bypassing change detection, and sublayer generation. (Especially if we call Object.freeze on the layer's props in the Layer constructor - React does something similar, I believe).

Would be very happy to see this as a PR or mark it as a prioritized ticket. It is mostly a question of the "lifecycle management" code being the most complex part of deck.gl, so this change should be carefully reviewed, and ideally be backed up with documentation clarifications as well as additional test cases to help make sure we don't break things.

@ibgreen ibgreen changed the title Layers are required to be different instances Support reusing Layer instances Dec 8, 2016
@ibgreen ibgreen mentioned this issue Dec 23, 2016
27 tasks
@ibgreen
Copy link
Collaborator

ibgreen commented Jan 9, 2017

@gnavvy - do you want to try your hand at this?

@ibgreen ibgreen added the v3.1 label Jan 9, 2017
dcposch added a commit to dcposch/yimby that referenced this issue Feb 13, 2017
@ibgreen
Copy link
Collaborator

ibgreen commented Feb 24, 2017

Looks like this won't be making it into v4.0.

We've added new benchmarks (npm run bench) that shows that a macbook pro can construct about 300.000 layer instances per second - so this should not be a significant performance bottleneck in practice.

Deprioritizing but keeping open.

@ibgreen
Copy link
Collaborator

ibgreen commented Feb 24, 2017

Closing, since listed in #83

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants