-
-
Notifications
You must be signed in to change notification settings - Fork 35.3k
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
Convert EdgesGeometry to use BufferGeometry #20327
Conversation
@@ -285,7 +285,7 @@ export default QUnit.module( 'Geometries', () => { | |||
|
|||
QUnit.test( "three triangles, coplanar first", ( assert ) => { | |||
|
|||
testEdges( vertList, [ 0, 1, 2, 0, 2, 3, 0, 4, 2 ], 7, assert ); | |||
testEdges( vertList, [ 0, 2, 3, 0, 1, 2, 0, 4, 2 ], 7, assert ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is testing a case where the source geometry is composed of three triangles that all meet at a single edge. Depending on which of the two triangles are compared at that edge first it will either be included or excluded from the final edge geometry. In one case the angles of the two triangles will meet the angle threshold and in the other it will not so the EdgeGeometry edge generation algorithm is sensitive to the order of these indices -- this new approach in this PR is just sensitive in a slightly different way. You can see in the test below this one that a different order is tested and 6 edges are returned (which is what this one returned before I changed the order).
I think I will leave review of computational geometry PRs to @Mugen87. :-)
EDIT: Correction. It is working in my tests. |
Can you describe how you tested so I can reproduce? I just tested on a sphere and box and am not seeing any issues. Here I've created a LineSegments object from EdgesGeometry for both a sphere and a box. I see the same when using the original |
@gkjohnson Apologies... I reproduced my tests from yesterday and everything is working. I am not sure what happened. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm glad you have removed the dependency to Geometry
.
Your PR actually solved #17793 (comment). |
Thanks! |
Removes the reference to
Geometry
fromEdgesGeometry
.