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

Classification primitive #5625

Merged
merged 17 commits into from
Jul 21, 2017
Merged

Classification primitive #5625

merged 17 commits into from
Jul 21, 2017

Conversation

bagnell
Copy link
Contributor

@bagnell bagnell commented Jul 13, 2017

  • Adds ClassificationPrimitive which defines a volume and draws the intersection of the volume with terrain and 3D Tiles.
  • Refactors GroundPrimitive which is a special type of ClassificationPrimitive.
  • Updates 3D Tiles to render during the CESIUM_3D_TILES pass when picking.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jul 13, 2017

Does this work OK with translucent objects?

Copy link
Contributor

@pjcozzi pjcozzi left a comment

Choose a reason for hiding this comment

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

Tests and coverage look good, except it looks like ClassificationPrimitive.debugShowShadowVolume needs tests! Profile looks good too.

<meta http-equiv="X-UA-Compatible" content="IE=Edge,chrome=1"> <!-- Use Chrome Frame in IE -->
<meta name="viewport" content="width=device-width, initial-scale=1, maximum-scale=1, minimum-scale=1, user-scalable=no">
<meta name="description" content="Draw the intersection of a volume and a photgrammetry dataset.">
<meta name="cesium-sandcastle-labels" content="Development">
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure this should go under Development? I would rather it be more prominent.

camera.viewBoundingSphere(boundingSphere, new Cesium.HeadingPitchRange(0.0, -0.5, boundingSphere.radius));
camera.lookAtTransform(Cesium.Matrix4.IDENTITY);

var dimensions = new Cesium.Cartesian3(40.0, 30.0, 50.0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use a slightly different geometry that shows how this is different than a GroundPrimitive? For example, something where the bottom cap is above the ground and/or even something on the underside of a tree, polyline on the roof, etc. It would be fine to have a few short examples here.

this.debugShowShadowVolume = defaultValue(options.debugShowShadowVolume, false);
this._debugShowShadowVolume = false;

this._extruded = defaultValue(options._extruded, false);
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps add a comment that this is used by the GroundPrimitive. At first glance, I thought this was just a leftover from copy-paste.

* @param {Boolean} [options.debugShowShadowVolume=false] For debugging only. Determines if the shadow volume for each geometry in the primitive is drawn. Must be <code>true</code> on
* creation for the volumes to be created before the geometry is released or options.releaseGeometryInstance must be <code>false</code>.
*
* @see Primitive
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you also want Primitive and GroundPrimitive to reference this with @see?

* @constructor
*
* @param {Object} [options] Object with the following properties:
* @param {Array|GeometryInstance} [options.geometryInstances] The geometry instances to render.
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 be of length 1, right? Mention it.

Leave it plural so we could easily relax the requirement later.

* be <code>undefined</code> if <code>options.releaseGeometryInstances</code>
* is <code>true</code> when the primitive is constructed.
* <p>
* Changing this property after the primitive is rendered has no effect.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be readonly here and in GroundPrimitive/Primitive?

@bagnell
Copy link
Contributor Author

bagnell commented Jul 19, 2017

Does this work OK with translucent objects?

Of course the primitive can be translucent, but does it highlight translucent geometry? No.Neither will GroundPrimitives. Our translucent pass doesn't write depth.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jul 19, 2017

Submitted #5659

@bagnell
Copy link
Contributor Author

bagnell commented Jul 20, 2017

@bagnell
Copy link
Contributor Author

bagnell commented Jul 20, 2017

@pjcozzi This is ready for another look.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jul 21, 2017

Looks great!

Just a small tweak: can the Sandcastle example just create all the classification primitives up front, and then use the buttons (or a dropdown) to fly from one view to the other.

Otherwise, the examples doesn't really show anything until you click one of the buttons.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jul 21, 2017

Actually, it's good enough to keep the example as is for now.

@pjcozzi pjcozzi merged commit 69a7cc8 into master Jul 21, 2017
@pjcozzi pjcozzi deleted the classification-primitive branch July 21, 2017 12:19
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.

2 participants