fix: arc-sense face rebuilds and id-keyed viewer selection#218
Merged
Conversation
Converted models showed spurious full cones/cylinders obfuscating the view.
The param-extent repair (for faces whose wire cannot trim an infinite
revolution surface) over-covered three ways, all fixed:
1. Arc sense. _sample_edge_points and make_edge_from_edge both walked
circular arcs in the circle's positive parametric direction from the
ORIENTED_EDGE's endpoints — sampling/building the complement arc
whenever the traversal was reversed, which widened the parameter extent
to the full period (a small wedge rebuilt as a full band). The occupied
arc is determined by the EDGE_CURVE alone: its own endpoints, positive
direction iff same_sense. Orientation only reverses traversal, and
readers differ on whether they pre-swap oriented-edge endpoints, so
both sites now consult the EdgeCurve.
2. Inner bounds. The rebuild filled cutout holes. Non-closure bounds
(a closure bound = full circles concentric with the revolution axis,
e.g. a closed cylinder's rims) are now re-added as hole wires —
tried in both windings, pcurves restored via ShapeFix_Face (safe
here: the face is bounded, unlike the known unbounded-face segfacts)
and accepted only when the area genuinely shrinks.
3. Area gate. A rebuilt face whose area exceeds 4x the squared bbox
diagonal of its own boundary samples is over-covering its evidence
and is dropped — a hole beats a phantom cone. Legitimate closed
revolution faces peak at ~1.2x, so the gate has >3x margin.
Face-repair counters (rebuilds, holes re-added/dropped, gate drops) ride
back from the tessellation workers and surface in the conversion stats
("rebuilds") and summary log, so a conversion now quantifies its
fidelity loss.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Real CAD models carry thousands of nodes sharing one display name, and every loaded GLB numbers its nodes from 0 — so the only globally unique identity for a picked range is (model_key, rangeId). Four lookups keyed by display name and returned the first match, highlighting/selecting the wrong element: - findNodeById scanned the whole tree comparing display names (used to sync the tree after a viewport click). Replaced by an O(1) reverse index in treeViewStore (buildRangeIndex: "model_key|rangeId" -> tree node, rebuilt on setTreeData/clearTreeData) — also drops a worker round-trip plus a full-tree recursive scan PER SELECTED RANGE. - getDrawRangeByName linearly scanned id_hierarchy by name AND read the ACTIVE model's userdata even when the cross-model link targeted another file. crossModelSelect now recovers the target model's key from its own meshes and resolves all member names in one batched worker call (getDrawRangesByMemberNames — existing, previously unused), off the main thread, all matches instead of first-match. - handleClickedNode's display-name mesh fallback could only ever bind a wrong same-named mesh; node_name (the merged-mesh name) is authoritative and a null simply means a pure group row. - AnimationController's name lookup is left with a TODO (FEA animation meshes have unique names in practice; the GLB node index is the future key). Unit test: duplicate names across two overlaid models resolve per (model_key, rangeId). Frontend typecheck clean, 33 tests green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
🚀 Profiling Results (Top 20 most expensive calls)
📈 Performance Impact per Commit
|
|
👋 Hi there! I have checked your PR and found no issues. Thanks for your contribution! PR Review:I found no pr-related issues.
Python Review:I found no python-related issues. Python Linting results:
Python Packaging results:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Two residuals after #217, both confirmed on a large converted assembly model:
Fixes
Geometry (
ada/occ/geom)EDGE_CURVEalone (its own endpoints, positive direction iffsame_sense; orientation only reverses traversal and is reader-convention-dependent). Both_sample_edge_pointsandmake_edge_from_edgenow consult it — the latter was the root cause of many failing wires (reversed arcs built as 300° complements)._make_face_from_param_extentre-adds non-closure hole bounds (tried in both windings, pcurves restored viaShapeFix_Face— safe on bounded faces — accepted only when area shrinks), and a last-resort area gate drops any rebuild exceeding 4× the squared bbox diagonal of its own boundary samples."rebuilds") and summary log.Viewer selection (frontend)
(model_key, rangeId) → tree nodeintreeViewStore, rebuilt on tree set/clear; both click handlers rekeyed to it (also removes a worker round-trip + full-tree scan per selected range).crossModelSelectresolves member names against the target model's hierarchy in one batched worker call (all matches, off the main thread) instead of scanning the active model's userdata first-match-wins.findNodeByIdandgetDrawRangeByName(name scans); removed the display-name mesh fallback in tree-click handling.Results on the reference model (~7k solids)
pythonocc 120 tests / adacpp 112 tests green; frontend typecheck + 33 tests green (new: arc-sense triple-authoring, wedge containment, closed-cylinder survival, hole re-add, gate unit, range-index uniqueness).
🤖 Generated with Claude Code