Reorient sketch loses constraints

[bejant]

the Constraints list detects a mismatch to the last accepted geometry and hides all its content.

Wouldn't it be better and more intuitive for the user if that content was displayed but with an error message?

[chrisf]

I believe it is a bug and the cause (my previous post wasn't very clear) appears to be in SketchObject::delConstraintsToExternal.

The offending line is in bold below. My understanding is the the First and Second elements of a Constraint hold either an integer index into the geometry list (0, 1, 2 etc), -1 or -2 for horizontal and vertical axes or an index into the external geometry list (-3, -4, -5, -6 etc) OR -2000 if the particular constraint only references one geometry (in the case of Second, a similar usage for Third).

The code below deletes any constraint that has First or Second <= -3. All single geometry constraints (horizontal, vertical, line lengths , radii) have Second = -2000 so they get deleted in error.

int SketchObject::delConstraintsToExternal()
{
const std::vector< Constraint * > &constraints = Constraints.getValues();
std::vector< Constraint * > newConstraints(0);
int GeoId = -3;
for (std::vector<Constraint *>::const_iterator it = constraints.begin();
it != constraints.end(); ++it) {
if ((*it)->First > GeoId && (*it)->Second > GeoId) {
newConstraints.push_back(*it);
}

[logari81]

yes, your diagnosis seems right to me, good catch.

[wmayer]

git commit ec7573d

[chrisf]

Hi wmayer,
Thanks for looking at that patch.

I'm also using the below patch in void SketchObject::onChanged(const App::Property* prop).

This patch cleans up a change of sketch support.
Without it, when sketch support changes, the external geometry list is wiped but nothing is done with the constraints list so there could be invalid constraints.
This clears the constraints before deleting each external geometry.

EDIT:
Should add that the patch deletes external geometry whenever the support changes. Previous behaviour only deleted external geometry if the support was null. This didn't make much sense because the external geometry was very unlikely to be valid after any support change.

Code: Select all

diff --git a/src/Mod/Sketcher/App/SketchObject.cpp b/src/Mod/Sketcher/App/SketchObject.cpp
index 0626294..86bf07a 100644
--- a/src/Mod/Sketcher/App/SketchObject.cpp
+++ b/src/Mod/Sketcher/App/SketchObject.cpp
@@ -1585,10 +1585,9 @@ void SketchObject::onChanged(const App::Property* prop)
         // make sure not to change anything while restoring this object
         if (!isRestoring()) {
             // if support face was cleared then also clear the external geometry
-            if (!Support.getValue()) {
-                std::vector<DocumentObject*> obj;
-                std::vector<std::string> sub;
-                ExternalGeometry.setValues(obj, sub);
+            delConstraintsToExternal();
+            for (int i=0; i < getExternalGeometryCount(); i++) {
+                delExternal(0);
             }
         }
     }

[wmayer]

git commit 07f3f82