WIP : fix PMP::compute_vertex_normal#2065
Conversation
the new weight is sin(angle at v) it is cheaper than computing (angle at v), and should behave similarly
janetournois
changed the title
PMP::compute_vertex_normal
| get(vpmap, target(next(he, pmesh), pmesh))); | ||
| //v(i) and v(i+1) must me seen in ccw order, from v, so we reverse v1 and v2 | ||
| Vector ni = traits.construct_cross_product_vector_3_object()(v2, v1); | ||
| ni = traits.construct_scaled_vector_3_object()(ni, CGAL::sqrt(1./(sqlen(v1)*sqlen(v2)))); |
There was a problem hiding this comment.
The expression CGAL::sqrt(1./(sqlen(v1)*sqlen(v2))) does not compile with Gmpq as the FT. That error was detected by Travis: https://travis-ci.org/CGAL/cgal/jobs/223571577#L4670
Did you get a mail from Travis, by the way?
There was a problem hiding this comment.
right. I should use CGAL::approximate_sqrt.
Just a remark : the documentation says that, even with an exact kernel, the sqrt inside will involve non exact computations.
And No, I did not get an email from Travis.
There was a problem hiding this comment.
You should also write FT(1)/x instead of 1./x, so that it compiles with number types that cannot be constructed from a double.
|
This PR conflicts with |
|
@janetournois, do you know why this pull-request was left unattended? It seems there was a discussion, and then... I do not know. |
|
Yes I remember. Imho it's not obsolete and the implemented solution should be improved at some point. |
|
Note that @MaelRL has a very good method implemented for another project and he will at some point export it properly. |
|
Sounds good! |
4 participants
This PR should be discussed in the issue before going further.
Summary of Changes
This PR intends to improve the
PMP::compute_vertex_normal(v)function, by changing the weights of face normals that are summed to compute a normal at vertexv.The former implementation used
1as weight, and the new weight issin(angle at v in f).It is cheaper than computing
(angle at v in f), and should behave similarly.Release Management