-
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
Outline geometry #1021
Outline geometry #1021
Conversation
var extrudedOutlineExtent = new Cesium.GeometryInstance({ | ||
geometry : new Cesium.ExtentOutlineGeometry({ | ||
extent : extent, | ||
|
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 indices; | ||
var positions; | ||
|
||
// Positions only - no need to duplicate corner points |
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 comment doesn't make sense for the outline. Remove it.
var eastVecScratch = new Cartesian3(); | ||
var northVecScratch = new Cartesian3(); | ||
|
||
function pointOnEllipsoid(theta, rotation, northVec, eastVec, aSqr, ab, bSqr, mag, unitPos, result) { |
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 and other functions below duplicate with EllipseGeometry
? If so, put them in a common file as @private
functions (no need to explicitly unit test or doc them) so both geometries can use them.
Same comment if other geometries have a similar situation.
Tests pass. JSHint is good. The Sandcastle example is good. I am OK with the design of separate geometries for outlines, but I think there is a lot of duplication with the implementation details that we can improve. @bagnell you can finish the review and merge. Also update the draft tutorial. |
@bagnell I saw this too, but I suspect it is depth-test related, not geometric. Could be wrong, I suppose. |
@pjcozzi I think you're right. It looks like there's some z fighting on the ellipsoids too. |
Yes, we already mentioned that - see above. |
Conflicts: CHANGES.md Source/Core/ExtentGeometry.js Source/Core/PolygonGeometry.js Source/Core/PolylinePipeline.js Source/Core/WallGeometry.js
Yes |
@hpinkos Can you merge in master? |
position.z = rSurfaceZ + nZ * maxHeight; | ||
} | ||
|
||
if ( minHeight !== 'undefined') { |
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.
Use defined
here.
Conflicts: CHANGES.md
Added the following
Geometry
types:BoxOutlineGeometry
CircleOutlineGeometry
CylinderOutlineGeometry
EllipseOutlineGeometry
EllipsoidOutlineGeometry
ExtentOutlineGeometry
PolygonOutlineGeometry
SphereOutlineGeometry
WallOutlineGeometry
I updated the sandcastle Geometry and Appearances example to include examples of these geometries.
I also changed the extrusion parameters for
ExtentGeometry
to make it consistent with the extrusion parameters for polygon and ellipse.