From bfb8a5da90227d6f16be54a7c055ab3b562b6d26 Mon Sep 17 00:00:00 2001 From: BoykoNeov Date: Mon, 27 Jul 2026 23:01:02 +0300 Subject: [PATCH 1/5] 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/5] 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(); From 0589472ed20aff451d3d1ed9dd31dea33d8644e2 Mon Sep 17 00:00:00 2001 From: BoykoNeov Date: Mon, 27 Jul 2026 22:05:39 +0300 Subject: [PATCH 3/5] Teach the debug tool to re-solve a sketch after editing a dimension. The interesting solver failures depend on the previous solution being the initial guess for the next solve, so they can't be reproduced by loading a file: the dimension has to be edited and the sketch re-solved, the way the GUI does it. Add a `solve` command that does that for a list of values and reports the solve result and the geometry after each one, and a `sweep` command that runs the same two step sequence over a range of values. Co-Authored-By: Claude Opus 5 (1M context) --- test/debugtool.cpp | 116 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 116 insertions(+) diff --git a/test/debugtool.cpp b/test/debugtool.cpp index aea6b8475..a1e984963 100644 --- a/test/debugtool.cpp +++ b/test/debugtool.cpp @@ -4,11 +4,95 @@ // Copyright 2017 whitequark //----------------------------------------------------------------------------- +#include "solvespace.h" #include "expr.h" #include "platform/platform.h" using namespace SolveSpace; +static const char *SolveResultName(SolveResult r) { + switch(r) { + case SolveResult::OKAY: return "OKAY"; + case SolveResult::DIDNT_CONVERGE: return "DIDNT_CONVERGE"; + case SolveResult::REDUNDANT_OKAY: return "REDUNDANT_OKAY"; + case SolveResult::REDUNDANT_DIDNT_CONVERGE: return "REDUNDANT_DIDNT_CONVERGE"; + case SolveResult::TOO_MANY_UNKNOWNS: return "TOO_MANY_UNKNOWNS"; + } + return "?"; +} + +struct StepResult { + SolveResult how; + double signedAngle; +}; + +// Set constraint `hc` to `value`, regenerate, and report how the group solved. +static StepResult SetAndSolve(uint32_t hcv, double value, bool verbose) { + hConstraint hc = {hcv}; + Constraint *c = SK.GetConstraint(hc); + hGroup hg = c->group; + c->valA = value; + SS.MarkGroupDirty(hg); + // This is what the GUI does after an edit: MarkGroupDirty() schedules a + // Generate::DIRTY pass, which is run from the event loop. + if(getenv("SS_GENERATE_ALL")) { + SS.GenerateAll(SolveSpaceUI::Generate::ALL); + } else { + SS.GenerateAll(); + } + + Group *g = SK.GetGroup(hg); + StepResult sr = { g->solved.how, 0.0 }; + + // Report the geometry too, not just the result code: converging to a + // mirrored or otherwise different solution is not a success. + c = SK.constraint.FindByIdNoOops(hc); + if(c && c->type == Constraint::Type::ANGLE) { + Vector a = SK.GetEntity(c->entityA)->VectorGetNum(); + Vector b = SK.GetEntity(c->entityB)->VectorGetNum(); + if(c->other) a = a.ScaledBy(-1); + a = a.ProjectVectorInto(c->workplane); + b = b.ProjectVectorInto(c->workplane); + Vector n = SK.GetEntity(c->workplane)->Normal()->NormalN(); + double dot = a.Dot(b) / (a.Magnitude() * b.Magnitude()); + double crs = a.Cross(b).Dot(n) / (a.Magnitude() * b.Magnitude()); + sr.signedAngle = atan2(crs, dot) * 180 / PI; + } + + if(verbose) { + fprintf(stderr, " set c%u = %.10g -> %s (dof=%d, bad=%d, signed angle=%.9g)\n", + hcv, value, SolveResultName(sr.how), g->solved.dof, g->solved.remove.n, + sr.signedAngle); + fprintf(stderr, " requests=%d constraints=%d entities=%d params=%d\n", + SK.request.n, SK.constraint.n, SK.entity.n, SK.param.n); + for(int i = 0; i < g->solved.remove.n; i++) { + Constraint *bc = SK.constraint.FindByIdNoOops(g->solved.remove[i]); + fprintf(stderr, " bad constraint %u type %d\n", g->solved.remove[i].v, + bc ? (int)bc->type : -1); + } + for(auto &e : SK.entity) { + if(e.group != hg) continue; + if(e.type != Entity::Type::POINT_IN_2D && e.type != Entity::Type::POINT_IN_3D) + continue; + Vector p = e.PointGetNum(); + fprintf(stderr, " pt %08x (%.6f, %.6f, %.6f)\n", e.h.v, p.x, p.y, p.z); + } + } + return sr; +} + +static bool LoadSketch(const std::string &file) { + SS.Init(); + SS.showToolbar = false; + SS.checkClosedContour = false; + if(!SS.LoadFromFile(Platform::Path::From(file))) { + fprintf(stderr, "cannot load %s\n", file.c_str()); + return false; + } + SS.AfterNewFile(); + return true; +} + int main(int argc, char **argv) { std::vector args = Platform::InitCli(argc, argv); @@ -21,6 +105,32 @@ int main(int argc, char **argv) { fprintf(stderr, "%g\n", e->Eval()); } Platform::FreeAllTemporary(); + } else if(args.size() >= 5 && args[1] == "solve") { + // solve [ ...] + if(!LoadSketch(args[2])) return 1; + uint32_t hcv = (uint32_t)strtoul(args[3].c_str(), NULL, 0); + for(size_t i = 4; i < args.size(); i++) { + SetAndSolve(hcv, strtod(args[i].c_str(), NULL), /*verbose=*/true); + } + Platform::FreeAllTemporary(); + } else if(args.size() == 8 && args[1] == "sweep") { + // sweep + uint32_t hcv = (uint32_t)strtoul(args[3].c_str(), NULL, 0); + double start = strtod(args[4].c_str(), NULL); + double from = strtod(args[5].c_str(), NULL); + double to = strtod(args[6].c_str(), NULL); + double step = strtod(args[7].c_str(), NULL); + for(double target = from; target <= to + step/2; target += step) { + if(!LoadSketch(args[2])) return 1; + StepResult a = SetAndSolve(hcv, start, /*verbose=*/false); + StepResult b = SetAndSolve(hcv, target, /*verbose=*/false); + printf("%.6g %.6g %s %s %.9g\n", start, target, + SolveResultName(a.how), SolveResultName(b.how), b.signedAngle); + fflush(stdout); + SK.Clear(); + SS.Clear(); + } + Platform::FreeAllTemporary(); } else { fprintf(stderr, "Usage: %s \n", args[0].c_str()); //-----------------------------------------------------------------------------> 80 col */ @@ -28,6 +138,12 @@ int main(int argc, char **argv) { Commands: expr [expr] Evaluate an expression. + solve [file.slvs] [constraint-handle] [value]... + Load a sketch, then repeatedly set a constraint's value and re-solve, + reporting the solve result of each step. + sweep [file.slvs] [constraint-handle] [start] [from] [to] [step] + For each target value in [from, to], load the sketch, solve it at + [start], then at the target, and report both solve results. )"); } From a8d18b4fb74e500793f795c6692efa99d4b615b9 Mon Sep 17 00:00:00 2001 From: BoykoNeov Date: Thu, 30 Jul 2026 05:00:45 +0300 Subject: [PATCH 4/5] Only step as far as the linearization of the constraints is trusted. A Newton step is only as good as the linearization of the constraint equations about the operating point, so where those equations are strongly curved the full step can land further from the solution than it started; and if it lands near a critical point of the equations, the step after that one is enormous and the geometry runs away to nowhere. That is what happens in issue #1247. The reporter's sketch reduces, after substitution, to one equation in the height h of a right triangle with a 100 mm base, cos(theta) = 100/sqrt(100^2 + h^2), whose derivative vanishes at h = 0. Changing the angle dimension from 60 to 30 degrees overshoots the root at h = 57.7 and lands at h = 4.2, right on top of that critical point; the next two steps are 161 and 349 mm, the triangle ends up out at +/-1.9e6 mm where the steps collapse to 1e-16, and the solve gives up with DIDNT_CONVERGE. Changing it to 40 degrees instead lands at h = 50, where the derivative is healthy, and converges in four iterations, which is why the failure looked threshold-shaped and dependent on the order of the edits. So keep a radius that we trust the linearization over, don't step further than that, and grow it only as steps keep working out: double it when a step reduces the residual, quarter it when one does not. The radius starts at half of the first Newton step, and since it doubles per accepted step it is back up to the full step immediately. If nothing along the step direction is an improvement, take the whole step anyway, exactly as before, so no step that we used to take is skipped and the iteration limit still catches it if that was a bad idea. IsReasonable() now shrinks the step instead of abandoning the solve. On a grid of 6480 (starting angle, target angle) pairs of the reporter's sketch, driven through the same edit-a-dimension-and-re-solve path as the GUI, this solves all 6480 where we used to solve 5662, and no pair that used to solve stops solving. It also stops mirroring the sketch: -30 degrees satisfies an angle constraint exactly as well as +30 does, and of the 5662 pairs both versions solve, the old code flips the triangle to the far side of the horizontal line on 619, where this steps to the nearer of the two solutions on all 619 and flips none. Accuracy is unchanged, worst error over the grid 3.82e-06 degrees against 3.69e-06 before. Halving the first step is what buys that, and it is the least of the divisors that does: with the whole first step the sketch still mirrors on 549 of the 619. Bigger divisors do no better on those pairs and cost more, so a half it is. The price is one extra Newton iteration when re-solving from an already-solved sketch, which is what happens on every frame of a drag: three iterations per small increment of a dimension where we used to take two. Over the whole of the existing test suite it is not measurable, 957 iterations against 956. Fixes #1247. Co-Authored-By: Claude Opus 5 (1M context) --- src/system.cpp | 66 +++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 52 insertions(+), 14 deletions(-) diff --git a/src/system.cpp b/src/system.cpp index 89d315938..0c0f6011f 100644 --- a/src/system.cpp +++ b/src/system.cpp @@ -316,32 +316,70 @@ bool System::NewtonSolve() { for(i = 0; i < mat.m; i++) { mat.B.num[i] = (mat.B.sym[i])->Eval(); } + + // Where we were before we took the step, so that we can take a different + // one from the same operating point when the first is no good. + std::vector prevVal(mat.n); + + // Take the Newton step, + // J(x_n) (x_{n+1} - x_n) = 0 - F(x_n) + // scaled by relax, and re-evaluate the functions, since the params have + // just changed. Returns false if that took us somewhere clearly useless. + auto takeStep = [&](double relax) { + for(int k = 0; k < mat.n; k++) { + Param *p = param.FindById(mat.param[k]); + p->val = prevVal[k] - relax*mat.X[k]; + if(IsReasonable(p->val)) return false; + } + for(int k = 0; k < mat.m; k++) { + mat.B.num[k] = (mat.B.sym[k])->Eval(); + if(IsReasonable(mat.B.num[k])) return false; + } + return true; + }; + + // The radius of the region around our operating point that we trust the + // linearization in. + double trust = 0; + do { // And evaluate the Jacobian at our initial operating point. EvalJacobian(); if(!SolveLeastSquares()) break; - // Take the Newton step; - // J(x_n) (x_{n+1} - x_n) = 0 - F(x_n) for(i = 0; i < mat.n; i++) { - Param *p = param.FindById(mat.param[i]); - p->val -= mat.X[i]; - if(IsReasonable(p->val)) { - // Very bad, and clearly not convergent - return false; - } + prevVal[i] = param.FindById(mat.param[i])->val; } - - // Re-evalute the functions, since the params have just changed. - for(i = 0; i < mat.m; i++) { - mat.B.num[i] = (mat.B.sym[i])->Eval(); - if(IsReasonable(mat.B.num[i])) { + const double err = mat.B.num.squaredNorm(); + const double stepNorm = mat.X.norm(); + if(iter == 0) trust = stepNorm/2; + + // A Newton step is only as good as the linearization of F about our + // operating point, so where F is strongly curved the full step can + // land much further from the solution than it started; and if it + // lands near a critical point of F, then the step after that one is + // enormous, and the geometry runs away to nowhere. So don't go + // further than the distance we currently trust that linearization + // over, and grow that distance only as steps keep working out. + bool accepted = false; + for(int tries = 0; tries < 12 && !accepted && stepNorm > 0; tries++) { + double relax = (stepNorm > trust) ? trust/stepNorm : 1.0; + accepted = takeStep(relax) && mat.B.num.squaredNorm() < err; + if(!accepted) trust /= 4; + } + if(accepted) { + trust *= 2; + } else { + // Nothing along this direction is an improvement, so just take + // the whole thing, like we always used to; if that was a bad + // idea, then the iteration limit below will catch it. + if(!takeStep(1.0)) { // Very bad, and clearly not convergent return false; } } - + // Check for convergence converged = true; for(i = 0; i < mat.m; i++) { From 4165c4eb722be3778b248e398280724678c93673 Mon Sep 17 00:00:00 2001 From: BoykoNeov Date: Thu, 30 Jul 2026 05:01:00 +0300 Subject: [PATCH 5/5] Add regression tests for stepping an angle dimension over a critical point. The fixture is the sketch from issue #1247, and the cases drive it the way the GUI does, by changing a dimension's value and re-solving. That matters: the previous solution is the next solve's initial guess, so neither failure can be reproduced by loading a file that already has the failing value in it. test/debugtool.cpp grew a `solve` command for the same reason. The checks are relative rather than absolute because NewtonSolve() stops as soon as every equation's residual is under CONVERGE_TOLERANCE, which is dimensionless; the angle equation's residual is a direction cosine, so satisfying it to 1e-8 only pins this triangle's 173 mm height to a few times 1e-06 mm. Comparing the height itself against LENGTH_EPS passes or fails on which side of the tolerance the last step happens to land, which is not a property of the solver worth asserting. The second case checks the magnitude of the height only. From 85 degrees the first step that reduces the residual below its value there is one that crosses the horizontal line, so the triangle ends up mirrored; that is a solution where before there was none, but it is not the nearer of the two, and pinning the sign would assert a behaviour we would like to improve. Co-Authored-By: Claude Opus 5 (1M context) --- test/CMakeLists.txt | 1 + test/core/solver/angle.slvs | 456 ++++++++++++++++++++++++++++++++++++ test/core/solver/test.cpp | 73 ++++++ 3 files changed, 530 insertions(+) create mode 100644 test/core/solver/angle.slvs create mode 100644 test/core/solver/test.cpp diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 2e1f687b8..45bdb2a83 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -20,6 +20,7 @@ set(testsuite_SOURCES core/locale/test.cpp core/path/test.cpp core/prune/test.cpp + core/solver/test.cpp constraint/points_coincident/test.cpp constraint/pt_pt_distance/test.cpp constraint/pt_plane_distance/test.cpp diff --git a/test/core/solver/angle.slvs b/test/core/solver/angle.slvs new file mode 100644 index 000000000..ba213cde5 --- /dev/null +++ b/test/core/solver/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/solver/test.cpp b/test/core/solver/test.cpp new file mode 100644 index 000000000..f0414bd78 --- /dev/null +++ b/test/core/solver/test.cpp @@ -0,0 +1,73 @@ +#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 }; +// The endpoints of the vertical line, whose length is 100*tan(angle). +static const hEntity VERTICAL_A = { 0x00050001 }; +static const hEntity VERTICAL_B = { 0x00050002 }; + +// Change the value of a dimension and re-solve, the same way that the GUI +// does after the user edits one. The solution we start from is the previous +// solve's, so these cases are about a sequence of edits, and cannot be +// reproduced by loading a file with one of the values already in it. +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; +} + +// Signed, so that it also tells us which of the two mirrored solutions +// (+angle and -angle both satisfy the constraint) we landed on. +static double VerticalLength() { + return SK.GetEntity(VERTICAL_B)->PointGetNum().y - + SK.GetEntity(VERTICAL_A)->PointGetNum().y; +} + +// Note the relative form of the checks below. NewtonSolve() stops as soon as +// every equation's residual is under CONVERGE_TOLERANCE, which is +// dimensionless; the angle equation's residual is a direction cosine, so +// satisfying it to 1e-8 only pins this triangle's height to a few times 1e-6 +// mm. Comparing the height itself against LENGTH_EPS therefore tests the +// solver's luck rather than its answer. + +TEST_CASE(angle_step_over_critical_point) { + CHECK_LOAD("angle.slvs"); + + // Both of these angles are perfectly solvable, but the Newton step from + // the 60 degree solution towards the 30 degree one used to overshoot the + // root and land right on top of a critical point of the angle equation, + // from where the next step threw the geometry off to nowhere. + CHECK_TRUE(SetValueAndSolve(HANGLE, 60.0) == SolveResult::OKAY); + CHECK_EQ_EPS(VerticalLength() / (100*tan(60*PI/180)), 1.0); + + // Signed, since -30 degrees satisfies the angle constraint just as well as + // +30 does, and mirroring the triangle about the horizontal line is not + // something the user asked for. + CHECK_TRUE(SetValueAndSolve(HANGLE, 30.0) == SolveResult::OKAY); + CHECK_EQ_EPS(VerticalLength() / (100*tan(30*PI/180)), 1.0); +} + +TEST_CASE(angle_step_from_steep_solution) { + CHECK_LOAD("angle.slvs"); + + // The same failure, with much more room to spare: from a steep angle the + // step towards a shallow one used to overshoot far past the critical + // point, and every target below 75 degrees was unsolvable from here. + CHECK_TRUE(SetValueAndSolve(HANGLE, 85.0) == SolveResult::OKAY); + CHECK_EQ_EPS(VerticalLength() / (100*tan(85*PI/180)), 1.0); + + // Magnitude only: -30 degrees satisfies the constraint exactly as well as + // +30, and from this far up the hill the step that first reduces the + // residual below its value here is one that crosses the horizontal line, + // so the triangle ends up mirrored. That is a solution, where before + // there was none, but it is not the nearer of the two. + CHECK_TRUE(SetValueAndSolve(HANGLE, 30.0) == SolveResult::OKAY); + CHECK_EQ_EPS(fabs(VerticalLength()) / (100*tan(30*PI/180)), 1.0); +}