Skip to content

Commit

Permalink
Sketcher: Fix crash on constraint rename
Browse files Browse the repository at this point in the history
========================================

Report:
#4183
realthunder/FreeCAD_assembly3#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.
  • Loading branch information
abdullahtahiriyo committed Dec 27, 2020
1 parent 1d56289 commit 64774bc
Show file tree
Hide file tree
Showing 3 changed files with 47 additions and 13 deletions.
21 changes: 21 additions & 0 deletions src/Mod/Sketcher/App/SketchObject.cpp
Expand Up @@ -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<Base::Vector3d> SketchObject::getOpenVertices(void) const
{
std::vector<Base::Vector3d> points;
Expand Down
29 changes: 24 additions & 5 deletions src/Mod/Sketcher/App/SketchObject.h
Expand Up @@ -361,11 +361,12 @@ class SketcherExport SketchObject : public Part::Part2DObject
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<Part::GeometryExtension> && ext)
{ return solvedSketch.updateExtension(geoId, std::move(ext));}

Expand Down Expand Up @@ -436,6 +437,8 @@ class SketcherExport SketchObject : public Part::Part2DObject
/// returns the number of redundant constraints detected
int autoRemoveRedundants(bool updategeo = true);

int renameConstraint(int GeoId, std::string name);

// Validation routines
std::vector<Base::Vector3d> getOpenVertices(void) const;

Expand Down Expand Up @@ -527,6 +530,22 @@ class SketcherExport SketchObject : public Part::Part2DObject
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<SketchObject> SketchObjectPython;

} //namespace Sketcher
Expand Down
10 changes: 2 additions & 8 deletions src/Mod/Sketcher/App/SketchObjectPyImp.cpp
Expand Up @@ -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;
}

Expand Down

0 comments on commit 64774bc

Please sign in to comment.