Slope and aspect #7415

spencerparkin · 2018-12-14T19:00:16Z

This pull request adds aspect (an angle) to the czm_MaterialInput structure and converts the slope member of this structure to an angle.

The following images show the same region (Mt. Superior in Utah), the first shows aspect coloring, the second shows slope coloring.

There may be just a few small kinks to work out, but I thought it a good idea to start the pull request so that you guys can get excited about this new feature. I already have a use for it in one of my personal Cesium-based projects.

cesium-concierge · 2018-12-14T19:00:18Z

Thank you so much for the pull request @spencerparkin! I noticed this is your first pull request and I wanted to say welcome to the Cesium community!

The Pull Request Guidelines is a handy reference for making sure your PR gets accepted quickly, so make sure to skim that.

❌ Missing CONTRIBUTORS.md entry.
- Please add yourself to the contributors file!
❌ Missing CLA.
- Please send in a Contributor License Agreement (CLA) and comment back here to let us know to check this!
❔ CHANGES.md was not updated.
- If this change updates the public API in any way, please add a bullet point to CHANGES.md.

Reviewers, don't forget to make sure that:

Cesium Viewer works.
Works in 2D/CV.
Works (or fails gracefully) in IE11.

I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome.

🌍 🌎 🌏

spencerparkin · 2018-12-14T19:05:05Z

Here is Half-Dome colored by aspect...

Here is Half-Dome colored by slope...

I think that the slope coloring looks better now that the parameter is linearized.

hpinkos · 2018-12-14T19:07:13Z

Looks awesome @spencerparkin! Please send us over a Contributor License Agreement when you get a chance so we can review and merge this =)

OmarShehata · 2018-12-17T16:30:24Z

Just for context this was also requested here: https://groups.google.com/d/msg/cesium-dev/s2Dr9YFG2fE/rY8LketHAAAJ

spencerparkin · 2018-12-17T16:43:45Z

One thought that occurred to me over the weekend is that if the shaders are compiled on-demand at run-time, then we can probably turn slope/aspect features on/off at run-time using pre-processor directives. This might be a good optimization for applications that don't care about slope or aspect, or just aspect. On the other hand, maybe the additions to the vertex shader are negligible across all applications; I don't know. Note that it is faster to do trig functions in the vertex shader rather in the pixel shader, if you're going to do them at all.

Sent the CLA.

pjcozzi · 2018-12-21T15:33:29Z

@spencerparkin thanks again for contributing, we received your CLA!

spencerparkin · 2018-12-26T04:24:17Z

Following is a screen-shot of a web-app I'm working on that takes advantage of the slope and aspect feature of this pull request. Here I'm showing Gobbler's Knob under the current avalanche forecast.

spencerparkin · 2018-12-26T06:50:19Z

Here's a working version of the app if anyone wants to give it a go...

https://utah-ava-map.herokuapp.com/

There is a lot of room for improvement, but this is the basic idea.

I'm not sure how many people would have to use this app before Heroku started charging my credit card. It's something that makes me feel uneasy, actually.

OmarShehata · 2018-12-26T11:09:47Z

@spencerparkin that looks pretty awesome! Thanks for sharing that. Are those polygon/entities on top of the terrain?

For Heroku, if you're on the free plan, it shouldn't charge you once you exceed the maximum, the app just stops working.

When you use all your free dyno hours for a given month, all free apps on your account will be forced to sleep for the rest of the month.

Source.

spencerparkin · 2018-12-26T16:29:49Z

@OmarShehata, thanks, and thanks for that info; that's good to know.

No, I'm not adding any polygons or entities to the scene. I'm just providing my own pixel shader for the terrain. You can see the details here.

pjcozzi · 2018-12-27T15:47:05Z

One thought that occurred to me over the weekend is that if the shaders are compiled on-demand at run-time, then we can probably turn slope/aspect features on/off at run-time using pre-processor directives. This might be a good optimization for applications that don't care about slope or aspect, or just aspect. On the other hand, maybe the additions to the vertex shader are negligible across all applications; I don't know. Note that it is faster to do trig functions in the vertex shader rather in the pixel shader, if you're going to do them at all.

Yes, good idea, please do this as it is a common GLSL optimization that we follow throughout CesiumJS.

pjcozzi · 2018-12-27T15:53:07Z

