compas-dev / compas

Core packages of the COMPAS framework.
https://compas.dev/compas/
MIT License
309 stars 105 forks source link

bug in compas.geometry orient_points #53

Closed juney-lee closed 5 years ago

juney-lee commented 6 years ago
from compas.geometry import orient_points

pts = [(1, 1, 0), (-1, 1, 0), (-1, -1, 0), (1, -1, 0)]
ref_plane = [(0, 0, 0),(0, 0, 1)]
print orient_points(pts, ref_plane)

The result of the above code is the same as the input pts as expected, since the default target plane is the global xy plane, or [(0,0,0), (0,0,1)], which in this case is the same as the reference plane. However, the function complains when the ref_plane is facing downward in negative z direction:

from compas.geometry import orient_points

pts = [(1, 1, 0), (-1, 1, 0), (-1, -1, 0), (1, -1, 0)]
ref_plane = [(0, 0, 0),(0, 0, -1)]
print orient_points(pts, ref_plane)

The expected result should be a reversed list of pts. However, it raises the following error:

"Message: Attempted to divide by zero.

Traceback: line 354, in normalize_vector, "C:\compas-dev\compas\src\compas\geometry\basic.py" line 279, in rotation_matrix, "C:\compas-dev\compas\src\compas\geometry\transformations.py" line 414, in rotate_points, "C:\compas-dev\compas\src\compas\geometry\transformations.py" line 684, in orient_points, "C:\compas-dev\compas\src\compas\geometry\transformations.py"

It seems like it's complaining because the normals of the reference and target planes are pointing in opposite directions, which means the length of the vector is zero, and therefore cannot be normalized (divide by zero)?

tomvanmele commented 6 years ago

looking at the implementation of the function, there is indeed a problem if the reference plane and the target plane are parallel.

the function finds the axis for rotating the points by computing the cross product of the normals of the planes. the rotation axis is normalised as part of this process, which is where the function seems to fail.

indeed the normalisation function does not handle the case properly where the vector is [0, 0, 0].

this is thus a bug of the normalisation function rather than of the current function. i will open a different issue for this...

thanks, tom

Rippmann commented 5 years ago

Why is this issue closed? I am using the orient_points a lot and it is commented out in the current version. Where is the issue regarding the normalisation function? any updates?

tomvanmele commented 5 years ago

added orient_points to transformations. should be accessible from compas.geometry. updated the example and added juney's boundary cases.

only change is that the ref and target planes are not optional anymore...

tomvanmele commented 5 years ago

can you guys check and close if resolved please

Rippmann commented 5 years ago

Thanks! Will check! Sometimes orient points using two planes is not enough. you would want to use frames (so a point and two perpendicular vectors) to orient points explicitly. Do we have something like this already in compas?

tomvanmele commented 5 years ago

no, i just rewrote the original orient function, which used simple planes. easy to add though...

tomvanmele commented 5 years ago

please add a separate feature request for the frame based orientation. will close this issue as the original problem is solved...