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

Frustum geometry #5649

Merged
merged 12 commits into from
Jul 19, 2017
Merged

Frustum geometry #5649

merged 12 commits into from
Jul 19, 2017

Conversation

bagnell
Copy link
Contributor

@bagnell bagnell commented Jul 18, 2017

Adds FrustumGeometry and FrustumOutlineGeometry

  • Update CHANGES.md

@pjcozzi
Copy link
Contributor

pjcozzi commented Jul 19, 2017

Submitted #5651

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.

Just those comments.


var frustumGeometry = new Cesium.FrustumGeometry({
frustum : frustum,
position : positionOnEllipsoid,
Copy link
Contributor

Choose a reason for hiding this comment

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

I guest position is good since it is consistent with the rest of the Cesium API; I wonder if there is something better in the context of frustum like viewer or origin (e.g., we use origin for a ray, not position).

Can you look around and come up with the best name?

Cesium.Matrix3.multiply(rotation, Cesium.Matrix3.fromRotationX(-Cesium.Math.PI_OVER_TWO), rotation);
var orientation = Cesium.Quaternion.fromRotationMatrix(rotation);

var frustum = new Cesium.PerspectiveFrustum();
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this have an options constructor we can use?

* @constructor
*
* @param {Object} options Object with the following properties:
* @param {PerspectiveFrustum|OrthographicFrustum} options.frustum The frustum.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it easy to compute the input to a box geometry given an orthographic frustum? If so, perhaps this should only take a perspective frustum and be renamed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, but I think it's simpler this way.

var orientation = options.orientation;
var position = options.position;
var vertexFormat = defaultValue(options.vertexFormat, VertexFormat.DEFAULT);
var drawNearPlane = defaultValue(options._drawNearPlane, true);
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a comment about why this is private...or I wonder if we would want it to be public.

* @alias FrustumOutlineGeometry
* @constructor
*
* @param {Object} options Object with the following properties:
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comments, of course.

* @param {Cartesian3} options.position The position of the frustum.
* @param {Quaternion} options.orientation The orientation of the frustum.
*/
function FrustumOutlineGeometry(options) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible for this to share any code with FrustumGeometry since all the positions are the same?

Or is that more trouble than it is worth?

@@ -246,7 +246,7 @@ define([
array[startingIndex++] = value.st ? 1.0 : 0.0;
array[startingIndex++] = value.tangent ? 1.0 : 0.0;
array[startingIndex++] = value.bitangent ? 1.0 : 0.0;
array[startingIndex++] = value.color ? 1.0 : 0.0;
array[startingIndex] = value.color ? 1.0 : 0.0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Good eye

@pjcozzi
Copy link
Contributor

pjcozzi commented Jul 19, 2017

Is there anywhere else that needs to be updated, e.g., is this used at all for 3D Tiles or terrain debugging?

@bagnell
Copy link
Contributor Author

bagnell commented Jul 19, 2017

Is there anywhere else that needs to be updated, e.g., is this used at all for 3D Tiles or terrain debugging?

No, it's only DebugCameraPrimitive, but that is used by the Cesium Inspector and ShadowMap.

@bagnell
Copy link
Contributor Author

bagnell commented Jul 19, 2017

@pjcozzi This is ready for another look.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jul 19, 2017

Very nice!

@pjcozzi pjcozzi merged commit 79ee716 into master Jul 19, 2017
@pjcozzi pjcozzi deleted the frustum-geometry branch July 19, 2017 20:50
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