@lilleyse could you please review and decide if the breaking change is OK in this case since it likely won't break anyone in practice? I also noticed several coding style inconsistencies but I'm sure you will flag them.

@spencerparkin thanks again for contributing and sharing the link to your app, great job! As we discussed offline, we would be pleased to showcase it when you are ready. Also please allow @lilleyse a few weeks to review as he is quite busy.

pjcozzi · 2018-12-27T15:54:29Z

One thought that occurred to me over the weekend...

Yes, good idea, please do this as it is a common GLSL optimization that we follow throughout CesiumJS.

Actually given that the whole material block is protected by APPLY_MATERIAL, I don't know if it is a big enough deal. @lilleyse can advise.

cesium-concierge · 2019-01-30T03:00:15Z

Thanks again for your contribution @spencerparkin!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

hpinkos · 2019-01-30T23:00:47Z

@lilleyse will you have time to review this soon?

@jasonbeverage could you do an initial review?

jasonbeverage · 2019-01-30T23:33:55Z

Unfortunately I haven't been keeping up with this commit. I played with the demo and it looked really great, so I'll leave it in Sean's able hands to do an official review and blessing :)

hpinkos · 2019-01-31T15:53:36Z

Thanks @jasonbeverage !

lilleyse · 2019-01-31T19:17:40Z

Source/Shaders/GlobeVS.glsl

-    vec3 ellipsoidNormal = normalize(position3DWC.xyz);
-    v_slope = abs(dot(ellipsoidNormal, finalNormal));
-    v_height = height;
+    vec3 northPolePositionMC = vec3(0.0, 0.0, 6356752.3142451793);


Might be better to use czm_getWgs84EllipsoidEC().radii.z rather than hardcoding

lilleyse · 2019-01-31T19:17:42Z

Source/Shaders/GlobeVS.glsl

+    vec3 normal_projected = v_normalMC - normal_rejected; // This is the terrain normal projected onto the tangent plane.
+    vec3 aspect_vector = normalize(normal_projected);   // This is our unit-length heading in the tangent space.  (Aspect direction.)
+    v_aspect = acos(dot(aspect_vector, vectorEastMC));  // Finally, here is the aspect angle.
+    v_height = height;  // Store height.


A couple style notes:

Try to reduce the number of comments and avoid informal language like "we". The code self-documents itself pretty well so maybe just a summary above the code and comments whenever lines aren't obvious is enough.

Except for varyings (v_aspect etc), use camel case for variable names.

lilleyse · 2019-01-31T19:18:25Z

Source/Shaders/Materials/AspectRampMaterial.glsl

+czm_material czm_getMaterial(czm_materialInput materialInput)
+{
+    czm_material material = czm_getDefaultMaterial(materialInput);
+    vec4 rampColor = texture2D(image, vec2(materialInput.aspect / (2.0 * 3.1415926536), 0.5));


Use czm_pi here.

lilleyse · 2019-01-31T19:19:16Z

Source/Shaders/Materials/SlopeRampMaterial.glsl

@@ -3,7 +3,7 @@ uniform sampler2D image;
 czm_material czm_getMaterial(czm_materialInput materialInput)
 {
    czm_material material = czm_getDefaultMaterial(materialInput);
-    vec4 rampColor = texture2D(image, vec2(materialInput.slope, 0.5));
+    vec4 rampColor = texture2D(image, vec2(materialInput.slope / (3.1415926536 / 2.0), 0.5));


czm_pi here as well.

lilleyse · 2019-01-31T19:19:24Z

Source/Shaders/GlobeVS.glsl

+    // There is one final matter to clear up.  Our aspect is in [0,pi), but we need it to be in [0,2pi).
+    float determ = dot(cross(vectorEastMC, aspect_vector), ellipsoidNormal); // Calculate a 3x3 determinant.
+    if(determ < 0.0)        // GLSL shouldn't actually have to do a branching operation here.
+        v_aspect = 2.0 * 3.1415926536 - v_aspect;


Alternatively use czm_branchFreeTernary

v_aspect = czm_branchFreeTernary(determ < 0.0, 2.0 * czm_pi - v_aspect, v_aspect)

lilleyse · 2019-01-31T19:29:11Z

@spencerparkin this looks great overall. I left mainly style comments. I also pushed a small style commit: d137096.

Could you add a test? It will look similar to renders a globe with a SlopeRamp

@lilleyse could you please review and decide if the breaking change is OK in this case since it likely won't break anyone in practice?

The breaking change is ok with me.

Actually given that the whole material block is protected by APPLY_MATERIAL, I don't know if it is a big enough deal. @lilleyse can advise.

Yeah, for that reason I think it is fine as it is.

…works

spencerparkin · 2019-02-01T22:22:21Z

@lilleyse i made some changes and tested them. Thanks for reviewing. Please let me know if I've missed anything.

lilleyse

Thanks @spencerparkin, just quick suggestions for the comments.

lilleyse · 2019-02-01T23:06:01Z

Source/Shaders/GlobeVS.glsl

-    v_slope = abs(dot(ellipsoidNormal, finalNormal));
+    float northPoleZ = czm_getWgs84EllipsoidEC().radii.z;
+    vec3 northPolePositionMC = vec3(0.0, 0.0, northPoleZ);
+    vec3 ellipsoidNormal = normalize(v_positionMC); // TODO: For a sphere this is correct, but not generally for an ellipsoid!


The comment is fine, but remove the TODO.

Usually if there is something actionable we would write up an issue, but in this case I think it's ok to remove the TODO and not write an issue.

okay @lilleyse, i deleted the "TODO" prefix.

lilleyse · 2019-02-01T23:06:08Z

Source/Shaders/GlobeVS.glsl

+    vec3 northPolePositionMC = vec3(0.0, 0.0, northPoleZ);
+    vec3 ellipsoidNormal = normalize(v_positionMC); // TODO: For a sphere this is correct, but not generally for an ellipsoid!
+    vec3 vectorEastMC = normalize(cross(northPolePositionMC - v_positionMC, ellipsoidNormal));
+    float dotProd = abs(dot(ellipsoidNormal, v_normalMC));      // Why absolute value here?  What about overhanging terrain?  (e.g., an arch.)


Since the terrain system is 2.5D (one height value per x,y coordinate) I don't think the comment necessarily applies.

okay @lilleyse, i deleted the comment

lilleyse · 2019-02-04T14:04:14Z

Great! Thanks again @spencerparkin!

cesium-concierge · 2019-02-04T14:04:23Z

Congratulations on closing the issue! I found these Cesium forum links in the comments above:

https://groups.google.com/d/msg/cesium-dev/s2Dr9YFG2fE/rY8LketHAAAJ

If this issue affects any of these threads, please post a comment like the following:

The issue at #7415 has just been closed and may resolve your issue. Look for the change in the next stable release of Cesium or get it now in the master branch on GitHub https://github.com/AnalyticalGraphicsInc/cesium.

Modestgentleman · 2019-07-17T05:54:32Z

我想问一下可不可以通过在地形上画面，使画面的一部分显示坡度与坡向？还有能不能拿到画面部分内的所有高程数据？

spencerparkin · 2019-07-22T16:55:19Z

If I understand your question correctly (according to Google translate), what you're asking about might require that the interpolated longitude and latitude information be given to the shader. I'd look for that in the documentation.

…

On Tue, Jul 16, 2019 at 11:55 PM 不卑不亢 ***@***.***> wrote: 我想问一下可不可以通过在地形上画面，使画面的一部分显示坡度与坡向？还有能不能拿到画面部分内的所有高程数据？ — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#7415>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAPCZM7Q3XCEKHCIZE3ADFDP72X3VANCNFSM4GKQIXXQ> .

Modestgentleman · 2019-07-30T08:35:18Z

I'm sorry to return your information so late.The analysis of topographic gradient and slope direction scope for globa material.How to Act on local material?

Spencer Parkin added 12 commits December 12, 2018 16:14

add slope and aspect; not sure how to calculate aspect yet

a6cca2a

be optimistic that i can actually get this to work

f305696

add AspectRamp build-in material type

ce39cda

expand sand-box demo to include aspect coloring

fb5941f

aspect calculation almost there

8452c23

fix bug; don't worry about if-statement in glsl code

c9fce49

satisfy the linter

9d45d82

fix glsl compile error

ff7444a

complete the aspect calculation

9f8f998

fix bug

b223669

fix bug

981341a

duuuuh!!!

bb6f9ed

Spencer Parkin added 3 commits December 14, 2018 12:11

fix bug

441f2e9

changes noted

bb4b116

delete stale comment

f3ded26

Merge branch 'master' into slope-and-aspect

1e1a9f6

pjcozzi assigned lilleyse Dec 27, 2018

Merge branch 'master' into slope-and-aspect

92e1e60

lilleyse added 2 commits January 31, 2019 11:36

Merge branch 'master' into slope-and-aspect

24ab9c2

Small JS style fixes

d137096

lilleyse reviewed Jan 31, 2019

View reviewed changes

Spencer Parkin added 8 commits February 1, 2019 14:28

rename variable; use camel-case

e154cc1

no need to maintain comment on every line

45e0fe2

make use of branch-free macro

e34d69a

use define for pi constant

d491707

sense model coord z for standard ellipsoid

c2c78dd

fix syntax error

ed5598f

have to add this comment

22a33be

add spect ramp test...admittedly, i'm not sure what this does/how it …

0c60ebf

…works

Merge branch 'master' into slope-and-aspect

d75c88e

lilleyse reviewed Feb 1, 2019

View reviewed changes

clean up comments

6960d16

lilleyse merged commit cee1a3a into CesiumGS:master Feb 4, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Slope and aspect #7415

Slope and aspect #7415

spencerparkin commented Dec 14, 2018

cesium-concierge commented Dec 14, 2018 •

edited by lilleyse

Loading

spencerparkin commented Dec 14, 2018 •

edited

Loading

hpinkos commented Dec 14, 2018

OmarShehata commented Dec 17, 2018

spencerparkin commented Dec 17, 2018 •

edited

Loading

pjcozzi commented Dec 21, 2018

spencerparkin commented Dec 26, 2018 •

edited

Loading

spencerparkin commented Dec 26, 2018

OmarShehata commented Dec 26, 2018

spencerparkin commented Dec 26, 2018

pjcozzi commented Dec 27, 2018

pjcozzi commented Dec 27, 2018

pjcozzi commented Dec 27, 2018

cesium-concierge commented Jan 30, 2019

hpinkos commented Jan 30, 2019

jasonbeverage commented Jan 30, 2019

hpinkos commented Jan 31, 2019

lilleyse Jan 31, 2019

lilleyse Jan 31, 2019

lilleyse Jan 31, 2019

lilleyse Jan 31, 2019

lilleyse Jan 31, 2019

lilleyse commented Jan 31, 2019

spencerparkin commented Feb 1, 2019

lilleyse left a comment

lilleyse Feb 1, 2019

spencerparkin Feb 3, 2019

lilleyse Feb 1, 2019

spencerparkin Feb 3, 2019

lilleyse commented Feb 4, 2019

cesium-concierge commented Feb 4, 2019

Modestgentleman commented Jul 17, 2019

spencerparkin commented Jul 22, 2019 via email

Modestgentleman commented Jul 30, 2019

Slope and aspect #7415

Slope and aspect #7415

Conversation

spencerparkin commented Dec 14, 2018

cesium-concierge commented Dec 14, 2018 • edited by lilleyse Loading

spencerparkin commented Dec 14, 2018 • edited Loading

hpinkos commented Dec 14, 2018

OmarShehata commented Dec 17, 2018

spencerparkin commented Dec 17, 2018 • edited Loading

pjcozzi commented Dec 21, 2018

spencerparkin commented Dec 26, 2018 • edited Loading

spencerparkin commented Dec 26, 2018

OmarShehata commented Dec 26, 2018

spencerparkin commented Dec 26, 2018

pjcozzi commented Dec 27, 2018

pjcozzi commented Dec 27, 2018

pjcozzi commented Dec 27, 2018

cesium-concierge commented Jan 30, 2019

hpinkos commented Jan 30, 2019

jasonbeverage commented Jan 30, 2019

hpinkos commented Jan 31, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilleyse commented Jan 31, 2019

spencerparkin commented Feb 1, 2019

lilleyse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilleyse commented Feb 4, 2019

cesium-concierge commented Feb 4, 2019

Modestgentleman commented Jul 17, 2019

spencerparkin commented Jul 22, 2019 via email

Modestgentleman commented Jul 30, 2019

cesium-concierge commented Dec 14, 2018 •

edited by lilleyse

Loading

spencerparkin commented Dec 14, 2018 •

edited

Loading

spencerparkin commented Dec 17, 2018 •

edited

Loading

spencerparkin commented Dec 26, 2018 •

edited

Loading