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

Height Reference for Corridor, Ellipse, Polygon and Rectangle #6434

Closed
wants to merge 9 commits into from

Conversation

hpinkos
Copy link
Contributor

@hpinkos hpinkos commented Apr 9, 2018

Replaces #6214
Open for early review

image

Adds GeometryHeightProperty for specifying a HeightReference for CorridorGraphics, EllipseGraphics, PolygonGraphics, and RectangleGraphics. This can be used for polygons with a height and/or extruded
height to specify HeightReference.RELATIVE_TO_GROUND or HeightReference.CLAMP_TO_GROUND. Note that using this property does not make the geometry conform to terrain. But it can be used to make the flat polygon be at a certain height above the terrain that is loaded.

The general idea is that we create the geometry, then when terrain LOD changes we move the positions in the shader so the height remains relative to terrain. When height is RELATIVE_TO_TERRAIN and extrudedHeight is CLAMP_TO_TERRAIN, we use ApproximateTerrainHeights.json to set the bottom height and only the positions of the top points change when terrain changes. In the Entity API, the end users can simply use the GeometryHeightProperty to specify the height reference and the rest happens behind the scenes.

Here are two use cases:

viewer.entities.add({
    name: 'Extruded polygon on terrain with a height of 10 m',
    polygon : {
        hierarchy : positions,
        material : Cesium.Color.fromRandom({alpha: 1}),
        height : new Cesium.GeometryHeightProperty(10.0, Cesium.HeightReference.RELATIVE_TO_GROUND),
        extrudedHeight : new Cesium.GeometryHeightProperty(0.0, Cesium.HeightReference.CLAMP_TO_GROUND)
    }
});

viewer.entities.add({
    name : 'Corridor at 30m above terrain',
    corridor : {
        positions : positions,
        width : 10.0,
        material : Cesium.Color.RED.withAlpha(0.5),
        height : new Cesium.GeometryHeightProperty(30.0, Cesium.HeightReference.RELATIVE_TO_GROUND),
    }
});

See PolygonLODTest Sandcastle example to see it in action.

Details:

Geometry/Primitive changes

  • Added a new offsetAttribute per vertex attribute to CorridorGeometry, EllipseGeometry, PolygonGeometry and RectangleGeometry. This is a boolean attribute used to specify which vertices the offset should be applied to.
  • Added private function setOptions to CorridorGeometry, EllipseGeometry, PolygonGeometry and RectangleGeometry. This is so I can use a scratch variable to find the bounding rectangle for these geometry types to pass to ApproximateTerrainHeights.
  • Factored ApproximateTerrainHeights out of GroundPrimitive, this is now used by both GroundPrimitive and TerrainOffsetProperty
  • Added OffsetGeometryInstanceAttribute which has a Cartesian3 value used to apply an offset to geometry vertices
  • Added Primitive._appendOffsetToShader which modifies the shader to add the offset vector to each vertex where applyOffset is true

