Support for model outlining #4314

jasonbeverage · 2016-09-12T20:03:22Z

Hi all,

I've done a bit of work adding support to Cesium to provide a halo effect for outlining models when they are selected (or any other reason you want to highlight them).

I've included a new sandcastle example that you can try here:

https://cesiumdemos.netlify.com/apps/sandcastle/?src=development/3D%20Models%20Outline.html&label=Development

Most of the modifications were to Model.js. It provides a two pass rendering approach using the stencil buffer, where the model is drawn normally first, filling the stencil buffer, then the model is drawn again using a shader that extrudes the normals and only renders where the original model didn't render.

This is my first time digging around the guts of the model rendering code in Cesium, so I'd appreciate any feedback on the approach I took.

Thanks!

Jason

@lilleyse TODO:

To support multiple models simultaneously the stencil values need to be cleared between commands. I have been having trouble thinking of a way to clear these values during the silhouette color pass, so most likely I will go with a clear command of the screen-space region of the model according to @pjcozzi's suggestion. This will require a few modifications including supporting a RenderState in ClearCommand. This approach sort of breaks down for models that contain both opaque and transparent commands since the stencil buffer will get trashed in-between the opaque and translucent passes. All commands could be treated as translucent, but if OIT isn't supported the commands will be sorted and the clear commands won't. Essentially what's required for this all to work is render all model commands at the same time, opaque and translucent, immediately followed by the clear command - but a paradigm like list doesn't really exist in the rendering engine. Alternatively, if only the extruded back faces are rendered then the stencil writes can be done in the silhouette color pass and no clear command is needed. At this point though, it's useless to use stenciling at all and the artifacts are worse as @jasonbeverage noted. One further solution is to support only 255 silhouettes at a time - but this is limiting and has some edge cases in terms of constantly updating render states. Decided to go with this last approach and accept the minimal and somewhat unlikely artifacts
If going with the clear command approach, determine the screen space region of the model and apply a scissor test for that. Not applicable
The shader references v_normal and assumes the normal is in view space, both of which are true for Cesium's sample models but not for all models.
More unit tests, especially interactions with color with translucency.
Update to CHANGES.md.
Update CZML documentation.

…ed model now

… 3D Models.html back to the original

hpinkos · 2016-09-12T20:11:19Z

Wow, the demo looks awesome @jasonbeverage! Thanks so much for submitting this.

Do we have a contributor license agreement (CLA) from you? If not, can you please send one over so we can review this? You can find instructions in CONTRIBUTING.md

jasonbeverage · 2016-09-12T20:46:10Z

Yup you should have one on file, I've had other PR's merged. Thanks!

On Mon, Sep 12, 2016 at 4:11 PM Hannah notifications@github.com wrote:

Wow, the demo looks awesome @jasonbeverage
https://github.com/jasonbeverage! Thanks so much for submitting this.

Do we have a contributor license agreement (CLA) from you? If not, can you
please send one over so we can review this? You can find instructions in
CONTRIBUTING.md
https://github.com/AnalyticalGraphicsInc/cesium/blob/master/CONTRIBUTING.md#contributor-license-agreement-cla

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#4314 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAT74lBxa1t5shZryD1EVrBKfWcAyxojks5qpbHvgaJpZM4J6-QG
.

hpinkos · 2016-09-13T13:45:07Z

Great, thanks @jasonbeverage!

You have some jsHint errors. And I think we'll probably want some unit tests for this. See the pull request guidelines in our contributing documentation.

We're a little busy around here this week and we have some people on vacation, so we might not be able to review this right away. I think this would be a really useful feature though, and I'd love to see it merged soon =)

mramato · 2016-09-13T13:50:21Z

Just wanted to say that this is awesome. I was talking offline with @bagnell and he thought the same technique could be extended to other primitives (having a generic way to highlight all primitives is definitely something we've wanted for a long time). Our selection icon is good for HTML, but having a highlight that scales with the object is better. While I don't expect it to be part of this PR, having a new issue to track adding similar capabilities to geometry/labels/billboards/points would be great.

While I'm not qualified to review and merge this, from a functionality standpoint my one comment would be that we map the width parameter to use screen space units so that it's easier for users to understand.

Thanks @jasonbeverage.

jasonbeverage · 2016-09-13T15:15:55Z

Glad you like it @mramato :) It seems like it would be easy to extend to other graphics like labels and billboards, if we did that perhaps it would make sense to stick the highlight properties on the entity object itself instead of on the model, billboard, etc.

I agree on the screen space mapping instead of the weird clip space setting I have now, let me see what I can do.

…pace size

jasonbeverage · 2016-09-13T16:13:39Z

I've pushed some changes that should fix the jsHint issues and also changed the highlightSize to be in pixels instead of clipspace.

I have a question about how the uniforms work in Cesium though. I added a new function called getUniformNameForSemantic that tries to get a uniform from GLTF based on the semantic. For the projection matrix I had no fallback to czm_projection if I couldn't find the projection matrix, but when I tried to get the VIEWPORT semantic I was getting no uniform name back (I assume b/c the models don't use VIEWPORT in their shaders) so I fell back to czm_viewport. If the uniform wasn't found I figured I needed to define it in the shader b/c it wasn't used, but when I tried to append it in createHighlightVertexShaderSource using something like this:
if (!viewportUniformDefined) {
highlightMain += "uniform vec4 " + viewportName + ";\n"
}

However, doing that I get get a czm_viewport already defined shader compiler error even though czm_viewport actually isn't defined in the model's shader. I can apparently use czm_viewport just fine without defining it in my shader, is the model's shader eventually merged with some uber shader down the line?

Thanks!

lilleyse · 2016-09-13T16:21:44Z

I can apparently use czm_viewport just fine without defining it in my shader, is the model's shader eventually merged with some uber shader down the line?

Yup, before the shader is finalized it checks for any of the cesium built-in uniforms like czm_viewport and appends the uniform declarations to the shader: https://github.com/AnalyticalGraphicsInc/cesium/blob/master/Source/Renderer/ShaderSource.js#L60

jasonbeverage · 2016-09-13T17:13:06Z

Oh great, so that means that I actually don't need to get the semantic name for projection and viewport, I can just use czm_projection and czm_viewport freely right? That would simplify the shader generation code.

lilleyse · 2016-09-13T17:15:57Z

Yeah that should be doable.

jasonbeverage · 2016-09-13T17:16:20Z

Great, I just pushed a change doing that and it worked fine. Thanks!

pjcozzi · 2016-09-24T20:47:04Z

Source/DataSources/ModelGraphics.js

@@ -51,6 +51,9 @@ define([
     * @param {Property} [options.receiveShadows=true] Deprecated, use options.shadows instead. A boolean Property specifying whether the model receives shadows from shadow casters in the scene.
     * @param {Property} [options.shadows=ShadowMode.ENABLED] An enum Property specifying whether the model casts or receives shadows from each light source.
     * @param {Property} [options.heightReference=HeightReference.NONE] A Property specifying what the height is relative to.
+     * @param {Property} [options.highlight=false] Whether to highlight the model using an outline
+     * @param {Property} [options.highlightColor=Color(1.0, 0.0, 0.0, 1.0)] The highlight color for the outline.


Can we just default this to the default constructed color?

pjcozzi · 2016-09-24T20:48:36Z

Source/Scene/Model.js

+         *
+         * @type {Cartesian4}
+         *
+         * @default Color(1.0, 0.0, 0.0, 1.0)


Same comment.

pjcozzi · 2016-09-24T20:48:41Z

Source/Scene/Model.js

+        /**
+         * The highlight color
+         *
+         * @type {Cartesian4}


This is Color.

pjcozzi · 2016-09-24T20:48:51Z

Source/Scene/Model.js

+         */
+        this.highlightSize = 0.002;
+
+


Minor, but extra whitespace.

pjcozzi · 2016-09-24T20:50:38Z

Apps/Sandcastle/gallery/development/3D Models Outline.html

+            uri : url,
+            minimumPixelSize : 128,
+            maximumScale : 20000,
+            highlight: viewModel.highlight,


Default this to true so the example is obvious as soon as it loads.

pjcozzi · 2016-09-24T21:01:52Z

Source/Scene/Model.js

+                uniformMap.u_highlightColor = createHighlightColorFunction(model);
+                uniformMap.u_highlightSize = createHighlightSizeFunction(model);
+
+                var highlightCommand = new DrawCommand({


Can we create these commands and related resources on-demand the first time highlight is true?

I tried to mimic what was done with the pickCommands, so kept the highlight command and resource creation close to where the pick commands are generated. If you don't mind I'll defer this to you to move around so I don't break anything :)

@lilleyse let me know if you have any questions on this one; I prefer creating these on-demand since they may not be used in many apps, and users should only pay for what they use when it's reasonable for us to implement.

pjcozzi · 2016-09-24T21:02:08Z

Source/Scene/Model.js

+                        pass : isTranslucent ? Pass.TRANSLUCENT : Pass.OPAQUE
+                });
+
+


Extra whitespace.

pjcozzi · 2016-09-24T21:02:19Z

Source/Scene/Model.js

+                    pickCommand2D : pickCommand2D,
+                    highlightCommand: highlightCommand,
+                    highlightCommand2D: highlightCommand2D
+


Whitespace.

pjcozzi · 2016-09-24T21:02:32Z

Source/Scene/Model.js

+                            Matrix4.clone(command.modelMatrix, highlightCommand.modelMatrix);
+                            BoundingSphere.clone(command.boundingVolume, highlightCommand.boundingVolume);
+
+


Whitespace.

pjcozzi · 2016-09-24T21:14:39Z

Source/Scene/Model.js

                    owner : owner,
                    pass : isTranslucent ? Pass.TRANSLUCENT : Pass.OPAQUE
                });

+                // Setup the stencil command for the highlight
+                var highlightRS = clone(rs);


Shouldn't the blend state for this depend on the alpha value of hightlightColor? We might also be able to get always with just always using alpha blending for this pass.

Good catch, fixed and updated the Sandcastle demo to have an opacity slider, it looks nice with a partially transparent outline.

pjcozzi · 2016-09-24T21:28:14Z

@jasonbeverage cool stuff, I'm sure users will appreciate this and will get a kick out of animating the highlight size and color/translucency!

Comments:

Have you tried multiple overlapping 3D models? 3D models partly behind terrain? Given that the state of the stencil buffer is not known before each model, I think we are going to run into issues here. We could, of course, scissor around the model and clear the stencil before each model, but I suspect there is a clearer and faster approach.
Maybe some of our code comments are out of date, but I am actually surprised that this works at all since getContext does not create a stencil buffer by default. I am fine with explicitly defaulting stencil : true, but the spec states that when this is false, it should not accidentally behave like there is one. Maybe I missed it since I only looked in GitHub.
- Either way, the number of stencil bits may be zero so we should give users the ability to know if highlighting will work, e.g., a supported property that checks https://github.com/AnalyticalGraphicsInc/cesium/blob/master/Source/Renderer/Context.js#L512. We should also not render the highlight pass if it won't work.
Add unit tests, see https://github.com/AnalyticalGraphicsInc/cesium/blob/master/Documentation/Contributors/TestingGuide/README.md
Update CHANGES.md

Let me know if you have any questions on my code comments. They were all minor except the lazy creation of renderer resources, which will also be easy, but is important so users don't pay for things they don't use.

Also, we planned to implement this with an upcoming post-processing framework; the screen-space technique will likely be much faster, especially given the typical CPU bottlenecks in Cesium apps, but I don't know how the quality will compare; I suspect it will be good. No concern now, just a heads up that we might significantly change this later...and you are more than welcome to help!

jasonbeverage · 2016-09-26T16:23:03Z

Thanks for the feedback Patrick, I'll update the PR here soon and try to address your comments. Can you point me to some example unit tests that be applicable in this situation that I could try to draw inspiration from?

pjcozzi · 2016-09-26T18:57:54Z

For unit tests, we often rendering into a 1x1 viewport and check the color; however, even this is not super reliable across browsers, drivers, etc. so we have been doing more tests that check what commands were created (in this case, for example, you could validate the stencil render state).

For simple property set/get, see https://github.com/AnalyticalGraphicsInc/cesium/blob/master/Specs/Scene/ModelSpec.js#L214-L233

For command testing, see https://github.com/AnalyticalGraphicsInc/cesium/blob/master/Specs/Scene/PrimitiveSpec.js#L447-L467

BTW, we will likely change this to use "derived commands" so that the renderer can automatically generate highlight commands for any type of primitive instead of require each primitive to have custom code to create these commands. It totally isn't required for this PR, but if it is something you are interested in, @bagnell and I could advise.

lilleyse · 2016-12-08T19:29:48Z

This is updated now.

There were too many special cases with the stencil-clearing approach so I decided to instead go with using the unique stencil value per model approach. 256 models can be outlined without any artifacts, but once it goes over there would be some minor stencil artifacts if two models with the same stencil id happen to overlap. I think this is a rare situation and a reasonable trade-off. Another plus is there are at most 2 render passes, whereas the other approach often required three if translucency was involved.

This PR should work fine when OIT is supported. When it's not, the sorting causes stencil commands to be rendered in arbitrary order. A possible solution here is to create a derived command that combines multiple commands and always renders them together. All the model commands would be rendered sequentially, followed by all the stencil commands. Does this approach sound ok?

So while this PR is not complete yet it's worth a look.

lilleyse · 2016-12-08T19:49:31Z

A possible solution here is to create a derived command that combines multiple commands and always renders them together. All the model commands would be rendered sequentially, followed by all the stencil commands. Does this approach sound ok?

Another idea for the non-OIT situation: have a separate render pass right after translucency. The stencil buffer will be completely ready. The downside is translucent silhouettes will not be rendered in the correct sorted order.

pjcozzi · 2016-12-08T20:18:49Z

Another idea for the non-OIT situation: have a separate render pass right after translucency. The stencil buffer will be completely ready. The downside is translucent silhouettes will not be rendered in the correct sorted order.

Given that this is a short/medium-term solution for silhouettes that will be replaced by a screen-space approach, I would not do this architecture change.

pjcozzi · 2016-12-08T20:20:47Z

256 models can be outlined without any artifacts, but once it goes over there would be some minor stencil artifacts if two models with the same stencil id happen to overlap. I think this is a rare situation and a reasonable trade-off.

Agreed, can you please concisely document in, e.g., if more than 256 models have silhouettes enabled, there is a small chance that overlapping models will have minor artifacts.

The is more a note to us than end users, but it is probably better in the reference doc than as a comment.

pjcozzi · 2016-12-08T20:21:46Z

Do we clear stencil after the GROUND pass?

Also, how will this play when we are soon able to render models / 3D Tiles tilesets in the GROUND pass?

pjcozzi · 2016-12-08T20:25:10Z

Source/Renderer/Context.js

