geocad: speed up OCCShapeCreation for partial geometry exports#58
Open
wdconinc wants to merge 6 commits into
Open
geocad: speed up OCCShapeCreation for partial geometry exports#58wdconinc wants to merge 6 commits into
wdconinc wants to merge 6 commits into
Conversation
Three performance improvements to TOCCToStep::OCCShapeCreation: Fix 1: Pre-collect relevant volumes before OCCShapeCreation Add CollectRelevantVolumes() which mirrors OCCPartialTreeCreation's skip logic to return only the TGeoVolume* set reachable from the requested sub-tree. CreatePartialGeometry now passes this filter to OCCShapeCreation so that only the few dozen volumes in the target sub-tree are converted to OCC shapes, rather than every volume in the full detector geometry (~3000 in ePIC). Fix 2: Replace O(V*N) nested TGeoIterator with a pre-built mother map The original OCCShapeCreation launched a fresh TGeoIterator from the top of the geometry for every volume in GetListOfVolumes() to find that volume's mother — O(V*N) with V~3000 volumes and N~60000 nodes. This is now replaced by a single O(N) tree walk that builds a TGeoVolume* -> mother TGeoVolume* map before the volume loop. Each volume's parent label is then looked up in O(1). Fix 3: Defer UpdateAssemblies() to end of method UpdateAssemblies() was called after every volume insertion (three call sites inside the loop). It is now called once at the end. Measured with 'npdet_to_step part LumiDirectPCAL_vol' on the ePIC epic_craterlake.xml full detector geometry: Before: >10 minutes (terminated) After: ~33 seconds Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Speeds up GeoCAD partial geometry exports (npdet_to_step part ...) by avoiding unnecessary ROOT→OCC conversions and reducing repeated geometry tree traversals inside TOCCToStep.
Changes:
- Added
CollectRelevantVolumes()to precompute the set of volumes needed for partial exports and pass that filter intoOCCShapeCreation(). - Replaced per-volume root-to-leaf scans with a single pre-built volume→mother map in
OCCShapeCreation(). - Deferred
UpdateAssemblies()to a single call at the end ofOCCShapeCreation().
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/geocad/src/TOCCToStep.h | Extends OCCShapeCreation() API to accept a volume filter; declares CollectRelevantVolumes() helpers. |
| src/geocad/src/TOCCToStep.cxx | Implements filtered volume collection and optimizes shape creation by precomputing parent relationships and deferring assembly updates. |
| src/geocad/src/TGeoToStep.cxx | Uses CollectRelevantVolumes() to avoid converting irrelevant volumes during partial export flows. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Capybara summary for PR 58
|
Contributor
Author
|
@copilot resolve the merge conflicts in this pull request |
Co-authored-by: wdconinc <4656391+wdconinc@users.noreply.github.com>
Contributor
Resolved the merge conflicts and merged |
Contributor
Author
- CollectRelevantVolumes: call SetType(1) when level reaches max_level to avoid visiting deeper nodes that will only be skipped. Guard with max_level >= 0 so unlimited-depth (-1) is unaffected. - Replace std::map<TGeoVolume*,TGeoVolume*> motherMap with std::unordered_map for O(1) average lookup instead of O(log N). Add <algorithm> and <unordered_map> includes. - When volume_filter is provided, sort volumes_to_process by name before processing to give deterministic label/shape creation order independent of pointer values across runs and platforms. - After building the full-tree mother map, post-process it so that every volume in the filter has its recorded mother within the filter (or Top). A full-tree scan may otherwise record a first-encountered mother that lies outside the selected sub-tree when a volume is reused at multiple positions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
Author
|
Export is unaffected by this (DRICH is hidden due to the large number of volumes making navigation hard): |
When max_level is -1 (unlimited depth, the default), the condition 'level > max_level' was always true for any visited node (level >= 1), causing every node to be skipped and leaving only the top volume in the filter set. OCCShapeCreation then processed no volumes, and OCCPartialTreeCreation found null labels and threw Standard_NullObject. Guard the depth-skip check the same way as the SetType(1) call above it: if (max_level >= 0 && level > max_level) continue; Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
TGeoIdentity() calls RegisterYourself() in its constructor, causing spurious 'Registered matrix was removed' warnings at destruction. TGeoHMatrix() is semantically equivalent (identity transform) but never auto-registers. This is consistent with the fix applied in PR #57 to the original OCCShapeCreation; the performance rewrite inadvertently reintroduced TGeoIdentity() at three call sites. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The crash occurred because OCCShapeCreation was processing filter volumes in alphabetical order, which could label a child volume before its parent. When the parent was encountered later it was skipped (already labeled), but with a hierarchy structure that misled FillOCCWithNode into calling NbComponents on null labels. Fix: build volumes_to_process in tree-traversal (DFS) order during the existing O(N) motherMap scan. DFS guarantees that each volume's first encounter comes after all its ancestors, so every parent label is non-null when the child is processed. The else-branch that created an on-the-fly mother label is kept as a safety fallback. As defense-in-depth, add null guards in FillOCCWithNode so that a null labelMother or fLabel causes the walk-up to skip that level instead of crashing with Standard_NullObject. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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.
Motivation
Running
npdet_to_step part <volume>on a large detector geometry (e.g. ePICepic_craterlake.xml) was extremely slow becauseOCCShapeCreationcontained three stacked performance problems.Changes
Fix 1 — Pre-collect relevant volumes (
CollectRelevantVolumes)For partial exports,
OCCShapeCreationpreviously converted every volume in the geometry to an OCC shape (~3000 unique volumes in ePIC), even though only the few dozen volumes inside the requested sub-tree are ever used.A new
CollectRelevantVolumes()method mirrors the skip logic ofOCCPartialTreeCreationto determine — in a single O(N) tree walk with no OCC operations — exactly which volumes will be needed.CreatePartialGeometrynow passes this filter toOCCShapeCreation, which then converts only those volumes.Fix 2 — Replace O(V×N) nested
TGeoIteratorwith a pre-built mother mapThe original
OCCShapeCreationlaunched a freshTGeoIteratorfrom the geometry root for each of the V volumes inGetListOfVolumes()in order to find that volume's mother — O(V×N) total with V ≈ 3000 volumes and N ≈ 60 000 nodes in ePIC (≈ 180 M node visits). This is replaced by a single O(N) tree walk that builds aTGeoVolume* → mother TGeoVolume*map before the volume loop; each lookup is then O(1).Fix 3 — Defer
UpdateAssemblies()to end of methodXCAFDoc_DocumentTool::ShapeTool::UpdateAssemblies()was called after every volume insertion (three call sites inside the loop). It is now called once at the end ofOCCShapeCreation.Performance
Test case:
npdet_to_step part LumiDirectPCAL_vol -o out $DETECTOR_PATH/epic_craterlake.xmlon the full ePICepic_craterlake.xmldetector geometry.The remaining ~33 s is dominated by DD4hep geometry loading; the
OCCShapeCreationstep itself is now negligible for small partial exports.Scope
The full-geometry path (
CreateGeometry/OCCTreeCreation) is unaffected in semantics and also benefits from fixes 2 and 3.