docs for hillshade layer and raster-dem source by andrewharvey · Pull Request #5805 · mapbox/mapbox-gl-js

andrewharvey · 2017-12-04T21:56:17Z

@mollymerp This isn't ready for merging, but posting it here for feedback. This PR includes:

added hillshade example
I'm not sure if we should have a dedicated example for the hillshade layer, but I put one together.
added raster-dem source and hillshade layer to docs.
included raster-dem source in the style spec rather than it being a type of the raster source.

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

Thanks so much for tackling this @andrewharvey! This is looking great – just a few small comments.

mollymerp · 2017-12-05T18:35:41Z

docs/pages/example/hillshade.html

+    map.removeLayer('hillshade_highlight_bright');
+
+    map.setLight({
+        position: [1.15, 335, 60]


I don't think we need to include this because we decided to use layer-specific lighting properties for the hillshade layer type (hillshade-illumination-direction and hillshade-illumination-anchor).

I did this since the debug map used a light position, but I see what you mean now.

mollymerp · 2017-12-05T18:50:59Z

docs/pages/example/hillshade.html

+        "id": "hillshading",
+        "source": "dem",
+        "type": "hillshade"
+    }, 'waterway-river-canal-shadow');


maybe add a comment explaining why we want the hillshade layer underneath this layer

mollymerp · 2017-12-05T22:11:08Z

src/style-spec/validate/validate_source.js

@@ -40,7 +40,7 @@ module.exports = function validateSource(options) {
        errors = errors.concat(validateObject({


I think we can actually get rid of this case now and add raster-dem into the previous case with vector and raster sources. you will have to edit line 26 to something like

valueSpec: styleSpec[`source_${type.replace('-','_'}`],

to get it to work.

Good point, that reduces code duplication.

mollymerp · 2017-12-05T22:21:34Z

docs/pages/example/hillshade.html

+var map = new mapboxgl.Map({
+    container: 'map',
+    style: 'mapbox://styles/mapbox/outdoors-v9',
+    center: [-74.50, 40],


let's change the map center to somewhere with ⛰ s. Z11, center: -119.5857, 37.7221 is yosemite valley for example

andrewharvey · 2017-12-05T23:22:54Z

docs/pages/example/hillshade.html

+    map.removeLayer('hillshade_shadow_med');
+    map.removeLayer('hillshade_shadow_faint');
+    map.removeLayer('hillshade_highlight_med');
+    map.removeLayer('hillshade_highlight_bright');


@mollymerp I'm not sure about this. In terms of an example for the hillshade layer, it might make better sense to just use the Mapbox Basic style and add hillshading to that. It would be a simpler example.

This example is more complex as it removes the existing hillshade layers from Outdoors client side. Especially given any real map wanting Outdoors with raster hillshading would first cut the Hillshade layers from Outdoors in Studio and save that as a new style rather than doing it client side... 💭

yeah good point – I made a new public style in the Mapbox account that is Outdoors minus the hillshade layers,so you can use this style id and explain with a comment what it is ?

mapbox://styles/mapbox/cjaudgl840gn32rnrepcb9b9g

Great. Assuming you're okay that people will copy the example and rely on that style not disappearing in the future?

yep – and we can replace it with a permanent style whenever our core styles start using this layer type.

mollymerp · 2017-12-07T04:42:37Z

Thanks for the fixes @andrewharvey – I added a couple tweaks to the example, and added in the explicit properties the raster-dem source type currently supports for consistency with the other source types, as well as to remove the scheme property, which isn't currently supported because this source only works with the mapbox terrain-rgb tileset at the moment.

andrewharvey · 2017-12-07T04:42:51Z

src/style-spec/reference/v8.json

+      "value": "string",
+      "doc": "An array of one or more tile source URLs, as in the TileJSON spec."
+    },
+    "bounds": {


That's interesting, I tested bounds and found it had no affect. I better re-test it.

in order for it to work you have to use the tiles: [...] property to specify the url because otherwise the bounds returned by the source's TileJSON will override the ones you specify. which I suppose we should also document 😅

this worked as expected for me:

"rasterTerrain": { "type": "raster-dem", "tiles": [`https://a.tiles.mapbox.com/v4/mapbox.terrain-rgb/{z}/{x}/{y}.png?access_token=${mapboxgl.accessToken}`], "bounds" : [-40, -40, 40, 40] }

ahh that makes sense now 😁

Agreed, it should be documented but I don't think it's critical for this PR. The same applies to the rest of these properties and raster sources too.

andrewharvey force-pushed the andrewharvey-hillshade-docs branch from 686f330 to eb74914 Compare December 4, 2017 21:58

andrewharvey added docs 📜 under development labels Dec 4, 2017

andrewharvey force-pushed the andrewharvey-hillshade-docs branch 4 times, most recently from fa57cc4 to 0121b03 Compare December 5, 2017 00:32

andrewharvey removed the under development label Dec 5, 2017

mollymerp reviewed Dec 5, 2017

View reviewed changes

andrewharvey force-pushed the andrewharvey-hillshade-docs branch from 0121b03 to f133fde Compare December 5, 2017 23:16

andrewharvey commented Dec 5, 2017

View reviewed changes

docs for hillshading

0f5be60

andrewharvey force-pushed the andrewharvey-hillshade-docs branch from f133fde to 0f5be60 Compare December 5, 2017 23:48

use mapbox style without the hillshade layers

9d9f834

andrewharvey changed the title ~~sketch of docs for hillshading~~ docs for hillshade layer and raster-dem source Dec 7, 2017

tweak center and comment

0d6da33

mollymerp force-pushed the andrewharvey-hillshade-docs branch from dd3ebfe to 0d6da33 Compare December 7, 2017 04:20

be explicit about which properties RasterDEMSource supports

879e977

mollymerp approved these changes Dec 7, 2017

View reviewed changes

andrewharvey commented Dec 7, 2017

View reviewed changes

mollymerp merged commit 93319ba into master Dec 7, 2017

mollymerp deleted the andrewharvey-hillshade-docs branch December 7, 2017 18:34

		@@ -40,7 +40,7 @@ module.exports = function validateSource(options) {
		errors = errors.concat(validateObject({

Conversation

andrewharvey commented Dec 4, 2017

Uh oh!

mollymerp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewharvey Dec 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mollymerp commented Dec 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

andrewharvey Dec 5, 2017 •

edited

Loading