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.

Sign up for GitHub

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

Adds normalShading option to the repo #5433

Closed
wants to merge 6 commits into from
Closed

Adds normalShading option to the repo #5433

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
eb7b76e
Adds normalShading option to the repo
AnimatedRNG Jun 7, 2017
62c6e54
Adds test for changing normal shading settings
AnimatedRNG Jun 8, 2017
106436c
backFaceCulling no longer depends on normalShading
AnimatedRNG Jun 8, 2017
b6355d9
normalShading should hopefully not break backFaceCulling now
AnimatedRNG Jun 9, 2017
e6f6de1
Adjustments
lilleyse Oct 27, 2017
356b09c
Merge branch 'master' into 3d-tiles-normals-options
lilleyse Oct 27, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 18 additions & 6 deletions Source/Scene/PointCloud3DTileContent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,10 @@ define([
this.backFaceCulling = false;
this._backFaceCulling = false;

// Whether or not the user enabled normal shading
this.normalShading = true;
this._normalShading = true;

this._opaqueRenderState = undefined;
this._translucentRenderState = undefined;

Expand Down Expand Up @@ -781,8 +785,11 @@ define([
var hasNormals = content._hasNormals;
var hasBatchIds = content._hasBatchIds;
var backFaceCulling = content._backFaceCulling;
var normalShading = content._normalShading;
var vertexArray = content._drawCommand.vertexArray;

var normalsEnabled = hasNormals && normalShading;
Copy link
Contributor

Choose a reason for hiding this comment

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

I actually think this boolean may not be the right approach. We still want backFaceCulling to work even if normalShading is false.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you referring to the part around line 1022 (https://github.com/AnimatedRNG/cesium/blob/eb7b76ece10dd4422a9377b94df8ac725c97b730/Source/Scene/PointCloud3DTileContent.js#L1022)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated with a new fix for this case

Copy link
Contributor

Choose a reason for hiding this comment

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

backFaceCulling will have issues because all the normal attribute setup is still behind the normalsEnabled boolean. Ultimately normalsEnabled should probably be removed.

Copy link
Contributor Author

@AnimatedRNG AnimatedRNG Jun 8, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

So that entire function should just ignore normalShading?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made those edits, although I'm don't totally understand the relationship between backface culling and normal shading in that function

Copy link
Contributor

Choose a reason for hiding this comment

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

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.


var colorStyleFunction;
var showStyleFunction;
var pointSizeStyleFunction;
Expand Down Expand Up @@ -834,7 +841,7 @@ define([
var userProperties = styleableProperties.filter(function(property) { return defaultProperties.indexOf(property) === -1; });

//>>includeStart('debug', pragmas.debug);
if (usesNormalSemantic && !hasNormals) {
if (usesNormalSemantic && !normalsEnabled) {
throw new DeveloperError('Style references the NORMAL semantic but the point cloud does not have normals');
}
//>>includeEnd('debug');
Expand Down Expand Up @@ -863,7 +870,7 @@ define([
if (usesColors) {
attributeLocations.a_color = colorLocation;
}
if (hasNormals) {
if (normalsEnabled) {
attributeLocations.a_normal = normalLocation;
}
if (hasBatchIds) {
Expand Down Expand Up @@ -919,7 +926,7 @@ define([
vs += 'attribute vec3 a_color; \n';
}
}
if (hasNormals) {
if (normalsEnabled) {
if (isOctEncoded16P) {
vs += 'attribute vec2 a_normal; \n';
} else {
Expand Down Expand Up @@ -976,7 +983,7 @@ define([
}
vs += ' vec3 position_absolute = vec3(czm_model * vec4(position, 1.0)); \n';

if (hasNormals) {
if (normalsEnabled) {
if (isOctEncoded16P) {
vs += ' vec3 normal = czm_octDecode(a_normal); \n';
} else {
Expand All @@ -1002,7 +1009,7 @@ define([

vs += ' color = color * u_highlightColor; \n';

if (hasNormals) {
if (normalsEnabled) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be normalsEnabled && normalShading

vs += ' normal = czm_normal * normal; \n' +
' float diffuseStrength = czm_getLambertDiffuse(czm_sunDirectionEC, normal); \n' +
' diffuseStrength = max(diffuseStrength, 0.4); \n' + // Apply some ambient lighting
Expand All @@ -1012,7 +1019,7 @@ define([
vs += ' v_color = color; \n' +
' gl_Position = czm_modelViewProjection * vec4(position, 1.0); \n';

if (hasNormals && backFaceCulling) {
if (normalsEnabled && backFaceCulling) {
vs += ' float visible = step(-normal.z, 0.0); \n' +
' gl_Position *= visible; \n';
}
Expand Down Expand Up @@ -1215,6 +1222,11 @@ define([
createShaders(this, frameState, tileset.style);
}

if (this.normalShading !== this._normalShading) {
this._normalShading = this.normalShading;
createShaders(this, frameState, tileset.style);
}

// Update the render state
var isTranslucent = (this._highlightColor.alpha < 1.0) || (this._constantColor.alpha < 1.0) || this._styleTranslucent;
this._drawCommand.renderState = isTranslucent ? this._translucentRenderState : this._opaqueRenderState;
Expand Down