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

Dont optimize without indices #155

Merged
merged 4 commits into from
Aug 10, 2016
Merged

Conversation

lasalvavida
Copy link
Contributor

Fixes #154, @lilleyse, could you look at this when you get a chance?

};
expect(function() {
cacheOptimization(gltf);
}).not.toThrowRuntimeError();
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a better way to test this, since we don't actually throw RuntimeErrors in gltf-pipeline.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just checked and this doesn't fail without the changes, so you're right, we need a better test here.

@lasalvavida
Copy link
Contributor Author

Updated

}
}
};
cacheOptimization(gltf);
Copy link
Contributor

@lilleyse lilleyse Aug 10, 2016

Choose a reason for hiding this comment

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

You should have at least one expect in here. Maybe just create a spy for GeometryPipeline.reorderForPostVertexCache and check that it's not called.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh well I guess that would pass for both...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, that should work. gltfPrimitiveToCesiumGeometry would work too.

@lasalvavida
Copy link
Contributor Author

Updated

@lilleyse lilleyse merged commit 73c3bf2 into master Aug 10, 2016
@lilleyse lilleyse deleted the dont-optimize-without-indices branch August 10, 2016 16:58
@lilleyse
Copy link
Contributor

Sorry just had a last minute thought, can you update CHANGES.md with the change? We should start getting in the habit of this now.

@lasalvavida
Copy link
Contributor Author

Yup

@lasalvavida lasalvavida mentioned this pull request Aug 10, 2016
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