ENH: Removes elevation max zoom from exaggeration calculation for raster-dem sources

[brendan-ward]

This resolves #9159 by replacing the max zoom of a raster-dem tileset used in the hillshade exaggeration calculations with a constant.

Originally, the calculation in hillshade_prepare.fragment.glsl applied an exaggeration factor (0.3 - 0.4 depending on zoom) to the result of: u_zoom - u_maxzoom.

Instead of maxzoom on the raster-dem tileset only controlling the zooms for which tiles are available, this also caused hillshade layers to render differently for the same source tiles depending on how maxzoom was set. For tilesets with fewer available zoom levels, this also meant that tiles could not be overzoomed, and for tilesets with greater available zoom levels, it meant that tiles were not sufficiently exaggerated at low zooms.

The tilesets used for implementing the original functionality have a max zoom of 15, and it looks like the original exaggeration calculations were based on what seemed visually pleasing across that range.

Originally, zooming in beyond 15 continued to vary exaggeration based on zoom level. The changes here remove exaggeration beyond zoom 15, as the original solution was really meant to address exaggeration at lower zoom levels instead. As such, this will introduce slight changes in the rendering of hillshade layers beyond zoom 15, if they are kept visible. For the existing visible tiles, hillshade layers don't perform well beyond zoom 15 anyway, so hopefully this has a negligible impact on those uses.

I do not have access to elevation tiles beyond 15, so I was unable to test how tilesets that have more zoom levels available perform under this change.

<changelog>Resolves #9159: decouples hillshade exaggeration from raster-dem maxzoom</changelog>

@mapbox/gl-native: it looks like the native implementation references maxzoom but hardcoded to zoom 15:

• https://github.com/mapbox/mapbox-gl-native/blob/master/src/mbgl/renderer/layers/render_hillshade_layer.cpp#L159
• https://github.com/mapbox/mapbox-gl-native/blob/master/src/mbgl/renderer/layers/render_hillshade_layer.hpp#L26

But I couldn't find that max zoom was used in the actual hillshade calculation.

/cc @JeffreyEarly

[brendan-ward]

This includes an update to debug/hillshade.html to demonstrate the changes. For zooms <= 7, the original mapbox data and new layer "Mapbox data < zoom 8" should be equivalent. Beyond zoom 7, this will demonstrate overzooming of the source tiles.

[arindam1993]

This overall sounds good to me, source max zoom should not influence styling in any way.

@brendan-ward could you please add some render tests to cover this?
Documentation for writing them lives here: https://github.com/mapbox/mapbox-gl-js/blob/master/test/integration/README.md

Happy to answer any questions if you have trouble writing them!

[brendan-ward]

@arindam1993 There are a few existing render tests, though I'd be happy to add more to exercise this specifically.

integration/render-tests/hillshade-*

I'm not able to successfully run these following the instructions (I have an environment setup on MacOS according to the contributor docs):

yarn run test-render hillshade-accent-color/default 

fails like this:

yarn run test-render hillshade-accent-color/default                                                                                                                                                  
$ node --max-old-space-size=2048 test/render.test.js hillshade-accent-color/default
* Error: ENOENT: no such file or directory, open './test/integration/mapbox:/styles/mapbox/streets-v11'
This page appears to be missing CSS declarations for Mapbox GL JS, which may cause the map to display incorrectly. Please ensure your page includes mapbox-gl.css, as described in https://www.mapbox.com/mapbox-gl-js/api/.
Error: Failed to initialize WebGL
    at Map._setupPainter (./src/ui/map.js:2323:38)
    at new Map (./src/ui/map.js:427:14)
    at _729‍.r.module.exports (./test/suite_implementation.js:45:17)
    at ./test/integration/lib/render.js:83:9
    at ./test/integration/lib/harness.js:71:17
    at start (./node_modules/d3-queue/build/d3-queue.js:74:11)
    at poke (./node_modules/d3-queue/build/d3-queue.js:58:11)
    at Server.<anonymous> (./node_modules/d3-queue/build/d3-queue.js:90:23)
    at Object.onceWrapper (events.js:417:28)
    at Server.emit (events.js:311:20)
* errored hillshade-accent-color/default

(note: this is for the existing test, which should work)

I'm a bit stumped as I followed the examples in the integration test docs. Can you please point me in the right direction? I didn't see this covered elsewhere in the docs.

[arindam1993]

That seems to be coming from headless-gl.
Ah, what version of node are you running, our tooling only works with ^10.16.

[brendan-ward]

I was running 12.6.1 but now see the requirement in the docs to only use 10.*. I switched to 10.21.0 and now the tests are working properly. Thank you!

[brendan-ward]

I added a couple integration tests to exercise this functionality, so this should be ready for review.

What I didn't see was a way to document the intent of an integration test: i.e., that setting a lower max zoom should produce the same image at zooms below that level. Otherwise, it is just assumed that whatever expected image is checked in is self-evidently right (beyond the person that added the test). Is there a place to add this info? I'm thinking just a one-liner.

[karimnaaji]

What I didn't see was a way to document the intent of an integration test: i.e., that setting a lower max zoom should produce the same image at zooms below that level. Otherwise, it is just assumed that whatever expected image is checked in is self-evidently right (beyond the person that added the test). Is there a place to add this info? I'm thinking just a one-liner.

I think that's a very good point, I've found myself in front of failing render tests without knowing exactly the why of the expectation other than the hint given by the test name itself. If that's what you mean?

[karimnaaji]

I checked what you could be using for that and there's the "description" tag in metadata in the test fixtures, so it would be the right place for adding such information. For example here: https://github.com/mapbox/mapbox-gl-js/blob/master/test/integration/render-tests/collator/resolved-locale/style.json

[brendan-ward]

Thanks @karimnaaji - I added those here. This might be a good thing to note on the integration test README

[karimnaaji]

Terrarium also matches this and goes up to 15 for max zoom.

[karimnaaji]

Looks good, thanks for the PR!

[karimnaaji]

Thanks @karimnaaji - I added those here. This might be a good thing to note on the integration test README

Yep! I'm glad you asked because I wasn't aware of such a tag. I'll document it, it could be one of the reasons it's underused.

[arindam1993]

CP'ed the tests onto master to check how the output changes, the restoration of detail looks good to me.

this branch:

master:

this branch:

master:

Thanks for the PR @brendan-ward , merging!

[arindam1993]

also, cc @mapbox/gl-native shader changes incoming.