Skip to content

Commit

Permalink
Merge branch 'czml-custom-properties' of github.com:klim705/cesium in…
Browse files Browse the repository at this point in the history
…to czml-custom-properties
  • Loading branch information
hpinkos committed Jan 5, 2018
2 parents 05b425b + a33f52f commit 68f9870
Show file tree
Hide file tree
Showing 8 changed files with 46 additions and 19 deletions.
1 change: 1 addition & 0 deletions CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ Change Log

* Breaking changes
*
* Added `ClippingPlaneCollection.isSupported` function for checking if rendering with clipping planes is supported.
* Improved CZML Custom Properties sandcastle example [#6086](https://github.com/AnalyticalGraphicsInc/cesium/pull/6086)

### 1.41 - 2018-01-02
Expand Down
26 changes: 14 additions & 12 deletions Documentation/Contributors/CodeReviewGuide/README.md
Original file line number Diff line number Diff line change
@@ -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

Expand All @@ -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
Expand All @@ -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.
Expand Down
2 changes: 1 addition & 1 deletion Source/Core/BingMapsApi.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ define([
console.log(errorString);
printedBingWarning = true;
}
return 'Aig5SkZ4pNMN8b4rX-RUH2c_95mK-wjb4WL9k50K51faErEGnNsxgpWHXiqS3Rhe';
return 'Ar9n20kTp-N8tEg3Dpx-Pgocmx3W0-GUnD_Bgt3h8g6pSeDL8yxByTVGHyMyjI2p';
}

return BingMapsApi.defaultKey;
Expand Down
11 changes: 11 additions & 0 deletions Source/Core/ClippingPlaneCollection.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ define([
'./defined',
'./defineProperties',
'./DeveloperError',
'./FeatureDetection',
'./Intersect',
'./Matrix4',
'./Plane'
Expand All @@ -19,6 +20,7 @@ define([
defined,
defineProperties,
DeveloperError,
FeatureDetection,
Intersect,
Matrix4,
Plane) {
Expand Down Expand Up @@ -362,6 +364,15 @@ define([
return intersection;
};

/**
* Determines if rendering with clipping planes is supported.
*
* @returns {Boolean} <code>true</code> if ClippingPlaneCollections are supported; otherwise, returns <code>false</code>
*/
ClippingPlaneCollection.isSupported = function() {
return !FeatureDetection.isInternetExplorer();
};

/**
* The maximum number of supported clipping planes.
*
Expand Down
2 changes: 1 addition & 1 deletion Source/DataSources/ModelGraphics.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 <code>colorBlendMode</code> is <code>MIX</code>. 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}
Expand Down
6 changes: 3 additions & 3 deletions Source/Scene/GlobeSurfaceTileProvider.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ define([
'../Core/Cartesian2',
'../Core/Cartesian3',
'../Core/Cartesian4',
'../Core/ClippingPlaneCollection',
'../Core/Color',
'../Core/ColorGeometryInstanceAttribute',
'../Core/combine',
Expand All @@ -13,7 +14,6 @@ define([
'../Core/destroyObject',
'../Core/DeveloperError',
'../Core/Event',
'../Core/FeatureDetection',
'../Core/GeometryInstance',
'../Core/GeometryPipeline',
'../Core/IndexDatatype',
Expand Down Expand Up @@ -51,6 +51,7 @@ define([
Cartesian2,
Cartesian3,
Cartesian4,
ClippingPlaneCollection,
Color,
ColorGeometryInstanceAttribute,
combine,
Expand All @@ -60,7 +61,6 @@ define([
destroyObject,
DeveloperError,
Event,
FeatureDetection,
GeometryInstance,
GeometryPipeline,
IndexDatatype,
Expand Down Expand Up @@ -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)) {
Expand Down
13 changes: 12 additions & 1 deletion Source/Scene/Model.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ define([
'../Core/Cartesian3',
'../Core/Cartesian4',
'../Core/Cartographic',
'../Core/ClippingPlaneCollection',
'../Core/clone',
'../Core/Color',
'../Core/combine',
Expand Down Expand Up @@ -81,6 +82,7 @@ define([
Cartesian3,
Cartesian4,
Cartographic,
ClippingPlaneCollection,
clone,
Color,
combine,
Expand Down Expand Up @@ -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 <code>colorBlendMode</code> is <code>MIX</code>. 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.
*
Expand Down Expand Up @@ -2116,7 +2127,7 @@ define([

var premultipliedAlpha = hasPremultipliedAlpha(model);
var finalFS = modifyShaderForColor(fs, premultipliedAlpha);
if (!FeatureDetection.isInternetExplorer()) {
if (ClippingPlaneCollection.isSupported()) {
finalFS = modifyShaderForClippingPlanes(finalFS);
}

Expand Down
4 changes: 3 additions & 1 deletion Source/Scene/PointCloud3DTileContent.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ define([
'../Core/Cartesian2',
'../Core/Cartesian3',
'../Core/Cartesian4',
'../Core/ClippingPlaneCollection',
'../Core/Color',
'../Core/combine',
'../Core/ComponentDatatype',
Expand Down Expand Up @@ -38,6 +39,7 @@ define([
Cartesian2,
Cartesian3,
Cartesian4,
ClippingPlaneCollection,
Color,
combine,
ComponentDatatype,
Expand Down Expand Up @@ -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 = [];
Expand Down

0 comments on commit 68f9870

Please sign in to comment.