BoxOutlineGeometry and BoxGeometry Cleanup #3073

ggetz · 2015-10-14T00:35:29Z

Fixes #2260.

Changed minimumCorner and maximumCorner properties to just minimum and maximum.

Added fromAxisAlignedBoundingBox function.

pjcozzi · 2015-10-14T00:50:34Z

Thanks @ggetz.

Please update CHANGES.md and say the old parameters will be deprecated in Cesium 1.17.
Add backwards compatibility for the old parameters in the meantime. See https://github.com/AnalyticalGraphicsInc/cesium/wiki/Deprecation-Guide.
Update the unit tests.

pjcozzi · 2015-10-14T00:51:44Z

Source/Core/BoxGeometry.js

+     * });
+     * var geometry = Cesium.BoxGeometry.createGeometry(box);
+     */
+    BoxGeometry.fromAxisAlignedBoundingBox = function(options) {


This doesn't need to take options. It can just take the bounding box object directly.

Mention this function and BoxOutlineGeometry.fromAxisAlignedBoundingBox in CHANGES.md.

For more info on opening a complete pull request, see CONTRIBUTING.md.

ggetz · 2015-10-14T14:10:42Z

Ready for another look

ggetz · 2015-10-14T14:17:35Z

I also did a search on the project and found no other references to minimumCorner or maximumCorner

pjcozzi · 2015-10-14T14:17:46Z

CHANGES.md

 * Decreased GPU memory usage in `BillboardCollection` and `LabelCollection` by using the WebGL ANGLE_instanced_arrays extension.
 * Added CZML examples to Sandcastle.  See the new CZML tab.
 * Fixed token issue in ArcGisMapServerImageryProvider.
 * `ImageryLayerFeatureInfo` now has an `imageryLayer` property, indicating the layer that contains the feature.
+* Added `BoxOutlineGeometry.fromAxisAlignedBoundingBox` and `BoxGeometry.fromAxisAlignedBoundingBox` functtions


"functtions" -> "functions."

pjcozzi · 2015-10-14T14:18:15Z

@ggetz can you give me commit access to your repo to help us collaborate on pull requests?

pjcozzi · 2015-10-14T14:19:26Z

Source/Core/BoxGeometry.js

@@ -34,6 +36,8 @@ define([
     * @constructor
     *
     * @param {Object} options Object with the following properties:
+     * @param {Cartesian3} options.minimum The minimum x, y, and z coordinates of the box.
+     * @param {Cartesian3} options.maximum The maximum x, y, and z coordinates of the box.
     * @param {Cartesian3} options.minimumCorner The minimum x, y, and z coordinates of the box.


Remove the doc for minimumCorner and maximumCorner (since we don't want users to use them). Same comment for BoxOutlineGeometry.

pjcozzi · 2015-10-14T14:26:05Z

Source/Core/BoxGeometry.js

+            throw new DeveloperError('boundingBox is required.');
+        }
+
+        var min = boundingBox.minimum;


Since we only use them in one place, there's no need to declare these locals. Let's keep it concise:

return new BoxGeometry({ minimum : boundingBox.minimum, maximum : boundingBox.maximum });

pjcozzi · 2015-10-14T14:28:16Z

@ggetz thanks for the updates.

Any comment I made about BoxGeometry also applies to BoxOutlineGeometry.

ggetz · 2015-10-14T14:51:25Z

@pjcozzi I made the changes you suggested

pjcozzi · 2015-10-14T15:09:23Z

Source/Core/BoxGeometry.js

@@ -1,28 +1,29 @@
 /*global define*/
 define([
-        './BoundingSphere',


Make sure WebStorm is inserting spaces for tabs. We did not need to change the formatting here. We just wanted to add a reference for deprecationWarning.

…into boxOutlineGeometry-cleanup

…into boxOutlineGeometry-cleanup # Conflicts: # Source/Core/BoxGeometry.js # Source/Core/BoxOutlineGeometry.js

…into boxOutlineGeometry-cleanup # Conflicts: # Source/Core/BoxGeometry.js

ggetz · 2015-10-14T16:11:00Z

@pjcozzi I think I've fixed the formatting changes.

pjcozzi · 2015-10-14T16:17:57Z

Source/Core/PolygonOutlineGeometry.js

-     *     ])
-     *   },
-     *   extrudedHeight: 300000
+     *     ]),


Looks like the branches got mixed up, and this reverts your previous fix. Can you please update this?

pjcozzi · 2015-10-14T16:23:15Z

When I run all the tests, I get two test failures:

pjcozzi · 2015-10-14T16:23:45Z

Just a heads up that I made tweaks in 6e97913 and 716f8a6

…cleanup

…into boxOutlineGeometry-cleanup

…cleanup

ggetz · 2015-10-14T18:19:27Z

@pjcozzi All the tests are now working and I merged the conflicting changes.

mramato · 2015-10-14T18:54:01Z

CHANGES.md

@@ -6,6 +6,8 @@ Change Log
 * Breaking changes
  * Deleted old `<subfolder>/package.json` and `*.profile.js` files, not used since we moved away from a Dojo-based build years ago.  This should allow future compatibility with newer systems like Browserify and Webpack.
 * Deprecated
+  * Depreciated `BoxGeometry.minimumCorner` and `BoxGeometry.maximumCorner`, use `BoxGeometry.minimum` and `BoxGeometry.maximum` instead. These will be removed in 1.17.


This should be Deprecated not Depreciated

pjcozzi · 2015-10-14T18:55:58Z

Tests are good.

pjcozzi · 2015-10-14T18:56:10Z

