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

Separate Appearance on z-fail #5160

Merged
merged 15 commits into from
Apr 12, 2017
Merged

Separate Appearance on z-fail #5160

merged 15 commits into from
Apr 12, 2017

Conversation

bagnell
Copy link
Contributor

@bagnell bagnell commented Mar 28, 2017

Adds support for a separate appearance when the primitive fails the depth test. Only polylines are supported at the moment. Example:

viewer.entities.add({
    polyline : {
        positions : cartesianSamples,
        followSurface : false,
        width : 5,
        material : new Cesium.PolylineOutlineMaterialProperty({
            color : Cesium.Color.ORANGE,
            outlineWidth : 2,
            outlineColor : Cesium.Color.BLACK
        }),
        depthFailMaterial : new Cesium.PolylineOutlineMaterialProperty({
            color : Cesium.Color.RED,
            outlineWidth : 2,
            outlineColor : Cesium.Color.BLACK
        })
    }
});
  • Fix existing tests.
  • Add new tests
  • Clean-up/add to Sandcastle
  • Update doc
  • Update CHANGES.md

@pjcozzi pjcozzi mentioned this pull request Mar 28, 2017
53 tasks
@pjcozzi
Copy link
Contributor

pjcozzi commented Mar 28, 2017

Add support to other primitives where it makes sense.

I could see this being useful for points/labels/billboards (think waypoints), but can you submit a new issue to consider these later?

@pjcozzi
Copy link
Contributor

pjcozzi commented Mar 28, 2017

At quick glance, the code looks good to me.

@lilleyse can you please test and review this? Once ready, we'll also merge this into the 3d-tiles branch.

@lilleyse
Copy link
Contributor

I'm noticing overlap of the two materials for z pass:
overlap
overlap2

Second screenshot is with this code:

var viewer = new Cesium.Viewer('cesiumContainer');
viewer.scene.globe.depthTestAgainstTerrain = true;
var position1 = Cesium.Cartesian3.fromRadians(0.0, 0.0, 400000.0);
var position2 = new Cesium.Cartesian3(0.0, 0.0, 0.0);
var cartesianSamples = [position1, position2];
viewer.entities.add({
    polyline : {
        positions : cartesianSamples,
        followSurface : false,
        width : 5,
        material : new Cesium.PolylineOutlineMaterialProperty({
            color : Cesium.Color.ORANGE,
            outlineWidth : 2,
            outlineColor : Cesium.Color.BLACK
        }),
        depthFailMaterial : new Cesium.PolylineOutlineMaterialProperty({
            color : Cesium.Color.RED,
            outlineWidth : 2,
            outlineColor : Cesium.Color.BLACK
        })
    }
});

modifiedFS += 'varying float v_WindowZ;\n' +
'void main() {\n' +
' czm_non_depth_clamp_main();\n' +
' gl_FragDepthEXT = min(v_WindowZ * gl_FragCoord.w, 1.0);\n' +
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is depth clamping used?

Copy link
Contributor

Choose a reason for hiding this comment

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

Still some overlap, but it seems to work ok without clamping. Maybe I'm missing some edge case.
clamp-off

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why is depth clamping used?

To prevent multi-frustum artifacts. If you turn it off and zoom in close, the line will be clipped.

Still some overlap, but it seems to work ok without clamping.

Maybe I didn't subsample the line enough or the lower res terrain LOD is rendered when zooming out. This isn't the best example.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you get a screenshot of that happening?

It may be worth supporting a fallback shader when the extension isn't supported, since it still mostly works without.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The shader now doesn't write fragment depth without the extension. Here is a gif showing the artifact for the sample you provided:
frag-depth-on-fail

this._closedColorBatches = new Array(numberOfShadowModes * 3);
this._closedMaterialBatches = new Array(numberOfShadowModes * 3);
this._openColorBatches = new Array(numberOfShadowModes * 3);
this._openMaterialBatches = new Array(numberOfShadowModes * 3);
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like this batching is starting to get out of hand (even when it was just shadows), but probably worth addressing later.

@lilleyse
Copy link
Contributor

Are there situations where the depth-fail won't work due the ordering of entities? Like I could imagine a large sphere occluding a polyline but the depth material not showing because of the order in which things are rendered. Is this generally ok because the depth-fail batch is last?

@pjcozzi
Copy link
Contributor

pjcozzi commented Mar 28, 2017

I feel like this batching is starting to get out of hand (even when it was just shadows), but probably worth addressing later.

Agreed, later.

Are there situations where the depth-fail won't work due the ordering of entities?

Yes. The main use case is depth fail vs. terrain or 3D Tiles. We are also adding an environment pass so that 3D Tiles can be rendered before these.

@lilleyse
Copy link
Contributor

In overlapping situations, could something like polygon offset help? Or stenciling?

@pjcozzi
Copy link
Contributor

pjcozzi commented Mar 29, 2017

On my integrated Intel MacBook Pro, the z-fail looks right, but z-pass does not:

image

Note the frustum boundary between the cyan and blue on the left.

EXT_frag_depth is supported.

http://cesium-dev.s3-website-us-east-1.amazonaws.com/cesium/3d-tiles-depth-fail/Apps/Sandcastle/index.html?src=Hello%20World.html&label=Showcases

@denverpierce
Copy link
Contributor

+1 for billboards/points. Sometimes I've got cases where I'll need to render a line's vertices along with the line itself.

