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

Options to turn off normal shading for point clouds #5152

Closed
lilleyse opened this issue Mar 27, 2017 · 8 comments
Closed

Options to turn off normal shading for point clouds #5152

lilleyse opened this issue Mar 27, 2017 · 8 comments

Comments

@lilleyse
Copy link
Contributor

lilleyse commented Mar 27, 2017

@Dylan-Brown, @glee2244 this will be a good thing to work on after #4759

For point clouds with normals (like Specs\Data\Cesium3DTiles\PointCloud\PointCloudNormals), shading is automatically applied in the shader here: https:/AnalyticalGraphicsInc/cesium/blob/3d-tiles/Source/Scene/PointCloud3DTileContent.js#L1005-L1010

Instead of shading always being applied, it should be controlled with a boolean, since some may want to apply their own shading with the styling language, or just not at all.

Shading on:
shading

Shading off:
unshaded

Look at how backFaceCulling is handled, a normalShading flag will act very similarly.

This was requested on the forum a little while back: https://groups.google.com/forum/#!msg/cesium-dev/GgfuEFgHw-g/0T3Ypsz7BAAJ

Afterwards we can look at how backFaceCulling and normalShading can be easily toggled from a tileset.

@glee2244
Copy link
Contributor

@lilleyse Hi Sean, just a couple clarifying questions:

  1. Looking at how backFaceCulling is handled - is there a significance in having backFaceCulling and _backFaceCulling (https:/AnalyticalGraphicsInc/cesium/blob/3d-tiles/Source/Scene/PointCloud3DTileContent.js#L119) ? If there is, is this something that needs to be also done with normalShading?

  2. Is all of the code below the line you specified responsible for adding shading (https:/AnalyticalGraphicsInc/cesium/blob/3d-tiles/Source/Scene/PointCloud3DTileContent.js#L1087)? I ask because I'm still slightly confused as to what lines of code will need to be encapsulated in a if (normalShading) block.

  3. How can I test my implementation of the normalShading flag?

@lilleyse
Copy link
Contributor Author

lilleyse commented Jun 6, 2017

Sorry @glee2244, I missed this comment. @AnimatedRNG will finish this up now.

  1. Having both allows us to check if the value has changed since the last frame. It is basically the same as having a dirty flag, but slightly less code. normalShading will act similarly.
  2. I updated the link in the top post. Everything in that hasNormals block applies the shading. The new code should be just hasNormals && normalShading.
  3. To visually test, switch to the Point Cloud Normals tileset in the 3D Tiles sandcastle demo. Add a Sandcastle button that toggles tileset._root.content.normalShading. For testing in the Spec file you can render once with normalShading on, once with normalShading off, and compare the pixel colors.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 14, 2017

Fixed in #5433.

@pjcozzi pjcozzi closed this as completed Jun 14, 2017
@Vineg
Copy link
Contributor

Vineg commented Nov 30, 2018

@pjcozzi it seems to be still actual since #5433 has been closed

@pjcozzi
Copy link
Contributor

pjcozzi commented Nov 30, 2018

@lilleyse please re-open if appropriate.

@lilleyse lilleyse reopened this Nov 30, 2018
@lilleyse
Copy link
Contributor Author

The option exists at the PointCloud.js level, which is private, but not the tileset level. So reopened this.

Vineg pushed a commit to geoscan/cesium that referenced this issue Dec 7, 2018
Vineg pushed a commit to geoscan/cesium that referenced this issue Dec 13, 2018
Vineg pushed a commit to geoscan/cesium that referenced this issue Dec 13, 2018
Vineg pushed a commit to geoscan/cesium that referenced this issue Dec 13, 2018
Vineg pushed a commit to geoscan/cesium that referenced this issue Dec 28, 2018
Vineg pushed a commit to geoscan/cesium that referenced this issue Dec 28, 2018
@lilleyse
Copy link
Contributor Author

The options were added to PointCloudShading which is accessible from the tileset level in #7399.

@cesium-concierge
Copy link

Congratulations on closing the issue! I found these Cesium forum links in the comments above:

https://groups.google.com/forum/#!msg/cesium-dev/GgfuEFgHw-g/0T3Ypsz7BAAJ

If this issue affects any of these threads, please post a comment like the following:

The issue at #5152 has just been closed and may resolve your issue. Look for the change in the next stable release of Cesium or get it now in the master branch on GitHub https:/AnalyticalGraphicsInc/cesium.

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

No branches or pull requests

5 participants