-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Adds support for arbitrary raster dem max zoom levels #6055
Conversation
Now I see that some tests (all combinations with hillshade-translucent) fail on your CI. Running these tests locally passes all of them. I suppose that could be tied with the difference in brightness with and without this modification but then, I don't understand how it works in my computer. |
thanks @ibesora – I mistakenly thought that the maxzoom for terrain-rgb was 14 but it is actually 15. I'm not sure we need to add a new uniform, however, because both mapbox and mapzen terrain rgb sources only go up to Z15 – are you thinking of the case where a user would generate their own tileset? @nickidlugash can you take a look at this change from the carto perspective? |
Yup, as I said, in the national mapping agency where I work we have a very detailed DEM, encoded in Mapbox terrain RGB and Mapzen's Terrarium formats, that can go to level 18. If the cost of setting the uniform is too much, seeing that the max zoom level of a layer doesn't change, could a shader define be set when the shader is compiled? |
ah yep sorry I didn't read the OP closely enough 😳 – I'm ok with adding the uniform if it will be useful to users 👍 to fix the test you'll need to run |
I've just generated and pushed the expected images. |
@mollymerp @ibesora sorry for the delay – I think this PR makes sense and looks good 👍 Since it makes the hillshading slightly more exaggerated at all zoom levels (compared to the implementation in master), I reviewed the hillshading display with a variety of values for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
merged as #6103 |
Launch Checklist
When the hillshade_prepare shader computes the exaggeration factor, a max zoom level of 14 is assumed. We have a raster dem pyramid generated from our own data that goes to level 16 so when the exaggeration factor is computed in levels 15 and 16, giving a result bigger than 1, the hillshade is scaled down.
This PR sends the source-defined max zoom level to the shader so the exaggeration factor can be correctly computed.
The mapbox.terrain-rgb style JSON defines a max zoom level of 15.
At level 15 and higher, the zoom uniform is bigger than 14 so, when computing the exaggeration factor, the slope is instead dimmed. With this modification, computing the exaggeration factor with a max zoom value of 15, increases the darkness of the hillshade at levels >= 15.
You can see the difference in this gif.
With our dem pyramid the hillshade is not dimmed when on levels bigger than 14.
@mollymerp Does this sound good to you? I have run all the tests and works as expected.