Oneliner to Zoom to 3DTileset #6104

hanbollar · 2018-01-09T21:32:12Z

Fixes #5626

Added Viewer.zoomToTileset
Updated sandcastle 3DTileset examples to use this added function

cesium-concierge · 2018-01-09T21:32:13Z

Welcome to the Cesium community @hanbollar!

Can you please send in a Contributor License Agreement (CLA) so that we can review and merge this pull request?

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

🌍 🌎 🌏

mramato · 2018-01-09T21:39:37Z

Thanks @hanbollar. Is there a reason we added zoomToTileset instead of updating zoomTo and flyTo to simply handle tilesets? For consistency, I think that is the better approach, since viewer.zoomTo/flyTo is how we handle everything else (entities, imagery, etc..)

hanbollar · 2018-01-09T21:51:54Z

Thanks for the feedback @mramato - i'll work on doing that modification

…se zoomTo. Updated Changes.md.

…set. modified zoomToTarget accordingly. added tests for zoomTo and flyTo.

hanbollar · 2018-01-12T21:50:16Z

@mramato - updated so that zooming to a tileset uses zoomTo and flyTo

ggetz

Thanks @hanbollar! Great start, especially for your second Cesium issue! I have a few comments before we merge.

ggetz · 2018-01-12T21:49:38Z

CHANGES.md

@@ -25,6 +25,8 @@ Change Log
   * Only one node is supported.
   * Only one mesh per node is supported.
   * Only one primitive per mesh is supported.
+* Updated documentation links to reflect new locations on cesiumjs.org and cesium.com. 


This is a duplicate of line 32

ggetz · 2018-01-12T21:51:04Z

Source/Widgets/Viewer/Viewer.js

@@ -1748,12 +1757,12 @@ Either specify options.terrainProvider instead or set options.baseLayerPicker to
     * target will be the range. The heading will be determined from the offset. If the heading cannot be
     * determined from the offset, the heading will be north.</p>
     *
-     * @param {Entity|Entity[]|EntityCollection|DataSource|ImageryLayer|Promise.<Entity|Entity[]|EntityCollection|DataSource|ImageryLayer>} target The entity, array of entities, entity collection, data source or imagery layer to view. You can also pass a promise that resolves to one of the previously mentioned types.
+     * @param {Entity|Entity[]|EntityCollection|DataSource|ImageryLayer|Cesium3DTileset|Promise.<Entity|Entity[]|EntityCollection|DataSource|ImageryLayer|Cesium3DTileset>} target The entity, array of entities, entity collection, data source, Cesium3DTileset, or imagery layer to view. You can also pass a promise that resolves to one of the previously mentioned types.


You should update the description to include tilesets as well. Same applies to zoomTo.

ggetz · 2018-01-12T21:51:37Z

Source/Widgets/Viewer/Viewer.js

@@ -1778,6 +1787,7 @@ Either specify options.terrainProvider instead or set options.baseLayerPicker to
        that._zoomOptions = options;

        when(zoomTarget, function(zoomTarget) {
+


Remove extra space.

ggetz · 2018-01-12T21:52:06Z

Source/Widgets/Viewer/Viewer.js

     * @param {Object} [options] Object with the following properties:
     * @param {Number} [options.duration=3.0] The duration of the flight in seconds.
     * @param {Number} [options.maximumHeight] The maximum height at the peak of the flight.
     * @param {HeadingPitchRange} [options.offset] The offset from the target in the local east-north-up reference frame centered at the target.
-     * @returns {Promise.<Boolean>} A Promise that resolves to true if the flight was successful or false if the entity is not currently visualized in the scene or the flight was cancelled.
+     * @returns {Promise.<Boolean>} A Promise that resolves to true if the flight was successful or false if the entity is not currently visualized in the scene or the flight was cancelled. //TODO: Cleanup entity mentions


Resolve this TODO

And do the same for zoomTo

ggetz · 2018-01-12T21:54:47Z

Source/Widgets/Viewer/Viewer.js

+            });
+        }
+
+        var entities = target;


For clarity, move this to after the check for an ImageryLayer, when we know the target is a list of entities.

ggetz · 2018-01-12T22:00:13Z

Specs/Widgets/Viewer/ViewerSpec.js

+            url : path
+        });
+
+        //spyOn(viewer.camera, 'viewBoundingSphere');


Remove commented code.

ggetz · 2018-01-12T22:05:04Z

Specs/Widgets/Viewer/ViewerSpec.js

