From bfb8a5da90227d6f16be54a7c055ab3b562b6d26 Mon Sep 17 00:00:00 2001 From: BoykoNeov Date: Mon, 27 Jul 2026 23:01:02 +0300 Subject: [PATCH 1/2] Don't delete the constraints that the solver couldn't satisfy. PruneOrphans() and PruneRequestsAndConstraints() tag the requests and constraints that they mean to delete and then call RemoveTagged(), which removes every element whose tag is nonzero -- but neither of them clears the tags first. System::Solve() tags the constraints whose equations it couldn't satisfy, so that they can be listed for the user, and so the regeneration that immediately follows a failed solve deleted exactly those constraints: fail to solve a sketch once, and the constraints that failed are gone, along with any chance of correcting the value that caused it. Clear the tags before setting them. The request lists get the same treatment, not because anything tags a request today, but because they are pruned by the same tag-then-RemoveTagged() protocol, and would lose requests the same way if anything ever did. Co-Authored-By: Claude Opus 5 (1M context) --- src/generate.cpp | 4 + test/CMakeLists.txt | 1 + test/core/prune/angle.slvs | 456 +++++++++++++++++++++++++++++++++++++ test/core/prune/test.cpp | 47 ++++ 4 files changed, 508 insertions(+) create mode 100644 test/core/prune/angle.slvs create mode 100644 test/core/prune/test.cpp diff --git a/src/generate.cpp b/src/generate.cpp index f5ba82c08..256a381e5 100644 --- a/src/generate.cpp +++ b/src/generate.cpp @@ -33,6 +33,7 @@ void SolveSpaceUI::MarkGroupDirty(hGroup hg, bool onlyThis) { bool SolveSpaceUI::PruneOrphans() { const int requests = SK.request.n; + SK.request.ClearTags(); for(Request &r : SK.request) { if(!GroupExists(r.group)) r.tag = 1; @@ -41,6 +42,7 @@ bool SolveSpaceUI::PruneOrphans() { deleted.requests += requests - SK.request.n; const int constraints = SK.constraint.n; + SK.constraint.ClearTags(); for(Constraint &c : SK.constraint) { if(!GroupExists(c.group)) c.tag = 1; @@ -106,6 +108,7 @@ bool SolveSpaceUI::PruneRequestsAndConstraints(hGroup hg) { }; const int requests = SK.request.n; + SK.request.ClearTags(); for(Request &r : SK.request) { if(r.group != hg) { continue; @@ -121,6 +124,7 @@ bool SolveSpaceUI::PruneRequestsAndConstraints(hGroup hg) { deleted.requests += requests - SK.request.n; const int constraints = SK.constraint.n; + SK.constraint.ClearTags(); for(Constraint &c : SK.constraint) { if(c.group != hg) continue; diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 0dafd42db..2e1f687b8 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -19,6 +19,7 @@ set(testsuite_SOURCES core/expr/test.cpp core/locale/test.cpp core/path/test.cpp + core/prune/test.cpp constraint/points_coincident/test.cpp constraint/pt_pt_distance/test.cpp constraint/pt_plane_distance/test.cpp diff --git a/test/core/prune/angle.slvs b/test/core/prune/angle.slvs new file mode 100644 index 000000000..ba213cde5 --- /dev/null +++ b/test/core/prune/angle.slvs @@ -0,0 +1,456 @@ +±²³SolveSpaceREVa + + +Group.h.v=00000001 +Group.type=5000 +Group.name=#references +Group.color=ff000000 +Group.skipFirst=0 +Group.predef.swapUV=0 +Group.predef.negateU=0 +Group.predef.negateV=0 +Group.visible=1 +Group.suppress=0 +Group.relaxConstraints=0 +Group.allowRedundant=0 +Group.allDimsReference=0 +Group.remap={ +} +AddGroup + +Group.h.v=00000002 +Group.type=5001 +Group.order=1 +Group.name=sketch-in-plane +Group.activeWorkplane.v=80020000 +Group.color=ff000000 +Group.subtype=6000 +Group.skipFirst=0 +Group.predef.q.w=1.00000000000000000000 +Group.predef.origin.v=00010001 +Group.predef.swapUV=0 +Group.predef.negateU=0 +Group.predef.negateV=0 +Group.visible=1 +Group.suppress=0 +Group.relaxConstraints=0 +Group.allowRedundant=0 +Group.allDimsReference=0 +Group.remap={ +} +AddGroup + +Param.h.v.=00010010 +AddParam + +Param.h.v.=00010011 +AddParam + +Param.h.v.=00010012 +AddParam + +Param.h.v.=00010020 +Param.val=1.00000000000000000000 +AddParam + +Param.h.v.=00010021 +AddParam + +Param.h.v.=00010022 +AddParam + +Param.h.v.=00010023 +AddParam + +Param.h.v.=00020010 +AddParam + +Param.h.v.=00020011 +AddParam + +Param.h.v.=00020012 +AddParam + +Param.h.v.=00020020 +Param.val=0.50000000000000000000 +AddParam + +Param.h.v.=00020021 +Param.val=0.50000000000000000000 +AddParam + +Param.h.v.=00020022 +Param.val=0.50000000000000000000 +AddParam + +Param.h.v.=00020023 +Param.val=0.50000000000000000000 +AddParam + +Param.h.v.=00030010 +AddParam + +Param.h.v.=00030011 +AddParam + +Param.h.v.=00030012 +AddParam + +Param.h.v.=00030020 +Param.val=0.50000000000000000000 +AddParam + +Param.h.v.=00030021 +Param.val=-0.50000000000000000000 +AddParam + +Param.h.v.=00030022 +Param.val=-0.50000000000000000000 +AddParam + +Param.h.v.=00030023 +Param.val=-0.50000000000000000000 +AddParam + +Param.h.v.=00040010 +Param.val=-59.55307006835937500000 +AddParam + +Param.h.v.=00040011 +Param.val=-21.02559858317918184412 +AddParam + +Param.h.v.=00040013 +Param.val=40.44692993164062500000 +AddParam + +Param.h.v.=00040014 +Param.val=-21.02559858317918184412 +AddParam + +Param.h.v.=00050010 +Param.val=40.44692993164062500000 +AddParam + +Param.h.v.=00050011 +Param.val=-21.02559858317918184412 +AddParam + +Param.h.v.=00050013 +Param.val=40.44692993164062500000 +AddParam + +Param.h.v.=00050014 +Param.val=36.70942833578337882727 +AddParam + +Param.h.v.=00060010 +Param.val=40.44692993164062500000 +AddParam + +Param.h.v.=00060011 +Param.val=36.70942833578337882727 +AddParam + +Param.h.v.=00060013 +Param.val=-59.55307006835937500000 +AddParam + +Param.h.v.=00060014 +Param.val=-21.02559858317918184412 +AddParam + +Request.h.v=00000001 +Request.type=100 +Request.group.v=00000001 +Request.construction=0 +AddRequest + +Request.h.v=00000002 +Request.type=100 +Request.group.v=00000001 +Request.construction=0 +AddRequest + +Request.h.v=00000003 +Request.type=100 +Request.group.v=00000001 +Request.construction=0 +AddRequest + +Request.h.v=00000004 +Request.type=200 +Request.workplane.v=80020000 +Request.group.v=00000002 +Request.construction=0 +AddRequest + +Request.h.v=00000005 +Request.type=200 +Request.workplane.v=80020000 +Request.group.v=00000002 +Request.construction=0 +AddRequest + +Request.h.v=00000006 +Request.type=200 +Request.workplane.v=80020000 +Request.group.v=00000002 +Request.construction=0 +AddRequest + +Entity.h.v=00010000 +Entity.type=10000 +Entity.construction=0 +Entity.point[0].v=00010001 +Entity.normal.v=00010020 +Entity.actVisible=1 +AddEntity + +Entity.h.v=00010001 +Entity.type=2000 +Entity.construction=1 +Entity.actVisible=1 +AddEntity + +Entity.h.v=00010020 +Entity.type=3000 +Entity.construction=0 +Entity.point[0].v=00010001 +Entity.actNormal.w=1.00000000000000000000 +Entity.actVisible=1 +AddEntity + +Entity.h.v=00020000 +Entity.type=10000 +Entity.construction=0 +Entity.point[0].v=00020001 +Entity.normal.v=00020020 +Entity.actVisible=1 +AddEntity + +Entity.h.v=00020001 +Entity.type=2000 +Entity.construction=1 +Entity.actVisible=1 +AddEntity + +Entity.h.v=00020020 +Entity.type=3000 +Entity.construction=0 +Entity.point[0].v=00020001 +Entity.actNormal.w=0.50000000000000000000 +Entity.actNormal.vx=0.50000000000000000000 +Entity.actNormal.vy=0.50000000000000000000 +Entity.actNormal.vz=0.50000000000000000000 +Entity.actVisible=1 +AddEntity + +Entity.h.v=00030000 +Entity.type=10000 +Entity.construction=0 +Entity.point[0].v=00030001 +Entity.normal.v=00030020 +Entity.actVisible=1 +AddEntity + +Entity.h.v=00030001 +Entity.type=2000 +Entity.construction=1 +Entity.actVisible=1 +AddEntity + +Entity.h.v=00030020 +Entity.type=3000 +Entity.construction=0 +Entity.point[0].v=00030001 +Entity.actNormal.w=0.50000000000000000000 +Entity.actNormal.vx=-0.50000000000000000000 +Entity.actNormal.vy=-0.50000000000000000000 +Entity.actNormal.vz=-0.50000000000000000000 +Entity.actVisible=1 +AddEntity + +Entity.h.v=00040000 +Entity.type=11000 +Entity.construction=0 +Entity.point[0].v=00040001 +Entity.point[1].v=00040002 +Entity.workplane.v=80020000 +Entity.actVisible=1 +AddEntity + +Entity.h.v=00040001 +Entity.type=2001 +Entity.construction=0 +Entity.workplane.v=80020000 +Entity.actPoint.x=-59.55307006835937500000 +Entity.actPoint.y=-21.02559858317918184412 +Entity.actVisible=1 +AddEntity + +Entity.h.v=00040002 +Entity.type=2001 +Entity.construction=0 +Entity.workplane.v=80020000 +Entity.actPoint.x=40.44692993164062500000 +Entity.actPoint.y=-21.02559858317918184412 +Entity.actVisible=1 +AddEntity + +Entity.h.v=00050000 +Entity.type=11000 +Entity.construction=0 +Entity.point[0].v=00050001 +Entity.point[1].v=00050002 +Entity.workplane.v=80020000 +Entity.actVisible=1 +AddEntity + +Entity.h.v=00050001 +Entity.type=2001 +Entity.construction=0 +Entity.workplane.v=80020000 +Entity.actPoint.x=40.44692993164062500000 +Entity.actPoint.y=-21.02559858317918184412 +Entity.actVisible=1 +AddEntity + +Entity.h.v=00050002 +Entity.type=2001 +Entity.construction=0 +Entity.workplane.v=80020000 +Entity.actPoint.x=40.44692993164062500000 +Entity.actPoint.y=36.70942833578337882727 +Entity.actVisible=1 +AddEntity + +Entity.h.v=00060000 +Entity.type=11000 +Entity.construction=0 +Entity.point[0].v=00060001 +Entity.point[1].v=00060002 +Entity.workplane.v=80020000 +Entity.actVisible=1 +AddEntity + +Entity.h.v=00060001 +Entity.type=2001 +Entity.construction=0 +Entity.workplane.v=80020000 +Entity.actPoint.x=40.44692993164062500000 +Entity.actPoint.y=36.70942833578337882727 +Entity.actVisible=1 +AddEntity + +Entity.h.v=00060002 +Entity.type=2001 +Entity.construction=0 +Entity.workplane.v=80020000 +Entity.actPoint.x=-59.55307006835937500000 +Entity.actPoint.y=-21.02559858317918184412 +Entity.actVisible=1 +AddEntity + +Entity.h.v=80020000 +Entity.type=10000 +Entity.construction=0 +Entity.point[0].v=80020002 +Entity.normal.v=80020001 +Entity.actVisible=1 +AddEntity + +Entity.h.v=80020001 +Entity.type=3010 +Entity.construction=0 +Entity.point[0].v=80020002 +Entity.actNormal.w=1.00000000000000000000 +Entity.actVisible=1 +AddEntity + +Entity.h.v=80020002 +Entity.type=2012 +Entity.construction=1 +Entity.actVisible=1 +AddEntity + +Constraint.h.v=00000001 +Constraint.type=80 +Constraint.group.v=00000002 +Constraint.workplane.v=80020000 +Constraint.entityA.v=00040000 +Constraint.other=0 +Constraint.other2=0 +Constraint.reference=0 +AddConstraint + +Constraint.h.v=00000002 +Constraint.type=20 +Constraint.group.v=00000002 +Constraint.workplane.v=80020000 +Constraint.ptA.v=00040002 +Constraint.ptB.v=00050001 +Constraint.other=0 +Constraint.other2=0 +Constraint.reference=0 +AddConstraint + +Constraint.h.v=00000003 +Constraint.type=81 +Constraint.group.v=00000002 +Constraint.workplane.v=80020000 +Constraint.entityA.v=00050000 +Constraint.other=0 +Constraint.other2=0 +Constraint.reference=0 +AddConstraint + +Constraint.h.v=00000004 +Constraint.type=20 +Constraint.group.v=00000002 +Constraint.workplane.v=80020000 +Constraint.ptA.v=00050002 +Constraint.ptB.v=00060001 +Constraint.other=0 +Constraint.other2=0 +Constraint.reference=0 +AddConstraint + +Constraint.h.v=00000005 +Constraint.type=20 +Constraint.group.v=00000002 +Constraint.workplane.v=80020000 +Constraint.ptA.v=00040001 +Constraint.ptB.v=00060002 +Constraint.other=0 +Constraint.other2=0 +Constraint.reference=0 +AddConstraint + +Constraint.h.v=00000006 +Constraint.type=30 +Constraint.group.v=00000002 +Constraint.workplane.v=80020000 +Constraint.valA=100.00000000000000000000 +Constraint.ptA.v=00040001 +Constraint.ptB.v=00040002 +Constraint.other=0 +Constraint.other2=0 +Constraint.reference=0 +Constraint.disp.offset.y=-10.00000000000000000000 +AddConstraint + +Constraint.h.v=00000007 +Constraint.type=120 +Constraint.group.v=00000002 +Constraint.workplane.v=80020000 +Constraint.valA=30.00000000000000000000 +Constraint.entityA.v=00060000 +Constraint.entityB.v=00040000 +Constraint.other=1 +Constraint.other2=0 +Constraint.reference=0 +Constraint.disp.offset.x=-10.62718505859374573674 +Constraint.disp.offset.y=-14.77174072265624893419 +AddConstraint + diff --git a/test/core/prune/test.cpp b/test/core/prune/test.cpp new file mode 100644 index 000000000..6e0bd7c04 --- /dev/null +++ b/test/core/prune/test.cpp @@ -0,0 +1,47 @@ +#include "solvespace.h" + +#include "harness.h" + +// angle.slvs is a right triangle: a horizontal line 100 mm long, a vertical +// line, and a third line closing them, with an angle constraint (handle 7) +// between that third line and the horizontal one. +static const hConstraint HANGLE = { 7 }; + +// Change the value of a dimension and re-solve, the same way that the GUI +// does after the user edits one. +static SolveResult SetValueAndSolve(hConstraint hc, double value) { + Constraint *c = SK.GetConstraint(hc); + hGroup hg = c->group; + c->valA = value; + SS.MarkGroupDirty(hg); + SS.GenerateAll(); + return SK.GetGroup(hg)->solved.how; +} + +TEST_CASE(failed_solve_keeps_constraints) { + CHECK_LOAD("angle.slvs"); + + const int constraints = SK.constraint.n; + const int requests = SK.request.n; + + // No triangle like this one has a 95 degree angle there -- the direction + // cosine is 100/sqrt(100^2 + h^2), which is positive for every h, and the + // cosine of 95 degrees is not -- so the solver gives up, and marks the + // constraints whose equations it couldn't satisfy so that they can be + // reported. Those marks used to be mistaken for delete-me tags by the + // prune pass that runs with the next regeneration, which silently + // destroyed exactly the constraints that had failed. Either of the + // didn't-converge results reaches the marking path, so don't insist on + // which one. + SolveResult how = SetValueAndSolve(HANGLE, 95.0); + CHECK_TRUE(how == SolveResult::DIDNT_CONVERGE || + how == SolveResult::REDUNDANT_DIDNT_CONVERGE); + + // The sketch is still all there, so the value that couldn't be solved is + // still on screen to be corrected. + CHECK_TRUE(SK.constraint.n == constraints); + CHECK_TRUE(SK.request.n == requests); + Constraint *c = SK.constraint.FindByIdNoOops(HANGLE); + CHECK_TRUE(c != NULL); + CHECK_EQ_EPS(c->valA, 95.0); +} From 81f473ff18e2b1ffa7de389b7cf76daf1ac739c2 Mon Sep 17 00:00:00 2001 From: BoykoNeov Date: Wed, 29 Jul 2026 15:04:04 +0300 Subject: [PATCH 2/2] Free the list of constraints that the solver couldn't satisfy. Group::Clear() frees every other dynamically-allocated member of a group, but not solved.remove -- the list that System::Solve() fills with the constraints whose equations it couldn't satisfy, and that FindWhichToRemoveToFixJacobian() fills with the ones it suggests removing to fix a redundant system. SolveGroup() empties that list before every solve, so it never grows during a session, but whatever is in it when the sketch is closed, the group is deleted, or the process exits is leaked. Nothing aliases the list: the undo stack zeroes solved when it shallow copies a group, and SolveGroup() clears the list directly rather than through Group::Clear(), so the report shown in the text window is unaffected. The leak is as old as the list, but it needs a solve to fail before there is anything to leak, and until the preceding commit's test nothing in the suite made one fail -- so the sanitizer build had nothing to find. Co-Authored-By: Claude Opus 5 (1M context) --- src/group.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/group.cpp b/src/group.cpp index 71f2e656c..aa9eb25d7 100644 --- a/src/group.cpp +++ b/src/group.cpp @@ -20,6 +20,7 @@ const hGroup Group::HGROUP_REFERENCES = { 1 }; // memory. This clears and frees them all. //----------------------------------------------------------------------------- void Group::Clear() { + solved.remove.Clear(); polyLoops.Clear(); bezierLoops.Clear(); bezierOpens.Clear();