Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disable clip plane shader code in IE, update docs #6079

Merged
merged 1 commit into from
Jan 2, 2018

Conversation

ggetz
Copy link
Contributor

@ggetz ggetz commented Jan 2, 2018

CC #6077

Disable clip plane shader code from generating in IE, updated docs and Sandcastle examples to reflect this.

Should I add tests to check that the code is not generated in IE, or is that not necessary?

@ggetz ggetz requested a review from lilleyse January 2, 2018 15:59
@cesium-concierge
Copy link

Signed CLA is on file.

@ggetz, thanks for the pull request! Maintainers, we have a signed CLA from @ggetz, so you can review this at any time.

⚠️ I noticed that CHANGES.md has not been updated. If this change updates the public API in any way, fixes a bug, or makes any non-trivial update, please add a bullet point to CHANGES.md and comment on this pull request so we know it was updated. For more info, see the Pull Request Guidelines.


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

🌍 🌎 🌏

@pjcozzi
Copy link
Contributor

pjcozzi commented Jan 2, 2018

Looks good and models work again in IE, thanks!

@pjcozzi pjcozzi merged commit 999f0ba into master Jan 2, 2018
@pjcozzi pjcozzi deleted the clip-planes-disable-ie branch January 2, 2018 16:23
@@ -844,7 +844,7 @@ define([
var hasColorStyle = defined(colorStyleFunction);
var hasShowStyle = defined(showStyleFunction);
var hasPointSizeStyle = defined(pointSizeStyleFunction);
var hasClippedContent = defined(clippingPlanes) && clippingPlanes.enabled && content._tile._isClipped;
var hasClippedContent = defined(clippingPlanes) && clippingPlanes.enabled && content._tile._isClipped && !FeatureDetection.isInternetExplorer();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this check could be centralized somewhere? ClippingPlaneCollection.isSupported?

For reference: https:/AnalyticalGraphicsInc/cesium/blob/master/Source/Scene/ClassificationPrimitive.js#L340-L348

@@ -340,7 +340,7 @@ define([
* @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.
* @param {ClippingPlaneCollection} [options.clippingPlanes] The {@link ClippingPlaneCollection} used to selectively disable rendering the model. Clipping planes are not currently supported in Internet Explorer.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sort of a side note, but can you list this option in Model.fromGltf as well?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add warning to ModelGraphics.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jan 2, 2018

@lilleyse do you want me to hold up the release for your comments?

@lilleyse
Copy link
Contributor

lilleyse commented Jan 2, 2018

No need, my comments aren't critical.

@ggetz
Copy link
Contributor Author

ggetz commented Jan 2, 2018

@lilleyse Thanks, I will address those in another PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants