Simple point cloud attenuation #5824

lilleyse · 2017-09-11T19:05:48Z

Before #5455 is ready, I've taken some code from there and tweaked a few things so we can support very basic point attenuation.

It just does a mix between the minimum point size and maximum point size based on the distance from the camera, given a max distance of 100 currently. Ideally this value should be tuned based on the tileset scale and point density, but these defaults are decent for now.

cesium-concierge · 2017-09-11T19:05:49Z

@lilleyse, thanks for the pull request! Maintainers, we have a signed CLA from @lilleyse, so you can review this at any time.

I am a bot who helps you make Cesium awesome! Thanks again.

lilleyse · 2017-09-11T19:08:32Z

Source/Scene/PointCloud3DTileContent.js

-                scratchPointSizeAndTilesetTime.x = content._pointSize;
-                scratchPointSizeAndTilesetTime.y = content._tileset.timeSinceLoad;
-                return scratchPointSizeAndTilesetTime;
+            u_pointAttenuationMaxSize : function() {


This is not bundled in u_pointSizeAndTilesetTimeAndAttenuation because #5455 overrides just this uniform.

Thanks; even though this behavior might seem redundant; it makes the uniform map easier to modify from future stages

pjcozzi · 2017-09-11T21:43:28Z

@AnimatedRNG could you please do a first review of this and then bump to me for a quick final review?

AnimatedRNG · 2017-09-13T20:04:14Z

@pcozzi Sure, I'll take a look soon!

AnimatedRNG · 2017-09-14T19:27:43Z

@lilleyse Your code looks pretty good! I left a few comments for review.

lilleyse · 2017-09-15T14:55:51Z

@AnimatedRNG is #5824 (comment) your only comment? Is this ready to merge?

AnimatedRNG · 2017-09-15T19:44:16Z

@lileyse I left three other comments on the code for review. Can you see them?

AnimatedRNG · 2017-09-14T18:17:41Z

Specs/Scene/PointCloud3DTileContentSpec.js

@@ -432,6 +428,28 @@ defineSuite([
        });
    });

