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

Convert EdgesGeometry to use BufferGeometry #20327

Merged
merged 5 commits into from
Sep 15, 2020

Conversation

gkjohnson
Copy link
Collaborator

Removes the reference to Geometry from EdgesGeometry.

@@ -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 );
Copy link
Collaborator Author

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).

@mrdoob mrdoob added this to the r121 milestone Sep 12, 2020
@WestLangley
Copy link
Collaborator

WestLangley commented Sep 12, 2020

I think I will leave review of computational geometry PRs to @Mugen87. :-)

I did test this PR on a simple BoxBufferGeometry and some edges were missing. I also tested on a Sphere. Same thing.

EDIT: Correction. It is working in my tests.

@WestLangley WestLangley removed their request for review September 12, 2020 21:15
@gkjohnson
Copy link
Collaborator Author

I did test this PR on a simple BoxBufferGeometry and some edges were missing. I also tested on a Sphere. Same thing.

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 EdgesGeometry class.

thresholdAngle = 1
image

thresholdAngle = 0
image

@WestLangley
Copy link
Collaborator

@gkjohnson Apologies... I reproduced my tests from yesterday and everything is working. I am not sure what happened.

Copy link
Collaborator

@Mugen87 Mugen87 left a 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.

@Mugen87
Copy link
Collaborator

Mugen87 commented Sep 13, 2020

Your PR actually solved #17793 (comment).

@mrdoob mrdoob merged commit cb392c3 into mrdoob:dev Sep 15, 2020
@mrdoob
Copy link
Owner

mrdoob commented Sep 15, 2020

Thanks!

@gkjohnson gkjohnson deleted the edges-buffer-geometry branch September 15, 2020 18:14
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.

None yet

4 participants