Protect EdgeWalker against bad input

src/Mod/TechDraw/App/AppTechDrawPy.cpp

@@ -134,6 +134,11 @@ private:
             throw Py::Exception(Part::PartExceptionOCCError, e->GetMessageString());
         }
 
+        if (edgeList.empty()) {
+            Base::Console().Log("LOG - edgeWalker: input is empty\n");
+            return Py::None();
+        }
+
         bool biggie;
         if (inclBig == Py_True) {
             biggie = true;
@@ -142,15 +147,23 @@ private:
         }
         PyObject* result = PyList_New(0);
 
-        EdgeWalker ew;
+        try {
-        ew.loadEdges(edgeList);
+            EdgeWalker ew;
-        ew.perform();
+            ew.loadEdges(edgeList);
-        std::vector<TopoDS_Wire> rw = ew.getResultNoDups();
+            bool success = ew.perform();
-        std::vector<TopoDS_Wire> sortedWires = ew.sortStrip(rw,biggie);   //false==>do not include biggest wires
+            if (success) {
-        for (auto& w:sortedWires) {
+                std::vector<TopoDS_Wire> rw = ew.getResultNoDups();
-            PyList_Append(result,new TopoShapeWirePy(new TopoShape(w)));
+                std::vector<TopoDS_Wire> sortedWires = ew.sortStrip(rw,biggie);   //false==>do not include biggest wires
+                for (auto& w:sortedWires) {
+                    PyList_Append(result,new TopoShapeWirePy(new TopoShape(w)));
+                }
+            } else {
+                Base::Console().Warning("edgeWalker: input is not planar graph. Wire detection not done\n");
+            }
+        }
+        catch (Base::Exception &e) {
+            throw Py::Exception(Base::BaseExceptionFreeCADError, e.what());
         }
-
         return Py::asObject(result);
     }
 
@@ -179,12 +192,31 @@ private:
             throw Py::Exception(Part::PartExceptionOCCError, e->GetMessageString());
         }
 
-        EdgeWalker ew;
+        if (edgeList.empty()) {
-        ew.loadEdges(edgeList);
+            Base::Console().Log("LOG - findOuterWire: input is empty\n");
-        ew.perform();
+            return Py::None();
-        std::vector<TopoDS_Wire> rw = ew.getResultNoDups();
+        }
-        std::vector<TopoDS_Wire> sortedWires = ew.sortStrip(rw,true);
+
-        PyObject* outerWire = new TopoShapeWirePy(new TopoShape(*sortedWires.begin()));
+        PyObject* outerWire = nullptr;
+        bool success = false;
+        try {
+            EdgeWalker ew;
+            ew.loadEdges(edgeList);
+            success = ew.perform();
+            if (success) {
+                std::vector<TopoDS_Wire> rw = ew.getResultNoDups();
+                std::vector<TopoDS_Wire> sortedWires = ew.sortStrip(rw,true);
+                outerWire = new TopoShapeWirePy(new TopoShape(*sortedWires.begin()));
+            } else {
+                Base::Console().Warning("findOuterWire: input is not planar graph. Wire detection not done\n");
+            }
+        }
+        catch (Base::Exception &e) {
+            throw Py::Exception(Base::BaseExceptionFreeCADError, e.what());
+        }
+        if (!success) {
+            return Py::None();
+        }
         return Py::asObject(outerWire);
     }
  };

src/Mod/TechDraw/App/DrawViewPart.cpp

@@ -198,7 +198,7 @@ App::DocumentObjectExecReturn *DrawViewPart::execute(void)
     }
     catch (Standard_Failure) {
         Handle_Standard_Failure e4 = Standard_Failure::Caught();
-        Base::Console().Log("LOG - DVP::execute - buildGeometryObject failed for %s - %s **\n",getNameInDocument(),e4->GetMessageString());
+        Base::Console().Log("LOG - DVP::execute - extractFaces failed for %s - %s **\n",getNameInDocument(),e4->GetMessageString());
         return new App::DocumentObjectExecReturn(e4->GetMessageString());
     }
 #endif //#if MOD_TECHDRAW_HANDLE_FACES
@@ -404,10 +404,21 @@ void DrawViewPart::extractFaces()
         faceEdges.insert(std::end(faceEdges), std::begin(edgesToAdd),std::end(edgesToAdd));
     }
 
+
+    if (faceEdges.empty()) {
+        Base::Console().Log("LOG - DVP::extractFaces - no faceEdges\n");
+        return;
+    }
+
+
 //find all the wires in the pile of faceEdges
     EdgeWalker ew;
     ew.loadEdges(faceEdges);
