Geometry Web Worker #1023
Merged
Geometry Web Worker #1023
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…y inspecting the parameters, requiring in the module, and routing the event.
Conflicts: Source/Core/GeometryPipeline.js
Conflicts: Source/Core/WallGeometry.js
…gon primitive tests.
| @@ -0,0 +1,22 @@ | |||
| /*global define*/ | |||
There was a problem hiding this comment.
Can/should this have unit tests?
|
This works well. JSHint is good. The tests are in the state advertised. Most of my comments minor. I expect we'll get this in soon after #1021. |
|
@bagnell also, after going through this, I am even more inclined to suggest a "Writing Web Worker Safe JavaScript" blog post in addition to the post on the batching pipeline. |
|
@bagnell if there are a lot to replace, let me know and I can either show you how to do it with regex, or I can just do it for oyu. |
Conflicts: CHANGES.md Source/Core/BoxGeometry.js Source/Core/ComponentDatatype.js Source/Core/CylinderGeometry.js Source/Core/EllipseGeometry.js Source/Core/ExtentGeometry.js Source/Core/GeometryPipeline.js Source/Core/PolygonGeometry.js Source/Core/WallGeometry.js Source/Renderer/Context.js Source/Scene/CentralBodySurface.js Source/Scene/Primitive.js Source/Scene/Scene.js Source/Scene/SkyAtmosphere.js
Conflicts: Apps/Sandcastle/gallery/Geometry and Appearances.html Source/Core/CylinderGeometry.js Source/Core/EllipsoidGeometry.js Source/Core/ExtentGeometry.js Source/Core/PolygonGeometry.js Source/Core/SphereGeometry.js Source/Core/WallGeometry.js Source/Scene/SkyAtmosphere.js Specs/Core/EllipsoidGeometrySpec.js Specs/Core/ExtentGeometrySpec.js Specs/Core/GeometryPipelineSpec.js Specs/Core/SphereGeometrySpec.js
|
@pjcozzi This is ready for another review. |
| @@ -798,62 +798,6 @@ defineSuite([ | |||
| expect(context.readPixels()).toNotEqual([0, 0, 0, 0]); | |||
| }); | |||
|
|
|||
| it('renders more than 64K vertices of different polylines of different widths', function() { | |||
There was a problem hiding this comment.
I suppose you deemed this test redundant/unnecessary?
|
Tests and JSHint pass. The Sandcastle example is good. New changes look good. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Moves
Geometrycreation and batching to a web worker.I'm opening this request to get some early feedback. There are two failing tests. The reflection built-in material and the draw debug bounding sphere tests fail. The material test is a bug that can be reproduced in master and I'm opening an issue for it. I haven't decided the best way to fix the other test yet.
#1021 should be reviewed and merged before this.