Damp the Newton step in the solver when the full step makes things worse. Fixes #1247

[BoykoNeov]

Fixes solvespace#1247 — @tc7000's sketch where changing an angle dimension from 60° to 30° reports "didn't converge", while 60° → 40° is fine, and the same 30° works if you approach it in smaller steps.

It is not a rank or redundancy problem

That was my first hypothesis, and it is wrong. rankOk is true at every failure, and TestRank / CalculateRank never fire on this model — the DIDNT_CONVERGE comes straight out of System::NewtonSolve(). Worth saying explicitly, because the symptoms (a threshold that depends on the previous value, order dependence) look exactly like a redundancy problem and it's a natural place to start digging.

Root cause: an undamped Newton step lands on a critical point

After substitution this sketch reduces to m = 2, n = 4, effectively one-dimensional in the height h of the triangle, with

f(h) = 100/sqrt(100² + h²) − cos θ

f'(0) = 0 — the function is flat at the origin. Solving for 30° means finding h = 57.735, and from the 60° solution the full Newton step overshoots it and lands at h ≈ 4.2, right on top of that critical point. With the derivative there near zero, the next steps are 161 mm, then 349 mm, and the geometry runs out to ±1.9e6 mm, where the parameters are so large that steps collapse to ~1e-16 and the solver gives up with the residual still at cos 30°.

60° → 40° lands at h = 50, where the derivative is healthy, and converges in four iterations. That is the whole "threshold-shaped, order-dependent" story: whether the full step happens to clear the flat spot.

The fix

A backtracking line search in System::NewtonSolve() (src/system.cpp): try relax = 1, ½, ¼ … 1/128, and accept the first that strictly reduces ‖F‖₂². If none does, take the full step exactly as before — so no step the old code would have taken is ever rejected, and a model that converges today cannot start failing because of this.

IsReasonable() failures now shrink the step instead of aborting the solve outright. (Note for anyone reading that function: it is misnamed upstream — it returns true when the value is unreasonable.)

Regression test

test/core/solver/angle_step_over_critical_point — set 60°, then 30°, and check both solve and that the triangle really has the right height, not just that a result code came back OKAY. Fails at the 30° step without the fix.

The repro is path-dependent: the previous solution is the next solve's initial guess, so it cannot be reproduced by editing the .slvs and reloading. The second commit therefore teaches test/debugtool.cpp to load a sketch and then set a dimension and re-solve repeatedly (solve), and to sweep a range of start/target pairs (sweep). That is what produced the numbers above and it is useful for any solver issue of this shape.

Verification

• A sweep of 138 (start, target) angle pairs: all 32 previously-failing pairs now solve, with zero regressions among the rest.
• Full suite passes in Debug and Release: 263 cases / 930 checks (master is 262 / 925).
• With src/system.cpp reverted, the new test fails at the 30° step.

Concerns you may want to raise

