Modeling - BRep Graph Builder and Editor clean up by dpasukhi · Pull Request #1237 · Open-Cascade-SAS/OCCT

dpasukhi · 2026-04-26T08:32:59Z

No description provided.

- Replaced calls to BRepGraph_Builder::Perform() with BRepGraph_Builder::Add() across multiple test files. - Ensured that myGraph.Clear() is called before adding shapes to the graph to maintain a clean state. - Updated related assertions and comments to reflect the changes in method usage. - This change improves clarity and consistency in the test code, aligning with the latest API updates.

Fix documentation that incorrectly attributed generation-counter reset to BRepGraph_Builder::Add(); the counter is incremented by BRepGraph::Clear() instead. Updated BRepGraph_UIDsView.hxx, BRepGraph_UID.hxx, BRepGraph_VersionStamp.hxx, and BRepGraph.hxx. Fix Result::Occurrence field type from BRepGraph_NodeId to the strongly-typed BRepGraph_OccurrenceId, restoring the type-safety present in the previous API. Remove the BRepGraph_NodeId() casts from the two assignment sites. Fix Flatten=true path in appendImpl: the NCollection_DynamicArray<BRepGraph_NodeId> aAppendedRoots populated by AppendFlattened was silently discarded, leaving Result::TopologyRoot invalid. appendImpl now accepts an optional out-param theOutFlatRoots; Add() passes it when Flatten=true and sets TopologyRoot to the first appended face root. Fix createRootProductForTopology: replace three out-params with a returned ProductBundle struct (Product, Occurrence, OccurrenceRef). Remove the dummy anUnusedOccurrence/anUnusedOccurrenceRef variables at the Product-parent Add() call site. Fix detectTopologyRoot: remove the dead aStorage binding and the (void)aStorage workaround in the default branch; snapshotCountForKind takes const BRepGraph& directly. Fix Options::Populate field: add {} in-class initialiser for consistency with the other bool fields that already carry explicit defaults. Fix ~478 test call sites across 39 test files that cast the [[nodiscard]] Result to void with (void)BRepGraph_Builder::Add(...). Replace with [[maybe_unused]] const BRepGraph_Builder::Result aBuildResN = BRepGraph_Builder::Add(...) using per-file sequential numbering to avoid shadowing in functions with multiple calls. Fix LinkProducts documentation: clarify that any active Product may serve as parent (not limited to assembly/part distinction), matching actual implementation.

Copilot

Pull request overview

This PR modernizes the BRepGraph build/edit lifecycle by replacing the older BRepGraph_Builder::Perform()-style “build from scratch” entry point with an explicit BRepGraph::Clear() + BRepGraph_Builder::Add() ingestion API, and updates editor/product/occurrence operations accordingly.

Changes:

Replace BRepGraph_Builder::Perform() with BRepGraph_Builder::Add() and introduce BRepGraph::Clear() as the rebuild boundary.
Refactor product/occurrence editing APIs (e.g., AddAssembly → CreateEmptyProduct, AddOccurrence → LinkProducts, occurrence mutation via Editor().Occurrences()).
Update a large set of GTests and inline docs to use the new lifecycle and API names.

Reviewed changes

Copilot reviewed 59 out of 59 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
src/ModelingData/TKBRep/GTests/BRepGraph_WireExplorer_Test.cxx	Update test setup to `Clear()` + `Builder::Add()`.
src/ModelingData/TKBRep/GTests/BRepGraph_Views_Test.cxx	Update build and product/occurrence API usage in view parity tests.
src/ModelingData/TKBRep/GTests/BRepGraph_VersionStamp_Test.cxx	Update rebuild boundary to `Clear()` + `Add()` for generation/GUID tests.
src/ModelingData/TKBRep/GTests/BRepGraph_Validate_Test.cxx	Update validator tests to new builder lifecycle and occurrence mutation API.
src/ModelingData/TKBRep/GTests/BRepGraph_Transform_Test.cxx	Update transform tests to use `Clear()` + `Add()`.
src/ModelingData/TKBRep/GTests/BRepGraph_Tool_Test.cxx	Update shape ingestion in tool/query tests.
src/ModelingData/TKBRep/GTests/BRepGraph_Test.cxx	Broad update to new builder options API and product/occurrence editing calls.
src/ModelingData/TKBRep/GTests/BRepGraph_Sharing_Test.cxx	Update sharing tests to new ingestion API.
src/ModelingData/TKBRep/GTests/BRepGraph_ScenarioMatrix_Test.cxx	Update scenario matrix builds and assembly wiring API calls.
src/ModelingData/TKBRep/GTests/BRepGraph_ReverseIterator_Test.cxx	Update test setup to `Clear()` + `Add()`.
src/ModelingData/TKBRep/GTests/BRepGraph_ReplaceVertex_Test.cxx	Update helper graph build to `Clear()` + `Add()`.
src/ModelingData/TKBRep/GTests/BRepGraph_RelatedIterator_Test.cxx	Update build + assembly wiring calls to new APIs.
src/ModelingData/TKBRep/GTests/BRepGraph_RefsIterator_Test.cxx	Update build + product/occurrence API usage.
src/ModelingData/TKBRep/GTests/BRepGraph_RefId_Test.cxx	Update UID/ref tests to new rebuild boundary.
src/ModelingData/TKBRep/GTests/BRepGraph_Reconstruct_Test.cxx	Update reconstruction tests to new ingestion API.
src/ModelingData/TKBRep/GTests/BRepGraph_Polygon_Test.cxx	Update polygon/triangulation tests to new ingestion API.
src/ModelingData/TKBRep/GTests/BRepGraph_ParentExplorer_Test.cxx	Update builds and product/occurrence creation to new APIs.
src/ModelingData/TKBRep/GTests/BRepGraph_NodeId_Test.cxx	Update ingestion call in NodeId test.
src/ModelingData/TKBRep/GTests/BRepGraph_MutationGen_Test.cxx	Update ingestion and setup logic.
src/ModelingData/TKBRep/GTests/BRepGraph_MutGuard_Test.cxx	Update helper graph build to `Clear()` + `Add()`.
src/ModelingData/TKBRep/GTests/BRepGraph_MeshCache_Test.cxx	Update helper graph build to `Clear()` + `Add()`.
src/ModelingData/TKBRep/GTests/BRepGraph_Iterator_Test.cxx	Update test setup to new ingestion API.
src/ModelingData/TKBRep/GTests/BRepGraph_History_Test.cxx	Update test setup to new ingestion API.
src/ModelingData/TKBRep/GTests/BRepGraph_Geometry_Test.cxx	Update geometry tests to new ingestion API.
src/ModelingData/TKBRep/GTests/BRepGraph_Fuzz_Test.cxx	Update fuzz seed builds to `Clear()` + `Add()`.
src/ModelingData/TKBRep/GTests/BRepGraph_EventBus_Test.cxx	Update event bus setup build call.
src/ModelingData/TKBRep/GTests/BRepGraph_EdgeCases_Test.cxx	Update edge-case build semantics to `Clear()` + `Add()`.
src/ModelingData/TKBRep/GTests/BRepGraph_DefsIterator_Test.cxx	Update builds + assembly wiring to new APIs.
src/ModelingData/TKBRep/GTests/BRepGraph_DeferredInvalidation_Test.cxx	Update assembly creation and occurrence ref mutation API calls.
src/ModelingData/TKBRep/GTests/BRepGraph_Copy_Test.cxx	Update copy tests to build via `Clear()` + `Add()`.
src/ModelingData/TKBRep/GTests/BRepGraph_Convenience_Test.cxx	Update convenience tests to new ingestion API.
src/ModelingData/TKBRep/GTests/BRepGraph_Compact_Test.cxx	Update compaction tests and related messages to new ingestion naming.
src/ModelingData/TKBRep/GTests/BRepGraph_ChildExplorer_Test.cxx	Update ingestion + product wiring + occurrence mutation API usage.
src/ModelingData/TKBRep/GTests/BRepGraph_Builder_Test.cxx	Update incremental build tests to use `Add()` options instead of editor append APIs.
src/ModelingData/TKBRep/GTests/BRepGraph_Benchmark_Test.cxx	Update benchmarks to new ingestion API and parallel option struct.
src/ModelingData/TKBRep/GTests/BRepGraphInc_Test.cxx	Update incidence parity tests to build via `Clear()` + `Add()`.
src/ModelingData/TKBRep/BRepGraphInc/BRepGraphInc_Storage.cxx	Update comment referring to old build entry point.
src/ModelingData/TKBRep/BRepGraphInc/BRepGraphInc_Populate.hxx	Update documentation to point to `Builder::Add()`.
src/ModelingData/TKBRep/BRepGraph/BRepGraph_VersionStamp.hxx	Update generation documentation to reference `BRepGraph::Clear()`.
src/ModelingData/TKBRep/BRepGraph/BRepGraph_UIDsView.hxx	Update generation/reset docs (needs alignment with actual counter behavior).
src/ModelingData/TKBRep/BRepGraph/BRepGraph_UID.hxx	Update generation documentation to `Clear()` cycle and adjust member formatting.
src/ModelingData/TKBRep/BRepGraph/BRepGraph_TransientCache.hxx	Update lifecycle docs to `Builder::Add()`.
src/ModelingData/TKBRep/BRepGraph/BRepGraph_Transform.hxx	Update usage snippet to `Builder::Add()`.
src/ModelingData/TKBRep/BRepGraph/BRepGraph_Transform.cxx	Update occurrence ref mutation path to `Editor().Occurrences().MutRef()`.
src/ModelingData/TKBRep/BRepGraph/BRepGraph_ShapesView.hxx	Update cache lifecycle docs to `Builder::Add()`.
src/ModelingData/TKBRep/BRepGraph/BRepGraph_RefTransientCache.hxx	Update lifecycle docs to `Builder::Add()`.
src/ModelingData/TKBRep/BRepGraph/BRepGraph_EditorView_Mut.cxx	Move occurrence mutation guards under `OccurrenceOps`.
src/ModelingData/TKBRep/BRepGraph/BRepGraph_EditorView.hxx	Add compound/compsolid append helpers, refactor product/occurrence APIs, remove append-shape APIs, introduce `OccurrenceOps`.
src/ModelingData/TKBRep/BRepGraph/BRepGraph_EditorView.cxx	Implement new editor ops (AddChild/AddSolid/LinkProductToTopology/CreateEmptyProduct/LinkProducts) and remove old append-shape wrappers.
src/ModelingData/TKBRep/BRepGraph/BRepGraph_Data.hxx	Update GraphGUID doc to `Builder::Add()`.
src/ModelingData/TKBRep/BRepGraph/BRepGraph_Copy.hxx	Update usage snippet to `Builder::Add()`.
src/ModelingData/TKBRep/BRepGraph/BRepGraph_Compact.cxx	Preserve part-root placement using new product linking API; update docs/comments.
src/ModelingData/TKBRep/BRepGraph/BRepGraph_Builder.hxx	Redesign builder API around `Add()` + `Options` + `Result`, introduce parented Add overloads.
src/ModelingData/TKBRep/BRepGraph/BRepGraph_Builder.cxx	Implement new `Add()` flow, incremental append implementation, topology-root detection helpers, and product wrapping helpers.
src/ModelingData/TKBRep/BRepGraph/BRepGraph.hxx	Update lifecycle docs and add `BRepGraph::Clear()`.
src/ModelingData/TKBRep/BRepGraph/BRepGraph.cxx	Implement `BRepGraph::Clear()` and move GUID generation here; update allocator assertion message.

Copilot · 2026-04-26T11:29:14Z

+  if (theOptions.Flatten && !aFlatRoots.IsEmpty())
+    aResult.TopologyRoot = aFlatRoots.First();
+  else
+    aResult.TopologyRoot = detectTopologyRoot(theGraph, theShape.ShapeType(), anOldCount);

-  populateUIDsIncremental(theGraph,
-                          anOldVtx,
-                          anOldEdge,
-                          anOldCoEdge,
-                          anOldWire,
-                          anOldFace,
-                          anOldShell,
-                          anOldSolid,
-                          anOldComp,
-                          anOldCS,
-                          anOldProduct,
-                          anOldOccurrence,
-                          anOldShellRef,
-                          anOldFaceRef,
-                          anOldWireRef,
-                          anOldCoEdgeRef,
-                          anOldVertexRef,
-                          anOldSolidRef,
-                          anOldChildRef);
+  if (theOptions.CreateAutoProduct)
+  {
+    const ProductBundle aBundle = createRootProductForTopology(theGraph,
+                                                               aResult.TopologyRoot,
+                                                               theShape.Location(),
+                                                               /*registerAsRoot*/ true);
+    aResult.Product             = aBundle.Product;
+    aResult.Occurrence          = aBundle.Occurrence;
+    theGraph.myData->myIncStorage.BuildReverseIndex();
+  }


BRepGraph_Builder::Add() determines TopologyRoot via count deltas (or flattened roots). If the appended shape is deduplicated (already present in storage), counts won't change and aFlatRoots can be empty, leaving TopologyRoot invalid. With CreateAutoProduct=true this still creates a Product without linking it to topology, and Result::Ok can become true even though no topology root was resolved. Consider resolving the root via existing TShape->NodeId mapping when no new entities are appended (or returning a no-op/invalid Result), and avoid creating an auto product unless a valid topology root is known.

Copilot · 2026-04-26T11:29:14Z

+//! so UIDs survive Compact() index remapping. Only BRepGraph::Clear() resets
 //! counters (new generation). The Generation field enables stale-reference
 //! detection when a graph is rebuilt.


