fix imported shapes validity (#30)

* add shapes validity tests

* fix wire closing detection and enforcement

* add edge case test

* bump version

ocpsvg/ocp.py

@@ -4,6 +4,7 @@
 from typing import Iterable, Iterator, Optional, Union, cast
 
 from OCP.Bnd import Bnd_Box
+from OCP.BRep import BRep_Tool
 from OCP.BRepAdaptor import BRepAdaptor_Curve
 from OCP.BRepBndLib import BRepBndLib
 from OCP.BRepBuilderAPI import BRepBuilderAPI_MakeEdge, BRepBuilderAPI_MakeFace
@@ -43,7 +44,6 @@
     gp_Trsf,
     gp_Vec,
 )
-from OCP.ShapeAnalysis import ShapeAnalysis_Wire
 from OCP.ShapeExtend import ShapeExtend_WireData
 from OCP.ShapeFix import ShapeFix_Face
 from OCP.Standard import Standard_Failure
@@ -120,10 +120,10 @@ def face_from_wires(
     outer_wire: TopoDS_Wire, inner_wires: Optional[Iterable[TopoDS_Wire]] = None
 ) -> TopoDS_Face:
     """Make a face from an outer wire and optional inner wire(s)."""
-    face_builder = BRepBuilderAPI_MakeFace(outer_wire, True)
+    face_builder = BRepBuilderAPI_MakeFace(closed_wire(outer_wire), True)
     if inner_wires:
         for inner_wire in inner_wires:
-            face_builder.Add(inner_wire)
+            face_builder.Add(closed_wire(inner_wire))
 
     face_fix = ShapeFix_Face(face_builder.Face())
     face_fix.FixOrientation()
@@ -191,8 +191,7 @@ def fix_wires():
 
 def is_wire_closed(wire: TopoDS_Wire) -> bool:
     """Check whether a wire is closed."""
-    a = ShapeAnalysis_Wire(wire, BRepBuilderAPI_MakeFace(wire).Face(), _TOLERANCE)
-    return a.CheckClosed()
+    return BRep_Tool.IsClosed_s(wire)
 
 
 def are_wires_coplanar(wires: Iterable[TopoDS_Wire]) -> bool:
