Added terrain drawing sandcastle example #6692

OmarShehata · 2018-06-15T19:58:57Z

This adds a Sandcastle example of how to dynamically draw lines and polygons over terrain. Click to add a vertex, right click to terminate current shape and start a new one.

The polygons are clamped to the terrain. The lines are clamped once #6689 is merged. You can test it in the mean time by commenting out clampToGround : true, in the two locations (but you won't see the lines if you're zoomed in because they'll be under the terrain).

The only issue is that there seems to be a bit of a lag when you terminate the shape. @mramato do you know a way to fix this? The relevant function is here.

cesium-concierge · 2018-06-15T19:58:59Z

@OmarShehata, thanks for the pull request! Maintainers, we have a signed CLA from @OmarShehata, so you can review this at any time.

⚠️ I noticed that CHANGES.md has not been updated. If this change updates the public API in any way, fixes a bug, or makes any non-trivial update, please add a bullet point to CHANGES.md and comment on this pull request so we know it was updated. For more info, see the Pull Request Guidelines.

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

🌍 🌎 🌏

ggetz

Nice job @OmarShehata! A very helpful (and fun!) example.

Some suggestions:

It would be nice if we could add minimal onscreen instructions for using the demo. We can just add this in the HTML, and maybe consider removing the instructions in the comments.
To avoid the infobox showing up when you click on an entity, I think we should disable it entirely.
Double-clicking to place a point sets the viewer's target entity, which causes some unexpected camera movement.

ggetz · 2018-06-18T15:34:23Z

Apps/Sandcastle/gallery/Drawing on Terrain.html

+            var dynamicPositions = new Cesium.CallbackProperty(function () {
+                // This is a work-around since polylines when `clampToGround` is true will
+                // crash if you try to draw a zero length line
+                if (activeShapePoints.length === 2) {


Is this fixed by the latest polyline on terrain PR? If not, can we open an issue if there isn't already one?

Just tested it with the fix in #6693 and it looks like we don't need this workaround anymore! I'll remove it.

ggetz · 2018-06-18T15:36:42Z

Apps/Sandcastle/gallery/Drawing on Terrain.html

+                    }
+                });
+            }
+            if (drawingMode === 'polygon') {


This structure of two independent if statements seem odd to me, even though only one will be exected-- We should use one behavior or the other

Since we only have two drawing modes, do you think a boolean would be a better way than a string to keep track of which mode?

I've moved this into a function as per @hpinkos 's suggestion, but I don't think we should make it just a boolean. If it were just something like drawingLines, then to me it's not obvious that when that's false then it draws polygons as opposed to something else. I think it makes more sense for it to be an explicit mode.

I did make it an else if since you are right that there's no scenario where you want both to run.

OK, I see your point. In that case, maybe it's worth renaming the mode and label from "line" to "polyline" to match the entity type used?

ggetz · 2018-06-18T15:39:22Z

Apps/Sandcastle/gallery/Drawing on Terrain.html

+<style>
+    @import url(../templates/bucket.css);
+
+    #viewChanged, #cameraChanged {


Unless I missed something, I don't think these styles are used anywhere.

hpinkos · 2018-06-18T15:44:44Z

Apps/Sandcastle/gallery/Drawing on Terrain.html

+            heightReference: Cesium.HeightReference.CLAMP_TO_GROUND
+        }
+    });
+    viewer.entities.add(point);


To be consistent with the rest of our sandcastle examples, use

var point = viewer.entities.add({ ... });

hpinkos · 2018-06-18T15:45:24Z

Apps/Sandcastle/gallery/Drawing on Terrain.html

+    selectionIndicator : false,
+    terrainProvider : Cesium.createWorldTerrain()
+});
+// Create a function to place a point given a world position.


We try to avoid cluttering up sandcastle examples with inline comments unless there is something truly non-obvious. I think most if not all of these comments can be removed.

Good point - I just noticed most of the other Sandcastle examples don't have comments. I was treating it more as a tutorial, but perhaps I could write up an actual tutorial for that later.

Yep, a tutorial would be the way to go with all that extra information

hpinkos · 2018-06-18T16:08:52Z

Apps/Sandcastle/gallery/Drawing on Terrain.html

