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

Add 2D/Columbus view support to the geometry pipeline. #895

Merged
merged 40 commits into from
Jun 26, 2013
Merged

Conversation

bagnell
Copy link
Contributor

@bagnell bagnell commented Jun 25, 2013

No description provided.

bagnell added 30 commits June 13, 2013 20:46
Conflicts:
	Source/Core/GeometryPipeline.js
Conflicts:
	Apps/CesiumViewer/CesiumViewer.js
	Source/Scene/Primitive.js
	Source/Shaders/Appearances/AllMaterialAppearanceVS.glsl
	Source/Shaders/Appearances/EllipsoidSurfaceAppearanceVS.glsl
	Source/Shaders/Appearances/PerInstanceColorAppearanceVS.glsl
Conflicts:
	Apps/CesiumViewer/CesiumViewer.js
Conflicts:
	Source/Core/Geometry.js
…riangle2D to use barycentricCoordinates. Fix tests. Move PolygonPipeline.wrapLongitude tests to GeometryPipelineSpec. Add tests for indexing triangle list, fans, and strips.
function encodePositions(primitive, geometry) {
if (primitive._allowColumbusView) {
// Compute 2D positions
GeometryPipeline.projectTo2D(geometry);
Copy link
Contributor

Choose a reason for hiding this comment

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

Move this to geometryPipeline, again, to keep things at the same level of abstraction. This function is for encoding positions, not projecting 3D to 2D.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 25, 2013

JSHint is good. Tests are good. Just those comments and we're all good.

@bagnell
Copy link
Contributor Author

bagnell commented Jun 26, 2013

@pjcozzi This is ready for another review.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 26, 2013

There are two new test failures. Ignore the first two Polygon test failures; I still need to fit them.

@@ -41,7 +42,10 @@ Beta Releases
* Added `GeometryPipeline.combine` to combine meshes for better batching.
* Added `GeometryPipeline.computeNormal` to compute normals for a geometry.
* Added `GeometryPipeline.computeBinormalAndTangent` to compute binormals and tangent vectors for a geometry.
* Added the following functions to `GeometryPipeline` to index non-indexed primitive types: `indexLines`, `indexLineLoop`, `indexLineStrip`, `indexTriangles`, `indexTriangleFan`, and `indexTriangleStrip`.
Copy link
Contributor

Choose a reason for hiding this comment

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

I might not have been clear: we should have made these functions private and then tested everything through the function that calls the right function based on the primitive type. This fits our less-is-more ideal. We'll have less public entry points, less to doc, and less for users to learn. They will never need to call these fine-grained functions directly so why expose them.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 26, 2013

Just those comments. Let's get this merged today.

@bagnell
Copy link
Contributor Author

bagnell commented Jun 26, 2013

@pjcozzi This is ready for another review. The new bounding sphere function needs tests, but I'll do that when I write tests for everything so I can continue working on per-instance attributes.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 26, 2013

I still have two test failures. Do you get them?

image

@bagnell
Copy link
Contributor Author

bagnell commented Jun 26, 2013

@pjcozzi I fixed the wrong two tests. All of the Polygon tests are fixed now.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 26, 2013

All good. We needed to fix those tests anyway. Thanks.

pjcozzi added a commit that referenced this pull request Jun 26, 2013
Add 2D/Columbus view support to the geometry pipeline.
@pjcozzi pjcozzi merged commit 4cce108 into batching Jun 26, 2013
@pjcozzi pjcozzi deleted the columbusView branch June 26, 2013 20:39
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