+        });
+    });
+
+    it('flys to target when target is Cesium3DTileset and offset is defined', function() {


I think your tests names can be a little more precise. For instance, this one can be "flyTo flies to Cesium3DTileset with offset".

"flys to when target is Cesium3DTileset and uses default offset when offset not defined in options" -> "flyTo flies to Cesium3DTileset with default offset"

ggetz · 2018-01-12T22:07:35Z

Specs/Widgets/Viewer/ViewerSpec.js

+
+    });
+
+    it('flys to when target is Cesium3DTileset and uses default offset when offset not defined in options', function() {


Just to make sure we're not breaking functionality, it might be best to add some tests to check zooming to entities. You should be able to duplicate this test and just pass an entity or array of entities instead of a tileset.

ggetz · 2018-01-12T22:12:47Z

Source/Widgets/Viewer/Viewer.js

+        var options;
+
+        // If zoomTarget was Cesium3DTileset
+        if (defined(target.readyPromise)) {


instanceof should work properly now since you fixed the error with the import.

ggetz · 2018-01-12T22:16:34Z

Source/Widgets/Viewer/Viewer.js

+                    zoomOptions.offset = new HeadingPitchRange(0.0, 0.0, 2.0 * bSphere.radius);
+                }
+
+                options = {


Make sure you are taking into account the following options that can be passed to the flyTo function.

* @param {Number} [options.duration=3.0] The duration of the flight in seconds. * @param {Number} [options.maximumHeight] The maximum height at the peak of the flight.

Add a tests for when these are provided, they are being passed to flyToBoundingSphere.

…ng zoomTo and flyTo when target is entity

ggetz

@hanbollar Thanks for the updates! I have a few more comments.

ggetz · 2018-01-16T17:30:51Z

Apps/Sandcastle/gallery/3D Tiles Adjust Height.html

@@ -61,9 +61,7 @@
 }));

 tileset.readyPromise.then(function() {


Since zoomTo now encapsulates the readyPromise, we can update these to be

viewer.zoomTo(tileset, new Cesium.HeadingPitchRange(...)) .otherwise(function(error) { throw(error); });

ggetz · 2018-01-16T18:14:45Z

Specs/Widgets/Viewer/ViewerSpec.js

+    });
+
+    it('zoomTo zooms to Cesium3DTileset with default offset when offset not defined', function() {
+        viewer = createViewer(container);


Indentation looks a little off, try running the autoformatter in Webstorm.

ggetz · 2018-01-16T18:15:51Z

Specs/Widgets/Viewer/ViewerSpec.js

+        // load the tileset then check tests
+        return tileset.readyPromise.then(function() {
+            var boundingSphere = tileset.boundingSphere;
+            var offset = new HeadingPitchRange(0.4, 1.2, 4 * boundingSphere.radius);


4 -> 4.0

https://github.com/AnalyticalGraphicsInc/cesium/blob/master/Documentation/Contributors/CodingGuide/README.md#basic-code-construction

ggetz · 2018-01-16T18:21:39Z

Specs/Widgets/Viewer/ViewerSpec.js

+
+        return viewer.zoomTo(entities, offset).then(function() {
+            // moved to new location
+            expect(viewer.camera.position).not.toEqual(camPos);


I think here and in the above tests, just spy on camera.flyToBoundingSphere or viewBoundingSphere and make sure it's being called with the options you expect it to be. You can also just pass a new hardcoded HeadingPitchRange with made up numbers since the function won't be called through. You don't need to iterate through all the bounding spheres in these tests

ggetz · 2018-01-16T18:25:45Z

Specs/Widgets/Viewer/ViewerSpec.js

+            viewer.render();
+
+            return promise.then(function() {
+                expect(wasCompleted).toEqual(true);


Also expect flyToBoundingSphere toHaveBeenCalledWith the proper options.

ggetz · 2018-01-16T18:41:09Z

Apps/Sandcastle/gallery/3D Tiles Point Cloud.html

-    var boundingSphere = tileset.boundingSphere;
-    viewer.camera.viewBoundingSphere(boundingSphere, new Cesium.HeadingPitchRange(0.0, -0.5, boundingSphere.radius));
-    viewer.camera.lookAtTransform(Cesium.Matrix4.IDENTITY);
+    viewer.zoomTo(tileset, new Cesium.HeadingPitchRange(0.0, -0.5, tileset.boundingSphere.radius));


Since this is used most commonly, can we make this the default?

Then we can use the shorthand:

viewer.zoomTo(tileset).otherwise(function(error) { throw(error); });

ggetz · 2018-01-16T18:41:40Z

Apps/Sandcastle/gallery/3D Tiles Batch Table Hierarchy.html

-    var boundingSphere = tileset.boundingSphere;
-    viewer.camera.viewBoundingSphere(boundingSphere, new Cesium.HeadingPitchRange(0.0, -0.3, 0.0));
-    viewer.camera.lookAtTransform(Cesium.Matrix4.IDENTITY);
+    viewer.zoomToTileset(tileset, new Cesium.HeadingPitchRange(0.0, -0.3, 0.0));


This one is not based on the bounding sphere, so we don't need to wait for the readyPromise.

…o's default offset. updated tests for zoomTo and flyTo to include entities and certain expectations

hpinkos · 2018-01-17T22:30:25Z

@hanbollar make sure to make a comment when this is ready for another look. The reviewer doesn't get notifications when you add new commits, so it's a good idea to comment to let them know.

ggetz · 2018-01-17T22:54:54Z

Thanks @hanbollar !

hanbollar added 6 commits January 9, 2018 14:26

changed zoom for tileset so 1 line call instead of 3

872b8dc

Merge remote-tracking branch 'cesium-upstream/master'

a28dcce

created function that zooms to a 3DTileset

4d31962

updated 3DTiles files in sandcastle to use zoomToTiles

4c8a5be

added update note to changes.md

ad555b4

added in missing

01af1c7

hanbollar added 7 commits January 11, 2018 10:57

modified zoomToOrFly to include target of Cesium3DTileset

e11e281

updated sandbox 3D Tiles to use zoomTo instead of separate function

d24e99f

Deleted Viewer.zoomToTileset. Updated Sandbox 3Dtileset examples to u…

6690763

…se zoomTo. Updated Changes.md.

updated zoomTo to use options parameter to helper instead of just off…

601e242

…set. modified zoomToTarget accordingly. added tests for zoomTo and flyTo.

updated CHANGED.md and removed items specified by eslint

2893507

Merge remote-tracking branch 'cesium-upstream/master'

a2a0348

updated CHANGES.md from merge

9cdb975

updated typo

4e5ba70

ggetz requested changes Jan 12, 2018

View reviewed changes

hanbollar added 3 commits January 12, 2018 17:21

updated typos due to last typo

b1f7296

modified as per ggetz's recommendations and added in tests for checki…

dfa8c2c

…ng zoomTo and flyTo when target is entity

removed unused variable

d899c13

ggetz requested changes Jan 16, 2018

View reviewed changes

ggetz reviewed Jan 16, 2018

View reviewed changes

hanbollar added 2 commits January 16, 2018 15:11

updated 3d tiles sandcastle examples for some to be able to use zoomT…

1ca28af

…o's default offset. updated tests for zoomTo and flyTo to include entities and certain expectations

removed commented code

254d8a4

ggetz approved these changes Jan 17, 2018

View reviewed changes

ggetz merged commit 4440bd9 into CesiumGS:master Jan 17, 2018

hanbollar deleted the zoom-to-tileset branch February 2, 2018 19:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Oneliner to Zoom to 3DTileset #6104

Oneliner to Zoom to 3DTileset #6104

hanbollar commented Jan 9, 2018

cesium-concierge commented Jan 9, 2018

mramato commented Jan 9, 2018

hanbollar commented Jan 9, 2018

hanbollar commented Jan 12, 2018

ggetz left a comment

ggetz Jan 12, 2018

ggetz Jan 12, 2018

ggetz Jan 12, 2018

ggetz Jan 12, 2018

ggetz Jan 12, 2018

ggetz Jan 12, 2018

ggetz Jan 12, 2018

ggetz Jan 12, 2018

ggetz Jan 12, 2018

ggetz Jan 12, 2018

ggetz Jan 12, 2018

ggetz left a comment

ggetz Jan 16, 2018

ggetz Jan 16, 2018

ggetz Jan 16, 2018

ggetz Jan 16, 2018

ggetz Jan 16, 2018

ggetz Jan 16, 2018

ggetz Jan 16, 2018

hpinkos commented Jan 17, 2018

ggetz commented Jan 17, 2018

		@@ -1778,6 +1787,7 @@ Either specify options.terrainProvider instead or set options.baseLayerPicker to
		that._zoomOptions = options;

		when(zoomTarget, function(zoomTarget) {


		});

		it('flys to when target is Cesium3DTileset and uses default offset when offset not defined in options', function() {

Oneliner to Zoom to 3DTileset #6104

Oneliner to Zoom to 3DTileset #6104

Conversation

hanbollar commented Jan 9, 2018

cesium-concierge commented Jan 9, 2018

mramato commented Jan 9, 2018

hanbollar commented Jan 9, 2018

hanbollar commented Jan 12, 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

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

hpinkos commented Jan 17, 2018

ggetz commented Jan 17, 2018