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

EdgesGeometry shows unnecessary cords #10517

Open
Emanuele-Spatola opened this issue Jan 6, 2017 · 10 comments
Open

EdgesGeometry shows unnecessary cords #10517

Emanuele-Spatola opened this issue Jan 6, 2017 · 10 comments

Comments

@Emanuele-Spatola
Copy link

@Emanuele-Spatola Emanuele-Spatola commented Jan 6, 2017

Description of the problem

The EdgesGeometry takes all the cords shared between faces and if the faces are not coplanar (or their "dot" is less than the thresholdDot) then it shows them as edges.

The problem is that in some cases a cord is the sum of two other cords in two different faces. In this case the cord is shown as edge even if its's not.

This is the wireframe:
screen shot 2017-01-06 at 12 02 01

and this is with the EdgesGeometry:
screen shot 2017-01-06 at 12 01 44

I've made a plunker with an example geometry:
https://plnkr.co/edit/43Sd9vsiAUsB7pTCUMEQ

I'm thinking we should, for each cord, check if there is a vertex in it, and if so, split the face in two using that vertex.
Any idea on how we could do it?

Three.js version
  • Dev
  • r82
  • ...
Browser
  • All of them
  • Chrome
  • Firefox
  • Internet Explorer
OS
  • All of them
  • Windows
  • Linux
  • Android
  • IOS
@Emanuele-Spatola

This comment has been minimized.

Copy link
Author

@Emanuele-Spatola Emanuele-Spatola commented Jan 6, 2017

@WestLangley you may be very helpful with this :)

@Emanuele-Spatola Emanuele-Spatola changed the title EdgesGeometry shows unnecessary edges EdgesGeometry shows unnecessary cords Jan 7, 2017
@WestLangley

This comment has been minimized.

Copy link
Collaborator

@WestLangley WestLangley commented Jan 7, 2017

The EdgesGeometry takes all the cords shared between faces and if the faces are not coplanar (or their "dot" is less than the thresholdDot) then it shows them as edges.

Right. But you have a long edge that is coincident with edges of two smaller triangles --coincident, but not shared.

That means, the long edge is not shared at all. Nor are the two short edges.

Edges that are not shared are rendered by EdgesGeometry.

@Emanuele-Spatola

This comment has been minimized.

Copy link
Author

@Emanuele-Spatola Emanuele-Spatola commented Jan 9, 2017

Right. But you have a long edge that is coincident with edges of two smaller triangles --coincident, but not shared.

Yes, that's why I suggested to split the triangle in two using the vertex of the coincident edge.

I updated the plunker adding EdgesGeometry2 that does that:
https://plnkr.co/edit/43Sd9vsiAUsB7pTCUMEQ

I'm sure the code can be optimised, but I think it will always be quite slow for complex geometries.

@WestLangley

This comment has been minimized.

Copy link
Collaborator

@WestLangley WestLangley commented Jan 9, 2017

I'm thinking we should, for each cord, check if there is a vertex in it, and if so, split the face in two using that vertex.

Sorry, I do not think it is the responsibility of EdgesGeometry to split faces in your geometry.

I also think that EdgesGeometry is doing exactly what it is supposed to do. I respect your alternate view, however.

@Emanuele-Spatola

This comment has been minimized.

Copy link
Author

@Emanuele-Spatola Emanuele-Spatola commented Jan 9, 2017

Well I think if a cord is not an edge it should't be shown.

I understand that splitting polygons may seem too much for the EdgeGeometry, but I also think it's wrong to split the geometry somewhere else to "make it good" for EdgeGeometry.

Maybe the the geometry I used is a particular case (it's the result of CSG operations), but it's a possible case and unless we say it's somehow an invalid geometry it should be handled correctly.

@WestLangley

This comment has been minimized.

Copy link
Collaborator

@WestLangley WestLangley commented Jan 9, 2017

Well I think if a cord is not an edge it should't be shown.

You are asking EdgesGeometry to render part of a triangle's edge.

unless we say it's somehow an invalid geometry it should be handled correctly.

I, too, do not think your geometry is "invalid". Maybe you can come up with an efficient algorithm to handle this use case.

@Wilt

This comment has been minimized.

Copy link
Contributor

@Wilt Wilt commented Feb 1, 2017

@Emanuele-Spatola Could you share this EdgesGeometry2 you are mentioning?
It seems not to be inside the plunker that you shared (both links direct to the same plunker).

I would like similar functionality. Could be nice to make a pull request for something like that. It could for example be called THREE.OutlinesGeometry

@Emanuele-Spatola

This comment has been minimized.

Copy link
Author

@Emanuele-Spatola Emanuele-Spatola commented Feb 1, 2017

@Wilt you are right, I shared the wrong link.
I created a new plunker, and called the new geometry as you suggested:
https://plnkr.co/edit/uEYo6L3pgbIaYXXzVzXd

I'd be happy to tidy the code up and create a pull request if the code is useful to other people.
My only concern is about the performance of the code, as it takes O(edges*vertices) to execute.

@WestLangley what do you think?

@Wilt

This comment has been minimized.

Copy link
Contributor

@Wilt Wilt commented Feb 1, 2017

@Emanuele-Spatola Thanks for sharing. I don't have time right now, but I will soon have a look at your code to see if I can come up with some possible optimizations.

@FishOrBear

This comment has been minimized.

Copy link
Contributor

@FishOrBear FishOrBear commented Aug 28, 2017

image
image
I think your proposal is useful.

@Mugen87 Mugen87 mentioned this issue Mar 18, 2018
3 of 13 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.