fix(cadling): repair ShapeIdentityRegistry + face UV-grid on pythonocc 7.8#13
Merged
Conversation
…c 7.8
Two latent bugs made core B-Rep helpers silently inert on pythonocc 7.8;
both hid because every existing test mocked the OCC path or guarded its
assertions with `if result is not None:`.
1. face_identity.py imported `from OCC.Core import topods`, which raises
ImportError on pythonocc 7.8 ("cannot import name 'topods' from
'OCC.Core'"). That set HAS_OCC=False at import, so
ShapeIdentityRegistry.register_all_* registered NOTHING and every
face/edge index was -1. Fixed to `from OCC.Core.TopoDS import topods`.
2. uv_grid_extractor.py computed the face trimming mask with a per-point
`face.visibility_status(uv)` loop; numpy-float UVs raised a gp_Pnt2d
SWIG overload error that the broad except swallowed, so
FaceUVGridExtractor.extract_uv_grid returned None for EVERY face.
Replaced with occwl's `uvgrid(method="inside")` (returns the mask
directly); removed the now-dead TopAbs_IN/TopAbs_OUT imports.
Regression tests (real OCC box, assertions unconditional) added to
test_face_identity.py and test_uv_grid_extractor.py; both fail before the
fix (0 faces registered; None grid). Verified: registry now 6/12/8 on a
box, face UV-grid (10,10,7); 41 topology+geometry tests pass.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Reviewer's GuideFixes two latent OCC integration bugs in cadling: the ShapeIdentityRegistry now correctly imports topods from OCC.Core.TopoDS so real OCC shapes are registered, and FaceUVGridExtractor now uses occwl’s uvgrid(method='inside') to build a trimming mask that works on pythonocc 7.8, with new regression tests that exercise real OCC box shapes without guarding on None results. Sequence diagram for updated FaceUVGridExtractor UV-grid extractionsequenceDiagram
participant Caller
participant FaceUVGridExtractor
participant occwl_uvgrid as uvgrid
participant Logger
Caller->>FaceUVGridExtractor: extract_uv_grid(face, num_u, num_v)
FaceUVGridExtractor->>occwl_uvgrid: uvgrid(face, num_u, num_v, method=inside)
occwl_uvgrid-->>FaceUVGridExtractor: inside_grid
alt inside_grid is None
FaceUVGridExtractor->>Logger: warning("Failed to extract trimming mask from face")
FaceUVGridExtractor-->>Caller: None
else inside_grid is not None
FaceUVGridExtractor->>FaceUVGridExtractor: inside_grid.astype(np.float32)
FaceUVGridExtractor-->>Caller: uv_grid[num_u, num_v, 7]
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
TestFaceUVGridRegression.test_box_face_uv_grid_succeeds, consider strengthening the trimming assertion toassert np.all(trimming == 1)for a planar box face (rather than justtrimming.sum() > 0) to better lock in the expected fully-inside behavior. - In
uv_grid_extractor.extract_uv_grid, you now handle aNoneinside_gridwith a warning andNonereturn; if there are known conditions whereuvgrid(..., method="inside")can legitimately returnNone, it might be worth logging more context (e.g., face type or parameters) to aid debugging when this happens.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `TestFaceUVGridRegression.test_box_face_uv_grid_succeeds`, consider strengthening the trimming assertion to `assert np.all(trimming == 1)` for a planar box face (rather than just `trimming.sum() > 0`) to better lock in the expected fully-inside behavior.
- In `uv_grid_extractor.extract_uv_grid`, you now handle a `None` `inside_grid` with a warning and `None` return; if there are known conditions where `uvgrid(..., method="inside")` can legitimately return `None`, it might be worth logging more context (e.g., face type or parameters) to aid debugging when this happens.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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
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.
What
Two latent bugs made core
cadlingB-Rep helpers silently inert onpythonocc 7.8. Both hid because every existing test either mocked the OCC
path or guarded its assertions with
if result is not None:, so the real-OCCfailure was never exercised.
Bugs fixed
ShapeIdentityRegistryregistered nothing —lib/topology/face_identity.pyimported
from OCC.Core import topods, which raisesImportErroronpythonocc 7.8 (
cannot import name 'topods' from 'OCC.Core'). That setHAS_OCC = Falseat import, soregister_all_faces/edges/verticesreturnedempty and every
face_index/edge_indexwas-1. Fixed tofrom OCC.Core.TopoDS import topods.Every face UV-grid failed —
lib/geometry/uv_grid_extractor.pybuilt thetrimming mask with a per-point
face.visibility_status(uv)loop; passingnumpy-float UVs raised a
gp_Pnt2dSWIG overload error that the broadexceptswallowed, soFaceUVGridExtractor.extract_uv_gridreturnedNonefor every face. Replaced with occwl's
uvgrid(method="inside")(returnsthe
[U,V,1]mask directly); removed the now-deadTopAbs_IN/TopAbs_OUTimports.
Tests
Regression tests added to
test_face_identity.pyandtest_uv_grid_extractor.pyusing a real in-memory OCC box, with unconditional assertions (no
if result is not None:guard). Both fail before the fix and pass after:(10, 10, 7)with a populated trimming mask (wasNone)Verified: 129/130 tests pass across
tests/unit/lib/{topology,geometry}(the onefailure,
test_distribution_analyzer::test_analyzers_produce_consistent_counts,is a pre-existing, unrelated counting issue). Lint-neutral (no new ruff findings
on either file vs
main).Why it matters
These helpers are reused across cadling's B-Rep processing (coedge extraction,
graph builders, geometry analysis). Restoring them fixes silently-degraded
behavior (everything keyed on
face_indexwas reading-1/0; all faceUV-grids were empty) on the current pythonocc 7.8 toolchain.
🤖 Generated with Claude Code
Summary by Sourcery
Restore correct OCC-backed behavior for B-Rep face identity registration and face UV-grid extraction on pythonocc 7.8.
Bug Fixes:
Tests: