Modeling Data - Performance optimizations for hot-path curve representation lookups and map access by dpasukhi · Pull Request #1142 · Open-Cascade-SAS/OCCT

dpasukhi · 2026-03-05T23:31:28Z

Add type discriminator enum to BRep_CurveRepresentation hierarchy to eliminate virtual dispatch in BRep_Tool inner loops. Each of the 9 concrete subclasses now stores a TypeEnum tag set at construction time; BRep_Tool::Curve(), CurveOnSurface(), Polygon3D(), IsGeometric(), Range(), IsClosed(), HasContinuity(), and MaxContinuity() use the enum instead of calling virtual Is*() methods per list element. The virtual Is*() API is kept unchanged for external compatibility.

Replace double hash-map lookups (IsBound + operator(), Contains + FindFromKey) with single Seek()/ChangeSeek() calls across BRepTools_Modifier, BRepTools_WireExplorer, BRepTools_Quilt, BRepTools_Substitution, BRepTools_History, and
BRepTools_NurbsConvertModification.

Apply minor optimizations: use NbChildren() instead of a counting loop in BRepTools_Modifier; guard TopAbs::Compose identity case in TopoDS_Iterator; combine two vertex-map passes into one loop in TopExp::Vertices; hoist per-iteration map allocations out of the loop in BRepTools_ReShape::History().

Add GTest BRep_CurveRepresentation_Test verifying enum consistency with virtual Is*() for all 9 concrete types.

…tation lookups and map access Add type discriminator enum to BRep_CurveRepresentation hierarchy to eliminate virtual dispatch in BRep_Tool inner loops. Each of the 9 concrete subclasses now stores a TypeEnum tag set at construction time; BRep_Tool::Curve(), CurveOnSurface(), Polygon3D(), IsGeometric(), Range(), IsClosed(), HasContinuity(), and MaxContinuity() use the enum instead of calling virtual Is*() methods per list element. The virtual Is*() API is kept unchanged for external compatibility. Replace double hash-map lookups (IsBound + operator(), Contains + FindFromKey) with single Seek()/ChangeSeek() calls across BRepTools_Modifier, BRepTools_WireExplorer, BRepTools_Quilt, BRepTools_Substitution, BRepTools_History, and BRepTools_NurbsConvertModification. Apply minor optimizations: use NbChildren() instead of a counting loop in BRepTools_Modifier; guard TopAbs::Compose identity case in TopoDS_Iterator; combine two vertex-map passes into one loop in TopExp::Vertices; hoist per-iteration map allocations out of the loop in BRepTools_ReShape::History(). Add GTest BRep_CurveRepresentation_Test verifying enum consistency with virtual Is*() for all 9 concrete types.

Copilot

Pull request overview

This PR optimizes modeling-data hot paths by reducing per-element virtual dispatch in BRep_Tool curve-representation scanning and by replacing double map lookups with single Seek()/ChangeSeek() calls across several BRepTools_* utilities. It also adds a GTest to validate the new type-discriminator tagging on BRep_CurveRepresentation subclasses.

Changes:

Introduce BRep_CurveRepresentation::TypeEnum tag stored in each concrete representation; update BRep_Tool to branch on Type() instead of virtual Is*() calls in inner loops.
Replace patterns like IsBound()+operator() / Contains()+FindFromKey() with Seek() / ChangeSeek() in multiple BRepTools_* implementations.
Add BRep_CurveRepresentation_Test and minor micro-optimizations (TopoDS_Iterator, TopExp::Vertices, BRepTools_ReShape::History(), etc.).

Reviewed changes

Copilot reviewed 28 out of 28 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
src/ModelingData/TKBRep/TopoDS/TopoDS_Iterator.cxx	Skip orientation composition when it would be identity.
src/ModelingData/TKBRep/TopExp/TopExp.cxx	Combine vertex-map passes into a single iterator loop.
src/ModelingData/TKBRep/GTests/FILES.cmake	Register new curve-representation GTest source.
src/ModelingData/TKBRep/GTests/BRep_CurveRepresentation_Test.cxx	New tests validating `Type()` vs virtual `Is*()` behavior for concrete reps.
src/ModelingData/TKBRep/BRepTools/BRepTools_WireExplorer.cxx	Use `ChangeSeek()`/`Bound()` to avoid double map lookups.
src/ModelingData/TKBRep/BRepTools/BRepTools_Substitution.cxx	Use `Seek()` to avoid `IsBound()+operator()` pattern.
src/ModelingData/TKBRep/BRepTools/BRepTools_ReShape.cxx	Hoist per-iteration map allocations out of the loop; reuse via `Clear()`.
src/ModelingData/TKBRep/BRepTools/BRepTools_Quilt.cxx	Replace `Contains()+FindFromKey()` with `Seek()` and reuse returned pointers.
src/ModelingData/TKBRep/BRepTools/BRepTools_NurbsConvertModification.cxx	Use `Seek()` for single-pass handle-map lookup.
src/ModelingData/TKBRep/BRepTools/BRepTools_Modifier.cxx	Use `Seek()` for maps and `NbChildren()` for progress sizing.
src/ModelingData/TKBRep/BRepTools/BRepTools_History.cxx	Use `ChangeSeek()` to remove items with one lookup.
src/ModelingData/TKBRep/BRep/BRep_Tool.cxx	Switch representation scanning to `Type()`-based branching and `static_cast` in hot paths.
src/ModelingData/TKBRep/BRep/BRep_PolygonOnTriangulation.hxx	Add protected ctor taking `TypeEnum` for tagged construction.
src/ModelingData/TKBRep/BRep/BRep_PolygonOnTriangulation.cxx	Delegate public ctor to tagged protected ctor.
src/ModelingData/TKBRep/BRep/BRep_PolygonOnSurface.hxx	Add protected ctor taking `TypeEnum` for tagged construction.
src/ModelingData/TKBRep/BRep/BRep_PolygonOnSurface.cxx	Delegate public ctor to tagged protected ctor.
src/ModelingData/TKBRep/BRep/BRep_PolygonOnClosedTriangulation.cxx	Ensure base ctor receives correct `TypeEnum`.
src/ModelingData/TKBRep/BRep/BRep_PolygonOnClosedSurface.cxx	Ensure base ctor receives correct `TypeEnum`.
src/ModelingData/TKBRep/BRep/BRep_Polygon3D.cxx	Ensure base ctor receives correct `TypeEnum`.
src/ModelingData/TKBRep/BRep/BRep_GCurve.hxx	Update protected ctor to accept `TypeEnum`.
src/ModelingData/TKBRep/BRep/BRep_GCurve.cxx	Pass `TypeEnum` to `BRep_CurveRepresentation` base.
src/ModelingData/TKBRep/BRep/BRep_CurveRepresentation.hxx	Add `TypeEnum` discriminator, `Type()`, and `myType` storage.
src/ModelingData/TKBRep/BRep/BRep_CurveRepresentation.cxx	Implement new base ctor initializing `myType`.
src/ModelingData/TKBRep/BRep/BRep_CurveOnSurface.hxx	Add protected ctor taking `TypeEnum` for tagged construction.
src/ModelingData/TKBRep/BRep/BRep_CurveOnSurface.cxx	Delegate public ctor to tagged protected ctor.
src/ModelingData/TKBRep/BRep/BRep_CurveOnClosedSurface.cxx	Ensure base ctor receives correct `TypeEnum`.
src/ModelingData/TKBRep/BRep/BRep_CurveOn2Surfaces.cxx	Ensure base ctor receives correct `TypeEnum`.
src/ModelingData/TKBRep/BRep/BRep_Curve3D.cxx	Ensure base ctor receives correct `TypeEnum`.

Copilot · 2026-03-06T08:11:54Z

+  //! Type discriminator enum for fast type checks without virtual dispatch.
+  enum TypeEnum : uint8_t
+  {
+    Type_Curve3D = 0,
+    Type_CurveOnSurface,
+    Type_CurveOnClosedSurface,
+    Type_CurveOn2Surfaces,
+    Type_Polygon3D,
+    Type_PolygonOnSurface,
+    Type_PolygonOnClosedSurface,
+    Type_PolygonOnTriangulation,
+    Type_PolygonOnClosedTriangulation
+  };
+
+  //! Returns the type discriminator of this representation.
+  TypeEnum Type() const { return myType; }
+


Adding a new data member (myType) to this exported base class changes its size/layout; if OCCT aims to preserve binary compatibility for this toolkit, this is an ABI break. Also, TypeEnum is a closed set and BRep_Tool now dispatches on Type() instead of virtual Is*() methods, so any third-party/custom BRep_CurveRepresentation subclasses (if supported) won’t be recognized unless they can map to an existing TypeEnum; consider introducing an Unknown/Custom value and having hot-path code fall back to the virtual Is*() checks when encountered.

Copilot · 2026-03-06T08:11:55Z

+    if (cr->Type() == BRep_CurveRepresentation::Type_Curve3D)
    {
      const BRep_Curve3D* GC = static_cast<const BRep_Curve3D*>(cr.get());
      L                      = E.Location() * GC->Location();


These code paths now rely on Type() and then static_cast to concrete subclasses. This is undefined behavior if the enum tag ever gets out of sync with the dynamic type (e.g., due to future edits or external subclasses). Consider adding a debug-only assertion (e.g., verifying IsCurve3D()/IsPolygon3D() or RTTI) before the static_cast, or providing an Unknown/Custom fallback that uses the virtual Is*() API.

Copilot · 2026-03-06T08:11:55Z

+#include <Poly_Polygon3D.hxx>
+#include <Poly_PolygonOnTriangulation.hxx>
+#include <Poly_Triangulation.hxx>
+#include <TopLoc_Location.hxx>


This test uses gp_Pnt/gp_Dir/gp_Pnt2d/gp_Dir2d and gp::XOY()/gp::ZOX() but doesn’t include the corresponding gp headers (e.g., gp.hxx, gp_Pnt.hxx, gp_Dir.hxx, gp_Pnt2d.hxx, gp_Dir2d.hxx). Please add the direct includes to avoid relying on transitive headers, as other GTests in this toolkit do.

Suggested change

#include <TopLoc_Location.hxx>

#include <TopLoc_Location.hxx>

#include <gp.hxx>

#include <gp_Pnt.hxx>

#include <gp_Dir.hxx>

#include <gp_Pnt2d.hxx>

#include <gp_Dir2d.hxx>

- Removed redundant virtual methods from BRep_CurveRepresentation, simplifying the interface. - Implemented inline methods for curve type checks in BRep_CurveRepresentation. - Updated BRep_Tool to utilize new methods for checking curve types and properties. - Added a new TypeEnum value for 'Other' to BRep_CurveRepresentation. - Created a custom test class for 'Other' type to validate default behavior. - Enhanced unit tests for various curve and polygon types to ensure correct behavior. - Cleaned up unnecessary comments and improved code readability.

…e comparisons with dedicated methods

Copilot

Pull request overview

Copilot reviewed 35 out of 35 changed files in this pull request and generated 3 comments.

Copilot · 2026-03-06T13:17:58Z

@@ -62,11 +80,11 @@ public:
                                            const TopLoc_Location&           L2) const;

  //! A 3D polygon representation.
-  Standard_EXPORT virtual bool IsPolygon3D() const;
+  bool IsPolygon3D() const;

  //! A representation by an array of nodes on a
  //! triangulation.
-  Standard_EXPORT virtual bool IsPolygonOnTriangulation() const;
+  bool IsPolygonOnTriangulation() const;

  //! Is it a polygon in the definition of <T> with
  //! location <L>.
@@ -75,10 +93,10 @@ public:

  //! A representation by two arrays of nodes on a
  //! triangulation.
-  Standard_EXPORT virtual bool IsPolygonOnClosedTriangulation() const;
+  bool IsPolygonOnClosedTriangulation() const;

  //! A polygon in the parametric space of a surface.
-  Standard_EXPORT virtual bool IsPolygonOnSurface() const;
+  bool IsPolygonOnSurface() const;

  //! Is it a polygon in the parametric space of <S> with
  //! location <L>.
@@ -87,7 +105,7 @@ public:

  //! Two 2D polygon representations in the parametric
  //! space of a surface.
-  Standard_EXPORT virtual bool IsPolygonOnClosedSurface() const;
+  bool IsPolygonOnClosedSurface() const;