+    it('point attenuation', function() {


If my understanding is correct, this code renders a point with point attenuation disabled, fails to find it, then enables point attenuation and finds it. This test is useful because it proves that point attenuation "grows" points which are close to the camera.

A nice complement test would also check that point attenuation does not grow points which are far away from the camera. This test would prove that the point size is actually being attenuated, rather than just being scaled uniformly.

Good idea, added a few more lines onto the existing test.

AnimatedRNG · 2017-09-14T18:27:00Z

Source/Scene/PointCloud3DTileContent.js

        } else {
            vs += '    gl_PointSize = u_pointSize; \n';
        }

-        vs += '    color = color * u_highlightColor; \n';
+        if (pointAttenuation) {
+            vs += '    vec4 positionEC = czm_view * vec4(positionWC, 1.0); \n' +


In general, PointCloud3DTileContent modifies the shader by conditionally appending some code to it (e.g no normals vs has normals, has a point size vs doesn't have a point size, etc.). The shader code incision/excision is made in Javascript, which makes sense given that the shader string is formed, bit by bit, in Javascript.

An alternative may involve using preprocessor defines to selectively enable/disable regions of code, and then merely adding the defines to the defines array, or just prepending those lines to the source.

If you prefer constructing the shader completely in Javascript, then the current method is totally fine too.

Yeah I could see it going either way. The one area that would be more complicated with defines is the attribute declaration section, but otherwise it falls pretty neatly into defines.

AnimatedRNG · 2017-09-14T18:28:51Z

Source/Scene/PointCloud3DTileContent.js

        this._pointSize = 1.0;
+        this._pointAttenuationMaxSize = 10.0;


Is there any way to configure the maximum size or the start distance without access to private variables?

I wasn't sure if I wanted to expose much of the internals yet since the attenuation is still kind of basic and not on by default. I could see those becoming options in the future though.

AnimatedRNG · 2017-09-15T19:45:19Z

@lilleyse Ah okay it looks like I hadn't submitted my review yet, sorry! Here it is.

lilleyse · 2017-09-18T21:46:32Z

@AnimatedRNG followed up with your comments.

mramato · 2017-09-27T14:43:24Z

@lilleyse please merge in master. Do we want this for the next release. @AnimatedRNG will you have time to do another quick review pass on it?

lilleyse · 2017-09-27T14:46:43Z

It would nice to get this in before the release.

AnimatedRNG · 2017-09-27T20:17:25Z

@mramato Sorry about the delay; everything looks good!

pjcozzi · 2017-09-27T22:41:14Z

Source/Scene/Cesium3DTileset.js

+         * @type {Boolean}
+         * @default true
+         */
+        this.pointAttenuation = true;


Mention this in CHANGES.md.

Are you sure this is the only field you want to expose, e.g., no parameters to change the size?

Are you also sure that it belongs here and not on the style? This seems out of place and more tightly coupled.

Are you also sure that it belongs here and not on the style? This seems out of place and more tightly coupled.

I don't think it makes sense to be on the style, all 3D Tiles-conformant engines would need to support point attenuation. I think of this as similar to the color blend mode for tilesets, just an engine option to tweak the visuals.

Are you sure this is the only field you want to expose, e.g., no parameters to change the size?

@AnimatedRNG also suggested this. I might make them private to start since I expect the attenuation to be more involved in the future.

@AnimatedRNG also suggested this. I might make them private to start since I expect the attenuation to be more involved in the future

Could be fine to be public and then deprecate everything when we have the new method; otherwise, I think users will ask for these properties right away.

pjcozzi · 2017-09-27T22:43:39Z

Just that comment.

pjcozzi · 2017-10-02T12:37:42Z

I don't think it makes sense to be on the style, all 3D Tiles-conformant engines would need to support point attenuation

What part of the spec says this?

Seems to be that this is exactly the type of behavior defined by a style.

pjcozzi · 2017-10-02T12:38:12Z

@lilleyse please bump when ready. Just a heads up that I will not have the bandwidth to review in time for the release today.

lilleyse · 2017-10-02T15:08:29Z

Updated to make all the point size controls public.

I'm still not sure about adding this to the style. The same question applies to point cloud surface reconstruction (#5455) - should a style be able to say whether it uses that technique? I feel like these options are too tied to the rendering engine to be useful as styles. Maybe we can discuss more offline.

pjcozzi · 2017-10-03T16:04:02Z

Discussed offline - we are going to try this with declarative styling.

lilleyse · 2017-12-22T01:36:21Z

Closing in favor of #6069

lilleyse added 2 commits September 11, 2017 14:54

Simple attenuation

859243d

Update Sandcastle

ac6be75

Added to CHANGES.md

629e8c1

lilleyse force-pushed the point-attenuation branch from d8dc16f to 629e8c1 Compare September 11, 2017 19:06

lilleyse commented Sep 11, 2017

View reviewed changes

Remove accidental duplicate spec

5548c38

AnimatedRNG reviewed Sep 15, 2017

View reviewed changes

Added test for camera not being in range for point attenuation

6be8902

Merge branch 'master' into point-attenuation

06bc2b6

Merge branch 'master' into point-attenuation

f4e0b53

pjcozzi reviewed Sep 27, 2017

View reviewed changes

lilleyse added 3 commits September 28, 2017 19:31

Exposed point attenuation properties through tileset

77ada33

Updated CHANGES.md

4851d7a

Merge branch 'master' into point-attenuation

89e7ba5

lilleyse added 2 commits October 2, 2017 10:48

Make private vars public

96688e3

Merge branch 'master' into point-attenuation

b2b3c77

Fix CHANGES.md

643157b

lilleyse added 2 commits November 1, 2017 10:59

Merge branch 'master' into point-attenuation

2c2c46a

Change default color for point clouds

8e89cda

lilleyse closed this Dec 22, 2017

lilleyse deleted the point-attenuation branch December 22, 2017 01:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simple point cloud attenuation #5824

Simple point cloud attenuation #5824

lilleyse commented Sep 11, 2017

cesium-concierge commented Sep 11, 2017

lilleyse Sep 11, 2017

AnimatedRNG Sep 14, 2017

pjcozzi commented Sep 11, 2017

AnimatedRNG commented Sep 13, 2017

AnimatedRNG commented Sep 14, 2017

lilleyse commented Sep 15, 2017

AnimatedRNG commented Sep 15, 2017

AnimatedRNG Sep 14, 2017

lilleyse Sep 18, 2017

AnimatedRNG Sep 14, 2017

lilleyse Sep 18, 2017

AnimatedRNG Sep 14, 2017

lilleyse Sep 18, 2017

AnimatedRNG commented Sep 15, 2017

lilleyse commented Sep 18, 2017

mramato commented Sep 27, 2017

lilleyse commented Sep 27, 2017

AnimatedRNG commented Sep 27, 2017

pjcozzi Sep 27, 2017

lilleyse Sep 28, 2017 •

edited

Loading

lilleyse Sep 28, 2017

pjcozzi Oct 2, 2017

pjcozzi commented Sep 27, 2017

pjcozzi commented Oct 2, 2017

pjcozzi commented Oct 2, 2017

lilleyse commented Oct 2, 2017

pjcozzi commented Oct 3, 2017

lilleyse commented Dec 22, 2017

Simple point cloud attenuation #5824

Simple point cloud attenuation #5824

Conversation

lilleyse commented Sep 11, 2017

cesium-concierge commented Sep 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjcozzi commented Sep 11, 2017

AnimatedRNG commented Sep 13, 2017

AnimatedRNG commented Sep 14, 2017

lilleyse commented Sep 15, 2017

AnimatedRNG commented Sep 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnimatedRNG commented Sep 15, 2017

lilleyse commented Sep 18, 2017

mramato commented Sep 27, 2017

lilleyse commented Sep 27, 2017

AnimatedRNG commented Sep 27, 2017

Choose a reason for hiding this comment

lilleyse Sep 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjcozzi commented Sep 27, 2017

pjcozzi commented Oct 2, 2017

pjcozzi commented Oct 2, 2017

lilleyse commented Oct 2, 2017

pjcozzi commented Oct 3, 2017

lilleyse commented Dec 22, 2017

lilleyse Sep 28, 2017 •

edited

Loading