Adds normalShading option to the repo #5433
Conversation
| var vertexArray = content._drawCommand.vertexArray; | ||
|
|
||
| var normalsEnabled = hasNormals && normalShading; |
There was a problem hiding this comment.
I actually think this boolean may not be the right approach. We still want backFaceCulling to work even if normalShading is false.
There was a problem hiding this comment.
Are you referring to the part around line 1022 (https:/AnimatedRNG/cesium/blob/eb7b76ece10dd4422a9377b94df8ac725c97b730/Source/Scene/PointCloud3DTileContent.js#L1022)?
There was a problem hiding this comment.
Updated with a new fix for this case
There was a problem hiding this comment.
backFaceCulling will have issues because all the normal attribute setup is still behind the normalsEnabled boolean. Ultimately normalsEnabled should probably be removed.
There was a problem hiding this comment.
So that entire function should just ignore normalShading?
There was a problem hiding this comment.
Yeah, except for the part that actually does normal shading.
Another approach is to continue to use normalsEnabled but set it equal to normalShading || backFaceCulling.
There was a problem hiding this comment.
I made those edits, although I'm don't totally understand the relationship between backface culling and normal shading in that function
There was a problem hiding this comment.
backFaceCulling and normalShading are independent of each other but both require normals. So normalsEnabled should be true if hasNormals is true and at least one of the options is true. When appending the back face culling code it will need to check normalsEnabled && backFaceCulling and when appending the normal shading code it will need to check normalsEnabled && normalShading.
The original idea I had was to keep most of the code as is and just wrap the normal shading code with hasNormals && normalShading.
|
Also be sure to write a test for this. It can be something like rendering with See |
|
Other than those comments, looks great! I tested that it works in Sandcastle. |
|
Updated with a new test |
|
What's the plan here? Do we still plan to do this in light of #5455? |
| var vertexArray = content._drawCommand.vertexArray; | ||
|
|
||
| var normalsEnabled = (hasNormals && normalShading) || backFaceCulling; |
There was a problem hiding this comment.
This should be:
var normalsEnabled = hasNormals && (normalShading || backFaceCulling);
| @@ -1002,7 +1009,7 @@ define([ | |||
|
|
|||
| vs += ' color = color * u_highlightColor; \n'; | |||
|
|
|||
| if (hasNormals) { | |||
| if (normalsEnabled) { | |||
There was a problem hiding this comment.
Should be normalsEnabled && normalShading
| @@ -1012,7 +1019,7 @@ define([ | |||
| vs += ' v_color = color; \n' + | |||
| ' gl_Position = czm_modelViewProjection * vec4(position, 1.0); \n'; | |||
|
|
|||
| if (hasNormals && backFaceCulling) { | |||
| if (normalsEnabled) { | |||
There was a problem hiding this comment.
Should be normalsEnabled && backFaceCulling.
|
I made some final adjustments here. @ggetz would you like to review? |
|
@lilleyse Looks good to me |
|
Quick out of touch question: since |
|
Right now it's not used, unless someone knows what they're doing. It's like the I don't think we ever concluded if these should be properties of the tileset, or the style, or just in the styling language. |
|
So are we sure we want to merge this? Seems like no. |
|
@lilleyse Are we planning to merge this at some point or should we close this? |
|
Personally I don't think there's any harm in merging this since it doesn't preclude a future PR, and everything in here is I also don't care that strongly if it's closed either. |
|
Is this something that is going to be useful or is it not going to be used? If it doesn't add any particular value we should probably just close it. |
|
It's minor enough to close. |
Part of #5152