@ggetz congrats on another merged pull request!

BoxOutlineGeometry and BoxGeometry Cleanup

ggetz · 2015-10-14T18:56:37Z

@pjcozzi thanks!

#3073 introduced a bug where the deprecated behavior did not work in release mode. Any code that handles deprecated behavior must be outside of pragma statements or else it won't be part of the minified release.

ggetz added 2 commits October 13, 2015 20:31

added fromAxisBoundingBox function to boxOutlineGeometry and boxGeometry

af7a839

Fixed error in boxGeometry

fda28bf

pjcozzi reviewed Oct 14, 2015
View reviewed changes

ggetz added 5 commits October 13, 2015 21:08

updated CHANGES

aaa6be2

updated CHANGES

add6c9f

removed option parameter

f6eff1d

Added unit tests

bf56a4a

Fixed error in documentation

8712156

pjcozzi reviewed Oct 14, 2015
View reviewed changes

Fixed error in CHANGES and updated documentation

4c3dc6d

pjcozzi reviewed Oct 14, 2015
View reviewed changes

Fixed Unit Tests

d18d6af

Tweak CHANGES.md

a76fecc

pjcozzi reviewed Oct 14, 2015
View reviewed changes

ggetz added 4 commits October 14, 2015 11:21

Removed formatting error

f905d2e

included deprecationWarning

8e365a1

Merge branch 'boxOutlineGeometry-cleanup' of github.com:ggetz/cesium …

ae4b234

…into boxOutlineGeometry-cleanup

Fixed formatting issues

2680429

ggetz added 4 commits October 14, 2015 11:43

Fixed deprecation warnings

3af93ef

Merge branch 'boxOutlineGeometry-cleanup' of github.com:ggetz/cesium …

d8ae47b

…into boxOutlineGeometry-cleanup # Conflicts: # Source/Core/BoxGeometry.js # Source/Core/BoxOutlineGeometry.js

Fixed Formatting

98a32e5

Merge branch 'boxOutlineGeometry-cleanup' of github.com:ggetz/cesium …

8d8d217

…into boxOutlineGeometry-cleanup # Conflicts: # Source/Core/BoxGeometry.js

pjcozzi added 2 commits October 14, 2015 12:15

Tweak

6e97913

Tweak

716f8a6

pjcozzi reviewed Oct 14, 2015
View reviewed changes

ggetz added 5 commits October 14, 2015 12:27

Merge remote-tracking branch 'cesium/master' into boxOutlineGeometry-…

713ad5e

…cleanup

Merge branch 'boxOutlineGeometry-cleanup' of github.com:ggetz/cesium …

e3801f7

…into boxOutlineGeometry-cleanup

Redo PolygonOutlineGeometry Fix

b7b89ae

fixed issue in unit test

3c22be6

Merge remote-tracking branch 'origin/master' into boxOutlineGeometry-…

a050e21

…cleanup

mramato reviewed Oct 14, 2015
View reviewed changes

Corrected Spelling error

4e2c188

pjcozzi added a commit that referenced this pull request Oct 14, 2015

Merge pull request #3073 from ggetz/boxOutlineGeometry-cleanup

29dfda4

BoxOutlineGeometry and BoxGeometry Cleanup

pjcozzi merged commit 29dfda4 into CesiumGS:master Oct 14, 2015

pjcozzi deleted the boxOutlineGeometry-cleanup branch October 14, 2015 18:56

pjcozzi mentioned this pull request Oct 14, 2015

Remove BoxGeometry minimumCorner and maximumCorner properties #3090

Closed

mramato mentioned this pull request Oct 20, 2015

Fix BoxGeometry deprecation. #3110

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BoxOutlineGeometry and BoxGeometry Cleanup #3073

BoxOutlineGeometry and BoxGeometry Cleanup #3073

ggetz commented Oct 14, 2015

pjcozzi commented Oct 14, 2015

pjcozzi Oct 14, 2015

pjcozzi Oct 14, 2015

pjcozzi Oct 14, 2015

ggetz commented Oct 14, 2015

ggetz commented Oct 14, 2015

pjcozzi Oct 14, 2015

pjcozzi commented Oct 14, 2015

pjcozzi Oct 14, 2015

pjcozzi Oct 14, 2015

pjcozzi commented Oct 14, 2015

ggetz commented Oct 14, 2015

pjcozzi Oct 14, 2015

ggetz commented Oct 14, 2015

pjcozzi Oct 14, 2015

pjcozzi commented Oct 14, 2015

pjcozzi commented Oct 14, 2015

ggetz commented Oct 14, 2015

mramato Oct 14, 2015

pjcozzi commented Oct 14, 2015

pjcozzi commented Oct 14, 2015

ggetz commented Oct 14, 2015

BoxOutlineGeometry and BoxGeometry Cleanup #3073

BoxOutlineGeometry and BoxGeometry Cleanup #3073

Conversation

ggetz commented Oct 14, 2015

pjcozzi commented Oct 14, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggetz commented Oct 14, 2015

ggetz commented Oct 14, 2015

Choose a reason for hiding this comment

pjcozzi commented Oct 14, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjcozzi commented Oct 14, 2015

ggetz commented Oct 14, 2015

Choose a reason for hiding this comment

ggetz commented Oct 14, 2015

Choose a reason for hiding this comment

pjcozzi commented Oct 14, 2015

pjcozzi commented Oct 14, 2015

ggetz commented Oct 14, 2015

Choose a reason for hiding this comment

pjcozzi commented Oct 14, 2015

pjcozzi commented Oct 14, 2015

ggetz commented Oct 14, 2015