search

mrdoob / three.js

JavaScript 3D Library.
https://threejs.org/
MIT License
102.03k stars 35.33k forks source link

EdgesGeometry shows unnecessary cords #10517

Open Emanuele-Spatola opened 7 years ago

Emanuele-Spatola commented 7 years ago
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'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
Browser
OS
Emanuele-Spatola commented 7 years ago

@WestLangley you may be very helpful with this :)

WestLangley commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

@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 commented 7 years ago

@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 commented 7 years ago

image image I think your proposal is useful.

stephan157 commented 3 years ago

@Emanuele-Spatola Is there also a version of the OutlinesGeometry which works with the current version of THREE.js? As I don't have that deep knowledge of THREE.js I don't know how I can rewrite the code to make it work with the current version.