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

Add zRange prop to TileLayer for use with TerrainLayer #4397

Merged
merged 9 commits into from
Mar 26, 2020

Conversation

kylebarron
Copy link
Collaborator

Closes #4347

Screen Recording 2020-03-15 at 4 14 27 PM
(Mt. Rainier, ~4000m; no missing tiles)

Change List

  • Adds a zRange prop to TileLayer, which is passed down to viewport.unproject to find tile indices given a range of elevations (in meters).
  • Updates TerrainLayer's zRange state on viewport load, calculated as the minimum and maximum of all triangle mesh heights. It does seem a little excessive to scan all mesh heights on every viewport change, so I'm open to ideas about more efficient z range calculation.

@CLAassistant
Copy link

CLAassistant commented Mar 15, 2020

CLA assistant check
All committers have signed the CLA.

@ibgreen
Copy link
Collaborator

ibgreen commented Mar 16, 2020

Side note: The long term solution for this might be to simply generate bounding boxes/volumes for each tile and then use frustum intersection logic implemented for 3D tiles. This aligns with our ambition to gradually unify the tile layers layers.

modules/geo-layers/src/terrain-layer/terrain-layer.js Outdated Show resolved Hide resolved
modules/geo-layers/src/tile-layer/tileset-2d.js Outdated Show resolved Hide resolved
modules/geo-layers/src/tile-layer/utils.js Outdated Show resolved Hide resolved
modules/geo-layers/src/tile-layer/utils.js Outdated Show resolved Hide resolved
modules/geo-layers/src/tile-layer/utils.js Outdated Show resolved Hide resolved
@Pessimistress
Copy link
Collaborator

CI is failing - You can run yarn test node locally to see the errors.

@kylebarron
Copy link
Collaborator Author

When I run tests locally on master, I get an unrelated gl error. I've run npm run bootstrap though, so I'm not sure why this is happening

Running node tests...
deck: deck.gl untranspiled source - set deck.log.level=1 (or higher) to trace attribute updates
/Users/kyle/github/mapping/deck.gl/test/node.js:30
canvas.width = gl.drawingBufferWidth;
                  ^

TypeError: Cannot read property 'drawingBufferWidth' of null
    at Object.<anonymous> (/Users/kyle/github/mapping/deck.gl/test/node.js:30:19)
    at Module._compile (internal/modules/cjs/loader.js:959:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:995:10)
    at Module.load (internal/modules/cjs/loader.js:815:32)
    at Function.Module._load (internal/modules/cjs/loader.js:727:14)
    at Module.require (internal/modules/cjs/loader.js:852:19)
    at require (internal/modules/cjs/helpers.js:74:18)
    at Object.<anonymous> (/Users/kyle/github/mapping/deck.gl/node_modules/ocular-dev-tools/node/test.js:45:5)
    at Module._compile (internal/modules/cjs/loader.js:959:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:995:10)

From travis it looks like the main error is

not ok 3268 non-geospatial with tile size
  ---
    operator: deepEqual
    expected: |-
      [ '1,2,3', '1,3,3', '2,2,3', '2,3,3', '3,2,3', '3,3,3', '4,2,3', '4,3,3' ]
    actual: |-
      [ '0,1,3', '1,1,3', '2,1,3' ]
    stack: |-
      Error: non-geospatial with tile size

I'm still working on figuring out how these changes caused that.

@Pessimistress
Copy link
Collaborator

Ah. headless-gl does not work with node.js>12.12.

@kylebarron
Copy link
Collaborator Author

I switched to node 10 and can run the tests locally now. I think there were two errors, and I fixed one but not the other yet.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.06%) to 80.633% when pulling 1985a37 on kylebarron:terrain-tiles-viewport into a5545e7 on uber:master.

@kylebarron
Copy link
Collaborator Author

I fixed failing tests but didn't add a zRange test. I could add tests that check against the current getTileIndices output, but I'm not sure I'd be able to independently compute the correctness of that output.

@kylebarron kylebarron mentioned this pull request Mar 21, 2020
7 tasks
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.

Incorrect extruded terrain tiles requested/displayed
5 participants