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.

Sign up for GitHub

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

Jump to bottom

Add property for extruding polygon entities down to terrain #6214

Closed
wants to merge 11 commits into from
Closed

Add property for extruding polygon entities down to terrain #6214

wants to merge 11 commits into from

Conversation

hpinkos
Copy link
Contributor

@hpinkos hpinkos commented Feb 13, 2018 •
edited
Loading

  • Move GroundPrimitive min/max terrain height code out into ApproximateTerrainHeights static class
  • Added MinimumTerrainHeightProperty, which takes a property for the positions and computes the height based on those positions
  • Added sandcastle example for using this option with a polygon entity
  • Minimal doc cleanup
var viewer = new Cesium.Viewer('cesiumContainer', {
    baseLayerPicker: false,
    terrainProvider: new Cesium.CesiumTerrainProvider({
        url : 'https://assets.agi.com/stk-terrain/v1/tilesets/world/tiles',
        requestWaterMask : true,
        requestVertexNormals : true
    })
});
viewer.scene.globe.depthTestAgainstTerrain = true;

var baseLon = -111;
var baseLat = 37.4;
function addEntity(i, j) {
    var lon1 = baseLon + 0.001*i;
    var lon2 = baseLon + 0.001*i + 0.001;
    
    var lat1 = baseLat + 0.001*j;
    var lat2 = baseLat + 0.001*j + 0.001;
    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];
    var centroid = new Cesium.CentroidPositionProperty(positions);
    viewer.entities.add({
        polygon : {
            hierarchy : positions,
            material : Cesium.Color.fromRandom({alpha: 1}),
            height : new Cesium.RelativeToTerrainHeightProperty(viewer.terrainProvider, centroid, 400.0),
            extrudedHeight : new Cesium.MinimumTerrainHeightProperty(positions)
        }
    });
}

for (var i = 0; i < 64; i++) {
    for (var j = 0; j < 64; j++) {
        addEntity(i, j);
    }
}
viewer.zoomTo(viewer.entities);

@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 hpinkos changed the title Helper function for extruding polygons down to terrain Add property for extruding polygon entties down to terrain Feb 15, 2018
@hpinkos hpinkos force-pushed the extruded-polygon-terrain branch from 27f8071 to c0fc209 Compare February 15, 2018 20:55
@hpinkos
Copy link
Contributor Author

hpinkos commented Feb 15, 2018

@mramato can you review this?

@mramato
Copy link
Contributor

mramato commented Feb 15, 2018

Will check it out tonight.

mramato
mramato reviewed Feb 15, 2018
View reviewed changes
Source/DataSources/MinimumTerrainHeightProperty.js Outdated
/**
* @private
*/
MinimumTerrainHeightProperty.prototype._getValue = function(time) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason this is not just inline in getValue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, because I blindly copied VelocityVectorProperty =P
It's probably not needed

mramato
mramato reviewed Feb 16, 2018
View reviewed changes
Source/DataSources/MinimumTerrainHeightProperty.js Outdated
'use strict';

