GeezerVison

@phkahler Could you take a look when you have a chance? This follows the approach discussed in #1448. I'd appreciate your feedback on the axial control-point formula and whether the regression coverage is sufficient.

@alken1t15 Good job. I wasn't sure if you were going to do this one or not, so I did #1814 myself. The only thing I don't like with yours is the tests. While it does need some testing, I don't think we need to commit a unit test to the repository.

I also see you found the incomplete copying in MakeFromHelicalRevolutionOf(). That was a surprise for me too. Every thing went red and I was confused. Claude found the problem in couple minutes. I'm guessing you used some AI for this, or that bug probably would have taken a long time to find - even I would have had trouble finding it and I wrote the helix code ;-)

Yours is more compact. Mine adds a new function in ratpoly.

@ruevs which do you prefer?

EDIT: Your test of distf and dists to determine degree 2 or 3 may need a tolerance, but also for revolve they are both zero so that should be OK.

@phkahler Thanks for looking! I've removed the tests from the PR and kept them locally. The diff now only touches surface.cpp and shell.cpp.

The two approaches use the same degree-elevated arc and endpoint slope condition. Here the cubic control points are computed directly in FromRevolutionOf(), rather than adding a general conversion method. I'm happy to use the separate helper if that fits the kernel better.

I left the distance comparison exact for now: ordinary revolutions have equal offsets, while a nonzero axial displacement takes the cubic path. The formula also has a finite zero-angle limit (axial = 1/3), giving a straight translation. For helical sweeps I also skip the existing ps.Equals(pf) collapse, since nearby endpoints at a small angle need not lie on the axis. A tolerance would be reasonable if we want to deliberately treat sufficiently small axial displacements as revolutions.

@ruevs which do you prefer?

Bot(h) fail the Turing test ;-) Give the "I" (without "A") a bit of time :-)

Just out of curiosity I (this "I") ran it's "AI" on the battle of #1813 vs. #1814.
My "AI" agrees with me (this "I").

Thinking:

I prefer #1813 as currently submitted. The two PRs produce essentially the same approximation for ordinary helices, but #1813 is more narrowly scoped and handles small/zero-angle sweeps better.

I compared heads 7d711a2 (#1813) and 84bd1a5 (#1814), including the surrounding shell-generation code.

The deciding difference: endpoint collapse

#1814 retains this existing code:

if(ps.Equals(pf)) {
    ps = c;
    ct = c;
    pf = c;
}

Nearby rotational endpoints do not imply that the point lies on the axis. They can simply mean the sweep angle is small.

For example, take a control point at (100, 0, 0), revolve about the z-axis through zero angle, and translate axially by 10:

The same collapse occurs for sufficiently small nonzero angles. It is a potentially large positional error, not merely an error within tolerance.

This is inherited from master, not introduced by #1814, but #1813 explicitly fixes it for sweeps with axial displacement. Its axial formula also handles the zero-angle limit correctly.

The principal mathematics is equivalent

Both implementations:

• Degree-elevate the rational circular arc to a cubic.
• Preserve its circular projection.
• Adjust the axial controls to match the analytic helix tangent at both endpoints.
• Keep ordinary revolutions quadratic.

With w = cos(dtheta/2) and dz = distf - dists, the inner control’s axial displacement from its endpoint is, in both cases:

dz * (2*sin(dtheta/2)/dtheta) / (1 + 2*w)

#1813 computes that directly. #1814 obtains it through degree elevation followed by an axial correction.

Independent numerical sampling reproduced the advertised improvement:

Errors are relative to patch height. The two implementations agreed to floating-point roundoff in the nondegenerate cases I sampled, including reverse sweeps and an arbitrary axis. Accuracy on normal helices is not a reason to choose between them.

Implementation tradeoffs

#1813’s exact distance comparison is defensible. I would not add a tolerance merely because these are doubles. This selects quadratic versus cubic representation; it does not divide by the displacement or require it to be appreciably nonzero. The ordinary-revolution callers explicitly supply equal offsets, and the cubic formula remains well-behaved for tiny displacement and angle.

#1814’s helper is legitimate, but optional. ConvertQuadraticToCubic() correctly expresses a reusable homogeneous degree-elevation operation. That separation is its strongest advantage. Here, however, it adds an API and a temporary-curve/conversion/copying path for one caller. I slightly prefer #1813’s direct construction for this targeted change.

Both make the necessary trim fix. Copying the surface boundary’s actual degree, control points, and all weights in shell.cpp is correct in both PRs.

Recommendation

Merge #1813, perhaps borrowing #1814’s explanatory derivation comment. If you prefer the reusable-helper architecture, #1814 is a reasonable alternative after adopting the small/zero-angle handling from #1813.

Neither current diff includes regression tests—#1813’s were removed following the discussion—so I would not use committed test coverage as a differentiator.

Validation here was source review and independent numerical reproduction, not builds of both PRs or GUI/STEP round-trip testing.

Here, however, it adds an API and a temporary-curve/conversion/copying path for one caller.

I missed that in 1814. I asked it to declare the curve object outside the loop and wanted it to simply assign the points to it rather than call a constructor for each one. I do need to slow down...

Here, however, it adds an API and a temporary-curve/conversion/copying path for one caller.

I missed that in 1814. I asked it to declare the curve object outside the loop and wanted it to simply assign the points to it rather than call a constructor for each one. I do need to slow down...

Who's going to read all that generated code? - as I keep asking - not specifically in SolveSpace - for the whole planet :-) And I'm generating more as I write this :-/

@ruevs Thanks for reviewing both versions. What specific changes, if any, would you like in this PR before it can be merged? In particular:

• For the distance comparison, should I use fabs(distf - dists) > LENGTH_EPS to select cubic rows while preserving the small/zero-angle handling? Your inline comment asks for a tolerance, while the later comparison says the exact check is defensible, so I'd like to confirm your preference.
• Would expanding the derivation comment be sufficient to improve readability, or would you prefer a change to the function's structure?

Please let me know if anything else is missing or needs correcting, and I'll update the PR.

@alken1t15 Can you use the comment explanation from the other PR in this one. Then we can merge this one and drop the other completely.

@phkahler Done in fbb0c94. I've brought over the explanation from #1814 and adapted it to the direct calculation here, including the axial offsets, endpoint slope condition and error reduction. Only comments changed; the implementation is unchanged.

@alken1t15 thank you for the contribution!

@phkahler You're welcome, and thanks for the review and guidance! Are there any open, unclaimed issues of moderate to high difficulty that you'd particularly like help with at the moment? Geometry, NURBS or solver work would interest me. Ideally something reported within the last six months, with a reasonably clear scope. I've also asked about #1723 and #1774; if either would be useful to take on, I'd be happy to start there.

@alken1t15 I'm not sure if this PR caused it, but the CI tests on Ununtu are failing on master. All the regression tests pass but it's failing with a memory leak when built with sanitizers on. Could you have a look?

https://github.com/solvespace/solvespace/actions/workflows/cd.yml

@phkahler @ruevs I reproduced the leak and opened #1817 with a one-line cleanup fix. SolveSpaceUI::Clear() wasn't releasing the linked-sketch caches.

Running group/link/normal_roundtrip alone reproduces the same 45,428-byte leak in 17 allocations, including with the pre-#1813 code from b34d744, so it predates this PR. The shuffled order explains why it appeared in this run: that linked-file test ran last.

With the cleanup call added, the isolated test and all 300 tests (1,246 checks) pass with sanitizers, leak detection and OpenMP enabled, using the failing CI seed 2062209549.