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

Fix bounding volume for tiny ground primitives #4485

Closed
wants to merge 2 commits into from

Conversation

hpinkos
Copy link
Contributor

@hpinkos hpinkos commented Oct 20, 2016

Fixes #4326

The bounding box minimum wasn't quite low enough for really really tiny ground primitives which resulted in z-fighting

var viewer = new Cesium.Viewer('cesiumContainer');
var r1 = new Cesium.Rectangle(-0.2865044365070024, 0.49644278153576343, -0.28647774383347757, 0.496452001539263);
viewer.scene.primitives.add(new Cesium.GroundPrimitive({
    geometryInstances : [new Cesium.GeometryInstance({
        geometry : new Cesium.RectangleGeometry({
            rectangle : r1,
            vertexFormat : Cesium.PerInstanceColorAppearance.VERTEX_FORMAT
        }),
        attributes: {
            color: Cesium.ColorGeometryInstanceAttribute.fromColor(new Cesium.Color(1.0, 0.0, 0.0, 0.5))
        }
    })],
    appearance : new Cesium.PerInstanceColorAppearance({
        closed : true
    })
}));
viewer.scene.camera.setView({
    destination: r1
});

@mramato
Copy link
Contributor

mramato commented Oct 20, 2016

Unit test?

@hpinkos
Copy link
Contributor Author

hpinkos commented Oct 21, 2016

ready

@@ -425,7 +425,7 @@ define([

function getComputeMinimumHeightFunction(primitive) {
return function(granularity, ellipsoid) {
return primitive._minHeight;
return primitive._minHeight - 15;
Copy link
Contributor

Choose a reason for hiding this comment

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

15.0 and please add a comment here.

This could also have a significant performance impact for horizon views. How did you come up with 15? I would much rather this depend on the view parameters since 15 meters, for example, could be millions of pixels when zoomed in close.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What view parameters? This function takes a granularity and an ellipsoid.

There's no specific reason I chose 15. For the code example I pasted above even subtracting 1.0 fixed the problem. From my understanding problem happens when the shadow volume ends exactly at the height the terrain does, so maybe 1.0 would work for all cases? I honestly don't know. @bagnell @tfili what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

View parameter - where is the camera and what is the FOV. This extrusion should happen in the vertex shader on a frame by frame basis so we don't create huge volumes when zoomed that create a high fillrate load.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah okay. Well that kind of goes beyond my realm of knowledge, so someone else will have to take over here.

@pjcozzi
Copy link
Contributor

pjcozzi commented Oct 21, 2016

Also, update CHANGES.md.

@mramato
Copy link
Contributor

mramato commented Oct 21, 2016

@tfili can you review this, or should it be @bagnell?

@pjcozzi
Copy link
Contributor

pjcozzi commented Oct 21, 2016

@mramato as discussed above, this needs a new approach, not a review. @bagnell should be able to fix this when available.

@mramato
Copy link
Contributor

mramato commented Oct 21, 2016

Ah, okay sorry for the noise; I missed that comment about the vertex shader.

@hpinkos
Copy link
Contributor Author

hpinkos commented Oct 24, 2016

Closing this. I added a comment to #4326 pointing here for more information

@hpinkos hpinkos closed this Oct 24, 2016
@hpinkos hpinkos deleted the groundPrimitive-bb-min branch December 12, 2016 20:14
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.

KML linestring clamp to ground fails
3 participants