• The merit function mixes units. ‖F‖₂² sums dimensionless cosine residuals from angle constraints — which the mult factor can gain up to ~1000× near 0°/180° — together with mm-valued distance residuals. This is harmless for a decrease-only acceptance test with a full-step fallback (the worst case is that a good step is judged "not an improvement" and we fall back to exactly today's behaviour), but it is not a principled merit function, and a residual scaling pass would be a real improvement. I did not attempt one.
• One visible behaviour change in the sweep: 59° → 25° now lands on +25° where master lands on −25°. Both are exact, mirrored solutions of the same constraint system, and the baseline already flips arbitrarily depending on the starting angle — but it is a difference, so I'd rather point at it than have it found later.
• 1/128 and 8 tries are arbitrary. They were enough for every pair in the sweep; there's no theory behind the specific bound.

This PR is independent of my other three and can be taken on its own. In particular, note that a failed solve on this branch still deletes the constraints that failed — that is a separate bug with its own PR, deliberately not mixed in here.

Please fetch and fast-forward this branch rather than using the merge button on my fork — that keeps it a clean fast-forward for upstream.

🤖 Generated with Claude Code

[BoykoNeov]

Written by Claude Opus 5 — both this text and the code it describes; posted by @BoykoNeov.

Status, and a small ask.

The separate bug the description mentions — a failed solve deleting the constraints it couldn't satisfy — is now upstream solvespace#1744, opened by @ruevs from that branch. It is green, including the ASan leak that held it up. This branch is independent of it, unchanged since I opened it, and still sits directly on master 790bf74c.

The ask: this is the one of these PRs that nobody has looked at yet.

@ruevs — on #4 you asked for someone with fresher linear algebra. This one is not linear algebra. It is a backtracking line search, and the whole claim worth checking is the fallback: try relax = 1, ½, ¼ … 1/128, accept the first that strictly reduces ‖F‖₂², and if none does, take the full step exactly as the old code did. So no step today's solver accepts is rejected by this.

One genuine behaviour change, stated precisely rather than glossed: where the old code aborted the solve on an IsReasonable() failure, this shrinks the step and continues. The set of trajectories is a superset of today's — nothing that converges now can start failing, but some solves that used to give up will keep going.

The things I would actually want a reviewer to push back on are in the description: the merit function mixes units (dimensionless cosine residuals against mm-valued distance ones), and one sweep pair now lands on the mirrored solution (59° → 25° gives +25° where master gives −25°; both are exact, and master already flips arbitrarily depending on where it starts).

[ruevs]

This one actually looks perfectly fine to me. I looked at it together with #2 when creating solvespace#1744. And I tested that it does fix solvespace#1247 test.slvs.zip.

Before opening a PR upstream for this; I wanted solvespace#1744 to be resolved and fixed. That is now done so I've rebased this on master and opened a PR here solvespace#1748.

[ruevs]

The things I would actually want a reviewer to push back on are in the description: the merit function mixes units (dimensionless cosine residuals against mm-valued distance ones), and one sweep pair now lands on the mirrored solution (59° → 25° gives +25° where master gives −25°; both are exact, and master already flips arbitrarily depending on where it starts).

59° → 25° (or a bigger jump) does indeed "flip" with this, while master did not... this is objectively worse since it makes the solver more "surprising" to the user and one of the things the SolveSpace solver is "famous" for is it's "predictability" from the point of view of the user. It is clear that big jumps in constraint values can always lead to "unexpected" solutions to the system and I am not sure how this can be resolved apart from internally "animating" each change in smaller steps and solving multiple times (but I personally don't think this is a good idea).

[BoykoNeov]

Written by Claude Opus 5 — both this text and the code it describes; posted by @BoykoNeov.

@ruevs Your flip objection above turned into a better fix than this branch, so this thread should not be where it lives.

The alternative is #7, and the full reply to you is on solvespace#1748 (the PR you opened from this branch), since that is where you pointed the discussion and where the other maintainers can weigh the tradeoff: solvespace#1748 (comment)

Short version — a trust region in NewtonSolve() instead of a line search: it solves all 6480 pairs of the sweep grid on its own, and it removes all 619 of the mirror flips master makes on the 5662 pairs master can solve, adding none. Scoped honestly: across the whole grid 417 flips remain, all of them inside the 818 pairs master cannot solve at all, where there is no previous behaviour to be surprising relative to. The steps that had to get smaller were the solver's Newton steps rather than the user's edit, which is why it does not need the "animating each change in smaller steps and solving multiple times" that you did not want.

What #7 gives up is this branch's cleanest argument: the line search only engages when the full step makes the residual worse, so its trajectories are a strict superset of today's. #7 clips the first step unconditionally, so every solve takes a different path, and the evidence there is empirical rather than structural. That is a real reason to prefer this one, and it is your call — I am not asking for this PR or solvespace#1748 to be closed.

I also owe a retraction here: the post-solve branch preference I offered you in the comment above, and my claim that it would need no double-solving, was wrong on its own terms — choosing a different mirror after the fact needs either a second solve or a reflection, and "the mirror" only exists for specially structured systems. It also turned out to be unnecessary, since the step rule delivers the preference by itself.