feat(viewer): radius/diameter measure with a coincident-point refusal (#2737) - #2973
feat(viewer): radius/diameter measure with a coincident-point refusal (#2737)#2973BIMvoice wants to merge 6 commits into
Conversation
…2737) Adds pure geometry for the radius/diameter item of #2737 (item 2; item 3 already shipped in #2514, minimum distance is being worked separately in #2805/#2815). A picked arc is scoped to explicit multi-point picks along a circular edge, mirroring edge-face-angle.ts's reasoning: SnapTarget.metadata.vertices yields tessellation chords, not topological edges, so fusing them into "the edge" would report a tessellation artefact. Silhouette and swept-profile provenance are out of scope for this pass; RadiusSource is shaped to add a parametric-profile variant later without reshaping the outcome type, mirroring QuantityBasis's net/gross/unqualified precedent in quantities.ts. The correctness bar is refusal, not accuracy: a Kasa least-squares circle fit explains near-collinear input almost perfectly with an enormous radius, so the fit's own residual cannot be the refusal signal. The gate instead measures sagitta - the fit-independent perpendicular deviation of the picks from their own chord - against a 100 um floor that sits between the measured straight-run noise ceiling (the 15.3 um snap/weld tolerance already anchoring three-point-angle.ts and edge-face-angle.ts) and the tessellator's own minimum curvature (381 um-0.5 mm, per snap-edge-runs.ts and curves_2d.rs). Only once curvature clears that floor is a circle fitted, and the fit's own residual is checked against the same floor (3x budget) to refuse a curve that isn't circular. Every fixture is off-origin and off-axis; the straight-run fixture mirrors the exact four-collinear-tessellation-chord shape #2199 measured on a real model. Mutation-tested: disabling the curvature gate and swapping the fitted center's coordinates each break a distinct set of assertions.
…2737 item 2) Adds a fourth Measure mode, 'radius', following the polyline/angle wiring pattern: unbounded click accumulation (min 3 points), finished via double-click or Enter (no fixed pick count, unlike angle). The panel derives the fit live on every render via fitRadius/formatRadius from the already-built pure module, so the in-progress readout reaches "fitted" or an explicit refusal ("Not circular (straight/poor fit)") without a separate finish step, and a finished measurement's readout updates automatically if the fit maths ever changes. Touches: store/types.ts (MeasureMode, ActiveRadius, RadiusMeasurement), measurementSlice.ts (state + actions, wired into every existing reset/clear home), MeasurePanel.tsx (mode cycle, list, live readout), selectionHandlers.ts (handleRadiusClick, finishRadiusFromDoubleClick), useMouseControls.ts and useKeyboardShortcuts.ts (double-click/Enter finish, Esc cancel). Does not touch quantities.ts / MeasureQuantities.tsx or radius.ts itself.
Three (or near-three) identical picks -- a user double-clicking the same
snap point -- were the one input shape radius.test.ts did not exercise.
Verified against the code rather than assumed: coincident points are
refused via the sagitta floor ("no-curvature", sagittaM: 0), not the
determinant-degeneracy guard, since the sagitta computed over identical
points is exactly zero and fires first. Mutation-tested against
distanceToLine's zero-length branch to confirm the sagittaM assertion is
load-bearing.
…nts' into consolidated/measure-radius-ui-and-coincident-points
|
Warning Review limit reached
Next review available in: 55 minutes Limit details: You’ve used all 2 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (11)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Viewer benchmark⚠ 1 metric(s) exceeded the regression threshold (advisory only, not blocking). 01_Snowdon_Towers_Sample_Structural(1).ifcBaseline recorded 2026-07-01T20:31:05.538Z on github-actions ubuntu-latest, viewer-benchmark-ci (headless Chrome, SwiftShader ANGLE), production build.
AC20-FZK-Haus.ifcBaseline recorded 2026-07-01T20:30:59.972Z on github-actions ubuntu-latest, viewer-benchmark-ci (headless Chrome, SwiftShader ANGLE), production build.
Refresh the baseline from a CI run: dispatch the Benchmark workflow with |
This PR has NOT been reviewed by CodeRabbit, and the green check does not mean it wasTwo separate things went wrong, and neither is visible from the checks list.
So the automated result for this PR is UNKNOWN, not clean. I am not reporting "no issues found", because nothing ran that could have found them. What follows is my own review: reading the diff, and running the actual code where a claim could be tested rather than argued. Everything below that reports a number is something I executed, not something I reasoned to. Reviewed at Real:
|
| picks | click orders giving a wrong radius | worst radius reported |
|---|---|---|
| 3 | 0.0% | 1.800 m |
| 4 | 32.8% | 484,639 m |
| 5 | 0.0% | 1.800 m |
| 6 | 12.9% | 86,259 m |
Three picks is always safe (Newell over three points is just the triangle normal). Even counts are where the cancellation bites.
This does not depend on my noise assumption. With picks made exactly coplanar, the way pointsOnCircle builds them, and changing nothing but the count from 5 to 4, 8 of the 24 orders still fail on that same fixture: 3 return a wrong radius and 5 refuse a perfect circle outright.
Concrete failure scenario, fully reproducible. A 90 degree arc of a true 2.000 m radius edge, picks A(0deg) B(30deg) C(60deg) D(90deg), a few tens of microns out of plane as real mesh picks are. Clicking them in order gives R 2.000 m / D 4.000 m. Clicking A, B, D, then going back for the gap at C gives:
R 3854.985 m / D 7709.970 m (fitted from 4 tessellation points)
8 of the 24 orders give that. "Click three along the arc, notice a gap, click the fourth into it" is an ordinary gesture, and the panel hint ("Click 3+ points on a circular edge") does not ask for any particular order.
Four picks is also what the documented finish gesture produces by default. handleRadiusClick appends on every click, a physical double-click dispatches click, click, dblclick, and finishRadius drops one trailing near-duplicate. So placing four points on the arc and double-clicking the last one records [p1, p2, p3, p4, p4] minus the duplicate, which is exactly four picks.
There is a second, milder face of the same bug. With exactly coplanar picks the sum can cancel to exactly zero, planeNormal returns null, and the fit is refused as 'no-curvature', which the panel renders as "Not circular (straight)" for a perfect circle. Four points at 0/90/180/270 degrees clicked as 90deg, 270deg, 0deg, 180deg does this, and clicking two diametrically opposite points first is a natural thing to do in a tool labelled radius/diameter.
The fix wants a plane fit that does not read the pick order. The smallest eigenvector of the covariance matrix of the centred picks is the usual one, and unlike Newell it is defined by the point set alone. Taking the normal from the largest-area triangle among the picks would also work.
Real, and the reason the above shipped: radius.test.ts:98 cannot fail
apps/viewer/src/components/viewer/tools/measure-modes/radius.test.ts:98-110 is called "does not depend on the order the arc was picked in" and it asserts exactly the property that is broken. It passes for two independent reasons, and both of them go away in the running app:
- It uses 5 picks. That is one of the counts where no ordering fails. Changing only the count to 4 in that same fixture breaks it.
pointsOnCirclebuilds points from an orthonormal basis, so they are exactly coplanar to float precision. Real picks come off a tessellated f32 mesh through snapping and are not.
It also tries only three of the 120 orderings rather than sweeping them. A test written to pin order-independence should permute exhaustively at the counts a user actually clicks, which includes 4.
Lower severity: the poor-fit gate is unreachable at the minimum pick count
radius.ts:308, with MIN_RADIUS_POINTS = 3. Three non-collinear points always lie exactly on one circle, so residualM is identically 0 at three picks and residualM > SAGITTA_FLOOR_M * RESIDUAL_BUDGET can never be true there. The second half of the documented two-check gate does nothing at the count the tool asks for.
That is not wrong on its own (three picks cannot be an S-bend), but it does mean the readout carries no fit-quality signal at all at n=3 while still being formatted to millimetres. Measured: a true 20 m radius, three picks over a 0.30 m span, one snap tolerance of noise each, reports between 19.37 m and 20.29 m across trials, never refusing, printed as e.g. R 19.530 m. toFixed(3) claims a precision the reading does not have at three picks.
Checked and found fine
- Radius picks are not drawn in the 3D overlay (
MeasurementVisuals.tsxhandlesactivePolylinebut notactiveRadius). Not a regression: angle mode (Measure: angle picking — three-point, edge-to-edge, face-to-face (from #2199 §4) #2735) is not drawn there either, so radius matches the existing precedent. updateMeasurementScreenCoordsreprojects radius picks through the sharedreprojectPoint, which setshasChangesitself, so the early exit cannot strand them.setMeasureModeclearsactiveRadiuson leaving,clearAllresets both new fields, and the double-click handler's polyline-then-radius fallthrough is safe because at most one of the two can be active.finishRadiusreturning{}below the minimum leaves the sequence in progress rather than dropping the picks, and both Enter and double-click surface a toast, so it is not silent.
|
This implements the diameter/radius half of #2737 but has no If it only covers part of #2737 (the issue also asks for minimum distance and a temporary reference), then leaving it unlinked is arguably right and the issue should stay open with the remainder recorded. Either way it is worth being deliberate about rather than accidental. |
…rder (#2737) `planeNormal` summed consecutive cross products with a `% points.length` wrap — Newell's method over the pick sequence treated as a closed polygon. That made the fitted plane a function of the order the user clicked in. When the click order traces a self-intersecting path the lobes have opposing winding, their area vectors cancel, the sum collapses toward zero, and the surviving direction is out-of-plane noise. The (u, v) basis is then not the arc's plane, the points project near-collinear, and the Kasa fit explains them with an enormous circle. Neither half of the two-check gate could see it: the sagitta is measured on the raw 3D picks and never consults the plane, and the residual is near zero precisely BECAUSE the points do sit on the huge circle a collapsed basis fits them with. Both passed and the reading came back `fitted`. Measured against the unmodified module, 1.8 m arc, 86 degrees, one MIN_SNAP_TOLERANCE of pick noise, every ordering swept: 3 picks 0.0% wrong 4 picks 30.5% wrong, worst 198.887 m 5 picks 0.0% wrong 6 picks 0.3% wrong + 12.7% refused, worst 576.989 m Even counts are where the cancellation bites, and four picks is what the documented finish gesture produces by default. It is not a noise artefact either: with exactly coplanar picks, changing only the count 5 -> 4 takes 8 of the 24 orders from correct to wrong. The milder face is a sum that cancels to exactly zero — `planeNormal` returned null and the panel rendered "Not circular (straight)" for a perfect circle, which four points at 0/90/180/270 clicked as 90,270,0,180 does. Replaced with the eigenvector of the smallest eigenvalue of the picks' covariance matrix (cyclic Jacobi on the symmetric 3x3): the total-least-squares plane, and a function of the point SET, since covariance is a sum of per-point outer products with no term referring to a point's neighbour or index. Picks are also run through a canonical lexicographic order first, so the answer is bit-identical under permutation rather than merely close — float addition is not associative, and that is a property a test can assert exactly. After the fix all four counts sweep clean at 0.0%. The test named "does not depend on the order the arc was picked in" asserted this exact property and could not fail: it used five picks (a count where no ordering fails — change only the count to 4 and it breaks) and points from `pointsOnCircle`, which builds from an orthonormal basis and so are coplanar to float precision, unlike real picks off a tessellated f32 mesh after snapping. It also tried 3 of 120 orderings. Replaced with an exhaustive permutation sweep at 3/4/5/6 picks over picks perturbed off-plane by multiples of MIN_SNAP_TOLERANCE, asserting both directions — that no order reports a wrong radius, and that no order refuses a real arc. Also documents, rather than changes, a lower-severity finding: at exactly MIN_RADIUS_POINTS the fit-quality half of the gate has no reach. Three non-collinear points lie exactly on one circle, so `residualM` is identically zero and `poor-fit` is unreachable there. Measured: a true 20 m radius picked three times across a 0.30 m span reports 19.098-21.003 m over 500 trials and refuses none. The module doc now says so, and a test pins it so the doc and the code cannot drift apart again. Raising the minimum pick count would close it, but that is a change to the tool's documented gesture rather than a fix to this defect.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
|
Both real, both fixed, pushed as The plane fitReplaced Newell with the smallest eigenvector of the covariance matrix of the centred picks (cyclic Jacobi on the symmetric 3×3), which is your first suggestion. I went with it over the largest-area triangle because it uses every pick rather than three, and the triangle variant puts the whole plane at the mercy of noise on the three it happens to select. Covariance is a sum of per-point outer products — no term refers to a point's neighbour, index, or arrival order. That alone is order-blind in its maths but not in its last bits, because float addition is not associative. So Sweep, before and afterYour fixture: 1.8 m arc, 86°, one
Your ABDC gesture: 2828.276 m before, 2.000 m after, and now bit-identical to ABCD. The 90/270/0/180 quad that was refused as I also confirmed your point about the finish gesture by reading the chain: The test that could not failReplaced. It now permutes exhaustively at 3/4/5/6 picks with off-plane perturbation, and asserts bit-identical output rather than merely close. RED against the old Both directions are pinned: the sweep requires Your third point — documented, not fixed, and here is whyYou are right that What I did fix is the overclaim: the module doc described the gate as two checks when at three picks it is one. It now says so, with a figure I measured rather than inherited — true 20 m radius over a 0.30 m span, one snap tolerance each, 19.098–21.003 m across 500 trials, zero refusals — and states that the millimetres Say the word if you would rather Two of my own comments were wrongCorrected while in there: a sign tie-break comment said the comparison "falls through" when x actually wins, and a test comment justified the S-bend fixture in terms of the Newell sum I had just removed. I checked that second one by running rather than reasoning — symmetric and antisymmetric zigzags now both reach
On the closing keyword: this covers the diameter/radius half of #2737 and not the minimum-distance or temporary-reference parts, so I have left it unlinked deliberately rather than by accident. Happy to add |
2 participants
Addresses part of #2737 — the radius/diameter measure, plus the coincident-point refusal and its UI wiring.
Verified
measurementSlice.radius.test.ts— 12 passed / 0 failedturbo run build --filter="@ifc-lite/viewer...", 38/38)The refusal contract holds through the real store path, not just the pure module
The pure-module tests cover coincident points, but the question that matters is what the panel does. Attack run: three exactly coincident world points pushed through the store (
startRadius/addRadiusPoint/finishRadius— not by callingfitRadiusdirectly), thenformatRadius(fitRadius(...)), which is the same callMeasurePanel.tsx'sformatRadiusPointsmakes on render:Matching the pure-module result exactly. The UI/store path does not bypass or contradict the refusal.
Also confirmed while there: the double-click dedup (
isDuplicateClickPoint) compares screen pixels, not world coordinates, and strips only one trailing point. It is a browser-artefact filter, unrelated to world-space coincidence, so it cannot accidentally hide a genuine coincident-point case or manufacture a spurious one.Scope
This is the radius/diameter half of #2737. The minimum-distance and temporary-reference parts of that issue are not in this PR — a separate fix scoped
buildElementMeshtomodels.get(ref.modelId)after it was found scanning all models byexpressIdand reporting 0.0 mm instead of 9.9 m; that lands on its own branch rather than riding along here.🤖 Generated with Claude Code