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

Geometric-error based point cloud attenuation and eye dome lighting #6069

Merged
merged 17 commits into from
Jan 30, 2018

Conversation

likangning93
Copy link
Contributor

This PR adds simple geometric-error-based attenuation and eye dome lighting for point clouds.

This trick only really works with replacement refinement point clouds right now, I'll follow up with some fixes for additive refinement.

@cesium-concierge
Copy link

Signed CLA is on file.

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

🌍 🌎 🌏

@likangning93
Copy link
Contributor Author

Awww who's a good bot? CHANGES.md updated.

@pjcozzi
Copy link
Contributor

pjcozzi commented Dec 21, 2017

Possible to have a shorter name? 😄

image

@pjcozzi
Copy link
Contributor

pjcozzi commented Dec 21, 2017

Can the Sandcastle example use better default parameters (and maybe camera view); currently, you can't really see the point cloud after load.

image

@pjcozzi
Copy link
Contributor

pjcozzi commented Dec 21, 2017

For defaults, perhaps

  • decrease SSE
  • Set attenuation and EDL to true

@pjcozzi
Copy link
Contributor

pjcozzi commented Dec 21, 2017

Maybe this is OK, but I found it strange that decreasing the max SSE - which should increase detail - forces the attenuation to become smaller.

aaa

@pjcozzi
Copy link
Contributor

pjcozzi commented Dec 21, 2017

Maybe this is OK, but I found it strange that decreasing the max SSE - which should increase detail - forces the attenuation to become smaller.

Maybe the default max attenuation needs to be higher.

@likangning93
Copy link
Contributor Author

Maybe this is OK, but I found it strange that decreasing the max SSE - which should increase detail - forces the attenuation to become smaller.

In this case maximumAttenuation is the SSE instead of a constant, which was an attempt to reduce the amount of parameter tuning required. It also looks like there isn't any more detail getting called down, either the view is already of a leaf or server issues popped up while you were running the demo.

Maybe we could remove the link between maximumAttenuation and SSE and just add a note to recommend using the SSE as maximumAttenuation when tuning?

Copy link
Contributor

@pjcozzi pjcozzi left a comment

Choose a reason for hiding this comment

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

Looks good, mostly trivial comments from me.