/**
* A {@link Property} which evaluates to a {@link Number} based on the minimum height of terrain
Copy link
Contributor

Choose a reason for hiding this comment

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

Number doesn't need a @link here.

@mramato
Copy link
Contributor

mramato commented Feb 16, 2018

Eventually, TerrainProvider will have a method on it for getting accurate min/max values, so when that happens, we'll make this property take an optional TerrainProvider as well, but I guess we'll always keep approx terrain heights around as a fallback.

mramato
mramato reviewed Feb 16, 2018
View reviewed changes
Apps/Sandcastle/gallery/Extruded Polygon on Terrain.html Outdated
requestWaterMask : true,
requestVertexNormals : true
});
viewer.terrainProvider = terrainProvider;
Copy link
Contributor

Choose a reason for hiding this comment

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

Disable the baselayer picker and pass the terrainProvider directly to the Viewer constructor, it avoid creating and immediately throwing out an Ellipsoid provider.

mramato
mramato reviewed Feb 16, 2018
View reviewed changes
Source/DataSources/MinimumTerrainHeightProperty.js Outdated
if (!isArray(positions)) {
positions = positions.positions; //positions is a PolygonHierarchy, just use the outer ring
}
var rectangle = Rectangle.fromCartesianArray(positions);
Copy link
Contributor

Choose a reason for hiding this comment

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

This should use a scratch parameter for the result rectangle.

mramato
mramato reviewed Feb 16, 2018
View reviewed changes
Source/DataSources/MinimumTerrainHeightProperty.js Outdated
*/
MinimumTerrainHeightProperty.prototype.getValue = function(time) {
//>>includeStart('debug', pragmas.debug);
if (!defined(time)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Use Check for new code.

@mramato
Copy link
Contributor

mramato commented Feb 16, 2018

I think that's all I have.

@hpinkos
Copy link
Contributor Author

hpinkos commented Feb 16, 2018

@mramato ready

@hpinkos hpinkos changed the title Add property for extruding polygon entties down to terrain Add property for extruding polygon entities down to terrain Feb 26, 2018
@hpinkos
Copy link
Contributor Author

hpinkos commented Feb 27, 2018

@mramato can you review my RelativeToTerrainHeightProperty class? I want to make sure I'm on the right track before I add specs

I updated the Sandcastle example to use this property too

mramato
mramato reviewed Feb 28, 2018
View reviewed changes
Source/DataSources/RelativeToTerrainHeightProperty.js Outdated
* }
* });
*/
function RelativeToTerrainHeightProperty(terrainProvider, positions, heightRelativeToTerrain) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since the name is generic, it might be easy to adapt this to work with a single position as well.

mramato
mramato reviewed Feb 28, 2018
View reviewed changes
Source/DataSources/RelativeToTerrainHeightProperty.js Outdated

if (defined(value)) {
if (!value.isConstant) {
throw new RuntimeError('positions must be a constant property');
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be a developer error?

Copy link
Contributor

Choose a reason for hiding this comment

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

We actually could support time-dynamic values if we just made it "best effort", i.e. it would only call sampleTerrain if a current request wasn't already in flight.

mramato
mramato reviewed Feb 28, 2018
View reviewed changes
Source/DataSources/RelativeToTerrainHeightProperty.js Outdated
}
this._subscription = value._definitionChanged.addEventListener(function() {
this._fetchTerrainHeight();
this._definitionChanged.raiseEvent(this);
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be better to remove this line (and below) and just let _fetchTerrainHeight raise definitionChanged as soon as it's complete.

mramato
mramato reviewed Feb 28, 2018
View reviewed changes
Source/DataSources/RelativeToTerrainHeightProperty.js Outdated
});

var centroidScratch = new Cartesian3();
function computeCentroid(positions) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I know we discussed this offline, but if I were doing this I would make a CentroidProperty that computes this value and then RelativeToTerrainHeightProperty can just always take a PositionProperty.

mramato
mramato reviewed Feb 28, 2018
View reviewed changes
Source/DataSources/RelativeToTerrainHeightProperty.js Outdated
* @returns {Number} The height relative to terrain
*/
RelativeToTerrainHeightProperty.prototype.getValue = function(time) {
return this._terrainHeight + this.heightRelativeToTerrain.getValue(time);
Copy link
Contributor

Choose a reason for hiding this comment

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

Always use Property.getValue when accessing values in our code. Probably Property.getValueOrDefault in this case to avoid NaN

@mramato
Copy link
Contributor

mramato commented Feb 28, 2018

@mramato can you review my RelativeToTerrainHeightProperty class? I want to make sure I'm on the right track before I add specs

Nothing crazy jumped out at me, I still have reservations as to how specific or general these properties should be. I'm leaning towards more general (as I'm sure you can tell by my comments).

@hpinkos hpinkos mentioned this pull request Mar 15, 2018
Merged
@hpinkos hpinkos closed this Apr 9, 2018
@hpinkos hpinkos mentioned this pull request Apr 9, 2018
Closed
10 tasks
@hpinkos hpinkos deleted the extruded-polygon-terrain branch July 2, 2018 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mramato mramato mramato left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hpinkos @cesium-concierge @mramato