-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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 #6717
Conversation
@hpinkos, thanks for the pull request! Maintainers, we have a signed CLA from @hpinkos, so you can review this at any time.
I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome. 🌍 🌎 🌏 |
@hpinkos who is the right person - or people? - to review this? Would like to get this merged soon for the next release and to finish this project up. |
@pjcozzi I'm already on the hook for reviewing this today. This is the second part of a partially reviewed PR (#6434) that was split into 2 (one for primitive changes one for Entity changes), so I'm not expecting any major surprises. This one is entity API one, the primitive one has already been merged. Of course if anyone else wants to take a crack at it, I'm more than happy to have the help. |
@@ -0,0 +1,86 @@ | |||
<!DOCTYPE html> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My comments in #6434 (comment) still apply here. I would also like to see some inline comments to explain the feature. If I'm just trying to learn Cesium's capabilities from Sandcastle examples, there's very little context here.
GeometryUpdater, | ||
GroundGeometryUpdater, | ||
Property) { | ||
'use strict'; | ||
|
||
var scratchColor = new Color(); | ||
var defaultOffset = Cartesian3.ZERO; | ||
var offsetScratch = new Cartesian3(); | ||
var scratchRectangleGeometry = new RectangleGeometry({rectangle: new Rectangle()}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this whole setOptions
approach because we are concerned about performance? Did we measure to see if it actually had any affect? Seems like a lot of changes for possibly no change in performance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't test performance. It just seemed like a good idea to be able to use a scratch variable. But I think I'm going to revert it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I'll wait to do this until you reply to my question on your other comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in #6733
|
||
if (extrudedHeight instanceof GeometryHeightProperty && Property.getValueOrDefault(extrudedHeight.height, Iso8601.MINIMUM_VALUE, HeightReference.NONE) === HeightReference.CLAMP_TO_GROUND) { | ||
scratchRectangleGeometry.setOptions(options); | ||
options.extrudedHeight = GeometryHeightProperty.getMinimumTerrainValue(scratchRectangleGeometry.rectangle); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is confusing, why are we setting extrudedHeight after we pass it to setOptions? Does order matter here because it looks like a bug if not and is confusing if so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Obviously this and other comments apply to the other geometries as well, but I won't comment anywhere except RectangleGeoemtryUpdater to avoid noise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I see what's going on here, but this seems like a really backwards way to do this. We are using a scratch geometry instance simply so we can call .rectangle
on it to pass to GeometryHeightProperty.getMinimumTerrainValue
? This makes me think that something is being done at the wrong level of abstraction. Why can't we add an option to the geometries so that we don't have to create a scratch one just for this use case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why can't we add an option to the geometries so that we don't have to create a scratch one just for this use case
What do you mean by this? I don't get it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I'm reading the code correctly, we are using a scratchRectangleGeometry
to compute the rectangle
property which we then use to compute another value and set it back onto options of a different RectangleGeometry that gets created later. That seems a little behind your back to get to your elbow to me.
It also means that rectangle gets computed up front for the geometry on the main thread (and I assume computed again later during tessellation?) Why doesn't the geometry just take the options needed to do the extrudedHeight computation itself during tessellation?
There could be a good reason we're doing it this way, but from the outside looking in it seems like there needs to be a cleaner way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in #6733
}; | ||
attributes.color = ColorGeometryInstanceAttribute.fromColor(currentColor); | ||
} | ||
if (defined(this._options.offsetAttribute)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not move this into the attributes initialization? Would keep the shape of the object the shame, also initialize color to undefined as well (for the same reason), This way attributes always has the same properties.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this matter? I'm pretty sure the result would be the same regardless.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's so browsers can better optimize the code. If attributes
always has the same set of properties (even if some are undefined) the browser can do a better job of optimizing compared to attributes
sometimes having an offset property and sometimes not.
} | ||
}); | ||
|
||
/** | ||
* @param {Entity} entity |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since none of the other private functions in this file have doc, I would just remove it for now for consistency. (if you really want to keep it, we should flesh it out).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the doc that's here is wrong anyway.
'use strict'; | ||
|
||
/** | ||
* A {@link Property} which computes height or extrudedHeight for a {@link CorridorGraphics}, {@link EllipseGraphics}, {@link PolygonGraphics} or {@link RectangleGraphics}. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps better explain flesh this out to mention that the computed height is in relation to the current terrain visualization and/or specified height reference. computes height
doesn't really explain anything without context.
this._closedMaterialBatches = new Array(numberOfShadowModes); | ||
this._openColorBatches = new Array(numberOfShadowModes); | ||
this._openMaterialBatches = new Array(numberOfShadowModes); | ||
this._outlineBatches = new Array(numberOfShadowModes*2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ugh, are we doubling the number of all batches here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, because not all geometry types have support for the applyOffset
vertex attribute required for the GeometryOffsetInstanceAttribute
. If we didn't want to double the size of the batches I suppose I could add an applyOffset
option to BoxGeometry
, CylinderGeometry
and all the others. This seemed like a better solution to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's only one GeometryVisualizer per-datasource these days, right? If so then I suppose this is OK for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although, ultimately wouldn't it make sense to have the offset and clamping capabilities to these other geometries as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, only one GeometryVisualizer
per datasource.
And yes, this should let us do clamping with the other geometry types but the solution would be a bit difference since this relies on setting height
and/or extrudedHeight
} else { | ||
this._closedMaterialBatches[shadows].add(time, updater); | ||
// eslint-disable-next-line no-lonely-if |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we disabling the rule here instead of following it? (same comment throughout)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I was afraid of messing up the if logic here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what unit tests are for. You just need to move the if/else up one, please remove these disables.
I'm still going through this, will have more comments either later tonight or tomorrow morning. |
var heightProperty = geometry.height; | ||
var extrudedHeightProperty = geometry.extrudedHeight; | ||
|
||
if (this._terrainOffsetProperty instanceof TerrainOffsetProperty) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this just be a defined check? When wouldn't _terrainOffsetProperty
be anything other than an instance of TerrainOffsetProperty
?
@@ -203,6 +232,12 @@ define([ | |||
options.granularity = defined(granularity) ? granularity.getValue(Iso8601.MINIMUM_VALUE) : undefined; | |||
options.stRotation = defined(stRotation) ? stRotation.getValue(Iso8601.MINIMUM_VALUE) : undefined; | |||
options.rotation = defined(rotation) ? rotation.getValue(Iso8601.MINIMUM_VALUE) : undefined; | |||
options.offsetAttribute = GeometryHeightProperty.computeGeometryOffsetAttribute(height, extrudedHeight, Iso8601.MINIMUM_VALUE); | |||
|
|||
if (extrudedHeight instanceof GeometryHeightProperty && Property.getValueOrDefault(extrudedHeight.height, Iso8601.MINIMUM_VALUE, HeightReference.NONE) === HeightReference.CLAMP_TO_GROUND) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to avoid checks like this in the Entity API because it means that the properties are no longer composable or referenceable (i.e. what if I use a CompositeProperty
so that sometimes the inner property is using GeometryHeightProperty
and sometimes it isn't?). This also means that things like ReferenceProperty
won't work.
06f31f1
to
31448d7
Compare
@mramato ready for another look |
Please merge in master, also looks like you have some eslint issues. |
} | ||
} | ||
|
||
viewer.scene.camera.flyTo({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't hard code camera numbers because the result is not the same for all screen sizes/aspect ratios and it also makes Cesium look hard to use. Use the viewer zoom functions instead, if you can't do this view with those, right up an issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I just did this because I was trying to find a spot that you could see the polygons both with and without terrain
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well actually, this doesn't work for the same reason viewer.flyTo
doesn't really work for billboards on terrain. The position changes as the terrain loads in.
}); | ||
viewer.scene.globe.depthTestAgainstTerrain = true; | ||
|
||
Sandcastle.addToolbarButton('Enable Terrain', function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would make this a checkbox instead of a button and perhaps fly the camera to the location when it changes (because the shapes went mostly out of view when I toggled it because they dropped so far).
'use strict'; | ||
//Sandcastle_Begin | ||
var cesiumTerrainProvider = Cesium.createWorldTerrain(); | ||
var ellipsoidTerrainProvider = new Cesium.EllipsoidTerrainProvider(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A line break could probably help here.
baseLayerPicker: false, | ||
terrainProvider: cesiumTerrainProvider | ||
}); | ||
viewer.scene.globe.depthTestAgainstTerrain = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a comment stating why we need to enable depthTestAgainstTerrain for this demo.
viewer.scene.terrainProvider = ellipsoidTerrainProvider; | ||
}); | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extra whitespace
var baseLat = 45.89546589994886; | ||
var delta = 0.001; | ||
function addEntity(i, j) { | ||
var lon1 = baseLon + delta*i; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Run the formatter on this example
|
||
var lat1 = baseLat + delta*j; | ||
var lat2 = baseLat + delta*j + delta; | ||
var a = Cesium.Cartesian3.fromDegrees(lon1, lat1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try looking at this with beginners eyes. Can be do better with variable names here? Is this north/source/east/west? Why are these polygons instead of rectangles?
@mramato ready for another look |
Source/Scene/Primitive.js
Outdated
@@ -1341,6 +1341,7 @@ define([ | |||
|
|||
for (i = 0; i < result.length; i++) { | |||
var boundingSphere = result[i].clone(primitive._boundingSpheres[i]); | |||
console.log(boundingSphere.radius); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whoops
height = 0; | ||
} | ||
|
||
var result = this._resultObject; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed offline, to not break the interface we can do height = new Number(height)
here and glob heightReference
onto that. I'm not crazy about it, but it will still be cleaner than what we're doing now.
Looks like the example doesn't work in IE11. |
I discussed some concerns with @hpinkos offline so she's going to split out the new properties into separate |
@hpinkos I think we're in the home stretch here. One performance oriented issue I noticed is that |
This is going to happen regardless because However, I did change it so it doesn't recompute everything every frame for static geometry. Instead, it will compute the position and normal up front, then only do the 'multiply by terrain height' calculation every frame. |
@hpinkos I merged in master. Have you tested this at all in 2D/CV? I think it's broken: |
@mramato I'm going to open a separate PR for that because it's a primitive level bug that's already in master. Anything else for this PR? |
@mramato ready. Apparently the position sent to the ground clamping callback is already projected to so I had to account for that |
Can someone please confirm that 2D and CV work and merge this ASAP? @mramato @ggetz @lilleyse @likangning93 |
Haven't looked at the rest of this, but the Sandcastle @mramato provided looks good in 2D and CV now. |
Can confirm that Sandcastle in the deployment works too. |
Sorry to worry you @hpinkos started to look at this an hour ago and got pulled away. Finishing up now. |
3D/2D/CV all work. Tests are passing and I've looked at the code too many times to have any more useful comments. 😄 Thanks @hpinkos this turned out to be a really cool feature. |
Finishes up #6434
Adds
GeometryHeightProperty
for specifying aHeightReference
forCorridorGraphics
,EllipseGraphics
,PolygonGraphics
, andRectangleGraphics
. This can be used for polygons with a height and/or extrudedheight to specify
HeightReference.RELATIVE_TO_GROUND
orHeightReference.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
isRELATIVE_TO_TERRAIN
andextrudedHeight
isCLAMP_TO_TERRAIN
, we useApproximateTerrainHeights.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 theGeometryHeightProperty
to specify the height reference and the rest happens behind the scenes.Here are two use cases:
Details:
GeometryHeightProperty
, which has aheight
number value and aheightReference
property.heightReference
defaults toNONE
.CLAMP_TO_GROUND
is used for theextrudedHeight
we find the value usingApproximateTerrainHeights
GeometryVisualizer
for geometries that have the offset attributeGroundGeometryUpdater
abstract class used byCorridorGeometryUpdater
,EllipseGeometryUpdater
,PolygonGeometryUpdater
andRectangleGeometryUpdater
(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)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)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 isRELATIVE_TO_GROUND
,GroundGeometryUpdater
creates athis.terrainOffsetProperty = new TerrainOffsetProperty(...)
. Otherwisethis.terrainOffsetProperty
is aConstantProperty
with a value ofCartesian3.ZERO
TODO