@bagnell
Copy link
Contributor Author

bagnell commented Apr 6, 2017

Second screenshot is with this code:

Interpolating depth across a triangle that long was causing the issue. If you add more samples to the line, it's fine. Here is the updated code:

var viewer = new Cesium.Viewer('cesiumContainer');
viewer.scene.globe.depthTestAgainstTerrain = true;
var cartesianSamples = [];
var step = 1000.0;
var end = (Cesium.Ellipsoid.WGS84.radii.x + 400000.0) / step;
for (var i = 0; i < end; ++i) {
    cartesianSamples.push(new Cesium.Cartesian3(i * step, 0.0, 0.0));
}
viewer.entities.add({
    polyline : {
        positions : cartesianSamples,
        followSurface : false,
        width : 5,
        material : new Cesium.PolylineOutlineMaterialProperty({
            color : Cesium.Color.ORANGE,
            outlineWidth : 2,
            outlineColor : Cesium.Color.BLACK
        }),
        depthFailMaterial : new Cesium.PolylineOutlineMaterialProperty({
            color : Cesium.Color.RED,
            outlineWidth : 2,
            outlineColor : Cesium.Color.BLACK
        })
    }
});

@denverpierce
Copy link
Contributor

I'm getting inconsistent results when toggling terrain on then off again in the same session:

image

@bagnell
Copy link
Contributor Author

bagnell commented Apr 6, 2017

@denverpierce Is that first screen shot when the example first loads? In the code example we set, depthTestAgainstTerrain to true. When you select terrain in the imagery/terrian picker, it will also be set to true. Then, when selecting the ellipsoid terrain, it will be set to false. From there it should alternate from true to false.

You can ensure it is always true with:

viewer.scene.terrainProviderChanged.addEventListener(function() {
    viewer.scene.globe.depthTestAgainstTerrain = true;
});

@bagnell
Copy link
Contributor Author

bagnell commented Apr 6, 2017

@pjcozzi @lilleyse This is ready for another look.

@lilleyse
Copy link
Contributor

lilleyse commented Apr 7, 2017

I'm still seeing z-fighting with the higher-samples and in the sandcastle demo:

overlap

@bagnell
Copy link
Contributor Author

bagnell commented Apr 7, 2017

@lilleyse I can't reproduce that. Maybe it's the same issue here #5160 (comment)

Can you try it with EXT_frag_depth disabled? Maybe that's the issue.

@lilleyse
Copy link
Contributor

lilleyse commented Apr 7, 2017

Yeah it works without - this screenshot from before disabled the extension: #5160 (comment)

@pjcozzi
Copy link
Contributor

pjcozzi commented Apr 9, 2017

The stock Sandcastle example on my MacBook Pro is the same as #5160 (comment). It's not clear to me what the solution is based on the discussion above: does the example need to be updated or there is a bug when EXT_frag_depth is enabled?

@pjcozzi
Copy link
Contributor

pjcozzi commented Apr 9, 2017

@bagnell can you also update the 3d-tiles-depth-fail branch when ready for final testing?

@bagnell
Copy link
Contributor Author

bagnell commented Apr 11, 2017

Yeah it works without - this screenshot from before disabled the extension: #5160 (comment)

That isn't the best example. I subsampled the line at the highest terrain resolution. It'll fail depth for lower resolution LODs. Also, I didn't subsample at too fine a granularity.

@pjcozzi I cant reproduce #5160 (comment) or #5160 (comment) . Perhaps its a problem with EXT_frag_depth on Macs? Do you also have issues when the camera enters the shadow volume of GroundPrimitives and the volume intersects multiple frustums?

@bagnell
Copy link
Contributor Author

bagnell commented Apr 11, 2017

@lilleyse I swapped the draw order like we discussed offline. Can you see if that fixed the issue?

@lilleyse
Copy link
Contributor

Awesome that fixed it for me. @pjcozzi can you also confirm?

@pjcozzi
Copy link
Contributor

pjcozzi commented Apr 11, 2017

Works for me now!

Please merge to master and 3d-tiles if this is ready.

@lilleyse lilleyse merged commit 9309df8 into master Apr 12, 2017
@lilleyse lilleyse deleted the depth-fail branch April 12, 2017 14:11
@lilleyse
Copy link
Contributor

Unfortunately I noticed this right after merging after testing with 3d-tiles.

If either material or depthFailMaterial is a plain color I notice z-fighting like before but for the depth fail. All other polyline material combinations work fine. Also the z-fighting is gone when disabling EXT_frag_depth like before.

material : Cesium.Color.ORANGE,
depthFailMaterial : Cesium.Color.RED

zfighting

@pjcozzi
Copy link
Contributor

pjcozzi commented Apr 13, 2017

@bagnell can you please look at this one soon?

@bagnell
Copy link
Contributor Author

bagnell commented Apr 13, 2017

@lilleyse Can you try the depth-fail-stencil branch to see if it fixed the issue?

@lilleyse
Copy link
Contributor

zfight

I see z-fighting for the pass portion, regardless of material.

@pjcozzi
Copy link
Contributor

pjcozzi commented Apr 18, 2017

What is the plan here? To roll back to a previous version that @bagnell mentioned offline?

@pjcozzi
Copy link
Contributor

pjcozzi commented Apr 20, 2017

Bump @bagnell - #5160 (comment)

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.

4 participants