The PR description says the virtual Is*() API is kept for external compatibility, but this change removes virtual (and Standard_EXPORT) from the public IsCurve3D()/IsCurveOnSurface()/IsRegularity()/... methods. That is an ABI/API breaking change for any downstream code overriding these methods (calls through BRep_CurveRepresentation* will no longer dispatch to derived overrides). Consider keeping these methods virtual (implementing them in terms of Type() for speed) and using Type() in hot loops where you want to avoid virtual dispatch, rather than de-virtualizing the API.

Copilot · 2026-03-06T13:17:58Z

 protected:
-  Standard_EXPORT BRep_CurveRepresentation(const TopLoc_Location& L);
+  Standard_EXPORT BRep_CurveRepresentation(TypeEnum theType, const TopLoc_Location& L);

  TopLoc_Location myLocation;
+
+private:
+  TypeEnum myType;
 };


Changing the protected constructor from BRep_CurveRepresentation(const TopLoc_Location&) to BRep_CurveRepresentation(TypeEnum, const TopLoc_Location&) breaks source compatibility for any external subclasses. If external derivation is supported, consider keeping the old constructor as an overload delegating to the new one with Type_Other (or similar), and/or providing a migration path with deprecation.

Copilot · 2026-03-06T13:17:58Z

+                              const TopLoc_Location& L,
+                              const double           First,
+                              const double           Last);



BRep_GCurve's protected constructor now requires a TypeEnum. This is another source-compatibility break for any external subclasses of BRep_GCurve (not just the built-in ones). If external derivation is expected, consider keeping the previous protected constructor overload and delegating it to the new one with an appropriate default type (or deprecating it first).

Suggested change

//! Backward-compatible constructor without explicit TypeEnum.

//! Delegates to the main constructor with a default curve type.

BRep_GCurve(const TopLoc_Location& theLocation,

const double theFirst,

const double theLast)

: BRep_GCurve(BRep_CurveRepresentation::Curve3D,

theLocation,

theFirst,

theLast)

{}

github-project-automation bot added this to Maintenance Mar 5, 2026

github-project-automation bot moved this to Todo in Maintenance Mar 5, 2026

// formatting

933de6e

dpasukhi marked this pull request as ready for review March 6, 2026 08:05

dpasukhi requested a review from Copilot March 6, 2026 08:05

Copilot started reviewing on behalf of dpasukhi March 6, 2026 08:06 View session

Copilot AI reviewed Mar 6, 2026

View reviewed changes

dpasukhi added 2 commits March 6, 2026 09:48

Modeling Data - Simplify curve representation checks by replacing typ…

c49d770

…e comparisons with dedicated methods

dpasukhi requested a review from Copilot March 6, 2026 13:12

Copilot started reviewing on behalf of dpasukhi March 6, 2026 13:13 View session

Copilot AI reviewed Mar 6, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modeling Data - Performance optimizations for hot-path curve representation lookups and map access#1142

Modeling Data - Performance optimizations for hot-path curve representation lookups and map access#1142
dpasukhi wants to merge 4 commits intoOpen-Cascade-SAS:IRfrom
dpasukhi:tkbrep_some_opt

dpasukhi commented Mar 5, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 6, 2026

Uh oh!

Copilot AI Mar 6, 2026

Uh oh!

Copilot AI Mar 6, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 6, 2026

Uh oh!

Copilot AI Mar 6, 2026

Uh oh!

Copilot AI Mar 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

-#include <TopLoc_Location.hxx>
+#include <TopLoc_Location.hxx>
+#include <gp.hxx>
+#include <gp_Pnt.hxx>
+#include <gp_Dir.hxx>
+#include <gp_Pnt2d.hxx>
+#include <gp_Dir2d.hxx>

+  //! Backward-compatible constructor without explicit TypeEnum.
+  //! Delegates to the main constructor with a default curve type.
+  BRep_GCurve(const TopLoc_Location& theLocation,
+              const double           theFirst,
+              const double           theLast)
+  : BRep_GCurve(BRep_CurveRepresentation::Curve3D,
+                theLocation,
+                theFirst,
+                theLast)
+  {}

Conversation

dpasukhi commented Mar 5, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants