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

Refactor TerrainLayer for performance and ease of use. #4275

Merged
merged 5 commits into from
Feb 17, 2020

Conversation

chrisgervang
Copy link
Collaborator

@chrisgervang chrisgervang commented Feb 14, 2020

Background

Progress on #4236

New TerrainLayer interface internalizes all data preparation logic so the user doesn't need to write that boilerplate. Also supports being used as a tiled layer (internally rendering a TileLayer of SimpleMeshLayers), or as a single mesh (internally rendering a SimpleMeshLayer).

This layer is suitable for rendering a few use cases:

  • Terrain tiles globally from a tile server (similar to TileLayer).
  • A static set of tiles from a tile server.
  • A custom tile of your own dataset (e.g. non-Slippy map tiles).

Interactions while loading new tiles now feels way smoother since we're now offloading terrain mesh generation to web workers.

New prop interface:

const defaultProps = {
  ...TileLayer.defaultProps,
  // Image url that encodes height data
  terrainImage: {type: 'string', value: null},
  // Image url to use as texture
  surfaceImage: {type: 'string', value: null},
  // Martini error tolerance in meters, smaller number -> more detailed mesh
  meshMaxError: {type: 'number', value: 4.0},
  // Bounding box of the terrain image, [minX, minY, maxX, maxY] in world coordinates
  bounds: {type: 'array', value: [0, 0, 1, 1], compare: true},
  // Color to use if surfaceImage is unavailable
  color: {type: 'color', value: [255, 255, 255]},
  // Object to decode height data, from (r, g, b) to height in meters
  elevationDecoder: {
    type: 'object',
    value: {
      rScaler: 1,
      gScaler: 0,
      bScaler: 0,
      offset: 0
    }
  },
  // Supply url to local terrain worker bundle. Only required if running offline and cannot access CDN.
  workerUrl: {type: 'string', value: null},
  // Same as SimpleMeshLayer wireframe
  wireframe: false
};

Change List

  • Changed TerrainLayer prop interface changed.
  • Integrated @loaders.gl/terrain for web worker mesh generation.
  • Updated example to reflect new usage.
  • Adding workerUrl prop

@coveralls
Copy link

coveralls commented Feb 14, 2020

Coverage Status

Coverage remained the same at 80.862% when pulling 6c123f0 on use-terrain-loader into 3b45d23 on master.

// eslint-disable-next-line handle-callback-err
return load(terrainTile).catch(err => null);
};
function getTerrain(tile) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's move this to a test. As a public-facing example, I prefer to keep things simple.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Are you referring to the two ways this function can be used, and reducing it to the simple TileLayer usage?

That’s fair since this becomes the website example which will just show one way.

I was going to prepare an example of how I’d typically use it without TileLayer and put it somewhere too, maybe explore some fun advance usages. Any suggestions?

Copy link
Collaborator

Choose a reason for hiding this comment

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

You can put the simple usage example in https://github.com/uber/deck.gl/blob/master/website/src/components/demos/layer-demos.js — these are the examples embedded in the layer docs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok cool! I’ll merge this and get into geo-layers PR which can have tests and layer-demos example.

@@ -5,17 +5,29 @@ const SATELLITE = 'https://api.mapbox.com/v4/mapbox.satellite';
// Set your mapbox token here
const MAPBOX_TOKEN = process.env.MapboxAccessToken; // eslint-disable-line

export const getSurface = ({x, y, z}, surface) => {
export const getSurface = (surface, tile) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same thing here

if (tile) {
const {x, y, z} = tile;
surfaceImage = `${STREET}/${z}/${x}/${y}.png`;
}
break;
case 'none':
default:
Copy link
Collaborator

Choose a reason for hiding this comment

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

The reason <string> -> null does not work has to do with the texture prop is async and default to null. The layer does not think the prop has changed. The fix will not be trivial. As an work around, you can set surfaceImage to 0.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok - good enough for me :) thanks for looking. I’ll just add a comment to document 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.

Hmm, this didn't work. I'm pushing up what I tried.

@chrisgervang
Copy link
Collaborator Author

I'm going to merge this and continue implementing feedback in a integration branch.

@chrisgervang chrisgervang merged commit a2dc854 into master Feb 17, 2020
@chrisgervang chrisgervang deleted the use-terrain-loader branch February 17, 2020 00:31
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