Nyc app #2

rahwang · 2017-07-05T22:17:25Z

Here's a rough draft of the app. Would you mind taking a big picture look? I'm still considering adding

Flyover camera mode
Visibility test, eg. how many points are currently visible in drone camera mode.

I also still need to create a CZML file of drone positions, which I'm still not sure how to approach. You mentioned that Ed might be able to help me with this, but he's out until July 10.

rahwang · 2017-07-10T20:18:18Z

Hey @hpinkos, would you mind taking a look at this for cleanliness and content? There's a lot that can definitely be improved! Thanks!

hpinkos · 2017-07-10T20:36:12Z

README.md

+A simple JavaScript app showcasing some features of [Cesium](http://cesium.agi.com/), the open-source WebGL virtual globe and map engine.  Just fork this repo and start coding.
+
+**Cesium version**: [1.34](http://cesiumjs.org/downloads.html).


Should this be Cesium 1.35? Or probably Cesium 1.36 because the conference is in August?

Yep! Should be 1.36

hpinkos · 2017-07-10T20:39:54Z

README.md

+This uses the unminified version of Cesium.js, which is great for debugging but is quite large for production deployments.  To use the minified version, run `ant` with `minify` instead of `combine` before updating `cesium-starter-app`:
+```
+./Tools/apache-ant-1.8.2/bin/ant clean minify


We don't use ant anymore. Where did you get this from? It needs to be updated to use gulp

hpinkos · 2017-07-10T20:42:24Z

Source/App.js

+    //////////////////////////////////////////////////////////////////////////
+
+    var viewer = new Cesium.Viewer('cesiumContainer', {
+        navigationHelpButton: false,


I wouldn't disable this. No one knows how to use our mouse controls the first time they bring up Cesium

hpinkos · 2017-07-10T20:47:12Z

Source/App.js

+    // Loading Imagery
+    //////////////////////////////////////////////////////////////////////////
+
+    Cesium.BingMapsApi.defaultKey = 'AsarFiDvISunWhi137V7l5Bu80baB73npU98oTyjqKOb7NbrkiuBPZfDxgXTrGtQ'; // For use on cesiumjs.org only


I don't think we want to use this in a public app. Should we need to use a different imagery provider?

Right, I sort of put this in as a placeholder because I wasn't sure -- what's the correct thing to do re Bing key for this application? Should we request a new one just for this repo? @hpinkos

@mramato what do you think?

hpinkos · 2017-07-10T20:47:58Z

Source/App.js

+    // Add Bing imagery
+    viewer.imageryLayers.addImageryProvider(new Cesium.BingMapsImageryProvider({
+        url : 'https://dev.virtualearth.net',
+        mapStyle: Cesium.BingMapsStyle.AERIAL // Can also use Cesium.BingMapsStyle.ROADS


I personally like AERIAL_WITH_LABELS. The satellite imagery is nice, and adding the labels makes it more accessible

hpinkos · 2017-07-10T20:49:28Z

Source/App.js

+                // Add to the neighborhoods group
+                entity.parent = neighborhoods;
+                // Generate Polygon center
+                var poly_center = Cesium.BoundingSphere.fromPoints(entity.polygon.hierarchy.getValue().positions).center;


Don't you need to pass a JulianDate with the current time to getValue?

hpinkos · 2017-07-10T20:50:36Z

Source/App.js

+                poly_center = Cesium.Ellipsoid.WGS84.scaleToGeodeticSurface(poly_center);
+                entity.position = poly_center;
+                // Generate labels
+                entity.label = new Cesium.LabelGraphics({


I would just set label equal to the in the object instead of explicitly creating a new LabelGraphics

Sure thing! But just for my own edification, what's the difference there? Aren't you creating a new object anyway?

I don't think it really makes a difference, no. That's just what we do in all our other sandcastle examples.

hpinkos · 2017-07-10T20:53:51Z

Source/App.js

+    });
+
+    // Interpolate smoothly between position sample points.
+    drone.position.setInterpolationOptions({


Should this be in the computeCircularFlight function?

computeCircularFlight is a placeholder until I've created a CZML file to read drone positions from.

hpinkos · 2017-07-10T20:54:34Z

Source/App.js

+            pickedEntity.billboard.color = Cesium.Color.ORANGERED;
+            previousPickedEntity = pickedEntity;
+        }
+    }, Cesium.ScreenSpaceEventType.MOUSE_MOVE);


I think it's better to do LEFT_CLICK than MOUSE_MOVE. I think doing things on MOUSE_MOVE can get annoying

I sort of like the on hover behavior, and it matches the original nyc-demo app. I'll let a couple of people try it and I'll go with whatever the majority opinion is.

hpinkos · 2017-07-10T20:55:28Z

Source/App.js

+    var scratch = new Cesium.Matrix4();
+    var camera =  viewer.scene.camera;
+    function followDrone() {
+        drone._getModelMatrix(viewer.clock.currentTime, scratch);


We shouldn't teach users to access private functions and variables. Is there another way to do this? If not, maybe we should add one to Cesium

Yep! I've done that in #5584. I'll switch it out as soon as its merged.

hpinkos · 2017-07-10T21:00:05Z

Make sure we properly credit the data sources in your presentation where necessary

hpinkos · 2017-07-10T21:00:50Z

README.md

+* `node server.js`
+
+Browse to `http://localhost:8000/`


The server.js included in this repo runs at 8080 by default

hpinkos · 2017-07-10T21:04:29Z

Source/App.js

+        if(droneModeElement.checked) {
+            viewer.scene.preRender.addEventListener(followDrone);
+        } else {
+            viewer.scene.preRender.removeEventListener(followDrone);


You need to reset the lookAt transform after we stop following the drone

hpinkos · 2017-07-10T21:06:01Z

Source/App.js

+        var entities = dataSource.entities.values;
+
+        for (var i = 0; i < entities.length; i++) {
+            var entity = entities[i];


Give the entity a name so the guid doesn't show up in the header of the infobox

hpinkos · 2017-07-10T21:06:46Z

Source/App.js

+
+                var description = '<table class="cesium-infoBox-defaultTable cesium-infoBox-defaultTable-lighter"><tbody>';
+                description += '<tr><th>' + "Latitude" + '</th><td>' + latitude + '</td></tr>';
+                description += '<tr><th>' + "Longitude" + '</th><td>' + longitude + '</td></tr>';


Do we have more interesting information to show here instead of just latitude and longitude?

hpinkos · 2017-07-10T21:07:58Z

Source/App.js

+    // Styling Data
+    //////////////////////////////////////////////////////////////////////////
+
+    var distanceDisplayCondition = new Cesium.DistanceDisplayCondition(10.0, 10000.0);


I think the far value could be a little further out. Especially for the ! billboards

hpinkos · 2017-07-10T21:09:54Z

Source/App.js

+
+            if (Cesium.defined(entity.polygon)) {
+                //Set the polygon material to a random, translucent color.
+                entity.polygon.material = Cesium.Color.fromRandom({


Did we try this on a less-than-stellar machine? I think this dataset might have too many polygons. Toggling them on caused my machine to lag a little and I have pretty good specs. We need something that will perform well on whatever low end laptops people might have with them

Alternatively, you could try running a polygon simplification algorithm like this one to create an easier dataset http://mourner.github.io/simplify-js/

Ho ho ho! I just found a different dataset that's 2 mb smaller which runs much better. Thanks for the suggestion though! I'll try it on a bad machine.

hpinkos · 2017-07-10T21:13:57Z

index.html

+        <input id="neighborhoods" type="checkbox"/>
+        <label for="neighborhoods">Neighborhoods</label>
+    </div>
+    <span> Cache max display distance (meters)</span>


I don't really understand what this slider is for

hpinkos · 2017-07-10T21:17:19Z

index.html

+    </div>
+    <span> Cache max display distance (meters)</span>
+    <div class="nowrap">
+        <input id="distanceSlider" type="range" min="1000" max="100000" value="10000" step="1000">


I think you need to add an input listener to this so the value updates as you move the slider, not just when you let go of it

hpinkos · 2017-07-10T21:19:42Z

index.css

+}
+
+.cover {
+    display: none;


What's the purpose of this if it's never shown?

Ah, fixed it. I do the loading at the end of the app. How can I check if all the visible terrain tiles are loaded?

hpinkos · 2017-07-10T21:20:30Z

I think the app overall has a good set of features. This are all of the comments I had about the implementation

mramato · 2017-08-04T15:57:42Z

Disclaimer, I intentionally did not read the workshop PR or look at the code yet, here's my thoughts after spending 10 minutes simply playing with the app:

Drone may be moving to fast, because even over a good connection the 3D Tiles can't stream in fast enough, meaning the drone spends a lot of time flying over nothing until everything loads
Why can't I move the camera while following the drone?
Clicking on polygons causes an improperly sized InfoBox
The Geocache display distance slider seems to not work (and it's disconnected from the number field next to it)
Switching Free from Drone View keeps the camera locked around a certain point. Are we forgetting to reset the transform back to IDENTITY?
Clicking the Home button while in "Drone View" doesn't seem to have any effect. We should fly to the home view and automatically go back to Free mode.
Terrain is on, but the tileset is not set up for terrian so it submerges some of the buildings (we can probably reprocess for terrain)
It would be nice if clicking on a building showed data or double click allowed you to zoom.
Double clicking on the drone tracks it, but the radio still indicates the camera is in Free view.
The Geocache placemarks do not appear to be on terrain, causing them to either be in the air or buried. Are we telling Cesium to clamp them to ground?
Would be nice to have a custom images for the geocache locations, it will make the app look nicer. (Could make it fun and use a treasure chest or something similar)
The styling examples could probably do something more interesting, I also noticed that when styling by height, the WTC is transparent but no other building is. Was that intentional?
I'm not going to point out "missing" Cesium features in the app because I know we're time contrained, but I assume you are going to discuss stuff like imagery, 2D, CV, etc.. elsehwere in the workshop?
I remember you saying something about writing a CameraLimiter to contrain the view to a certain bounding sphere or volume, is that still the plan? It would work nicely with this app.
Neighborhoods appear to be displayed by default but the checkbox is unchecked.

mramato · 2017-08-04T16:07:14Z

Looking at the code (only a high-level glance to start):

There are some bad habits here, I understand that we are trying to keep things simple for the workshop, but we also need to enforce good practices. At the very least we should wrap the entirety of App.js in an IIFE and enable strict mode (strict mode also improves performance). Basically this:

(function () {
    "use strict";
    /* globals Cesium */
...
Existing app.js
...
}());

You included the minified version of Cesium.js in your app, which means you won't be getting any error checking from Cesium itself. We strongly encourage users to use the unminified version of the app and then switch to the minified version for release.
Are we going to cover actually deploying a Cesium project? What files you need and other best practices? While general deployment is outside the scope of the project, we should at least cover the Cesium specific parts, since Cesium has more moving parts (by necessity) in order to accomplish what it does.

mramato · 2017-08-04T16:07:56Z

Source/App.js

+    //////////////////////////////////////////////////////////////////////////
+
+    // // If the mouse is over a point of interest, change the entity billboard scale and color
+    var previousPickedEntity = undefined;


There's no reason to explicitly set this to undefined.

mramato · 2017-08-04T16:38:07Z

Source/App.js

+    // // If the mouse is over a point of interest, change the entity billboard scale and color
+    var previousPickedEntity = undefined;
+
+    var handler = new Cesium.ScreenSpaceEventHandler(viewer.scene.canvas);


There's no reason to create a SSE here, you can use viewer.screenSpaceEventHandler.

Though in general, I push users away from using SSE and towards using standard mouse move events. It's important that we stress that using Cesium is similar to using any other browser widget/element. There's nothing special about mouse events.

rahwang · 2017-08-08T19:50:55Z

Thank you for the review, @mramato !

Drone may be moving to fast, because even over a good connection the 3D Tiles can't stream in
fast enough, meaning the drone spends a lot of time flying over nothing until everything loads

Agreed! Changed it to 2x, which seems to look much better.

Why can't I move the camera while following the drone?

This was intentional? I felt that the follow drone camera should have a fixed transform relative to the camera.

Clicking on polygons causes an improperly sized InfoBox

How do I control this??

The Geocache display distance slider seems to not work (and it's disconnected from the ?> number field next to it)

Removed. People weren't particularly interested in this feature at the dry-run anyway.

Switching Free from Drone View keeps the camera locked around a certain point. Are we ?
forgetting to reset the transform back to IDENTITY?

Clicking the Home button while in "Drone View" doesn't seem to have any effect. We should > fly to the home view and automatically go back to Free mode.

Fixed!

Terrain is on, but the tileset is not set up for terrian so it submerges some of the buildings ?
(we can probably reprocess for terrain)

How do I get this setup?

It would be nice if clicking on a building showed data or double click allowed you to zoom.

I'll add it if time allows.

Double clicking on the drone tracks it, but the radio still indicates the camera is in Free view.

The Geocache placemarks do not appear to be on terrain, causing them to either be in the ?> air or buried. Are we telling Cesium to clamp them to ground?

Yep, they're definitely ground clamped...

Would be nice to have a custom images for the geocache locations, it will make the app look > nicer. (Could make it fun and use a treasure chest or something similar)

Sure -- this has to be relatively low priority, but I'll take a look.

The styling examples could probably do something more interesting, I also noticed that
when styling by height, the WTC is transparent but no other building is. Was that intentional?

I actually copied the NYC demo app for that style. I'm open to other suggestions, but I figured this was enough since we have a more extensive sandcastle demo.

I'm not going to point out "missing" Cesium features in the app because I know we're time
constrained, but I assume you are going to discuss stuff like imagery, 2D, CV, etc.. elsehwere in > the workshop?

Yep, I mentioned a lot of things not directly in the tutorial during the dry run, I'll try to add as many of them as I can.

I remember you saying something about writing a CameraLimiter to contrain the view to a
certain bounding sphere or volume, is that still the plan? It would work nicely with this app.

Good idea! I'll try to get to it.

Neighborhoods appear to be displayed by default but the checkbox is unchecked.

Fixed.

lilleyse · 2017-08-10T18:11:41Z

Maybe turn off the water mask? The water looks especially pixely in this area.

lilleyse

Reviewed the code only, looks good.

lilleyse · 2017-08-10T18:13:03Z

Source/App.js

+    // Add Bing imagery
+    viewer.imageryLayers.addImageryProvider(new Cesium.BingMapsImageryProvider({
+        url : 'https://dev.virtualearth.net',
+        mapStyle: Cesium.BingMapsStyle.ROADS // Can also use Cesium.BingMapsStyle.ROADS


Comment seems outdated.

lilleyse · 2017-08-10T18:17:26Z

Source/App.js

+
+    // Load the NYC buildings tileset
+    var city = viewer.scene.primitives.add(new Cesium.Cesium3DTileset({
+        url: 'https://cesiumjs.org/NewYork/3DTilesGml',


Use the composer link instead: 'https://beta.cesium.com/api/assets/1461?access_token=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJqdGkiOiJkYWJmM2MzNS02OWM5LTQ3OWItYjEyYS0xZmNlODM5ZDNkMTYiLCJpZCI6NDQsImFzc2V0cyI6WzE0NjFdLCJpYXQiOjE0OTkyNjQ3NDN9.vuR75SqPDKcggvUrG_vpx0Av02jdiAxnnB1fNf-9f7s'

lilleyse · 2017-08-10T18:18:12Z

Source/App.js

+
+    city.readyPromise.then(function(tileset) {
+        tileset.style = defaultStyle;
+    });


I think you can set the style before the tileset is ready, but double check that.

lilleyse · 2017-08-10T18:19:11Z

Source/App.js

+    var handler = viewer.screenSpaceEventHandler;
+    handler.setInputAction(function (movement) {
+        var pickedPrimitive = viewer.scene.pick(movement.endPosition);
+        var pickedEntity = (Cesium.defined(pickedPrimitive)) ? pickedPrimitive.id : undefined;


No need to wrap with ().

lilleyse · 2017-08-10T18:19:42Z

Source/App.js

+
+    // Create a follow camera by tracking the drone entity
+    function setViewMode() {
+        if(droneModeElement.checked) {


Formatting: if (

lilleyse · 2017-08-10T18:21:37Z

Source/AppSkeleton.js

@@ -0,0 +1,343 @@
+(function () {


Just to be sure, is this file meant to be included?

Yes, see #2 (comment)

rahwang · 2017-08-10T21:01:05Z

Anything else @lilleyse ? Can this be merged?

lilleyse · 2017-08-10T23:16:22Z

That's all from me, but someone else who looked closer at everything should merge.

rahwang · 2017-08-11T13:14:02Z

Ok, thanks all for the reviews! In the interest of giving workshop attendees time to download, I'm going to go ahead and merge this, but please feel free to look at it some more.

rahwang added 3 commits June 20, 2017 13:19

updating cesium

f1435d7

all basic functionality in

4f36827

reorganizing

0c9e664

hpinkos reviewed Jul 10, 2017

View reviewed changes

rahwang added 2 commits July 12, 2017 10:55

clean up edits

4ea5962

edits

9cb96b4

rahwang force-pushed the nyc-app branch from a536def to 9cb96b4 Compare July 25, 2017 18:34

reorganizing

e49423b

rahwang force-pushed the nyc-app branch from a9b0991 to e49423b Compare July 28, 2017 23:21

mramato reviewed Aug 4, 2017

View reviewed changes

rahwang added 2 commits August 8, 2017 15:51

edits

b9a3fe8

many reorganization edits

a172ec1

lilleyse reviewed Aug 10, 2017

View reviewed changes

rahwang force-pushed the nyc-app branch from 2cad37c to fe238d7 Compare August 10, 2017 18:47

rahwang force-pushed the nyc-app branch 2 times, most recently from 478ac76 to ab92787 Compare August 11, 2017 13:18

tweaks

8fb14ff

rahwang force-pushed the nyc-app branch from ab92787 to 8fb14ff Compare August 11, 2017 13:25

rahwang merged commit dce6b6a into master Aug 11, 2017

rahwang mentioned this pull request Aug 14, 2017

Clean up issues #3

Open

		A simple JavaScript app showcasing some features of [Cesium](http://cesium.agi.com/), the open-source WebGL virtual globe and map engine. Just fork this repo and start coding.

		Cesium version: [1.34](http://cesiumjs.org/downloads.html).

Nyc app #2

Nyc app #2

Conversation

rahwang commented Jul 5, 2017

rahwang commented Jul 10, 2017

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

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 Jul 10, 2017

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 Jul 10, 2017

mramato commented Aug 4, 2017 • edited Loading

mramato commented Aug 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rahwang commented Aug 8, 2017

lilleyse commented Aug 10, 2017

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

Choose a reason for hiding this comment

lilleyse Aug 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rahwang commented Aug 10, 2017

lilleyse commented Aug 10, 2017

rahwang commented Aug 11, 2017

mramato commented Aug 4, 2017 •

edited

Loading

lilleyse Aug 10, 2017 •

edited

Loading