feat(geometry): native geometry-data export + Python (PyO3) binding - #1316
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
📝 WalkthroughWalkthroughAdds a Changesifclite-geom geometry export and Python bindings
Sequence DiagramsequenceDiagram
participant PyCaller as Python caller
participant ifclite_geom as ifclite_geom (PyO3 module)
participant WorkerThread as Worker thread (8 MB stack)
participant ProcessGeom as process_geometry
participant BuildExport as build_geometry_data_export
PyCaller->>ifclite_geom: geometry_data_buffers(ifc_bytes) or geometry_data_json(ifc_bytes)
ifclite_geom->>ifclite_geom: py.allow_threads
ifclite_geom->>WorkerThread: spawn named thread
WorkerThread->>ProcessGeom: process_geometry(ifc_bytes)
ProcessGeom-->>WorkerThread: MeshData[], coordinate_space, rtc_offset
WorkerThread->>BuildExport: build_geometry_data_export(meshes, rtc_offset, site_rotation?)
BuildExport-->>WorkerThread: GeometryDataExport
WorkerThread-->>ifclite_geom: Ok(GeometryDataExport)
ifclite_geom-->>PyCaller: dict{up_axis, units, rtc_offset, element_count, elements{bytes}} OR JSON String
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
…coords) New ifc_lite_processing::geometry_export — a per-entity geometry dump for analysis consumers (compas_ifc and others), distinct from the render GLB: - IFC Z-up (reads MeshData before the wasm Y-up boundary) - absolute-world metres: vertex = position + origin + rtc_offset (per-element local frame + model RTC offset folded back; offset recorded in the output) - position-welded indexed triangles (1um) so closed-mesh consumers (volume, watertightness) work; the GLB's per-face duplication is avoided - occurrences only (geometry_class==0); keyed by IFC STEP id; submeshes merged - serializes to the ifc-lite-geometry-data JSON contract Native test (synthetic corpus, skips if the gitignored fixture is absent): cube welds to 8 corners/12 faces at world [0,0,0]-[1,1,1] Z-up, control at [40..41], faces valid, JSON round-trips.
rust/python (cdylib `ifclite_geom`, abi3-py39): exposes geometry_data_json(bytes) -> the ifc-lite-geometry-data JSON, calling process_geometry + build_geometry_data_export natively (rayon, 256MiB worker stack, GIL released). This is the Python path for compas_ifc and other consumers: no Node, no wasm, no subprocess, no GLB round-trip. Geometry comes out welded, IFC Z-up, absolute-world metres - so compas closed-mesh volume works (the GLB unweld bug disappears) and coordinates match the IfcOpenShell frame. Built with `maturin develop -m rust/python/Cargo.toml --release`.
- geometry_data_buffers(bytes): returns per-entity vertices/faces as raw LE byte buffers (f64 xyz / u32 tris) for numpy.frombuffer on the Python side, plus the existing geometry_data_json for debug/interchange. Dropping JSON lifted the geomean speedup vs IfcOpenShell from ~8.8x to ~11x (AC-20: 8.9x -> 15x). - rust/python/pyproject.toml (maturin backend, abi3-py39, dist `ifclite-geom`). - .github/workflows/python-wheels.yml: per-platform abi3 wheels (manylinux x86_64 + aarch64, macOS arm64 + x86_64, Windows) via maturin-action; PyPI trusted publishing on `ifclite-geom-v*` tags. Verified `maturin build` locally (1.3MB).
The PyO3 cdylib has its own wheel CI (python-wheels.yml), so keep it OUT of the core Rust lanes (cargo test --workspace, clippy -D, cargo doc --workspace): make it a workspace `exclude` with explicit version + path deps and its own Cargo.lock. Root Cargo.lock no longer carries pyo3.
ecbf3e3 to
d379b51
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ecbf3e343a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| p[0] as f64 + o[0] + rtc_offset[0], | ||
| p[1] as f64 + o[1] + rtc_offset[1], | ||
| p[2] as f64 + o[2] + rtc_offset[2], |
There was a problem hiding this comment.
When process_geometry selects site_local for a translated IfcSite with a non-identity rotation, processor.rs:1562 passes site_transform into convert_mesh_to_site_local, which inverse-rotates both positions and mesh.origin (processor.rs:168-179). These lines only add the translation/RTC back, so those models are exported in site-local axes shifted to the world origin rather than the promised absolute IFC/world coordinates; reapply the site 3x3 transform, or avoid site-local rotation for this export path.
Useful? React with 👍 / 👎.
|
|
||
| use ifc_lite_processing::{build_geometry_data_export, process_geometry}; | ||
|
|
||
| const FIXTURE: &str = "../../tests/models/synthetic_box.ifc"; |
There was a problem hiding this comment.
I checked tests/models/manifest.json and repo-wide references, and tests/models/synthetic_box.ifc is neither committed nor fetchable via pnpm fixtures. In CI this read always falls into the skip path, so the new geometry-data exporter has no running regression coverage despite the assertions below; use an existing manifest fixture or add/upload this one so the test actually exercises the feature.
Useful? React with 👍 / 👎.
| @@ -0,0 +1,186 @@ | |||
| //! Analysis-ready geometry-data export. | |||
There was a problem hiding this comment.
AGENTS.md/LICENSE_HEADER.md require the MPL-2.0 header at the top of every new Rust source file, but this new file starts with module docs; the same applies to the other new Rust source files in this commit, so license/header checks or release review will reject the change until the standard header precedes the docs.
Useful? React with 👍 / 👎.
- Reapply the IfcSite forward 3x3 rotation when the model is processed into the `site_local` axis frame, so exported vertices are true IFC world coordinates (world = R*(position+origin)+rtc) rather than site-local axes shifted to the origin. Common (identity-site) models are unaffected (R = None). - Add the MPL-2.0 header to the new Rust sources (AGENTS.md/LICENSE_HEADER.md). - Make the native test self-contained via an inline minimal IFC cube so it actually runs in CI (the previous gitignored fixture always hit the skip path). - Linux wheels: `before-script-linux: rustup component add rust-src` so the manylinux container can rebuild std for the repo's global [unstable] build-std.
|
Thanks for the review — all three addressed in
Also: the Linux wheel jobs were failing because the manylinux container's toolchain lacked |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
.github/workflows/python-wheels.yml (2)
35-35: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd
persist-credentials: falsefor defense-in-depth.The checkout action persists git credentials by default. While this workflow doesn't push to git, disabling credential persistence is good security hygiene and prevents accidental credential leakage if the workflow is later modified.
- uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v4 + with: + persist-credentials: false🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/python-wheels.yml at line 35, The actions/checkout action is using the default configuration which persists git credentials, creating a potential security risk. Add the `persist-credentials: false` parameter to the actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 action configuration to disable credential persistence and prevent accidental credential leakage if the workflow is modified in the future.Source: Linters/SAST tools
32-32: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winConsider replacing
macos-13withmacos-15-intelfor longevity.GitHub is deprecating
macos-13runners. While the static analysis warning is technically a false positive (macos-13 still works today), the runner is on a deprecation path. Usingmacos-15-intelprovides a more future-proof x86_64 macOS build environment.- - { os: macos-13, target: x86_64 } + - { os: macos-15-intel, target: x86_64 }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/python-wheels.yml at line 32, In the python-wheels.yml workflow file, replace the deprecated macos-13 runner with macos-15-intel in the matrix configuration. Locate the line containing `- { os: macos-13, target: x86_64 }` and change macos-13 to macos-15-intel to use a non-deprecated, future-proof x86_64 macOS runner that GitHub will continue to support.Source: Linters/SAST tools
rust/python/src/lib.rs (1)
72-85: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueUnsafe byte reinterpretation relies on little-endian targets.
The comment correctly notes that all wheel targets (x86_64, aarch64) are little-endian, making this transmute sound. The safety invariants are satisfied:
- The
Vec<[f64;3]>/Vec<[u32;3]>memory is contiguousPyBytes::new_boundcopies the data beforeexportis dropped- All matrix targets in the CI workflow are LE architectures
The code is correct for current targets, but consider adding a compile-time assertion to guard against future big-endian targets:
🛡️ Optional: add compile-time LE assertion
#[cfg(not(target_endian = "little"))] compile_error!("ifclite_geom byte buffers assume little-endian targets");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/python/src/lib.rs` around lines 72 - 85, Add a compile-time assertion to guard against future big-endian targets, since the unsafe byte reinterpretation of el.vertices and el.faces assumes little-endian architecture. Place a compile_error! macro within a cfg attribute that triggers when the target is not little-endian, positioned before the unsafe slice creation code that generates vbytes and fbytes to ensure this architectural requirement is enforced at compile time.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rust/processing/src/geometry_export.rs`:
- Around line 96-99: Validate that the positions buffer length is divisible by 3
before processing with chunks_exact(3) in the vertex parsing logic.
Additionally, add bounds checking when remapping faces to use vertex indices
from the parsed positions, skipping or handling any invalid triangle references
instead of using unchecked indexing. Apply these validation checks consistently
across all affected locations where chunks_exact(3) is used for position parsing
and face remapping occurs (the position parsing near chunks_exact(3) and the
subsequent face index access operations).
In `@rust/python/src/lib.rs`:
- Around line 28-47: The run_export function attempts to handle thread panics
using .join().map_err(), but the workspace's release profile configuration has
panic set to 'abort', which causes panics to terminate the entire process rather
than unwind, making the error handler unreachable. Add an explicit
[profile.release] section to the rust/python/Cargo.toml file that overrides the
workspace setting by configuring panic to "unwind" instead of "abort", which
will allow panics in the spawned geometry worker thread to be caught and
properly handled by the .join().map_err() error handler.
---
Nitpick comments:
In @.github/workflows/python-wheels.yml:
- Line 35: The actions/checkout action is using the default configuration which
persists git credentials, creating a potential security risk. Add the
`persist-credentials: false` parameter to the
actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 action configuration
to disable credential persistence and prevent accidental credential leakage if
the workflow is modified in the future.
- Line 32: In the python-wheels.yml workflow file, replace the deprecated
macos-13 runner with macos-15-intel in the matrix configuration. Locate the line
containing `- { os: macos-13, target: x86_64 }` and change macos-13 to
macos-15-intel to use a non-deprecated, future-proof x86_64 macOS runner that
GitHub will continue to support.
In `@rust/python/src/lib.rs`:
- Around line 72-85: Add a compile-time assertion to guard against future
big-endian targets, since the unsafe byte reinterpretation of el.vertices and
el.faces assumes little-endian architecture. Place a compile_error! macro within
a cfg attribute that triggers when the target is not little-endian, positioned
before the unsafe slice creation code that generates vbytes and fbytes to ensure
this architectural requirement is enforced at compile time.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9b3b0e90-550b-491e-b7d4-aa540ccafdc7
⛔ Files ignored due to path filters (1)
rust/python/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
.github/workflows/python-wheels.ymlCargo.tomlrust/processing/src/geometry_export.rsrust/processing/src/lib.rsrust/processing/tests/geometry_data_export_test.rsrust/python/Cargo.tomlrust/python/pyproject.tomlrust/python/src/lib.rs
| let verts: Vec<[f64; 3]> = m | ||
| .positions | ||
| .chunks_exact(3) | ||
| .map(|p| { |
There was a problem hiding this comment.
Validate triangle buffer integrity before face remap to avoid panic.
chunks_exact(3) drops trailing scalars, and later remap uses unchecked indexing from face indices. If any malformed mesh slips through, export can panic on out-of-bounds. Add tri-alignment checks and safe remap access (skip invalid triangles).
💡 Proposed fix
@@
- if m.geometry_class != 0 || m.indices.is_empty() {
+ if m.geometry_class != 0
+ || m.indices.is_empty()
+ || m.positions.len() % 3 != 0
+ || m.indices.len() % 3 != 0
+ {
continue;
}
@@
- for f in faces {
- let (a, b, c) = (
- remap[f[0] as usize],
- remap[f[1] as usize],
- remap[f[2] as usize],
- );
+ for f in faces {
+ let (Some(&a), Some(&b), Some(&c)) = (
+ remap.get(f[0] as usize),
+ remap.get(f[1] as usize),
+ remap.get(f[2] as usize),
+ ) else {
+ continue;
+ };
if a != b && b != c && a != c {
out_faces.push([a, b, c]);
}
}Also applies to: 130-133, 185-190
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rust/processing/src/geometry_export.rs` around lines 96 - 99, Validate that
the positions buffer length is divisible by 3 before processing with
chunks_exact(3) in the vertex parsing logic. Additionally, add bounds checking
when remapping faces to use vertex indices from the parsed positions, skipping
or handling any invalid triangle references instead of using unchecked indexing.
Apply these validation checks consistently across all affected locations where
chunks_exact(3) is used for position parsing and face remapping occurs (the
position parsing near chunks_exact(3) and the subsequent face index access
operations).
| fn run_export(ifc_bytes: Vec<u8>) -> Result<GeometryDataExport, String> { | ||
| std::thread::Builder::new() | ||
| .stack_size(GEOMETRY_STACK_BYTES) | ||
| .name("ifclite-geometry".into()) | ||
| .spawn(move || { | ||
| let result = process_geometry(&ifc_bytes); | ||
| let rtc = result.metadata.coordinate_info.origin_shift; | ||
| // Reapply the IfcSite rotation only in the site-local axis frame; | ||
| // model_rtc / raw_ifc keep true IFC world axes (R = identity). | ||
| let site_rotation = if result.mesh_coordinate_space.as_deref() == Some("site_local") { | ||
| result.site_transform.as_deref() | ||
| } else { | ||
| None | ||
| }; | ||
| build_geometry_data_export(&result.meshes, rtc, site_rotation) | ||
| }) | ||
| .map_err(|e| format!("spawn failed: {e}"))? | ||
| .join() | ||
| .map_err(|_| "geometry worker panicked".to_string()) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if rust/python/Cargo.toml has a release profile with panic setting
echo "=== Checking rust/python/Cargo.toml for panic profile ==="
cat rust/python/Cargo.toml
echo ""
echo "=== Checking root Cargo.toml for panic profile ==="
rg -n '\[profile' -A 5 Cargo.toml || echo "No profile section found in root"
echo ""
echo "=== Checking if there's a rust-toolchain or workspace profile that applies ==="
cat rust-toolchain.toml 2>/dev/null || echo "No rust-toolchain.toml"Repository: LTplus-AG/ifc-lite
Length of output: 1878
Add explicit panic profile to rust/python to ensure error recovery.
The rust/python crate inherits the workspace's [profile.release] with panic = 'abort', but the run_export function (lines 45–46) attempts to handle thread panics with .join().map_err(). With panic = 'abort', panics will terminate the entire Python process rather than unwind, making the error handler unreachable and crashing the interpreter.
Add to rust/python/Cargo.toml:
[profile.release]
panic = "unwind"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rust/python/src/lib.rs` around lines 28 - 47, The run_export function
attempts to handle thread panics using .join().map_err(), but the workspace's
release profile configuration has panic set to 'abort', which causes panics to
terminate the entire process rather than unwind, making the error handler
unreachable. Add an explicit [profile.release] section to the
rust/python/Cargo.toml file that overrides the workspace setting by configuring
panic to "unwind" instead of "abort", which will allow panics in the spawned
geometry worker thread to be caught and properly handled by the
.join().map_err() error handler.
Source: Coding guidelines
1 participant
What
An analysis-oriented geometry-data export plus a native Python binding for the ifc-lite kernel.
rust/processing/src/geometry_export.rs—build_geometry_data_export(&[MeshData], rtc_offset): per-entity geometry distinct from the render GLB:MeshDatabefore the wasm Y-up boundary)vertex = position + origin + rtc_offset(per-element local frame + model RTC folded back; offset recorded in the output)geometry_class == 0), submeshes merged, keyed by IFC STEP idifc-lite-geometry-dataJSON contractrust/python— PyO3 cdylibifclite_geom(abi3-py39):geometry_data_buffers(bytes)(raw LE byte buffers fornumpy.frombuffer) andgeometry_data_json(bytes). Runsprocess_geometrynatively (rayon, 256 MiB worker stack, GIL released)..github/workflows/python-wheels.yml— per-platform abi3 wheels (manylinux x86_64/aarch64, macOS arm64/x86_64, Windows) via maturin-action; PyPI trusted publish onifclite-geom-v*tags.Why
Python consumers (compas_ifc and others) had no clean path to ifc-lite geometry: the CLI is Node + wasm + subprocess, and the wasm path can't run
process_geometry(rayon +Instant::now). This is a direct in-process native path — no Node, no wasm, no GLB round-trip. Reading the kernel mesh directly, geometry comes out welded + Z-up + world-coords by construction (the IfcOpenShell frame), so volume/closedness work with no Y-up/recenter handling downstream.Results (measured through the compas_ifc integration, separate repo)
Notes
ifc-lite-pythonis excluded from the core workspace (it has its own wheel CI), socargo test --workspace/ clippy /cargo docare unaffected and the rootCargo.lockcarries no pyo3. It keeps its ownCargo.lock+ path deps.rust/processing/tests/geometry_data_export_test.rsverifies welded / Z-up / absolute-world output (skips when the gitignored fixture is absent, per the repo convention).Summary by CodeRabbit
Release Notes
New Features
Chores