From 64774bc1fec34eda5c24bdd7f54ab3447cc4b813 Mon Sep 17 00:00:00 2001 From: Abdullah Tahiri Date: Thu, 24 Dec 2020 11:54:18 +0100 Subject: [PATCH] Sketcher: Fix crash on constraint rename ======================================== Report: https://github.com/FreeCAD/FreeCAD/pull/4183 https://github.com/realthunder/FreeCAD_assembly3/issues/387 Problem: renameConstraint() previously implemented exclusively in SketchObjectPyImp.cpp, will change the Constraints property without updating the solver. A prospective drag operation would rely on a deleted pointer constraint which leads to the crash. Solution: - mark the solver status as needing an update - leverage new through sketchobject r/w interface to ensure solver is synchronised before the temporary move operation starts Bonus: move the core of the function to SketchObject.cpp so that input data validity check on constraint change is inhibited. --- src/Mod/Sketcher/App/SketchObject.cpp | 21 ++++++++++++++++ src/Mod/Sketcher/App/SketchObject.h | 29 ++++++++++++++++++---- src/Mod/Sketcher/App/SketchObjectPyImp.cpp | 10 ++------ 3 files changed, 47 insertions(+), 13 deletions(-) diff --git a/src/Mod/Sketcher/App/SketchObject.cpp b/src/Mod/Sketcher/App/SketchObject.cpp index 7ba4d567ed..39acd7a882 100644 --- a/src/Mod/Sketcher/App/SketchObject.cpp +++ b/src/Mod/Sketcher/App/SketchObject.cpp @@ -8002,6 +8002,27 @@ int SketchObject::autoRemoveRedundants(bool updategeo) return redundants.size(); } +int SketchObject::renameConstraint(int GeoId, std::string name) +{ + // only change the constraint item if the names are different + const Constraint* item = Constraints[GeoId]; + + if (item->Name != name) { + Base::StateLocker lock(managedoperation, true); // no need to check input data validity as this is an sketchobject managed operation. + + Constraint* copy = item->clone(); + copy->Name = name; + + Constraints.set1Value(GeoId, copy); + delete copy; + + solverNeedsUpdate = true; // make sure any prospective solver access updates the constraint pointer that just got invalidated + + return 0; + } + return -1; +} + std::vector SketchObject::getOpenVertices(void) const { std::vector points; diff --git a/src/Mod/Sketcher/App/SketchObject.h b/src/Mod/Sketcher/App/SketchObject.h index 66050695a0..28356326df 100644 --- a/src/Mod/Sketcher/App/SketchObject.h +++ b/src/Mod/Sketcher/App/SketchObject.h @@ -361,11 +361,12 @@ public: /* Solver exposed interface */ inline void setRecalculateInitialSolutionWhileMovingPoint(bool recalculateInitialSolutionWhileMovingPoint) {solvedSketch.setRecalculateInitialSolutionWhileMovingPoint(recalculateInitialSolutionWhileMovingPoint);} /// Forwards a request for a temporary initMove to the solver using the current sketch state as a reference (enables dragging) - inline int initTemporaryMove(int geoId, PointPos pos, bool fine=true) - { return solvedSketch.initMove(geoId,pos,fine);} - /// Forwards a request for point or curve temporary movement to the solver using the current state as a reference (enables dragging) - inline int moveTemporaryPoint(int geoId, PointPos pos, Base::Vector3d toPoint, bool relative=false) - { return solvedSketch.movePoint(geoId, pos, toPoint, relative);} + inline int initTemporaryMove(int geoId, PointPos pos, bool fine=true); + /** Forwards a request for point or curve temporary movement to the solver using the current state as a reference (enables dragging). + * NOTE: A temporary move operation must always be preceded by a initTemporaryMove() operation. + */ + inline int moveTemporaryPoint(int geoId, PointPos pos, Base::Vector3d toPoint, bool relative=false); + /// forwards a request to update an extension of a geometry of the solver to the solver. inline void updateSolverExtension(int geoId, std::unique_ptr && ext) { return solvedSketch.updateExtension(geoId, std::move(ext));} @@ -436,6 +437,8 @@ public: /// returns the number of redundant constraints detected int autoRemoveRedundants(bool updategeo = true); + int renameConstraint(int GeoId, std::string name); + // Validation routines std::vector getOpenVertices(void) const; @@ -527,6 +530,22 @@ private: bool managedoperation; // indicates whether changes to properties are the deed of SketchObject or not (for input validation) }; +inline int SketchObject::initTemporaryMove(int geoId, PointPos pos, bool fine/*=true*/) +{ + // if a previous operation did not update the geometry (including geometry extensions) + // or constraints (including any deleted pointer, as in renameConstraint) of the solver, + // here we update them before starting a temporary operation. + if(solverNeedsUpdate) + solve(); + + return solvedSketch.initMove(geoId,pos,fine); +} + +inline int SketchObject::moveTemporaryPoint(int geoId, PointPos pos, Base::Vector3d toPoint, bool relative/*=false*/) +{ + return solvedSketch.movePoint(geoId, pos, toPoint, relative); +} + typedef App::FeaturePythonT SketchObjectPython; } //namespace Sketcher diff --git a/src/Mod/Sketcher/App/SketchObjectPyImp.cpp b/src/Mod/Sketcher/App/SketchObjectPyImp.cpp index 7cfd04577a..82104c4d4e 100644 --- a/src/Mod/Sketcher/App/SketchObjectPyImp.cpp +++ b/src/Mod/Sketcher/App/SketchObjectPyImp.cpp @@ -417,14 +417,8 @@ PyObject* SketchObjectPy::renameConstraint(PyObject *args) } } - // only change the constraint item if the names are different - const Constraint* item = this->getSketchObjectPtr()->Constraints[Index]; - if (item->Name != Name) { - Constraint* copy = item->clone(); - copy->Name = Name; - this->getSketchObjectPtr()->Constraints.set1Value(Index, copy); - delete copy; - } + this->getSketchObjectPtr()->renameConstraint(Index, Name); + Py_Return; }