Skip to content

Fix applying translucent style to point cloud #6113

New issue

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

#

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

Already on GitHub? # to your account

Jump to bottom
Merged
merged 4 commits into from
Jan 17, 2018
Merged

Fix applying translucent style to point cloud #6113

merged 4 commits into from
Jan 17, 2018

Conversation

lilleyse
Copy link
Contributor

When setting a point cloud style's color to a translucent color only a single tile would become translucent. The fix is to mark whether the cached shader is translucent.

@cesium-concierge
Copy link

Signed CLA is on file.

@lilleyse, thanks for the pull request! Maintainers, we have a signed CLA from @lilleyse, 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.

🌍 🌎 🌏

@lilleyse lilleyse force-pushed the translucent-shader-update branch from bae2977 to e53672d Compare January 11, 2018 22:38
ggetz
ggetz requested changes Jan 15, 2018
View reviewed changes
Copy link
Contributor

@ggetz ggetz left a comment

Choose a reason for hiding this comment

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

Other than that, code changes look good, does this warrant a spec?

Source/Scene/Cesium3DTileStyle.js Outdated
@@ -1454,11 +1456,13 @@ define([
Cesium3DTileStyle.prototype.getColorShaderFunction = function(functionName, attributePrefix, shaderState) {
if (this._colorShaderFunctionReady) {
// Return the cached result, may be undefined
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment applies to next line, not this new line

@lilleyse
Copy link
Contributor Author

does this warrant a spec?

Yeah, I'll need to create new sample tileset since all our test point clouds are a single tile. I'll bump when ready.

@lilleyse
Copy link
Contributor Author

Updated.

@lilleyse lilleyse mentioned this pull request Jan 17, 2018
Merged
@ggetz
Copy link
Contributor

ggetz commented Jan 17, 2018

@lilleyse It looks like that new test is failing.

@lilleyse lilleyse force-pushed the translucent-shader-update branch from 94d7113 to 38cfc31 Compare January 17, 2018 15:18
@lilleyse
Copy link
Contributor Author

Updated - it seems like it was a capitalization issue. Hopefully the test passes on Travis now.

@ggetz ggetz merged commit 3f4a499 into master Jan 17, 2018
@ggetz ggetz deleted the translucent-shader-update branch January 17, 2018 17:01
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@ggetz ggetz

Assignees

@ggetz ggetz

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lilleyse @cesium-concierge @ggetz