Skip to content

STEP import: keep faces whose edge vertices sit slightly off their curves (#3351) - #3352

Closed
raykholo wants to merge 2 commits into
tangency-labs:mainfrom
raykholo:step-import-file-tolerance
Closed

raykholo wants to merge 2 commits into
tangency-labs:mainfrom
raykholo:step-import-file-tolerance

Conversation

@raykholo

@raykholo raykholo commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

Change

Fixes #3351.

Some exporters write an edge's vertex a little beside the end of its curve. A surface export, for example, declares an accuracy of 0.01 mm. It then joins two spline edges whose ends are 0.38 mm apart at a vertex halfway between them, so the vertex is 0.19 mm off each curve. The STEP reader refused such an edge ("edge vertices are 1.903e-1 mm off their curve") and left out the whole face. Where the face was the body's only face, the body was lost too.

main this branch
Vertex-on-curve limit the file's tolerance: 10 × its UNCERTAINTY_MEASURE_WITH_UNIT, at least 1e-3 mm (0.1 mm for the file above) the file's tolerance, or 1/1000 of the model's size when that is larger. Never more than 1 mm, the ceiling the reader already had
A vertex within the limit the edge kept its curve, so the curve ended up to the limit away from the vertex the curve's end control point moves onto the vertex, so every spline and open-conic edge meets its vertices exactly
A vertex past the limit face left out, with a warning unchanged
  • Model size. The model's size is the diagonal of the box around the file's VERTEX_POINTs. It is worked out once per file, in Exchange::vertex_extent.
  • Why 1/1000. That share is the chord error the kernel's own coarse meshes allow at that size (Body::size() * 1e-3). Moving a vertex that far changes the shape by less than those meshes already do.
  • Hostile files. Small parts stay strict: a 10 mm part allows 0.014 mm. A hostile file can't turn the check off. A stray point far away can only raise the limit to the existing 1 mm ceiling.
  • The file's declared accuracy is still read and still used for every other check: closed curves, seams and circles.
  • docs/step-import.md describes the rule.

Validation

  • Two new tests in crates/kernel/src/step_in/tests.rs:

    • a_vertex_off_its_curve_by_a_small_share_of_the_model_is_moved_onto_it: a 1 m sheet whose spline edge ends 0.19 mm beside its corner. It imports as 1 face of 1e6 mm², and every edge's curve meets its vertices within 1e-9 mm. It fails on main: the face is left out.
    • a_vertex_further_off_its_curve_than_the_model_allows_leaves_the_face_out: 0.19 mm on a 10 mm sheet is still refused. So is a 2 mm gap on a 1 m sheet whose stray vertex 1 km away makes the model look huge.
  • The kernel's whole suite passes (--release): 785 unit tests plus the integration tests. After the rebase, the 77 STEP and IGES tests pass again.

  • cargo clippy -p solidcraft-kernel --lib --tests -- -D warnings and cargo fmt are clean.

  • Real files: two STEP exports of a downloaded surface model of a standing figure. They are not attached, because their licence is unknown.

    File main this branch
    figure, 597 surface bodies 595 bodies; 2 faces left out (0.190 mm off) 597 bodies, 597 faces, no warnings about vertices off their curves
    figure with hard hat, 982 surface bodies 977 bodies; 5 faces left out (0.117 to 0.190 mm off) 982 bodies, 982 faces

    Import time is unchanged within noise (13 s to 20 s and 15 s to 16 s on a loaded machine).

Screenshots of the behavior

Reproduction: four 500 mm square sheets in a 2 × 2 grid. The top-right sheet's right edge is a straight spline that ends 0.19 mm beside its corner vertex. The file declares an accuracy of 0.01 mm.

main — the top-right sheet is missing; warning "face #211: edge vertices are 1.900e-1 mm off their curve"

before-gapped-sheets.png

this branch — all four sheets import

after-gapped-sheets.png

Not in this PR

On the same files, one face's mesh reaches 630 mm across a part about 350 mm wide. The model's box then grows from 351 mm to 816 mm in X. That face's boundary curves and all its surface control points lie between x = 161 and 227 mm. The stray triangles are therefore produced by meshing, not by the STEP reader, and this PR does not change them. They look like the far-out boundary parameter in #1400.

Integration status

Rebased onto origin/main: main moved Reader::item_solid into parallel.rs, so the snap tolerance is now passed at its new call sites (parallel.rs and conic_tests.rs). The kernel STEP tests pass (74), including both tests from this PR. This PR and #3289 both touch step_in/mod.rs and its tests, so whichever merges second needs one more rebase.

@github-actions github-actions Bot added area:infra CI, tooling, repo, docs area:kernel B-rep kernel, booleans, blends, STEP/IGES labels Oct 20, 2026
raykholo added a commit to raykholo/solidcraft that referenced this pull request Oct 20, 2026
@raykholo
raykholo force-pushed the step-import-file-tolerance branch from cfbf88c to 34b8642 Compare October 20, 2026 17:06
@bherbruck

Copy link
Copy Markdown
Collaborator

Thanks, keeping faces whose edge vertices sit slightly off their curves, instead of dropping them, is a good fix for imports, and the tests in step_in/tests.rs cover it. One ask: the comment above snap_limit in step_in/mod.rs names a commercial CAD product as an example of an exporter. Please make it generic ("some exporters"), we keep references to those products out of the code.

@bherbruck bherbruck added the needs changes review asks are open; waiting on the author label Oct 21, 2026
@raykholo
raykholo force-pushed the step-import-file-tolerance branch from 34b8642 to 652db5e Compare October 21, 2026 01:01
@raykholo

Copy link
Copy Markdown
Contributor Author

Rebased onto current main. Also checked against the public generated model step/vertices-off-curves: this branch reads 1 surface body with all 16 faces (bbox 0–200 × 0–200 × 0–8); 0.18.1 imports nothing ('face #100: edge vertices are 1.900e-1 mm off their curve').

vertices-off-curves imported on this branch: one surface body, 16 faces

…rves

Some exporters write an edge's vertex beside the end of its curve, further off
than the accuracy the file declares (a surface export declares
0.01 mm and puts a vertex 0.19 mm off two spline ends). The reader refused
such edges and left the face, and with single-face bodies the body, out.

A vertex may now sit up to the file's tolerance or 1/1000 of the model's size
(the diagonal around its VERTEX_POINTs), whichever is larger, never more than
the existing 1 mm ceiling. Spline and open-conic edges have the end control
point moved onto the vertex, so every such edge meets its vertices exactly.
The file's declared accuracy still governs closed-curve, seam and circle
checks.
@raykholo
raykholo force-pushed the step-import-file-tolerance branch from 652db5e to d0b471d Compare October 21, 2026 02:03
@jyviko

jyviko commented Oct 21, 2026

Copy link
Copy Markdown
Collaborator

Thank you, @raykholo, this is a great catch, and the write-up made the problem obvious at a glance. I'm closing this one only because the same fix (snapping an edge's spline ends onto vertices that sit beside its curve, instead of dropping the face) is already in the display and STEP-import work for #2814, which reworks this part of the importer anyway.

Your vertices-off-curves model in the models repo is exactly what we used to check it: before, SolidCraft refused the whole body; now all 16 faces come in. It goes into the regression suite for the new kernel too (#3992), so this stays fixed whatever kernel we end up on.

Since you clearly know this area well: we'd love your help in solidcore, the kernel we're evaluating. Porting cases like this one over as tests would be a perfect start.

@jyviko jyviko closed this Oct 21, 2026
@jyviko

jyviko commented Oct 21, 2026

Copy link
Copy Markdown
Collaborator

A correction to my note above: rather than our own version, your commit itself now goes in with the #2814 work, cherry-picked onto current main with your authorship. It covers more than ours did (open conics too, and the docs). The one change on top is a test of ours that needed the new snap field. Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:infra CI, tooling, repo, docs area:kernel B-rep kernel, booleans, blends, STEP/IGES needs changes review asks are open; waiting on the author

Projects

None yet

Development

Successfully merging this pull request may close these issues.

STEP import drops faces whose edge vertices sit slightly off their curves

3 participants