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 raster-dem source type and hillshade layer type #5286

Merged
merged 48 commits into from
Dec 2, 2017
Merged

Conversation

mollymerp
Copy link
Contributor

@mollymerp mollymerp commented Sep 14, 2017

Moving to a fresh PR in place of #4701

This PR implements client-side hillshading using the Mapbox Terrain RGB tileset – right now it is not extensible to other DEM encoding formats.

Todo:

cc @kkaefer

Launch Checklist

  • briefly describe the changes in this PR
  • write tests for all new functionality
  • document any changes to public APIs
  • post benchmark scores
  • manually test the debug page

@mollymerp mollymerp force-pushed the raster-hillshade branch 4 times, most recently from f7eb44b to d000949 Compare September 21, 2017 21:24
@mollymerp mollymerp added cross-platform 📺 Requires coordination with Mapbox GL Native (style specification, rendering tests, etc.) feature 🍏 under development labels Sep 21, 2017
@mollymerp mollymerp force-pushed the raster-hillshade branch 3 times, most recently from 57840c2 to 3e6a146 Compare September 22, 2017 04:44
@mollymerp mollymerp requested a review from kkaefer September 22, 2017 05:02
@mollymerp
Copy link
Contributor Author

@kkaefer making progress with testing + getting through the backlog. The raster masking did fix the overlapping tile flashes, but there is still flickering coming from the texture getting updated with more border data as neighboring tiles load. Tomorrow, I will be focusing on alternative solutions to that tile flickering and writing more unit tests, but whenever you have time to review the PR in its current state that would be much appreciated. Thanks!

I may have questions about performance + profiling as well as I try to get this through the finish line.

@mollymerp
Copy link
Contributor Author

note that hillshade layer types aren't rendering and don't produce console errors on the 2 android devices that I've tested it on. will try and get my hands on a test device to debug more thoroughly ASAP

Copy link
Member

@kkaefer kkaefer left a comment

Choose a reason for hiding this comment

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

I tried out this PR and I'm still seeing a few issues.

  • In Safari and Firefox, rendering looks weird (Chrome looks fine):

mapbox gl js debug page 2017-09-27 17-15-53

* Sometimes, loading a tile won't complete (see above) * Sometimes, backfilling doesn't happen even though tiles are loaded, which leads to borders being shown around the tiles:

mapbox gl js debug page 2017-09-27 17-17-39

* When loading the map, we seem to be double-drawing the tiles for a few frames:

a
b
c

  • Tiles are clipped when they don't have a backfilled border, but it's still showing a 1px border for some tiles:

mapbox gl js debug page 2017-09-27 17-25-10

* We should vertically exaggerate shading for lower zoom levels, e.g. z3 on the Himalaya currently looks like this:

mapbox gl js debug page 2017-09-27 17-28-16

  • There appears to be a drawing problem (possibly related with the double draw?) when overzooming tiles. These frames are select from a zoom in animation:

vlcsnap-2017-09-27-17h36m54s650
vlcsnap-2017-09-27-17h37m00s041
vlcsnap-2017-09-27-17h37m03s072
vlcsnap-2017-09-27-17h37m07s137
vlcsnap-2017-09-27-17h37m19s928
vlcsnap-2017-09-27-17h37m27s040
vlcsnap-2017-09-27-17h37m31s593
vlcsnap-2017-09-27-17h37m40s484
vlcsnap-2017-09-27-17h37m44s647
vlcsnap-2017-09-27-17h37m50s885

  • When zooming in, we seem to be evicting parent terrain tiles before we can render the new ones. These are 6 consecutive frames after zooming in:

vlcsnap-2017-09-27-17h43m36s281
vlcsnap-2017-09-27-17h43m39s493
vlcsnap-2017-09-27-17h43m43s779
vlcsnap-2017-09-27-17h43m45s318
vlcsnap-2017-09-27-17h43m46s463
vlcsnap-2017-09-27-17h43m48s771





Copy link
Member

Choose a reason for hiding this comment

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

Nit: whitespace

@mollymerp
Copy link
Contributor Author

In Safari and Firefox, rendering looks weird (Chrome looks fine):

When loading the map, we seem to be double-drawing the tiles for a few frames:

these were both issues w the debug page (not ensuring the file extension of the terrain source was pngraw and changing the light on load.

There appears to be a drawing problem (possibly related with the double draw?) when overzooming tiles. These frames are select from a zoom in animation:

fix for this is up at #5380

(i've collapsed some of the rendering images already being addressed into details for scrolling brevity)

will be working on the tile border issues and the disappearing child tiles tomorrow.

@mollymerp
Copy link
Contributor Author

I can't reproduce the parent tile eviction on zoom in but I did find an issue where we were evicting child tiles prematurely on zoom out if the zoom out is > 1 zoom level. I think this is a bug in native as well.

@mollymerp mollymerp force-pushed the raster-hillshade branch 2 times, most recently from d5798f2 to 605a487 Compare October 4, 2017 00:47
@mollymerp
Copy link
Contributor Author

I tested out a new approach to tile borders that I think looks pretty good compared to the flashing white borders – when a tile first loads I create a 1px border around the original image populated with the elevation data from the nearest pixel with real data, and then I replace this value with the real neighboring tile data once it loads. I think it looks a lot better than the previous approach, and it solves the issue of flashing / persistent white seams.

@mollymerp
Copy link
Contributor Author

@nickidlugash when you have a sec to look at the shaders / default style-spec properties that would be great. you'll need to pull down this branch and then you can run yarn start-debug and load http://localhost:9966/debug/terrain-rendering.html in your browser. I'm particularly interested in what you think about the intensity by zoom level and what happens at points of extreme slope (there are some craters in the Himalayas that are good examples of this as well as the grand canyon).

To modify the intensity / zoom function you'll want to play around with this line, and then the final shading color is set here

let me know if you have any questions!

craters in the Himalayas
pasted image at 2017_10_03 05_40 pm

@mollymerp
Copy link
Contributor Author

@kkaefer today I was able to isolate the missing tiles to an issue with the terrainPrepare step where apparently the data we're putting in the texture is incomplete/invalid/not ready on the first frame that tries to render a tile.

If you change the code so that terrainPrepare runs on every frame, the missing tiles eventually appear.

When I render out the raw pixel data we're passing into the tile.demTexture I get black squares, but when I log out the pixelData I'm not seeing any tiles with missing/weird looking values.

image

if (this.map._refreshExpiredTiles) tile.setExpiryData(img);

if (img) {
if (this.map._refreshExpiredTiles) tile.setExpiryData(img);
Copy link
Member

Choose a reason for hiding this comment

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

We're already executing this code two lines above. Is this needed again?

if (!tile.workerID || tile.state === 'expired' || tile.state === 'unloaded') {
tile.workerID = this.dispatcher.send('loadDEMTile', params, done.bind(this));
}
}
Copy link
Member

Choose a reason for hiding this comment

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

What happens when there's no img object? Can this happen?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm pretty sure that ajax#getImage prevents a case where there'd be no img and no err

rawImageData: rawImageData
};

if (!tile.workerID || tile.state === 'expired' || tile.state === 'unloaded') {
Copy link
Member

Choose a reason for hiding this comment

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

Why are we starting a new loadDEMTile worker when the the tile is expired or unloaded/aborted?

@mollymerp
Copy link
Contributor Author

💀 the missing/flickering tiles were due to UNPACK_PREMULTIPLY_ALPHA_WEBGL being set to true elsewhere in the rendering loop.

I also was able to fix the rendering failure on android by setting precision to highp in the shaders.

as I see it the outstanding items (copied from above) are

  • decide how we want to handle ensuring we use .pngraw when requesting terrain-rgb tiles
  • get shader/rendering input from @nickidlugash
  • code reviews 🔍 📝
  • bonus: benchmarking?

@mollymerp mollymerp force-pushed the raster-hillshade branch 3 times, most recently from 63a0fb5 to 0bfea45 Compare December 2, 2017 00:09
@mollymerp
Copy link
Contributor Author

thanks for sticking with me y'all ⛰ 🗻 ⛰
thanks @anandthakker and @kkaefer for continued reviews and feedback!

note that there may be some caching on the api side that causes the endpoint to still return webp files so you may need to use the following transform function for the next few hours

transformRequest: (url, resourceType) =>{
    if (resourceType === 'Tile' && url.search('terrain')) {
        return {
            url: url.replace('@2x', '').replace(/(\.webp)|(\.png)/, '.pngraw')
        };
    }
}

@mollymerp mollymerp merged commit 722ae7c into master Dec 2, 2017
@stevewillard
Copy link

I have been following this PR for awhile. This is some really great work @mollymerp Nice job! 🎉

I look forward to trying this layer type out

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cross-platform 📺 Requires coordination with Mapbox GL Native (style specification, rendering tests, etc.) feature 🍏
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants