generate: make GenerateAll non recursive and optimise pruning logic - #1688

@iscgar I'm in favor of this change, and now is a good time to make it - after a release, not just before. @ruevs what do you think?

@phkahler I have not looked at this PR closely - sorry. I'll try to do it next week.

This results in a reduction of up to 50% in regeneration duration since it only prunes sequentially. Pruning should be further optimised so that it avoid traversing the full request and constraint list for every group again.

I had this change sitting in my fork for a few months alreay and just forgot to submit it. Sorry for the delay.

Good Idea since I was not very impressed by double regeneration (but it was me who did it).
But it is better to make your changes separated (optimize pruning and removing double regeneration for chord tolerance calculation should be different commits.)

@iscgar Could you resolve the conflicts with this PR? I'd like to merge it now that v3.3 is done.

@copilot resolve the merge conflicts in this pull request.

Since copilot sucks, I asked Claude (the free one on the web) to resolve this conflict. I said to take the PR options in all 3 cases and drop the code from master. It then said to add the two cleartags calls:

`
// Prune any requests and constraints that belong to non-existent groups

SK.request.ClearTags();
SK.constraint.ClearTags();

`

When I asked if he's sure, further analysis suggests this may cause a regression of PR #1744 which is probably what caused there to be a conflict in this area. Here is the conversation after fixing the conflict and adding those two lines:
====== claudversation==== Me:
Are you sure about adding those two lines? Are they cleared elsewhere and this is adding to the tagged list, or is it a bug without those?
Checking whether the ClearTags lines are actually necessary.
Checking whether the ClearTags lines are actually necessary.
====== claudversation==== Claude:
Good question. I recommended that from the conflict text alone, without checking why master added them. I’ll check master’s history.
Read 2 pages, searched the web

Yes, it’s a real bug without them, but looking at this more closely, those two lines alone don’t fully fix the merged code. I was too quick to call it done.

Why master added them. A failed solve leaves tags behind. In System::Solve, the didnt_converge: branch calls SK.constraint.ClearTags() and then sets c->tag = 1 on every constraint it couldn’t satisfy, so they can be reported to the user. Nothing clears those tags afterward. Before master’s fix (#1744, as far as I can tell from a fork’s PR snippet), the next prune pass hit RemoveTagged() and silently deleted those constraints. That is the data loss the ClearTags() calls prevent. This comes from the system.cpp source and a search snippet, not from master’s commit history, which GitHub blocks me from reading.

Why my placement isn’t enough. Your PR keeps prune tags alive for the whole GenerateAll loop and deletes everything tagged at the end. But SolveGroupAndReport runs inside that loop. If a group fails to solve:

didnt_converge calls ClearTags(), which wipes any prune tags set earlier in the pass, so those items won’t be removed.
It then tags the unsatisfied constraints, and the final RemoveTagged() deletes them. That is the #1744 bug again, now in the same pass.

The two lines I gave you still fix stale tags carried in from before GenerateAll, so keep them. They don’t cover tags set during the loop.

What I’d do:

Resolve the conflicts as I described, with the two ClearTags() lines, and commit. It’s your PR branch, so you can fix it afterward.
Test it. Make a sketch with two contradictory constraints (say, two different dimensions on the same line), let it fail to solve, and trigger a regenerate. If the constraints disappear, the problem is confirmed.
If it’s confirmed, tell the maintainers on the PR. The fix probably means not sharing the tag field between pruning and the solver (for example, removing pruned items per group instead of at the end), and that’s a design decision for you and the reviewers. I haven’t tested any specific fix, so I’d rather not hand you code that might break something else.

Sources: solvespace system.cpp (master), BoykoNeov fork PR #7 (mentions the tag-clearing fix)

And with the merge commit added to this PR it's failing the regression test for #1744 hahaha.... I'm gonna cry.

And with the merge commit added to this PR it's failing the regression test for #1744 hahaha.... I'm gonna cry.

This is tough... I tried a few variants quickly. Did not work. Will try again tomorrow.

And by the way no merge commits.

Reapplied in #1798