The comment says "Only BRepGraph::Clear() resets counters (new generation)", but Clear() increments myGeneration without resetting myNextUIDCounter, and BRepGraph_UID also documents that counters are monotonic and never reset. Please align the documentation with the actual behavior (either reset myNextUIDCounter in Clear, or update the comment to reflect that only Generation/GUID reset).

Suggested change

//! so UIDs survive Compact() index remapping. Only BRepGraph::Clear() resets

//! counters (new generation). The Generation field enables stale-reference

//! detection when a graph is rebuilt.

//! so UIDs survive Compact() index remapping and are not reset by

//! BRepGraph::Clear(). Clear() starts a new graph generation and refreshes

//! the graph GUID, which enable stale-reference detection when a graph is

//! rebuilt.

Copilot · 2026-04-26T11:29:15Z

+  myGraph->allocateRefUID(aChildRefId);
+  aStorage.ChangeCompound(theCompoundEntity).ChildRefIds.Append(aChildRefId);
+
+  aStorage.BuildReverseIndex();


CompoundOps::AddChild() rebuilds the entire reverse index (BuildReverseIndex) after appending a single ChildRef. This defeats the incremental reverse-index updates already supported by BRepGraphInc_Storage::BuildDeltaReverseIndex (used by Populate::Append/AppendFlattened) and can make incremental graph construction unnecessarily expensive. Consider updating only the affected reverse-index portions (or providing an incremental/ref-only update path) instead of rebuilding the full reverse index here.

Suggested change

aStorage.BuildReverseIndex();

aStorage.BuildDeltaReverseIndex();

Copilot · 2026-04-26T11:29:15Z

+  aStorage.ChangeCompSolid(theCompSolidEntity).SolidRefIds.Append(aSolidRefId);
+
+  aStorage.BuildReverseIndex();
+  myGraph->markModified(theCompSolidEntity);
+  return aSolidRefId;
+}


CompSolidOps::AddSolid() rebuilds the entire reverse index (BuildReverseIndex) after appending a single SolidRef. For incremental workflows this can be a hotspot; consider an incremental update (e.g., BuildDeltaReverseIndex with old counts, or a specialized reverse-index update for SolidRefs) rather than rebuilding everything.

…d methods to use LinkProductToTopology

…ation for clarity

dpasukhi added 2 commits April 26, 2026 09:04

github-project-automation Bot added this to Maintenance Apr 26, 2026

github-project-automation Bot moved this to Todo in Maintenance Apr 26, 2026

// formatting

bee5db1

dpasukhi force-pushed the brep_graph_upd branch from dd6c3e3 to bee5db1 Compare April 26, 2026 09:11

dpasukhi requested a review from Copilot April 26, 2026 11:23

Copilot started reviewing on behalf of dpasukhi April 26, 2026 11:24 View session

Copilot AI reviewed Apr 26, 2026

View reviewed changes

dpasukhi added 3 commits April 26, 2026 13:12

Refactor BRepGraph: remove createRootProductForTopology and update Ad…

485b295

…d methods to use LinkProductToTopology

Refactor BRepGraph: remove redundant comments and streamline document…

6f7c439

…ation for clarity

// formatting

df1e85f

dpasukhi merged commit 048a292 into Open-Cascade-SAS:IR Apr 26, 2026
18 checks passed

dpasukhi deleted the brep_graph_upd branch April 26, 2026 14:02

github-project-automation Bot moved this from Todo to Done in Maintenance Apr 26, 2026

gsdali mentioned this pull request Jul 17, 2026

Graph construction skips BRepGraph::Clear(), leaving generation 0 and GraphGUID all-zeros SecondMouseAU/OCCTSwift#303

Closed

Navigation Menu

Uh oh!

Modeling - BRep Graph Builder and Editor clean up - #1237

Modeling - BRep Graph Builder and Editor clean up#1237
dpasukhi merged 6 commits into
Open-Cascade-SAS:IRfrom
dpasukhi:brep_graph_upd

dpasukhi commented Apr 26, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 26, 2026

Uh oh!

Copilot AI Apr 26, 2026

Uh oh!

Copilot AI Apr 26, 2026

Uh oh!

Copilot AI Apr 26, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

-//! so UIDs survive Compact() index remapping. Only BRepGraph::Clear() resets
-//! counters (new generation). The Generation field enables stale-reference
-//! detection when a graph is rebuilt.
+//! so UIDs survive Compact() index remapping and are not reset by
+//! BRepGraph::Clear(). Clear() starts a new graph generation and refreshes
+//! the graph GUID, which enable stale-reference detection when a graph is
+//! rebuilt.

	aStorage.BuildReverseIndex();
	aStorage.BuildDeltaReverseIndex();

Uh oh!

Conversation

dpasukhi commented Apr 26, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 26, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants