GeezerVison

Ugh. I thought I was commenting on specific code sections.

"Without that change, Boolean classification could test a point lying off the other incident face and make an incorrect keep/discard decision. The rational/non-affine-plane cube-union regression exposed precisely that failure."

So part of this was in response to a boolean failure?

My main concern is using an overly complicate IsPlanar() check for speedups. And also that the check is testing way more than solvespace needs. On the other hand, if you're going to be linking STEP files who knows what's in those. I always thought we should implement enough STEP to link our own output step files. I hope you're thinking along those lines because STEP is huge and handles NURBS up to something like degree 11, with knots and everything.

@ruevs BTW now I see what I've been doing to you! ;-)

Also, that's a really really big commit message, and I don't like the language GPT uses ;-b

@phkahler I agree with you about the performance - as soon as I saw the new functions that was my first thought - two integer comparisons replaced by all this floating point math... But the new logic is correct - that is why I opened the PR early and requested your review - without mentioning any of this - to not bias you :-)

Now to the point - the LLM is good at this so I asked it to read your review comments and think about it. Here is the result:

Next I'll ask it to implement it's own suggestions.

I agree with phkahler’s overall direction, but I would optimize the implementation rather than undo the correctness fixes. The PR mixes two things: removing invalid geometric assumptions, and recognizing more kinds of planar surfaces. The latter need not make every common kernel operation more expensive.

I read all four comments and traced the affected paths. Here is my assessment.

1. boolean.cpp: the cost concern is right; the degree-one argument is not generally right

The new code sends every exact straight edge through SBezier::ClosestPointTo(), which still samples and iterates for degree-one curves. That is unnecessary work on ordinary native models.

However, a straight XYZ trim does not necessarily have a straight UV image. For example, on the planar patch

S(u,v) = (u, v·(1 + u/4), 0)

the straight trim y = 0.5 has UV endpoints (0, 0.5) and (1, 0.4). Their UV midpoint maps to y = 0.50625, not 0.5. The PR’s existing straight-trim regression exercises this problem.

I would therefore not restore the unconditional exact.deg != 1 exclusion. Instead, handle exact degree-one trims with a direct XYZ line projection—or the XYZ midpoint of the current segment—then recover UV. That preserves correctness without invoking the generic curve-refinement routine.

2. ratpoly.cpp: agree, but distinguish call overhead from classification work

IsAffine() already starts with the degree check and short-circuits. Adding an outer degree check does not avoid substantial geometric work on higher-degree surfaces; it mainly avoids an out-of-line function call when the compiler cannot inline it.

Keeping that cheap guard is reasonable. Making the small predicate available for inlining could also avoid duplicated arithmetic between the predicate and the inverse calculation. The weight checks and corrected skew-axis inverse should remain.

A potentially larger performance problem is EqualsExactly(). Ordinary floating-point construction and transformation can make intended-affine native planes fail that bit-for-bit parallelogram check. They then lose the direct inverse. In particular, the mustConverge=false fallback can perform a 20 × 20 initial search.

I would measure the affine fast-path hit rate on translated and rotated native models—not just the predicate’s execution time. Simply loosening the check to LENGTH_EPS would be the wrong remedy, because that could restore the small-warp correctness bug.

3. raycast.cpp: this is where I agree most strongly

IsPlanar() currently makes three control-net passes, including finding a suitable plane and normalizing its normal. That is a substantial replacement for two integer comparisons, especially when repeated for rays against curved surfaces.

For this optional shortcut, a cheap degree-(1,1) check followed by a fourth-corner coplanarity test is sensible. Degenerate configurations should fall back conservatively, and the tolerance should remain roundoff-scale rather than model tolerance.

Higher-degree patches can take the general intersection route, provided its behavior is validated for the supported cases. We do not need to discover every possible plane during every ray query.

His lathe observation is also correct: MakeFirstOrderRevolvedSurfaces() already converts the ordinary planar lathe patches to degree (1,1).

4. surfinter.cpp: the block has a correctness purpose, but needs clearer separation

The overall clipping step is an optimization. The non-affine branch makes that optimization valid for the newly recognized planes.

The old calculation assumes:

u(t) = up + t·ud
v(t) = vp + t·vd

That holds for an affine parameterization, but not for an arbitrary rational or non-affine planar patch. Using it anyway can incorrectly shorten or eliminate a real intersection.

The added block instead bounds the XYZ intersection line conservatively; subsequent splitting and trimming remove excess portions. It is not merely another early-out.

I would retain that distinction, but:

• Keep the ordinary affine route inexpensive.
• Extract the conservative clipping into a clearly named helper.
• Reuse the bounding boxes already calculated at the start of IntersectAgainst().
• Avoid repeatedly rediscovering planarity inside loops that already know the surface is planar.

My recommendation

Make a focused performance revision around cheap common-case recognition, direct straight-line handling, and reuse of classification results. Keep the full control-net checks where actual planarity affects correctness; do not mechanically replace them everywhere with degree checks.

Then compare Release builds against the parent commit using large planar Boolean models, translated/rotated versions, ordinary curved/lathed models, and the existing rational/non-affine regressions. A global classification cache would need care because control nets are publicly mutable and some paths run in parallel.

The extra work is evident from the code, but I have not measured an end-to-end slowdown. His review identifies real optimization opportunities; it does not invalidate the underlying planarity-versus-affinity correction.

No code or branch changes made.

Assistant

Committed 791d9efe — NURBS: Preserve inexpensive planar surface fast paths, on NURBSPlanes directly above 0c63d8ca.

The change covers every original replacement, not just the reviewed locations:

• Cheap bilinear planarity checks and inline, strict affine recognition.
• Better inverse initialization without relaxing correctness checks.
• Direct straight-line Boolean probe projection.
• Operation-local classification reuse in intersection, merging, coincidence and curve containment.
• Classification passed through triangulation; unrolled common-case section checks.
• Cylinder-first recognition while retaining direct higher-degree planar ray intersections.

Validation: Release and Debug/OpenMP both passed 312 cases / 1,867 checks, with shuffle seeds 1 and 2026. Existing regressions remain, with additional kernel coverage.

Same-machine Release load/regenerate benchmarks versus 0c63d8ca:

Tracked working tree is clean. STEP work, fixtures, build/bin, and pre-existing untracked directories remain untouched. Nothing pushed.

It went to town a bit.... I'm going to bed :-)

The new code sends every exact straight edge through SBezier::ClosestPointTo(), which still samples and iterates for degree-one curves. That is unnecessary work on ordinary native models.

However, a straight XYZ trim does not necessarily have a straight UV image. For example, on the planar patch

S(u,v) = (u, v·(1 + u/4), 0)

the straight trim y = 0.5 has UV endpoints (0, 0.5) and (1, 0.4). Their UV midpoint maps to y = 0.50625, not 0.5. The PR’s existing straight-trim regression exercises this problem.

There is some confusion here. In some CAD software they do have trim curves as Beziers or maybe even rational ones in UV space. In solvespace ALL trims are in XYZ model space. The exact flag means the curve is exact in XYZ. It also means it follows exactly along the surface. A curve in XYZ may be a straight line in UV, thats what curved surfaces do. So I'm not sure why it cares that a straight line in XYZ might not be straight in UV, the XYZ one is the actual exact trim curve. All of our curves have a PWL representation, but the ones tagged exact also have an exact XYZ rational bezier representation.

The new code sends every exact straight edge through SBezier::ClosestPointTo(), which still samples and iterates for degree-one curves. That is unnecessary work on ordinary native models.
However, a straight XYZ trim does not necessarily have a straight UV image. For example, on the planar patch

S(u,v) = (u, v·(1 + u/4), 0)

the straight trim y = 0.5 has UV endpoints (0, 0.5) and (1, 0.4). Their UV midpoint maps to y = 0.50625, not 0.5. The PR’s existing straight-trim regression exercises this problem.

There is some confusion here. In some CAD software they do have trim curves as Beziers or maybe even rational ones in UV space. In solvespace ALL trims are in XYZ model space. The exact flag means the curve is exact in XYZ. It also means it follows exactly along the surface. A curve in XYZ may be a straight line in UV, thats what curved surfaces do. So I'm not sure why it cares that a straight line in XYZ might not be straight in UV, the XYZ one is the actual exact trim curve. All of our curves have a PWL representation, but the ones tagged exact also have an exact XYZ rational bezier representation.

@phkahler the model replied:

Thinking:

He is correct about how SolveSpace stores trims. My earlier explanation should have pointed directly at the probe calculation—not suggested that SolveSpace stores UV trim curves.

The distinction is: the stored trim is exact, but the initially constructed probe is not necessarily on it.

In src/srf/boolean.cpp:432, EdgeNormalsWithinSurface() starts with:

Point2d muv = (auv.Plus(buv)).ScaledBy(0.5);
*pt = PointAt(muv);

That point comes from evaluating the surface at an averaged UV coordinate, not from evaluating sc->exact.

The call chain confirms this:

• MakeTrimEdgesInto() converts the XYZ PWL vertices to UV when asked for MakeAs::UV.
• MakeCopyTrimAgainst() passes those UV edge endpoints to EdgeNormalsWithinSurface().
• Previously, an exact degree-one curve bypassed the correction onto the XYZ trim.

The example satisfies his definition of “exact”

For the existing regression’s non-rational variant:

Surface:        S(u,v) = (u, v·(1 + u/4), 2)
Exact XYZ trim: C(t)   = (t, 0.5, 2)

Every point of that XYZ line lies exactly on the surface: choose u=t, v=0.5/(1+t/4).

Nevertheless:

Endpoint UVs:                 (0, 0.5), (1, 0.4)
Their UV midpoint:            (0.5, 0.45)
Surface evaluated there:      (0.5, 0.50625, 2)

The probe misses the exact trim—and an incident face at y=0.5—by 0.00625 mm.

So his description and the fix are consistent. The updated code projects this computed probe onto the actual exact XYZ line, then recovers its UV coordinates. No UV Bézier trim representation is involved or needed.

I would respond by showing those two lines of EdgeNormalsWithinSurface() first; that is the missing connection in my earlier explanation.

No code changes made.

On the other hand, if you're going to be linking STEP files who knows what's in those. I always thought we should implement enough STEP to link our own output step files. I hope you're thinking along those lines because STEP is huge and handles NURBS up to something like degree 11, with knots and everything.

@phkahler I (or rather the future Übermensch) have STEP import/linking mostly working locally. Yes; it will only support geometry that SSurface can already represent. However that is still much more geometry than what can be constructed within SolveSpace with the available requests. For example saddles - curved surfaces with 1 == degm && 1 == degn. They are the reason for this PR.

The detection of higher order surfaces as actual planes is not needed, but presumably the LLM added it as an extra optimisation (in the otherwise slower new function).

The distinction between planar and affine SSurface objects would not have occurred to me, and (again) for geometry constructed within SolveSpace they overlap - all planar faces we can construct are affine (I'm 98% sure), but this is not the case for geometry in an imported STEP file.

This PR, the step export fix in #1816 and one more much bigger export fix (that I need to review carefully before opening a PR) all come from the work on STEP import/linking.

The final goal is to be able to link in all STEP files that we can export, as well as any "external" ones that we can represent as NURBS with our SSurface.

The importer is intentionally very strict about the file format - both syntactically and as geometry - and rejects anything invalid or not representable by SSurface with an obnoxiously detailed error message. My hope is that if it ever gets merged and people start linking STEP files, they can easily open issues with failing ones - to improve both us and maybe the software that exported them. I'll push the branch for you to play with when I sit on the computer again.

BTW now I see what I've been doing to you! ;-)

:-D

And now I'm doing it to you, and myself :-D Probably with same reasoning - STEP linking/import is "worth it" (and curiosity) :-D

P.S. Electricity killed the cat. ;-)

And now I'm doing it to you, and myself :-D Probably with same reasoning - STEP linking/import is "worth it" (and curiosity) :-D

@ruvs That would exactly why. Next release will have all the bigger things that people have wanted for 10 years.