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

Outline geometry #1021

Merged
merged 31 commits into from
Aug 15, 2013
Merged

Outline geometry #1021

merged 31 commits into from
Aug 15, 2013

Conversation

hpinkos
Copy link
Contributor

@hpinkos hpinkos commented Aug 7, 2013

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.

@bagnell bagnell mentioned this pull request Aug 9, 2013
var extrudedOutlineExtent = new Cesium.GeometryInstance({
geometry : new Cesium.ExtentOutlineGeometry({
extent : extent,

Copy link
Contributor

Choose a reason for hiding this comment

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

Extra whitespace

@pjcozzi
Copy link
Contributor

pjcozzi commented Aug 11, 2013

There are some artifacts with the ellipsoid outline:

image

I assume this is because we are using cube-map tessellation for the fill as we discussed and/or there are multi-frustum issues. As we discussed, we can change the fill tessellation if need be. Let me know what you decide.

var indices;
var positions;

// Positions only - no need to duplicate corner points
Copy link
Contributor

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) {
Copy link
Contributor

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.

@pjcozzi
Copy link
Contributor

pjcozzi commented Aug 11, 2013

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
Copy link
Contributor

bagnell commented Aug 12, 2013

There's an artifact with polygons. A line on the extruded wall pops in and out.

image

image

@pjcozzi
Copy link
Contributor

pjcozzi commented Aug 12, 2013

@bagnell I saw this too, but I suspect it is depth-test related, not geometric. Could be wrong, I suppose.

@bagnell
Copy link
Contributor

bagnell commented Aug 12, 2013

@pjcozzi I think you're right. It looks like there's some z fighting on the ellipsoids too.

image

@pjcozzi
Copy link
Contributor

pjcozzi commented Aug 12, 2013

Yes, we already mentioned that - see above.

@hpinkos
Copy link
Contributor Author

hpinkos commented Aug 13, 2013

@pjcozzi @bagnell I tried changing the fill ellipsoid tessellation to the lat/lon tessellation used by the outline to fix the artifact issues. It looks a lot better than the cube-map tessellation.

image

Should I go ahead and commit those changes?

Conflicts:
	CHANGES.md
	Source/Core/ExtentGeometry.js
	Source/Core/PolygonGeometry.js
	Source/Core/PolylinePipeline.js
	Source/Core/WallGeometry.js
@pjcozzi
Copy link
Contributor

pjcozzi commented Aug 13, 2013

Should I go ahead and commit those changes?

Yes

@pjcozzi
Copy link
Contributor

pjcozzi commented Aug 14, 2013

@hpinkos is this ready for another review? If so, @bagnell can you review and merge when ready?

@hpinkos
Copy link
Contributor Author

hpinkos commented Aug 14, 2013

@pjcozzi @bagnell yep, this is ready for review

@bagnell
Copy link
Contributor

bagnell commented Aug 15, 2013

@hpinkos Can you merge in master?

position.z = rSurfaceZ + nZ * maxHeight;
}

if ( minHeight !== 'undefined') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Use defined here.

bagnell added a commit that referenced this pull request Aug 15, 2013
@bagnell bagnell merged commit 26b4c49 into CesiumGS:master Aug 15, 2013
@hpinkos hpinkos deleted the outlineGeometry branch August 15, 2013 21:25
@pjcozzi
Copy link
Contributor

pjcozzi commented Aug 16, 2013

@bagnell make sure to update the tutorial.

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.

4 participants