Skip to content

Commit

Permalink
fix(face): Fix issue with duplicate Face3D vertices and tolerance
Browse files Browse the repository at this point in the history
It seems that duplicate vertices with coordinate differences very close to Python floating point tolerance can cause the normal calculation to go haywire.  I am changing the normal calculation to ignore any vertices that are closer than a value that's a bit larger than floating point tolerance (1e-9) rather than using a ZeroDivisionError indicating a perfectly duplicate vertex.

I also realized that the remove_colinear_vertices method can accidentally remove both versions of a duplicate vertex, which is usually not the desired behavior as we want to leave one of them. I'm tweaking the method so that it corrects itself whenever it has already removed colinear vertices.
  • Loading branch information
chriswmackey authored and Chris Mackey committed Jan 26, 2021
1 parent 8c2281a commit e4a3d10
Show file tree
Hide file tree
Showing 3 changed files with 114 additions and 7 deletions.
18 changes: 11 additions & 7 deletions ladybug_geometry/geometry3d/face.py
Original file line number Diff line number Diff line change
Expand Up @@ -704,7 +704,7 @@ def remove_colinear_vertices(self, tolerance):
return self
if not self.has_holes: # we only need to evaluate one list of vertices
new_vertices = self._remove_colinear(
self._vertices, self.polygon2d, tolerance)
self._vertices, self.polygon2d, tolerance)
_new_face = Face3D(new_vertices, self.plane, enforce_right_hand=False)
return _new_face
# the face has holes
Expand Down Expand Up @@ -1738,11 +1738,15 @@ def _remove_colinear(self, pts_3d, pts_2d, tolerance):
triangle formed by 3 vertices is less than the tolerance.
"""
new_vertices = []
skip = 0
for i, _v in enumerate(pts_2d):
_a = pts_2d[i - 2].determinant(pts_2d[i - 1]) + \
pts_2d[i - 1].determinant(_v) + _v.determinant(pts_2d[i - 2])
_a = pts_2d[i - 2 - skip].determinant(pts_2d[i - 1]) + \
pts_2d[i - 1].determinant(_v) + _v.determinant(pts_2d[i - 2 - skip])
if abs(_a) >= tolerance:
new_vertices.append(pts_3d[i - 1])
skip = 0
else:
skip += 1
return new_vertices

def _is_sub_face(self, face):
Expand Down Expand Up @@ -1913,6 +1917,7 @@ def _plane_from_vertices(verts):
normal = [x1 + x2, y1 + y2, z1 + z2]
# walk around the whole shape to avoid colinear vertices
for i in range(len(verts) - 2):
tri_verts = verts[i:i + 3]
x, y, z = Face3D._normal_from_3pts(*verts[i:i + 3])
normal[0] += x
normal[1] += y
Expand All @@ -1936,11 +1941,10 @@ def _normal_from_3pts(pt1, pt2, pt3):
# normalize the vectors to make them unit vectors
d1 = math.sqrt(v1[0] ** 2 + v1[1] ** 2 + v1[2] ** 2)
d2 = math.sqrt(v2[0] ** 2 + v2[1] ** 2 + v2[2] ** 2)
try:
v1 = (v1[0] / d1, v1[1] / d1, v1[2] / d1)
v2 = (v2[0] / d2, v2[1] / d2, v2[2] / d2)
except ZeroDivisionError: # duplicate vertices in triplet
if d1 < 1e-9 or d2 < 1e-9: # effectively duplicate vertices in triplet
return (0, 0, 0)
v1 = (v1[0] / d1, v1[1] / d1, v1[2] / d1)
v2 = (v2[0] / d2, v2[1] / d2, v2[2] / d2)
# return the cross product
return (v1[1] * v2[2] - v1[2] * v2[1],
-v1[0] * v2[2] + v1[2] * v2[0],
Expand Down
13 changes: 13 additions & 0 deletions tests/face3d_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -602,6 +602,19 @@ def test_remove_colinear_vertices():
assert len(face_2.remove_colinear_vertices(0.0001).vertices) == 4


def test_remove_colinear_vertices_custom():
"""Test the remove_colinear_vertices method with some custom geometry."""
geo_file = './tests/json/dup_vert_face.json'
with open(geo_file, 'r') as fp:
geo_dict = json.load(fp)
face_geo = Face3D.from_dict(geo_dict)

assert -1e-3 < face_geo.normal.z < 1e-3
assert len(face_geo.vertices) == 17

assert len(face_geo.remove_colinear_vertices(0.0001).vertices) == 16


def test_triangulated_mesh_and_centroid():
"""Test the triangulation properties of Face3D."""
pts_1 = (Point3D(0, 0, 2), Point3D(2, 0, 2), Point3D(2, 2, 2), Point3D(0, 2, 2))
Expand Down
90 changes: 90 additions & 0 deletions tests/json/dup_vert_face.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
{
"type": "Face3D",
"boundary": [
[
-32.921611201331253,
-412.66628343081106,
1.5082130414885986
],
[
-32.92161450752139,
-412.66628142228097,
0.79654637483244661
],
[
-33.537845196710784,
-413.8757310807286,
0.79654582428383403
],
[
-33.537842348260362,
-413.8757221976216,
-0.0047076652449124547
],
[
-22.359531583272815,
-391.95842979489032,
-0.0044249400893101778
],
[
-22.359554793487792,
-391.95850217769248,
6.5244813094684488
],
[
-33.537865558475346,
-413.87579458042376,
6.5241985843128463
],
[
-33.537858287046141,
-413.8757719039246,
4.4787854075706282
],
[
-32.921597400939412,
-412.66629181463094,
4.4787859581113754
],
[
-32.921600013420004,
-412.66629022753347,
3.9164422081196832
],
[
-33.537856287918139,
-413.87576566949343,
3.9164416575777357
],
[
-33.537858044533692,
-413.87576395490083,
3.8528999909160246
],
[
-32.921600308615552,
-412.66629004820044,
3.8529005414539577
],
[
-32.921610906135705,
-412.66628361014409,
1.5717547081543268
],
[
-32.921610906135712,
-412.66628361014409,
1.5717547081543268
],
[
-33.537857160655768,
-413.87573498333205,
1.5717541576266509
],
[
-33.537847726680695,
-413.87573897063021,
1.5082124909415064
]
]
}

0 comments on commit e4a3d10

Please sign in to comment.