Entity changes

  • Added GeometryHeightProperty, which has a height number value and a heightReference property. heightReference defaults to NONE.
  • If CLAMP_TO_GROUND is used for the extrudedHeight we find the value using ApproximateTerrainHeights
  • Because all geometry in a batch have to have the same attributes, I created new batches in GeometryVisualizer for geometries that have the offset attribute
  • Added GroundGeometryUpdater abstract class used by CorridorGeometryUpdater, EllipseGeometryUpdater, PolygonGeometryUpdater and RectangleGeometryUpdater (I made the same change in, it just happens that the same geometry types that can be z-ordered are the ones with the height/extrudedHeight properties Z-ordering for ground geometry entities #6362)
  • The GroundGeometryUpdater child classes have a _computeCenter function to return the position on terrain the height is relative to (polygon, ellipse and rectangle height is relative to the position in the center of the geometry, and I arbitrarily decided corridor height is relative to the middle position in the list of positions)
  • Added TerrainOffsetProperty, which is a private class. This is a dynamic property that is used to update the value of the offset geometry instance attribute when terrain LOD changes. When height reference is RELATIVE_TO_GROUND, GroundGeometryUpdater creates a this.terrainOffsetProperty = new TerrainOffsetProperty(...). Otherwise this.terrainOffsetProperty is a ConstantProperty with a value of Cartesian3.ZERO

TODO:

  • Geometry across the IDL crashes because the geometry has multiple bounding spheres
  • Sometimes the clamping doesn't update and I don't know why. I'm using surface.updateHeight the same way billboards do to update position when terrain LOD changes. Not sure if the event isn't being fired or the geometry isn't refreshing.
  • Wire up everything so it works when the geometry is dynamic
  • Make sure the correct thing happens for both height > extrudedHeight and extrudedHeight > height
  • Make offset changes to StaticOutlineGeometryBatch and StaticGeometryPerMaterialBatch
  • Geometry might be inside out if height < terrain height
  • Code cleanup
  • CHANGES.md
  • Doc
  • Specs

@cesium-concierge
Copy link

Signed CLA is on file.

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

🌍 🌎 🌏

@hpinkos
Copy link
Contributor Author

hpinkos commented Apr 9, 2018

@mramato, can you please review the entity related changes? I still have some code cleanup to do, but please give me your opinion on the the overall strategy. Right now it only works for static geometry with no outline, but I'm working on making the same changes for materials and outlines.

@bagnell the geometry and primitive changes are ready for a full code review, I just have to figure out how to fix the bounding sphere IDL issue.

@mramato
Copy link
Contributor

mramato commented Apr 10, 2018

I started to look at this, but I have a bunch of questions (that I'm not sure how to articulate in text). Let's talk offline as soon as you have time.

@mramato
Copy link
Contributor

mramato commented May 21, 2018

Please merge in master when you get a chance.

@mramato
Copy link
Contributor

mramato commented May 21, 2018

@hpinkos I had a hard time convincing myself that this PR was doing what you said given the Sandcastle example. I ended up tweaking it to make the difference more obvious. I also put the baselayer picker back because I think toggling terrain on/off (or with different terrains) gives the full affect. We probably want to keep the BaseLayerPicker off, but add some more toggles to the polygon height changing (maybe even something dynamic?) One I toggled terrain on/off, it was immediately clear to me how cool this feature is 😄 nice work.

var viewer = new Cesium.Viewer('cesiumContainer');

var baseLon = 6.850615989890521;
var baseLat = 45.89546589994886;
function addEntity(i, j) {
    var lon1 = baseLon + 0.015*i;
    var lon2 = baseLon + 0.015*i + 0.015;

    var lat1 = baseLat + 0.015*j;
    var lat2 = baseLat + 0.015*j + 0.015;
    var a = Cesium.Cartesian3.fromDegrees(lon1, lat1);
    var b = Cesium.Cartesian3.fromDegrees(lon1, lat2);
    var c = Cesium.Cartesian3.fromDegrees(lon2, lat2);
    var d = Cesium.Cartesian3.fromDegrees(lon2, lat1);

    var positions = [a, b, c, d];
    viewer.entities.add({
        polygon : {
            hierarchy : positions,
            material : Cesium.Color.fromRandom({alpha: 1}),
            height : new Cesium.GeometryHeightProperty(2000.0, Cesium.HeightReference.RELATIVE_TO_GROUND),
            extrudedHeight : new Cesium.GeometryHeightProperty(0.0, Cesium.HeightReference.CLAMP_TO_GROUND)
        }
    });
}

for (var i = 0; i < 4; i++) {
    for (var j = 0; j < 4; j++) {
        addEntity(i, j);
    }
}

Sandcastle.addToolbarButton('Zoom', function() {
    viewer.flyTo(viewer.entities);
});

@mramato
Copy link
Contributor

mramato commented May 21, 2018

At first I thought having a GeometryHeightProperty was not the correct level of abstraction because we could just have two new properties on the XXXXGraphics objects instead; but I think each have their pros and cons and make an equal amount of sense, so we can probably keep it the way it is (plus I convinced myself that your way uses less memory over all). We'll probably want to add CZML support somehow and update czml-writer (can be a separate PR but let's make sure we didn't do anything to make that harder than it should be CC @shunter)

I haven't looked at the code yet (I'll wait until master is merged) but the only real API question I have is whether we are OK with this feature effectively being "Entity" only. The primitive features add to support this are probably more general, but it's a really leaky abstraction (which I guess primitives are on purpose).

We could probably even break this into two PRs, one for the low-level primitive stuff and one for the higher level stuff. But I'm OK keeping it as one if the review stays managemable. @bagnell @likangning93 or @lilleyse should probably thoroughly review the primitive changes.

P.S. This is the best, most complete PR description you ever wrote. Thank you for it.

P.S.S. This works for dynamic geometry too, right?

@hpinkos
Copy link
Contributor Author

hpinkos commented May 21, 2018

Thanks @mramato! Yeah, I think I'll break this up into two PRs to separate the primitive level changes and the entity changes.

And I don't think I wired this up to work with dynamic geometry yet, but it will work by the time this is ready

@hpinkos
Copy link
Contributor Author

hpinkos commented May 23, 2018

Closing because I'm splitting this into several PRs

@hpinkos hpinkos closed this May 23, 2018
@hpinkos hpinkos deleted the geometry-height-reference branch July 2, 2018 15:35
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.

3 participants