From 5cb51e57788dc710eced383e67c8a22272d2d920 Mon Sep 17 00:00:00 2001 From: ggetz Date: Wed, 3 Jan 2018 12:26:54 -0500 Subject: [PATCH 1/4] Clipping planes support cleanup --- Source/Core/ClippingPlaneCollection.js | 11 +++++++++++ Source/DataSources/ModelGraphics.js | 2 +- Source/Scene/GlobeSurfaceTileProvider.js | 6 +++--- Source/Scene/Model.js | 13 ++++++++++++- Source/Scene/PointCloud3DTileContent.js | 4 +++- 5 files changed, 30 insertions(+), 6 deletions(-) diff --git a/Source/Core/ClippingPlaneCollection.js b/Source/Core/ClippingPlaneCollection.js index e2d9e44f5837..17df18873fcd 100644 --- a/Source/Core/ClippingPlaneCollection.js +++ b/Source/Core/ClippingPlaneCollection.js @@ -7,6 +7,7 @@ define([ './defined', './defineProperties', './DeveloperError', + './FeatureDetection', './Intersect', './Matrix4', './Plane' @@ -19,6 +20,7 @@ define([ defined, defineProperties, DeveloperError, + FeatureDetection, Intersect, Matrix4, Plane) { @@ -362,6 +364,15 @@ define([ return intersection; }; + /** + * Determines if rendering with clipping planes is supported. + * + * @returns {Boolean} true if ClippingPlaneCollections are supported; otherwise, returns false + */ + ClippingPlaneCollection.isSupported = function() { + return !FeatureDetection.isInternetExplorer(); + }; + /** * The maximum number of supported clipping planes. * diff --git a/Source/DataSources/ModelGraphics.js b/Source/DataSources/ModelGraphics.js index 5d583551cff3..4c4676a5cea8 100644 --- a/Source/DataSources/ModelGraphics.js +++ b/Source/DataSources/ModelGraphics.js @@ -55,7 +55,7 @@ define([ * @param {Property} [options.color=Color.WHITE] A Property specifying the {@link Color} that blends with the model's rendered color. * @param {Property} [options.colorBlendMode=ColorBlendMode.HIGHLIGHT] An enum Property specifying how the color blends with the model. * @param {Property} [options.colorBlendAmount=0.5] A numeric Property specifying the color strength when the colorBlendMode is MIX. A value of 0.0 results in the model's rendered color while a value of 1.0 results in a solid color, with any value in-between resulting in a mix of the two. - * @param {Property} [options.clippingPlanes] A property specifying the {@link ClippingPlaneCollection} used to selectively disable rendering the model. + * @param {Property} [options.clippingPlanes] A property specifying the {@link ClippingPlaneCollection} used to selectively disable rendering the model. Clipping planes are not currently supported in Internet Explorer. * * @see {@link http://cesiumjs.org/2014/03/03/Cesium-3D-Models-Tutorial/|3D Models Tutorial} * @demo {@link http://cesiumjs.org/Cesium/Apps/Sandcastle/index.html?src=3D%20Models.html|Cesium Sandcastle 3D Models Demo} diff --git a/Source/Scene/GlobeSurfaceTileProvider.js b/Source/Scene/GlobeSurfaceTileProvider.js index 918ecd1ee83a..f1a092cc86ab 100644 --- a/Source/Scene/GlobeSurfaceTileProvider.js +++ b/Source/Scene/GlobeSurfaceTileProvider.js @@ -4,6 +4,7 @@ define([ '../Core/Cartesian2', '../Core/Cartesian3', '../Core/Cartesian4', + '../Core/ClippingPlaneCollection', '../Core/Color', '../Core/ColorGeometryInstanceAttribute', '../Core/combine', @@ -13,7 +14,6 @@ define([ '../Core/destroyObject', '../Core/DeveloperError', '../Core/Event', - '../Core/FeatureDetection', '../Core/GeometryInstance', '../Core/GeometryPipeline', '../Core/IndexDatatype', @@ -51,6 +51,7 @@ define([ Cartesian2, Cartesian3, Cartesian4, + ClippingPlaneCollection, Color, ColorGeometryInstanceAttribute, combine, @@ -60,7 +61,6 @@ define([ destroyObject, DeveloperError, Event, - FeatureDetection, GeometryInstance, GeometryPipeline, IndexDatatype, @@ -1322,7 +1322,7 @@ define([ uniformMapProperties.clippingPlanesEdgeWidth = clippingPlanes.edgeWidth; } - var clippingPlanesEnabled = defined(clippingPlanes) && clippingPlanes.enabled && (uniformMapProperties.clippingPlanes.length > 0) && !FeatureDetection.isInternetExplorer(); + var clippingPlanesEnabled = defined(clippingPlanes) && clippingPlanes.enabled && (uniformMapProperties.clippingPlanes.length > 0) && ClippingPlaneCollection.isSupported(); var unionClippingRegions = clippingPlanesEnabled ? clippingPlanes.unionClippingRegions : false; if (defined(tileProvider.uniformMap)) { diff --git a/Source/Scene/Model.js b/Source/Scene/Model.js index 59678a227458..49ddb8acf0d5 100644 --- a/Source/Scene/Model.js +++ b/Source/Scene/Model.js @@ -4,6 +4,7 @@ define([ '../Core/Cartesian3', '../Core/Cartesian4', '../Core/Cartographic', + '../Core/ClippingPlaneCollection', '../Core/clone', '../Core/Color', '../Core/combine', @@ -81,6 +82,7 @@ define([ Cartesian3, Cartesian4, Cartographic, + ClippingPlaneCollection, clone, Color, combine, @@ -1135,6 +1137,15 @@ define([ * @param {ShadowMode} [options.shadows=ShadowMode.ENABLED] Determines whether the model casts or receives shadows from each light source. * @param {Boolean} [options.debugShowBoundingVolume=false] For debugging only. Draws the bounding sphere for each {@link DrawCommand} in the model. * @param {Boolean} [options.debugWireframe=false] For debugging only. Draws the model in wireframe. + * @param {HeightReference} [options.heightReference] Determines how the model is drawn relative to terrain. + * @param {Scene} [options.scene] Must be passed in for models that use the height reference property. + * @param {DistanceDisplayCondition} [options.distanceDisplayCondition] The condition specifying at what distance from the camera that this model will be displayed. + * @param {Color} [options.color=Color.WHITE] A color that blends with the model's rendered color. + * @param {ColorBlendMode} [options.colorBlendMode=ColorBlendMode.HIGHLIGHT] Defines how the color blends with the model. + * @param {Number} [options.colorBlendAmount=0.5] Value used to determine the color strength when the colorBlendMode is MIX. A value of 0.0 results in the model's rendered color while a value of 1.0 results in a solid color, with any value in-between resulting in a mix of the two. + * @param {Color} [options.silhouetteColor=Color.RED] The silhouette color. If more than 256 models have silhouettes enabled, there is a small chance that overlapping models will have minor artifacts. + * @param {Number} [options.silhouetteSize=0.0] The size of the silhouette in pixels. + * @param {ClippingPlaneCollection} [options.clippingPlanes] The {@link ClippingPlaneCollection} used to selectively disable rendering the model. Clipping planes are not currently supported in Internet Explorer. * * @returns {Model} The newly created model. * @@ -2116,7 +2127,7 @@ define([ var premultipliedAlpha = hasPremultipliedAlpha(model); var finalFS = modifyShaderForColor(fs, premultipliedAlpha); - if (!FeatureDetection.isInternetExplorer()) { + if (ClippingPlaneCollection.isSupported()) { finalFS = modifyShaderForClippingPlanes(finalFS); } diff --git a/Source/Scene/PointCloud3DTileContent.js b/Source/Scene/PointCloud3DTileContent.js index 0fbfc826578f..ad6d18208e6a 100644 --- a/Source/Scene/PointCloud3DTileContent.js +++ b/Source/Scene/PointCloud3DTileContent.js @@ -2,6 +2,7 @@ define([ '../Core/Cartesian2', '../Core/Cartesian3', '../Core/Cartesian4', + '../Core/ClippingPlaneCollection', '../Core/Color', '../Core/combine', '../Core/ComponentDatatype', @@ -38,6 +39,7 @@ define([ Cartesian2, Cartesian3, Cartesian4, + ClippingPlaneCollection, Color, combine, ComponentDatatype, @@ -844,7 +846,7 @@ define([ var hasColorStyle = defined(colorStyleFunction); var hasShowStyle = defined(showStyleFunction); var hasPointSizeStyle = defined(pointSizeStyleFunction); - var hasClippedContent = defined(clippingPlanes) && clippingPlanes.enabled && content._tile._isClipped && !FeatureDetection.isInternetExplorer(); + var hasClippedContent = defined(clippingPlanes) && clippingPlanes.enabled && content._tile._isClipped && ClippingPlaneCollection.isSupported(); // Get the properties in use by the style var styleableProperties = []; From c8948f33f752ee9ac587c30d0eeadd09537159fa Mon Sep 17 00:00:00 2001 From: ggetz Date: Wed, 3 Jan 2018 13:54:10 -0500 Subject: [PATCH 2/4] Updated CHANGES.md --- CHANGES.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGES.md b/CHANGES.md index 5f977351338f..67e40e0b2423 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -1,6 +1,10 @@ Change Log ========== +### 1.42 - 2018-02-01 + +* Added `ClippingPlaneCollection.isSupported` function for checking if rendering with clipping planes is supported. + ### 1.41 - 2018-01-02 * Breaking changes From 221ef8c2d4d351d5102a72db5fa8b351200cdc15 Mon Sep 17 00:00:00 2001 From: ggetz Date: Wed, 3 Jan 2018 17:54:35 -0500 Subject: [PATCH 3/4] Update Bing Key for feb release --- Source/Core/BingMapsApi.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Source/Core/BingMapsApi.js b/Source/Core/BingMapsApi.js index bc93c0c79cf0..0f55ac502556 100644 --- a/Source/Core/BingMapsApi.js +++ b/Source/Core/BingMapsApi.js @@ -40,7 +40,7 @@ define([ console.log(errorString); printedBingWarning = true; } - return 'Aig5SkZ4pNMN8b4rX-RUH2c_95mK-wjb4WL9k50K51faErEGnNsxgpWHXiqS3Rhe'; + return 'Ar9n20kTp-N8tEg3Dpx-Pgocmx3W0-GUnD_Bgt3h8g6pSeDL8yxByTVGHyMyjI2p'; } return BingMapsApi.defaultKey; From b9cc59dbceb1fbce67147a7774968338fb61b754 Mon Sep 17 00:00:00 2001 From: Gabby Getz Date: Thu, 4 Jan 2018 15:35:38 -0500 Subject: [PATCH 4/4] Update PR guide based on team feeback --- .../Contributors/CodeReviewGuide/README.md | 26 ++++++++++--------- 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/Documentation/Contributors/CodeReviewGuide/README.md b/Documentation/Contributors/CodeReviewGuide/README.md index d56d5d6c44a9..45ed410458b6 100644 --- a/Documentation/Contributors/CodeReviewGuide/README.md +++ b/Documentation/Contributors/CodeReviewGuide/README.md @@ -1,22 +1,23 @@ +All code in Cesium is publicly peer reviewed. We review code to share knowledge, foster shared ownership, and improve code quality and consistency. - -All code in Cesium is publicly peer reviewed. We review code to share knowledge, foster shared ownership, and improve code quality and consistency. Just knowing that code will be reviewed improves its quality. - -This guide describes best practices for code reviews. +This guide describes best practices for code reviewers. * [General](#general) * [Reviewing](#reviewing) * [Changes to the Public Cesium API](#changes-to-the-public-cesium-api) * [Testing](#testing) * [Merging](#merging) +* [Useful Git Commit Management](#useful-git-commit-management) * [Resources](#resources) ## General -* GitHub has great tools for code reviews in pull requests. Become familiar with [them](https://help.github.com/articles/using-pull-requests/#reviewing-proposed-changes). -* If we don't have a CLA for the contributor who opened the pull request (or, more precisely, any contributor to the branch), politely ask for one before reviewing the pull request ([example](https://github.com/AnalyticalGraphicsInc/cesium/pull/2918#issuecomment-127805425)). -* Most pull requests require additional work, often minor but sometimes major, before being merged. It's not a big deal. Sometimes we open a pull request with a [task list](https://github.com/blog/1375%0A-task-lists-in-gfm-issues-pulls-comments) for early feedback. -* Anyone is encouraged to review any pull request that interests them. However, someone familiar with the changed code should ultimately merge it. +* It is ultimately the responsibility of the pull request opener to get their changes merged. They should champion their code being merged and should bump the PR or @ mention a specific developer if it is not getting the neccisary attention. +* GitHub has great [tools for code reviews in pull requests](https://help.github.com/articles/using-pull-requests/#reviewing-proposed-changes) that you should become familiar with. +* We need a CLA for any contribution. If we don't have a CLA for the contributor who opened the pull request (or, more precisely, any contributor to the branch), the Cesium Concierge will ask for one. If you receive no updates, politely ask for one before reviewing the pull request ([example](https://github.com/AnalyticalGraphicsInc/cesium/pull/2918#issuecomment-127805425)). +* Most pull requests require additional work, minor or major, before being merged. Sometime pull requests are submitted incomplete for early feedback. Include a [task list](https://github.com/blog/1375%0A-task-lists-in-gfm-issues-pulls-comments) covering the steps that must be completed before merging. +* Anyone is encouraged to review any pull request that interests them. However, someone familiar with the changed code should ultimately merge it. +* It's OK to provide a few comments without taking responsibility for the final merge, for example commenting on the state of the public API or a Sandcastle example. However, be explicit that you will not be reviewing again. ## Reviewing @@ -25,9 +26,9 @@ This guide describes best practices for code reviews. * Provide motivation when it isn't obvious. Suggest why a change should be made. * Point contributors to a relevant part of the [Coding Guide](../CodingGuide/README.md) when useful. * _Be concise_. Make every word tell. -* _Be responsive_. The contributor should expect prompt feedback from reviewers, and reviewers should expect the same. If not, politely ask for it. We all want pull requests to get into master. +* _Be responsive_. The contributor should expect prompt feedback from reviewers, and reviewers should expect the same. If not, politely ask for it. We all want pull requests to get into master. Like an email, strive to respond to mentions and requests within 24 hours. * _Limit the scope_. As a reviewer, it is easy to want to increase the scope, e.g., "why don't we do this everywhere?". These are often fair questions but can be better served by submitting a separate issue to allow more incremental pull requests. -* Bring others into the conversation sparingly. If someone has expertise with a particular language feature or problem domain under review, invite them to comment. +* Bring others into the conversation sparingly. If someone has expertise with a particular language feature or problem domain under review, invite them to comment with an @mention. * If an experienced contributor makes a occasional whitespace or trivial mistake, just fix it to save on noise and speedup the review. ## Changes to the Public Cesium API @@ -47,9 +48,10 @@ This guide describes best practices for code reviews. ## Merging -* Cesium uses Travis CI for continuous integration. Travis automatically builds Cesium, runs ESLint, and generates the documentation for each branch pushed to GitHub. Before merging a pull request, verify that all Travis checks pass, indicated by the green check-mark and green `Merge pull request` button: +* Hitting the "Merge pull request" button ideally means you know enough about this code that you could personally support it in the future. +* Cesium uses Travis CI for continuous integration. Travis automatically builds Cesium, runs ESLint, and generates the documentation for each branch pushed to GitHub. Before merging a pull request, verify that all Travis checks pass, indicated by the green check-mark and green "Merge pull request" button: -![](Travis.jpg) +![Travis CI checks](Travis.jpg) * Delete the branch after merging the pull request. * Verify that the corresponding issue (if any) was closed.