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

Implement Mapbox custom layer API RFC #2306

Merged
merged 2 commits into from Sep 26, 2018

Conversation

Pessimistress
Copy link
Collaborator

@Pessimistress Pessimistress commented Sep 25, 2018

For #2132

Background

API proposal to provide a path for deck.gl pure-js and React users to use Mapbox custom layers. This change allows user to leverage deck.gl top-level API such as pickingRadius, onLayerHover etc.

Example:

import Deck, {ScatterplotLayer} from '@deck.gl/core';
import {MapboxLayer} from '@deck.gl/mapbox';

const map = new mapboxgl.Map({...});

const deck = new Deck({
  width: '100%',
  height: '100%',
  longitude: -122.4,
  latitude: 37.8
  layers: [
    new ScatterplotLayer({
	  id: 'my-scatterplot',
	  ...
	})
  ]
});

map.on('load', () => {
  map.addLayer(new MapboxLayer({id: 'my-scatterplot', deck}), getFirstTextLayerId(map.getStyle()));
});

Change List

  • Support composite layers in Mapbox integration
  • Support Deck top-level props in Mapbox integration

Copy link
Collaborator

@ibgreen ibgreen left a comment

Choose a reason for hiding this comment

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

Let's discuss the RFC before closing on this.

@@ -471,7 +473,7 @@ export default class Deck {
});

// Note: avoid React setState due GL animation loop / setState timing issue
this.layerManager = new LayerManager(gl, {stats: this.stats});
this.layerManager = new LayerManager(gl, {stats: this.stats, deck: this});
Copy link
Collaborator

@ibgreen ibgreen Sep 25, 2018

Choose a reason for hiding this comment

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

On the surface, this coupling is obviously very undesirable. Having two components that both know about each other breaks encapsulation and usually causes pain down the road.

Part of the problem is calling it deck. Normally it is better to just allow the lower level component (LayerManager in this case) store an 'unspecified' reference to the owning object, that can only be interpreted by higher layers.

Maybe the right approach is to reconsider who owns the context. With the emergence of Deck, ViewManager etc classes we have already removed a lot of functionality out of LayerManager, and I my thinking has been that it should be further "gutted" to just handle layer matching. drawing/picking for instance really doesn't need to be placed in LayerManager

offsetCenter: event.point,
srcEvent: event.originalEvent
});
callback(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: Can we avoid nested functions?

Add a bit more explanation in the header about why this is done (picking...).

pointerleave: deck._onPointerLeave
});
deck.eventManager.on({
click: event => handleMouseEvent(event, deck._onClick),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Call it handlePickingMouseEvent or something that hints at what it does?

onAdd(map, gl) {
this.map = map;
this.deck = getDeckInstance({map, gl});
this.deck = getDeckInstance({map, gl, deck: this.props.deck});
Copy link
Collaborator

Choose a reason for hiding this comment

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

Storing _deck on the map could avoid having to pass deck around like this...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is for the use case where we add a MapboxLayer referencing a layer from an existing Deck instance:

new MapboxLayer({id: 'my-layer', deck});

@Pessimistress Pessimistress changed the title [For Discussion] Mapbox custom layer API proposal Implement Mapbox custom layer API RFC Sep 26, 2018
@Pessimistress Pessimistress merged commit b1c6962 into visgl:master Sep 26, 2018
@Pessimistress Pessimistress deleted the mapbox-api branch September 26, 2018 18:32
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.

None yet

2 participants