@@ -212,7 +211,7 @@ def wire_from_continuous_edges(
     for edge in edges:
         extend.AddOriented(edge, 0)
 
-    wire = extend.Wire()
+    wire = extend.WireAPIMake()
     return closed_wire(wire) if closed else wire
 
 
@@ -226,15 +225,14 @@ def closed_wire(wire: TopoDS_Wire) -> TopoDS_Wire:
 
     it = topoDS_iterator(wire)
     try:
-        first_edge = next(it)
-        last_edge = next(it)
+        first_edge = last_edge = next(it)
         while True:
             try:
                 last_edge = next(it)
             except StopIteration:
                 break
     except StopIteration:
-        # wire has fewer that 2 edges
+        # wire has no edges
         return wire
 
     adaptor = BRepAdaptor_Curve(TopoDS.Edge_s(first_edge))
@@ -247,7 +245,7 @@ def closed_wire(wire: TopoDS_Wire) -> TopoDS_Wire:
         extend = ShapeExtend_WireData()
         extend.AddOriented(wire, 0)
         extend.AddOriented(edge_from_curve(segment_curve(end, start)), 0)
-        wire = extend.Wire()
+        wire = extend.WireAPIMake()
 
     return wire
 

ocpsvg/svg.py

@@ -787,5 +787,5 @@ def _continuous_subpaths(
 ) -> Iterator[tuple[svgelements.Path, bool]]:
     for subpath in path.as_subpaths():
         if subpath:
-            is_closed = bool(subpath[0] == subpath[-1])  # type: ignore
+            is_closed = isinstance(subpath[-1], svgelements.Close)
             yield svgelements.Path(subpath), is_closed

pyproject.toml

@@ -2,7 +2,7 @@
 [project]
 name = "ocpsvg"
 readme = "readme-pypi.md"
-version = "0.3.1"
+version = "0.3.2"
 requires-python = ">=3.9"
 dependencies = [
     "cadquery-ocp >= 7.7.0",

tests/ocp.py

@@ -1,14 +1,14 @@
+from OCP.BRepCheck import BRepCheck_Analyzer
 from OCP.BRepGProp import BRepGProp, BRepGProp_Face
 from OCP.BRepTools import BRepTools
 from OCP.gp import gp_Pnt, gp_Vec
 from OCP.GProp import GProp_GProps
-from OCP.TopoDS import TopoDS_Face
+from OCP.TopoDS import TopoDS_Face, TopoDS_Shape
 
 
 def face_area(face: TopoDS_Face):
     properties = GProp_GProps()
     BRepGProp.SurfaceProperties_s(face, properties)
-
     return properties.Mass()
 
 
@@ -22,3 +22,9 @@ def face_normal_at_uv(face: TopoDS_Face, u: float, v: float) -> gp_Vec:
     normal = gp_Vec()
     BRepGProp_Face(face).Normal(u, v, gp_pnt, normal)
     return normal
+
+
+def is_valid(shape: TopoDS_Shape):
+    check = BRepCheck_Analyzer(shape)
+    check.SetParallel(True)
+    return check.IsValid()

tests/test_ocp.py

@@ -3,6 +3,8 @@
 import pytest
 from OCP.Geom import Geom_BezierCurve, Geom_Curve, Geom_TrimmedCurve
 from OCP.gp import gp_Pnt, gp_Vec
+from OCP.ShapeExtend import ShapeExtend_WireData
+from OCP.TopoDS import TopoDS_Wire
 from pytest import approx, raises
 
 from ocpsvg.ocp import (
@@ -179,12 +181,9 @@ def test_closed_wire_already_closed():
     assert closed_wire(loop) == loop
 
 
-def test_closed_wire_single_segment_noop():
-    a = gp_Pnt(0, 0, 0)
-    b = gp_Pnt(10, 0, 0)
-    incomplete_loop = polyline_wire(a, b)
-    incomplete_loop_fixed = closed_wire(incomplete_loop)
-    assert incomplete_loop_fixed == incomplete_loop
+def test_closed_wire_empty():
+    wire = ShapeExtend_WireData(TopoDS_Wire()).Wire()
+    assert closed_wire(wire) == wire
 
 
 def test_face_from_wire_soup_winding():

tests/test_svg.py

@@ -45,7 +45,7 @@
     wires_from_svg_path,
 )
 
-from .ocp import face_area, face_normal
+from .ocp import face_area, face_normal, is_valid
 from .test_ocp import XY, Pnt, as_Pnt, as_Pnts, as_tuple
 
 
@@ -192,6 +192,7 @@ def test_path_regression1():
     """
     wires = list(wires_from_svg_path(d))
     assert len(wires) == 1
+    assert is_valid(wires[0])
     assert is_wire_closed(wires[0])
 
 
@@ -222,6 +223,7 @@ def test_path_regression2():
     """
     wires = list(wires_from_svg_path(d))
     assert len(wires) == 1
+    assert is_valid(wires[0])
     assert is_wire_closed(wires[0])
 
 
@@ -242,6 +244,7 @@ def test_path_regression3():
     """
     faces = list(faces_from_svg_path(d))
     assert len(faces) == 1
+    assert is_valid(faces[0])
     assert len(face_inner_wires(faces[0])) == 3
 
 
@@ -306,13 +309,15 @@ def test_invalid_path(svg_d: str):
         ("M 0,0 v 1 h 1 z", 1),
         ("M 0,0 v 1 h 1", 1),
         ("M 10 80 Q 95 10 180 80", 1),
-        ("M 10 80 C 40 10, 65 10, 95 80 S 150 150, 180 80", 1),
+        ("M 10 80 C 40 10, 65 10, 95 80 S 150 150, 60 80", 1),
     ],
 )
 def test_faces_from_svg_path(svg_d: str, expected_count: int):
     res = list(faces_from_svg_path(svg_d))
     assert len(res) == expected_count
-    assert all(isinstance(x, TopoDS_Face) for x in res)
+    for face in res:
+        assert isinstance(face, TopoDS_Face)
+        assert is_valid(face)
 
 
 @pytest.mark.parametrize(
@@ -327,7 +332,9 @@ def test_faces_from_svg_path(svg_d: str, expected_count: int):
 def test_edges_from_svg_path(svg_d: str, expected_count: int):
     res = list(edges_from_svg_path(svg_d))
     assert len(res) == expected_count
-    assert all(isinstance(x, TopoDS_Edge) for x in res)
+    for edge in res:
+        assert isinstance(edge, TopoDS_Edge)
+        assert is_valid(edge)
 
 
 @pytest.mark.parametrize(
@@ -342,7 +349,9 @@ def test_edges_from_svg_path(svg_d: str, expected_count: int):
 def test_wires_from_svg_path(svg_d: str, expected_count: int):
     res = list(wires_from_svg_path(svg_d))
     assert len(res) == expected_count
-    assert all(isinstance(x, TopoDS_Wire) for x in res)
+    for wire in res:
+        assert isinstance(wire, TopoDS_Wire)
+        assert is_valid(wire)
 
 
 @pytest.mark.parametrize("count", range(3, 10))
@@ -354,6 +363,7 @@ def test_concentric_path_nesting(count: int):
         expected_hole_counts = [1] * n
     res = list(faces_from_svg_path(nested_squares_path(count)))
     assert hole_counts(res) == expected_hole_counts
+    assert all(map(is_valid, res))
 
 
 def test_path_nesting():
@@ -396,6 +406,7 @@ def test_path_nesting():
 
     expected_hole_counts = [0, 0, 1, 2]
     assert hole_counts(res) == expected_hole_counts
+    assert all(map(is_valid, res))
 
 
 def test_svg_doc_from_file():
@@ -437,6 +448,7 @@ def test_svg_doc_shapes():
     buf = StringIO(svg_src)
     imported = list(import_svg_document(buf))
     assert len(imported) == 3
+    assert all(map(is_valid, imported))
 
 
 @pytest.mark.parametrize("flip_y", [False, True])
@@ -508,6 +520,7 @@ def test_svg_doc_ignore_visibility():
     buf = StringIO(svg_src)
     imported = list(import_svg_document(buf, ignore_visibility=True))
     assert len(imported) == 2
+    assert all(map(is_valid, imported))
 
 
 @pytest.mark.parametrize("flip_y", [False, True])