+         */
+        stencilBuffer : {
+            get : function() {
+                return this._stencilBits >= 8;


I've never seen less than 8 (or anything other than 8), but are you sure this is the right test?

When the context is created without stencil : true, gl.getParameter(gl.STENCIL_BITS); returns 0 so this test should be fine.

I get that it is fine in practice (we think), but are you sure that > 0 isn't more correct according to the spec?

According to the spec 8 bits is the minimum: https://www.khronos.org/registry/webgl/specs/latest/1.0/#2.2

OK, thanks for checking.

pjcozzi · 2016-12-08T20:25:36Z

Source/Renderer/RenderState.js

@@ -589,7 +589,7 @@ define([
            // Section 6.8 of the WebGL spec requires the reference and masks to be the same for
            // front- and back-face tests.  This call prevents invalid operation errors when calling
            // stencilFuncSeparate on Firefox.  Perhaps they should delay validation to avoid requiring this.
-            gl.stencilFunc(stencilTest.frontFunction, stencilTest.reference, stencilTest.mask);
+            gl.stencilFunc(frontFunction, reference, mask);


pjcozzi · 2016-12-08T20:28:33Z

Source/Scene/Model.js

+        return (Math.floor(currAlpha) !== Math.floor(prevAlpha)) || (Math.ceil(currAlpha) !== Math.ceil(prevAlpha));
+    }
+
+    // TODO : alternatively increment silhouette length as part of frame state and edit the model render states on the fly


What's the plan here?

If we are able to use derived commands to easily add silhouettes to other primitives, we could move this; otherwise, I think it is OK here.

pjcozzi · 2016-12-08T20:30:10Z

@lilleyse can you move the tasklist, #4314 (comment), to the top comment (so it is easier to find) and update it?

pjcozzi · 2016-12-08T20:31:48Z

A possible solution here is to create a derived command that combines multiple commands and always renders them together. All the model commands would be rendered sequentially, followed by all the stencil commands. Does this approach sound ok?

Yes, it does.

lilleyse · 2016-12-12T17:03:14Z

Also, how will this play when we are soon able to render models / 3D Tiles tilesets in the GROUND pass?

Just to be sure, do you mean the GLOBE pass? Either way it will be problematic...

Edit: Though if the silhouettes are opaque then it is doable to clear the stencil after the GLOBE pass. Translucent silhouettes won't be easily possible.

pjcozzi · 2016-12-12T21:37:00Z

Also, how will this play when we are soon able to render models / 3D Tiles tilesets in the GROUND pass?

Just to be sure, do you mean the GLOBE pass? Either way it will be problematic...

Yes, GLOBE.

Edit: Though if the silhouettes are opaque then it is doable to clear the stencil after the GLOBE pass. Translucent silhouettes won't be easily possible.

Couldn't the GLOBE pass have OPAQUE and TRANSLUCENT passes with proper stencil clearing? Or is OIT the issue?

lilleyse · 2016-12-12T22:15:42Z

Couldn't the GLOBE pass have OPAQUE and TRANSLUCENT passes with proper stencil clearing? Or is OIT the issue?

Ok yeah, if the globe pass has opaque and translucent passes in it this should all be fine.

lilleyse · 2016-12-12T22:21:01Z

I should have this ready later tonight after testing on my other machine.

lilleyse · 2016-12-13T16:33:32Z

The tests are in. I'm still figuring out how to improve the silhouettes in 2D.

pjcozzi · 2016-12-13T17:09:58Z

I am actually surprised that this works at all since getContext does not create a stencil buffer by default

Mystery solved by @lilleyse: #4747

pjcozzi · 2016-12-14T18:18:51Z

@lilleyse let merge this. If silhouettes in 2D/CV get widely used, we can improve them.

Likewise, this can also use derived commands when ready.

pjcozzi · 2016-12-14T18:19:09Z

Thanks again for kicking this off, @jasonbeverage. BIG contribution!

jasonbeverage · 2016-12-14T18:36:19Z

Thanks for merging this @pjcozzi and all the cleanup @lilleyse. Happy to be able to contribute to Cesium!

pjcozzi · 2016-12-14T18:49:55Z

@shunter do you, by chance, want to do the CZML updates here?

jasonbeverage added 11 commits August 31, 2016 10:21

Working on drawing highlighted models

9f58e9c

Changed demo to be able to modify the highlight size

de364e9

Better outlining algorithm

cea7291

Documentation and slight tweaks

c72757b

Changed Cartesian4 to Color

b8a2c44

Making vertex shader for highlighting more generic, works with animat…

6a532d1

…ed model now

Terrible code for looking up the projection matrix uniform name

2362111

Merge branch 'master' into modelhighlight

aa759ac

Spelling and added a highlightCommand2D

590bafc

Comment

73d9fb2

Moved outline example to it's own sandcastle example and reverted the…

116bdfc

… 3D Models.html back to the original

Fix jshint issues

fa23733

jasonbeverage added 2 commits September 13, 2016 11:34

Added new getUniformNameForSemantic function

751fbda

Changed the highlightSize parameter to be in pixels instead of clip s…

f41bb6e

…pace size

Simplified highlight shader just using cesium builtins

36f046d

Merge branch 'master' into modelhighlight

d247a65

pjcozzi reviewed Sep 24, 2016

View reviewed changes

lilleyse added 2 commits December 8, 2016 14:33

Fix spec

246dc83

Change order of stencilReference

60caefa

pjcozzi reviewed Dec 8, 2016

View reviewed changes

lilleyse added 3 commits December 12, 2016 13:22

Doc and stencil clear after ground pass

11fc0da

No longer hardcoding v_normal

cb35414

Updated CHANGES.md

95d024e

lilleyse mentioned this pull request Dec 13, 2016

Use depth-stencil for FXAA #4747

Merged

Added tests

077e225

lilleyse force-pushed the modelhighlight branch from e89915d to 077e225 Compare December 13, 2016 03:29

Merge branch 'master' into modelhighlight

7a399f2

pjcozzi merged commit 403af69 into CesiumGS:master Dec 14, 2016

		Matrix4.clone(command.modelMatrix, highlightCommand.modelMatrix);
		BoundingSphere.clone(command.boundingVolume, highlightCommand.boundingVolume);

Support for model outlining #4314

Support for model outlining #4314

Conversation

jasonbeverage commented Sep 12, 2016 • edited by lilleyse Loading

hpinkos commented Sep 12, 2016

jasonbeverage commented Sep 12, 2016

hpinkos commented Sep 13, 2016

mramato commented Sep 13, 2016

jasonbeverage commented Sep 13, 2016

jasonbeverage commented Sep 13, 2016

lilleyse commented Sep 13, 2016

jasonbeverage commented Sep 13, 2016 • edited Loading

lilleyse commented Sep 13, 2016

jasonbeverage commented Sep 13, 2016

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjcozzi commented Sep 24, 2016

jasonbeverage commented Sep 26, 2016

pjcozzi commented Sep 26, 2016

lilleyse commented Dec 8, 2016

lilleyse commented Dec 8, 2016 • edited Loading

pjcozzi commented Dec 8, 2016

pjcozzi commented Dec 8, 2016

pjcozzi commented Dec 8, 2016

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

Choose a reason for hiding this comment

pjcozzi commented Dec 8, 2016

pjcozzi commented Dec 8, 2016

lilleyse commented Dec 12, 2016 • edited Loading

pjcozzi commented Dec 12, 2016

lilleyse commented Dec 12, 2016

lilleyse commented Dec 12, 2016

lilleyse commented Dec 13, 2016

pjcozzi commented Dec 13, 2016

pjcozzi commented Dec 14, 2016

pjcozzi commented Dec 14, 2016

jasonbeverage commented Dec 14, 2016

pjcozzi commented Dec 14, 2016

jasonbeverage commented Sep 12, 2016 •

edited by lilleyse

Loading

jasonbeverage commented Sep 13, 2016 •

edited

Loading

lilleyse commented Dec 8, 2016 •

edited

Loading

lilleyse commented Dec 12, 2016 •

edited

Loading