Cesium.knockout.getObservable(viewModel, 'geometricErrorScale').subscribe(
function(newValue) {
tileset.pointAttenuationOptions.geometricErrorScale = parseFloat(newValue);
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure about the "options" suffix for pointAttenuationOptions.

Perhaps pointShading?

Copy link
Contributor

Choose a reason for hiding this comment

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

This will also make eyeDomeLightingStrength and eyeDomeLightingRadius make more sense since it doesn't seem - at least from the user's perspective - that they should be a property on an attenuation object.

@@ -69,6 +71,8 @@ define([
Cesium3DTilesetTraversal,
Cesium3DTileStyleEngine,
LabelCollection,
PointAttenuationOptions,
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

Throughout.

* pointclouds using 3D Tiles.
*
* @param {Object} [options] Object with the following properties:
* {Boolean} [options.geometricErrorAttenuation=false] Perform point attenuation based on geometric error.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just call this attenuation? The fact that is uses geometric error for sizing is an implementation detail and does not need to cloud the API and user's code.

Copy link
Contributor Author

@likangning93 likangning93 Dec 29, 2017

Choose a reason for hiding this comment

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

I was hoping to distinguish this from Srinivas's post-process attenuation, since we might merge this in. It might also help to run both at the same time, with some tweaking.

I think it's also an important detail for the semi-technical user, since it works best based on the assumptions we're making for geometric error.

Copy link
Contributor

Choose a reason for hiding this comment

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

What is the one in the other branch called and what is the context?

In general, make the most common option the most obvious name. I suspect that this should be called attenuation and the other branch should be called specialAttenuationForWhateverReason.

* {Number} [options.maximumAttenuation] Maximum attenuation in pixels. Defaults to the Cesium3DTileset's maximumScreenSpaceError.
* {Number} [options.baseResolution] Average base resolution for the dataset in meters. Substitute for Geometric Error when not available.
* {Boolean} [options.eyeDomeLighting=false] When true, use eye dome lighting when drawing with point attenuation.
* {Number} [options.eyeDomeLightingStrength=1.0] Increasing this value increases contrast on slopes and edges.
Copy link
Contributor

Choose a reason for hiding this comment

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

Here and below, what is the typical range?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

eyeDomeLightingStrength looks best to me between 0.5 and 4.0, otherwise it starts getting too light or too dark..
eyeDomeLightingRadius is in pixels, so between 1.0 and 2.0 seems to be a good range.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, please document.

var pointAttenuationOptions = defaultValue(options, {});

/**
* Perform point attenuation based on geometric error.
Copy link
Contributor

Choose a reason for hiding this comment

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

Throughout, include @default.

vec2 depthAndLog2Depth = texture2D(u_pointCloud_ecAndLogDepthTexture, v_textureCoordinates + padding).zw;
if (depthAndLog2Depth.x == 0.0) // 0.0 is the clear value for the gbuffer
{
return vec2(0.0, 0.0); // throw out this sample
Copy link
Contributor

Choose a reason for hiding this comment

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

Just vec2(0.0) - it sets all components to zero.

vec4 color = texture2D(u_pointCloud_colorTexture, v_textureCoordinates);

// sample from neighbors up, down, left, right
float padx = (1.0 / czm_viewport.z) * u_edlStrengthAndDistance.y;
Copy link
Contributor

Choose a reason for hiding this comment

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

Not 100% sure, but I think it is faster to make u_edlStrengthAndDistance a vec4 and then precompute these in the w and z channels.

@@ -436,6 +438,102 @@ defineSuite([
});
});

function countRenderedPixels(rgba) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of having this here, maybe add a new expectation function, e.g.,

expect(scene).toRenderPixelCount(5)
expect(scene).toRenderPixelCountGreaterThan(5)

return count;
}

it('attenuates points based on geometric error', function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be one test? Or should each call to expect... be a separate test so it is easier to track down test failures?

scene.primitives.removeAll();
});

it('Adds a clear command and a post-processing draw call', function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Here and below it, use lowercase I think.

@pjcozzi
Copy link
Contributor

pjcozzi commented Dec 21, 2017

Maybe we could remove the link between maximumAttenuation and SSE and just add a note to recommend using the SSE as maximumAttenuation when tuning?

Whatever you and @lilleyse think.

@likangning93
Copy link
Contributor Author

@pjcozzi updated!

@pjcozzi
Copy link
Contributor

pjcozzi commented Dec 30, 2017

Thanks for the updates @likangning93!

@lilleyse please merge when ready.

No need for this to make the 1.41 release, but if it does, it should be merged on Jan 1 as I am starting the 1.41 release very early on Jan 2.

float shade = exp(-response * 300.0 * u_distancesAndEdlStrength.z);
color.rgb *= shade;
gl_FragColor = vec4(color);
gl_FragDepthEXT = czm_eyeToWindowCoordinates(vec4(ecAlphaDepth.xyz, 1.0)).z;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just realized we shouldn't need this fragment depth write, since this still happens during rasterization and this data doesn't actually change...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

since this still happens during rasterization and this data doesn't actually change

Or... not. I stand corrected. Looks like taking this out breaks depth picking. Whoops...

@lilleyse
Copy link
Contributor

lilleyse commented Jan 2, 2018

I haven't looked at the code yet, but it would be great to add the attenuation/EDL controls to its own panel in the 3D Tiles Inspector.

@lilleyse
Copy link
Contributor

lilleyse commented Jan 4, 2018

Also make sure to investigate the multi-frustum rendering bug.

@likangning93
Copy link
Contributor Author

Hmm, this doesn't currently work with shadows, which raises some concerns.

@lilleyse
Copy link
Contributor

lilleyse commented Jan 9, 2018

Ultimately it would be nice if we could move away from geometric error. One example is if we have really dense leaf tiles we might want to decrease their parent's geometric error so that they are not loaded as soon, but tweaking the geometric error like this has the side affect of making the parents render thinner. It's almost like we need a similar concept to geometric error but just for attenuation.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jan 9, 2018

It's almost like we need a similar concept to geometric error but just for attenuation.

Storing something like per-tile (or maybe maybe per-point) density is not out of the question. Adding to the 3D Tiles spec is OK.

One example is if we have really dense leaf tiles we might want to decrease their parent's geometric error so that they are not loaded as soon, but tweaking the geometric error like this has the side affect of making the parents render thinner

Thinkingly broadly here, maybe additional refinement parameters need to be considered so we aren't just fudging geometric error for cases like this.

@likangning93
Copy link
Contributor Author

likangning93 commented Jan 23, 2018

One example is if we have really dense leaf tiles we might want to decrease their parent's geometric error so that they are not loaded as soon, but tweaking the geometric error like this has the side affect of making the parents render thinner

This also sounds like a case where we would want a few more LODs between the current parent and leaves in the tileset itself.

The current controls also incorporate a geometric error multiplier and attenuation limit that might help a little bit. I'll add [EDIT] to the sandcastle example showing/documenting how this can "kind of work" in cases where just straight-up geometric error doesn't produce good results.

@likangning93
Copy link
Contributor Author

@lilleyse updated! Post-merge TODOs:

  • open issue for shadows
  • open issue for Firefox draw buffer problem

* Computes the radius of the BoundingSphere.
* @returns {Number} The radius of the BoundingSphere.
*/
BoundingSphere.prototype.volume = function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Add this to CHANGES.md.

*/
BoundingSphere.prototype.volume = function() {
var radius = this.radius;
return (4.0 / 3.0) * CesiumMath.PI * radius * radius * radius;
Copy link
Contributor

Choose a reason for hiding this comment

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

Assign (4.0 / 3.0) * CesiumMath.PI to a file-scoped constant so that it is not recomputed each time this function is called. I'm not 100% sure the JIT will optimize it.

* @type {Boolean}
* @default false
*/
this.geometricErrorAttenuation = defaultValue(pointShading.geometricErrorAttenuation, false);
Copy link
Contributor

Choose a reason for hiding this comment

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

I know I've said this online and offline, but I still think we are going with a naming mistake here not just calling this attenuation for the same reason that Primitive is just called "Primitive" and the special cases have more precise names like GroundPrimitive. Always use the most obvious name for the common case.

Please reconsider here.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jan 28, 2018

@lilleyse are you able to review this soon so this can potentially make the 1.42 release on Thursday?

@likangning93
Copy link
Contributor Author

@lilleyse updated!

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.

Awesome work @likangning93. The mechanics all look good - shaders, options, etc.

The point cloud shading object will also be helpful in exposing things like lighting and culling for point clouds with normals.

CHANGES.md Outdated
@@ -41,6 +41,8 @@ Change Log
* Fixed camera movement and look functions for 2D mode [#5884](https://github.com/AnalyticalGraphicsInc/cesium/issues/5884)
* Fixed discrepancy between default value used and commented value for default value for halfAxes of OrientedBoundingBox. [#6147](https://github.com/AnalyticalGraphicsInc/cesium/pull/6147)
* Added `Cartographic.toCartesian` to convert from Cartographic to Cartesian3. [#6163](https://github.com/AnalyticalGraphicsInc/cesium/pull/6163)
* Added geometric-error-based point cloud attenuation and eye dome lighting for point clouds using additive refinement.
Copy link
Contributor

Choose a reason for hiding this comment

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

Include the PR number like the other entries. Same below.

CHANGES.md Outdated
@@ -41,6 +41,8 @@ Change Log
* Fixed camera movement and look functions for 2D mode [#5884](https://github.com/AnalyticalGraphicsInc/cesium/issues/5884)
* Fixed discrepancy between default value used and commented value for default value for halfAxes of OrientedBoundingBox. [#6147](https://github.com/AnalyticalGraphicsInc/cesium/pull/6147)
* Added `Cartographic.toCartesian` to convert from Cartographic to Cartesian3. [#6163](https://github.com/AnalyticalGraphicsInc/cesium/pull/6163)
* Added geometric-error-based point cloud attenuation and eye dome lighting for point clouds using additive refinement.
* Added volume computation to `BoundingSphere`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Better to say what the actual function is:

Added BoundingSphere.volume for computing the volume of a BoundingSphere.

@@ -124,6 +128,7 @@ define([
* @param {Boolean} [options.debugShowRenderingStatistics=false] For debugging only. When true, draws labels to indicate the number of commands, points, triangles and features for each tile.
* @param {Boolean} [options.debugShowMemoryUsage=false] For debugging only. When true, draws labels to indicate the texture and geometry memory in megabytes used by each tile.
* @param {Boolean} [options.debugShowUrl=false] For debugging only. When true, draws labels to indicate the url of each tile.
* @param {Object} [options.pointShading] Options for constructing a PointShading object to control point attenuation based on geometric error and lighting.
Copy link
Contributor

Choose a reason for hiding this comment

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

Use {@link PointShading}

CHANGES.md Outdated
@@ -41,6 +41,8 @@ Change Log
* Fixed camera movement and look functions for 2D mode [#5884](https://github.com/AnalyticalGraphicsInc/cesium/issues/5884)
* Fixed discrepancy between default value used and commented value for default value for halfAxes of OrientedBoundingBox. [#6147](https://github.com/AnalyticalGraphicsInc/cesium/pull/6147)
* Added `Cartographic.toCartesian` to convert from Cartographic to Cartesian3. [#6163](https://github.com/AnalyticalGraphicsInc/cesium/pull/6163)
* Added geometric-error-based point cloud attenuation and eye dome lighting for point clouds using additive refinement.
Copy link
Contributor

Choose a reason for hiding this comment

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

using additive refinement

Should this be replacement?

@@ -0,0 +1,274 @@
<!DOCTYPE html>
Copy link
Contributor

Choose a reason for hiding this comment

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

This demo might look better with terrain on.

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 gave it a quick try, but it looks like Chappes church is below terrain, and the terrain also doesn't perfectly align with the Mt. St. Helens points, it punches through in some views.

});
});


Copy link
Contributor

Choose a reason for hiding this comment

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

Remove whitespace.


scene.destroyForSpecs();
});
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Given that all these tests are really similar, there could be a common function that all these tests call. Each test would pass in a callback function that makes the change (e.g. in here, the callback would contain scene.morphTo2D(0);.

PointCloudEyeDomeLighting,
Cesium3DTilesTester,
createScene) {
'use strict';
Copy link
Contributor

Choose a reason for hiding this comment

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

Fix indentation on includes.


beforeAll(function() {
scene = createScene();
scene.frameState.passes.render = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this line necessary?

@@ -241,6 +241,40 @@ define([
};
},

toRenderPixelCount : function(util, customEqualityTesters) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead could this and below be a single function that just returns the number of the pixels? The comparison logic (equals and greater than) seems like it should belong in the individual tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pjcozzi suggested adding these as new matchers: #6069 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the new matchers idea is good, but I think we can get away with just a single new matcher.

@likangning93
Copy link
Contributor Author

@lilleyse updated!

this.eyeDomeLightingRadius = defaultValue(pointCloudShading.eyeDomeLightingRadius, 1.0);
}

PointCloudShading.isSupported = PointCloudEyeDomeLighting.isSupported;
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

For consistency perhaps have PointCloudShading.isSupported take the scene instead of context, and pass scene.context through to PointCloudEyeDomeLighting.isSupported.

@lilleyse
Copy link
Contributor

Looks great, just that one comment. Fix the merge conflict and this should be ready.

@lilleyse lilleyse merged commit f1fad41 into CesiumGS:master Jan 30, 2018
@lilleyse lilleyse deleted the attenuation-and-EDL branch January 30, 2018 18:31
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.

5 participants