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

Option to turn off normal shading #7399

Merged
merged 12 commits into from
Mar 14, 2019

Conversation

Vineg
Copy link
Contributor

@Vineg Vineg commented Dec 10, 2018

Small fixes.
#5152

@cesium-concierge
Copy link

Thanks for the pull request @Vineg!

  • ✔️ Signed CLA found.
  • CHANGES.md was not updated.
    • If this change updates the public API in any way, please add a bullet point to CHANGES.md.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.
  • Works (or fails gracefully) in IE11.

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

🌍 🌎 🌏

@mramato
Copy link
Contributor

mramato commented Dec 10, 2018

Thanks for the PR @Vineg! @lilleyse can you take a look a this?

@mramato mramato requested a review from lilleyse December 10, 2018 13:59
@lilleyse
Copy link
Contributor

Thanks @Vineg. Would you mind opening a separate PR for the point cloud changes so that we can focus on the geometric error fix exclusively?

@Vineg
Copy link
Contributor Author

Vineg commented Dec 11, 2018

Ok, tomorrow

@Vineg Vineg force-pushed the point-cloud-scale-fixes branch from ca38e70 to a563bda Compare December 13, 2018 16:36
@Vineg Vineg changed the title Scale 3dTiles geometric error with model Option to turn off normal shading Dec 13, 2018
@hpinkos
Copy link
Contributor

hpinkos commented Dec 27, 2018

@Vineg is this ready for review now? Next time, make a comment when it's ready so we get notified. We don't get notified when you push new commits. Thanks!

@Vineg
Copy link
Contributor Author

Vineg commented Dec 28, 2018

@lilleyse @hpinkos this one is ready

@cesium-concierge
Copy link

Thanks again for your contribution @Vineg!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

@hpinkos
Copy link
Contributor

hpinkos commented Jan 28, 2019

@lilleyse can you review this?

Copy link
Contributor

@lilleyse lilleyse left a comment

Choose a reason for hiding this comment

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

Thanks @Vineg, the approach looks good. Just some minor comments.

Source/Scene/PointCloud.js Outdated Show resolved Hide resolved
Source/Scene/PointCloud.js Outdated Show resolved Hide resolved
Source/Scene/PointCloudShading.js Outdated Show resolved Hide resolved
Source/Scene/PointCloudShading.js Outdated Show resolved Hide resolved
Source/Scene/PointCloudShading.js Outdated Show resolved Hide resolved
Source/Scene/PointCloudShading.js Outdated Show resolved Hide resolved
@hpinkos
Copy link
Contributor

hpinkos commented Feb 21, 2019

@lilleyse I think this is ready.

@Vineg @geoscan-builder next time you push commits after a review, post a comment when the PR is ready for another look. We don't get notifications when commits are pushed up so we missed that you were working on this. Thanks!

@lilleyse
Copy link
Contributor

Thanks for the updates @Vineg. I just tweaked a few things and updated documentation and CHANGES.md.

@lilleyse lilleyse merged commit 5cbb5e5 into CesiumGS:master Mar 14, 2019
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.

6 participants