+    // `earthPosition` will be undefined if our mouse is not over the globe.
+    if (Cesium.defined(earthPosition)) {
+        // If this is the first point, we need to initialize the shape.
+        if (activeShapePoints.length === 0) {


This can be combined with the above if statement.
if (Cesium.defined(earthPosition) && activeShapePoints.length === 0)

There's actually a couple of things that happen even if activeShapePoints.length != 0 that's why it's 2 statements.

hpinkos · 2018-06-18T16:11:48Z

Apps/Sandcastle/gallery/Drawing on Terrain.html

+// On right click, terminate active shape and redraw it as a new entity without the `CallbackProperty`.
+function terminateShape() {
+    activeShapePoints.pop();
+    if (drawingMode === 'line') {


This looks duplicated with L88-105. Maybe make a function to handle this like you did for createPoint

hpinkos · 2018-06-18T16:15:08Z

Use the View as Thumbnail button on the Sandcastle toolbar to get a snapshot that is the right size and dimensions. It looks like yours is cutting off the timeline/animation widgets.

OmarShehata · 2018-06-19T15:41:45Z

@hpinkos I didn't realize View As Thumbnail was a feature! That looks super useful. I'm having trouble with that though - it scales it down to what looks like the right aspect ratio but not the right dimensions. The correct size for thumbnails seems to be 225x150 (based on what most of the other ones are), does that sound right?

OmarShehata · 2018-06-19T16:07:04Z

Once the tests run, this should be ready for review again @ggetz !

hpinkos · 2018-06-19T16:32:38Z

it scales it down to what looks like the right aspect ratio but not the right dimensions

Yes, it should be 225x150px. I've always used the thumbnail button, taken a screenshot, and cropped it using a paint program and it's turned out correctly.

ggetz · 2018-06-19T21:36:07Z

Apps/Sandcastle/gallery/Drawing on Terrain.html

+    activeShapePoints.pop();
+    drawShape(activeShapePoints);
+    viewer.entities.remove(floatingPoint);
+    viewer.entities.remove(activeShape);


The shapes flash when you terminate the shape. Is it possible to re-use the same entity and create the position property, or do we have to delete the old one and create a new dynamic one?

I just tested that out, and it'll still take a few seconds to render when you switch the CallbackProperty with just an array of points.

I talked briefly with @mramato about this. We could do any of the following to remove that flicker:

Keep all the shapes with isConstant : false

Don't remove the dynamic shape right away, keep it there for a few seconds to hide the gap until the new one renders.

But none of those seem like good practices we want to encourage in an official Sandcastle example.

It might be nice to have a "flush" command that would try to immediately render an entity.

This is a long standing issue in "sometimes changing" geometry data in Cesium. I recommend this PR keeps whatever is cleanest for example code. I would love to see us improve interactive geometry like this, but it's probably going to result in a request-render style method for entities that needs to be thought through.

ggetz · 2018-06-19T21:43:32Z

@OmarShehata Thanks for the updates! Have you merged cesium/master into this branch? I get developer errors when drawing polylines.

OmarShehata · 2018-06-20T13:03:50Z

Sorry about that @ggetz , just pushed the merge!

ggetz · 2018-06-21T13:38:17Z

A very nice Sandcastle example, @OmarShehata ! And useful- I've already linked to it from a forum question.

Thanks!

Added terrain drawing sandcastle example

b6a1068

ggetz requested changes Jun 18, 2018

View reviewed changes

hpinkos reviewed Jun 18, 2018

View reviewed changes

Omar Shehata added 3 commits June 18, 2018 16:13

Added on screen instructions

bf07321

Disabled infobox

5c39a4b

Disabled double click

137398d

OmarShehata mentioned this pull request Jun 18, 2018

check for adjacent duplicate positions in GroundPolylineGeometry #6693

Merged

Omar Shehata added 2 commits June 19, 2018 08:51

Merge branch 'master' into terrain-drawing-example

38a2a3a

Removed polyline crash workaround and extraneous comments

2b49eda

Omar Shehata added 4 commits June 19, 2018 11:43

Put repeated code into a function

7acde7a

Updated thumbnail

eb4db3f

Removed one more workaround

d1b8b64

Fix linting issue

17e9447

ggetz reviewed Jun 19, 2018

View reviewed changes

Merge branch 'master' into terrain-drawing-example

ee49788

ggetz approved these changes Jun 21, 2018

View reviewed changes

ggetz merged commit 58ba7b5 into CesiumGS:master Jun 21, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added terrain drawing sandcastle example #6692

Added terrain drawing sandcastle example #6692

OmarShehata commented Jun 15, 2018 •

edited

Loading

cesium-concierge commented Jun 15, 2018

ggetz left a comment

ggetz Jun 18, 2018

OmarShehata Jun 18, 2018

ggetz Jun 18, 2018

OmarShehata Jun 19, 2018

ggetz Jun 19, 2018

ggetz Jun 18, 2018

hpinkos Jun 18, 2018

hpinkos Jun 18, 2018

OmarShehata Jun 18, 2018

hpinkos Jun 18, 2018

hpinkos Jun 18, 2018

OmarShehata Jun 18, 2018

hpinkos Jun 18, 2018

hpinkos commented Jun 18, 2018

OmarShehata commented Jun 19, 2018

OmarShehata commented Jun 19, 2018

hpinkos commented Jun 19, 2018

ggetz Jun 19, 2018

OmarShehata Jun 20, 2018

mramato Jun 20, 2018

ggetz commented Jun 19, 2018

OmarShehata commented Jun 20, 2018

ggetz commented Jun 21, 2018

Added terrain drawing sandcastle example #6692

Added terrain drawing sandcastle example #6692

Conversation

OmarShehata commented Jun 15, 2018 • edited Loading

cesium-concierge commented Jun 15, 2018

ggetz 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

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

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

hpinkos commented Jun 18, 2018

OmarShehata commented Jun 19, 2018

OmarShehata commented Jun 19, 2018

hpinkos commented Jun 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggetz commented Jun 19, 2018

OmarShehata commented Jun 20, 2018

ggetz commented Jun 21, 2018

OmarShehata commented Jun 15, 2018 •

edited

Loading