-    ew.perform();
+    bool success = ew.perform();
+    if (!success) {
+        Base::Console().Warning("DVP::extractFaces - input is not planar graph. No face detection\n");
+        return;
+    }
     std::vector<TopoDS_Wire> fw = ew.getResultNoDups();
 
     std::vector<TopoDS_Wire> sortedWires = ew.sortStrip(fw,true);

src/Mod/TechDraw/App/DrawViewSection.cpp

@@ -245,7 +245,9 @@ App::DocumentObjectExecReturn *DrawViewSection::execute(void)
                                             inputCenter,
                                             Direction.getValue(),
                                             validXDir);
-             builder.Add(newFaces,pFace);
+             if (!pFace.IsNull()) {
+                 builder.Add(newFaces,pFace);
+             }
 
         }
         sectionFaces = newFaces;
@@ -393,28 +395,37 @@ TopoDS_Face DrawViewSection::projectFace(const TopoDS_Shape &face,
 //        }
 //    }
 
+    TopoDS_Face projectedFace;
+
+    if (faceEdges.empty()) {
+        Base::Console().Log("LOG - DVS::projectFace - no faceEdges\n");
+        return projectedFace;
+    }
+
+
 //recreate the wires for this single face
     EdgeWalker ew;
     ew.loadEdges(faceEdges);
-    ew.perform();
+    bool success = ew.perform();
-    std::vector<TopoDS_Wire> fw = ew.getResultNoDups();
+    if (success) {
+        std::vector<TopoDS_Wire> fw = ew.getResultNoDups();
 
-    TopoDS_Face projectedFace;
+        if (!fw.empty()) {
+            std::vector<TopoDS_Wire> sortedWires = ew.sortStrip(fw, true);
+            if (sortedWires.empty()) {
+                return projectedFace;
+            }
 
-    if (!fw.empty()) {
+            BRepBuilderAPI_MakeFace mkFace(sortedWires.front(),true);                   //true => only want planes?
-        std::vector<TopoDS_Wire> sortedWires = ew.sortStrip(fw, true);
+            std::vector<TopoDS_Wire>::iterator itWire = ++sortedWires.begin();          //starting with second face
-        if (sortedWires.empty()) {
+            for (; itWire != sortedWires.end(); itWire++) {
-            return projectedFace;
+                mkFace.Add(*itWire);
+            }
+            projectedFace = mkFace.Face();
         }
-
+    } else {
-        BRepBuilderAPI_MakeFace mkFace(sortedWires.front(),true);                   //true => only want planes?
+        Base::Console().Warning("DVS::projectFace - input is not planar graph. No face detection\n");
-        std::vector<TopoDS_Wire>::iterator itWire = ++sortedWires.begin();          //starting with second face
-        for (; itWire != sortedWires.end(); itWire++) {
-            mkFace.Add(*itWire);
-        }
-        projectedFace = mkFace.Face();
     }
-
     return projectedFace;
 }
 

src/Mod/TechDraw/App/EdgeWalker.cpp

@@ -41,8 +41,10 @@
 #include <BRepGProp.hxx>
 
 #endif
+#include <sstream>
 
 #include <Base/Console.h>
+#include <Base/Exception.h>
 
 #include "DrawUtil.h"
 #include "EdgeWalker.h"
@@ -89,7 +91,8 @@ void edgeVisitor::setGraph(TechDraw::graph& g)
 //* EdgeWalker
 //*******************************************************
 
-EdgeWalker::EdgeWalker()
+EdgeWalker::EdgeWalker() :
+    duplicateInput(false)
 {
 }
 
@@ -97,6 +100,7 @@ EdgeWalker::~EdgeWalker()
 {
 }
 
+//loads a list of unique edges into the traversal mechanism
 bool EdgeWalker::loadEdges(std::vector<TechDraw::WalkerEdge> edges)
 {
     for (auto e: edges) {
@@ -107,10 +111,14 @@ bool EdgeWalker::loadEdges(std::vector<TechDraw::WalkerEdge> edges)
 
 bool EdgeWalker::loadEdges(std::vector<TopoDS_Edge> edges)
 {
+    if (edges.empty()) {
+        throw Base::Exception("EdgeWalker has no edges to load\n");
+    }
+
     std::vector<TopoDS_Vertex> verts = makeUniqueVList(edges);
     setSize(verts.size());
+
     std::vector<WalkerEdge>  we  = makeWalkerEdges(edges, verts);
-    saveInEdges = edges;
 
     return loadEdges(we);
 }
@@ -133,15 +141,45 @@ bool EdgeWalker::perform()
     for(boost::tie(ei, ei_end) = edges(m_g); ei != ei_end; ++ei)
       put(e_index, *ei, edge_count++);
 
-    // Test for planarity - we know it is planar, we just want to
+    // Test for planarity
-    // compute the planar embedding as a side-effect
     typedef std::vector< graph_traits<TechDraw::graph>::edge_descriptor > vec_t;
     std::vector<vec_t> embedding(num_vertices(m_g));
-    boyer_myrvold_planarity_test(boyer_myrvold_params::graph = m_g,
+    typedef std::vector< graph_traits<TechDraw::graph>::edge_descriptor > kura_edges_t;
-                                 boyer_myrvold_params::embedding = &embedding[0]);
+    kura_edges_t kEdges;
+    kura_edges_t::iterator ki, ki_end;
+    graph_traits<TechDraw::graph>::edge_descriptor e1;
+
+// Get the index associated with edge
+    graph_traits<TechDraw::graph>::edges_size_type
+        get(boost::edge_index_t,
+            const TechDraw::graph& m_g,
+            graph_traits<TechDraw::graph>::edge_descriptor edge);
+
+
+
+    bool isPlanar = boyer_myrvold_planarity_test(boyer_myrvold_params::graph = m_g,
+                                 boyer_myrvold_params::embedding = &embedding[0],
+                                 boyer_myrvold_params::kuratowski_subgraph =
+                                       std::back_inserter(kEdges));
+    if (!isPlanar) {
+        Base::Console().Log("LOG - EW::perform - input is NOT planar\n");
+        ki_end = kEdges.end();
+        std::stringstream ss;
+        ss << "EW::perform - obstructing edges: ";
+        for(ki = kEdges.begin(); ki != ki_end; ++ki)
+        {
+            e1 = *ki;
+            ss << boost::get(edge_index,m_g,e1) << ",";
+        }
+        ss << std::endl;
+        Base::Console().Log("LOG - %s\n",ss.str().c_str());
+        return false;
+    }
 
     m_eV.setGraph(m_g);
+    //Base::Console().Message("TRACE - EW::perform - setGraph complete\n");
     planar_face_traversal(m_g, &embedding[0], m_eV);
+    //Base::Console().Message("TRACE - EW::perform - traversal complete\n");
 
     return true;
 }
@@ -161,6 +199,16 @@ std::vector<TopoDS_Wire> EdgeWalker::getResultWires()
         return fw;
     }
 
+     //convert from noduplicate index to duplicates index
+     for (auto& w:result.wires) {
+        for (auto& we:w.wedges) {
+            //we.idx is the edge index in the short list (no duplicates)
+            //saveIndex[we.idx] should be the index in the long list
+            we.idx = saveIndex[we.idx];
+        }
+    }
+
+
     std::vector<ewWire>::iterator iWire = result.wires.begin();     // a WE within [WE]
     for (;iWire != result.wires.end(); iWire++) {
         std::vector<WalkerEdge>::iterator iEdge = (*iWire).wedges.begin();
@@ -182,15 +230,26 @@ std::vector<TopoDS_Wire> EdgeWalker::getResultNoDups()
     if (result.wires.empty()) {
         return fw;
     }
+
+    //convert from noduplicate index to duplicates index
+    for (auto& w:result.wires) {
+        for (auto& we:w.wedges) {
+            //we.idx is the edge index in the short list (no duplicates)
+            //saveIndex[we.idx] should be the index in the long list
+            we.idx = saveIndex[we.idx];
+        }
+    }
     result = result.removeDuplicates();
 
     std::vector<ewWire>::iterator iWire = result.wires.begin();
+    int edgeCount = 1;
     for (;iWire != result.wires.end(); iWire++) {
         std::vector<WalkerEdge>::iterator iEdge = (*iWire).wedges.begin();
         std::vector<TopoDS_Edge> topoEdges;
         for (;iEdge != (*iWire).wedges.end(); iEdge++) {
             TopoDS_Edge e = saveInEdges.at((*iEdge).idx);
             topoEdges.push_back(e);
+            edgeCount++;
         }
     TopoDS_Wire w = makeCleanWire(topoEdges);             //make 1 clean wire from its edges
     fw.push_back(w);
@@ -253,21 +312,30 @@ std::vector<TopoDS_Vertex> EdgeWalker:: makeUniqueVList(std::vector<TopoDS_Edge>
 }
 
 //!make WalkerEdges (unique Vertex index pairs) from edge list
+//remove duplicate edges from input
 std::vector<WalkerEdge> EdgeWalker::makeWalkerEdges(std::vector<TopoDS_Edge> edges,
                                                       std::vector<TopoDS_Vertex> verts)
 {
-    std::vector<WalkerEdge> walkerEdges;
+    saveInEdges = edges;
+    std::vector<WalkerEdge> rawList;
     for (auto e:edges) {
         TopoDS_Vertex ev1 = TopExp::FirstVertex(e);
         TopoDS_Vertex ev2 = TopExp::LastVertex(e);
         int v1dx = findUniqueVert(ev1, verts);
         int v2dx = findUniqueVert(ev2, verts);
-        WalkerEdge we;
+        WalkerEdge rl;
-        we.v1 = v1dx;
+        rl.v1 = v1dx;
-        we.v2 = v2dx;
+        rl.v2 = v2dx;
-        walkerEdges.push_back(we);
+        rawList.push_back(rl);
     }
-    return walkerEdges;
+
+    std::vector<WalkerEdge> we = removeDuplicateInput(rawList);
+    for (auto& w:we)
+    {
+        saveIndex.push_back(w.idx);
+    }
+
+    return we;
 }
 
 int EdgeWalker::findUniqueVert(TopoDS_Vertex vx, std::vector<TopoDS_Vertex> &uniqueVert)
@@ -284,7 +352,50 @@ int EdgeWalker::findUniqueVert(TopoDS_Vertex vx, std::vector<TopoDS_Vertex> &uni
     return result;
 }
 
-/*static*/ bool WalkerEdge::weCompare(WalkerEdge i, WalkerEdge j)
+//removes duplicates from input and sets idx to position in original list
+std::vector<WalkerEdge> EdgeWalker::removeDuplicateInput(std::vector<WalkerEdge> input)
+{
+    std::vector<WalkerEdge> result;
+    //std::vector<int> ref;
+    if (input.empty()) {
+        return result;
+    }
+
+    result.push_back(*(input.begin()));                           //save the first WE
+    result[0].idx = 0;
+    //ref.push_back(0);
+    std::vector<WalkerEdge>::iterator iWE = (input.begin()) + 1;    //starting with second
+    int i = 1;
+    for (; iWE != input.end(); iWE++, i++) {
+        bool addToResult = true;
+        for (auto& w:result) {
+            if ((*iWE).isEqual(w))  {             //already in result?
+                addToResult = false;
+                Base::Console().Log("LOG - EW::removeDuplicateInput - input edge: %d is a duplicate\n",i);
+                break;
+            }
+        }
+        if (addToResult) {
+            (*iWE).idx = i;
+            result.push_back((*iWE));
+            //ref.push_back(i);
+        }
+    }
+    return result;
+}
+
+bool WalkerEdge::isEqual(WalkerEdge w)
+{
+    bool result = false;
+    if ((( v1 == w.v1) && (v2 == w.v2))  ||
+        (( v1 == w.v2) && (v2 == w.v1)) ) {
+        result = true;
+    }
+    return result;
+}
+
+
+/*static*/ bool WalkerEdge::weCompare(WalkerEdge i, WalkerEdge j)    //used for sorting
 {
     return (i.idx < j.idx);
 }

src/Mod/TechDraw/App/EdgeWalker.h

@@ -36,6 +36,7 @@
 #include <boost/graph/graph_traits.hpp>
 #include <boost/property_map/property_map.hpp>
 #include <boost/graph/boyer_myrvold_planar_test.hpp>
+#include <boost/graph/is_kuratowski_subgraph.hpp>
 #include <boost/graph/planar_face_traversal.hpp>
 #include <boost/ref.hpp>
 
@@ -59,6 +60,7 @@ class WalkerEdge
 {
 public:
     static bool weCompare(WalkerEdge i, WalkerEdge j);
+    bool isEqual(WalkerEdge w);
 
     std::size_t v1;
     std::size_t v2;
@@ -119,6 +121,8 @@ public:
     std::vector<TopoDS_Vertex> makeUniqueVList(std::vector<TopoDS_Edge> edges);
     std::vector<WalkerEdge>    makeWalkerEdges(std::vector<TopoDS_Edge> edges,
                                                std::vector<TopoDS_Vertex> verts);
+    std::vector<WalkerEdge> removeDuplicateInput(std::vector<WalkerEdge> input);
+
     int findUniqueVert(TopoDS_Vertex vx, std::vector<TopoDS_Vertex> &uniqueVert);
     std::vector<TopoDS_Wire> sortStrip(std::vector<TopoDS_Wire> fw, bool includeBiggest);
     std::vector<TopoDS_Wire> sortWiresBySize(std::vector<TopoDS_Wire>& w, bool reverse = false);
@@ -128,10 +132,12 @@ public:
 protected:
     static bool wireCompare(const TopoDS_Wire& w1, const TopoDS_Wire& w2);
     std::vector<TopoDS_Edge> saveInEdges;
+    std::vector<int> saveIndex;
 
 private:
     edgeVisitor m_eV;
     TechDraw::graph m_g;
+    bool duplicateInput;
 };
 
 }  //end namespace TechDraw