Foundation Classes - Enhance BVH Implementation by dpasukhi · Pull Request #62 · dpasukhi/OCCT

dpasukhi · 2025-11-20T12:18:13Z

Updated BVH_BinnedBuilder to improve leaf node size condition and SAH cost evaluation.
Refactored BVH_Box to utilize constexpr for better compile-time evaluation.
Enhanced BVH_LinearBuilder to replace std::vector with NCollection_Vector for performance.
Introduced new unit tests for BVH_BinnedBuilder, BVH_Box, BVH_LinearBuilder, and other components to ensure robustness and correctness.
Added various utility functions and constants to improve code clarity and maintainability.
Updated CMake files to include new test files for comprehensive testing coverage.

- Updated BVH_BinnedBuilder to improve leaf node size condition and SAH cost evaluation. - Refactored BVH_Box to utilize constexpr for better compile-time evaluation. - Enhanced BVH_LinearBuilder to replace std::vector with NCollection_Vector for performance. - Introduced new unit tests for BVH_BinnedBuilder, BVH_Box, BVH_LinearBuilder, and other components to ensure robustness and correctness. - Added various utility functions and constants to improve code clarity and maintainability. - Updated CMake files to include new test files for comprehensive testing coverage.

- Updated BVH_Box to utilize constexpr and noexcept for improved compile-time evaluation and safety. - Refactored BVH_BuildQueue to use atomic operations for thread-safe size tracking and improved performance. - Removed the obsolete BVH_BuildQueue.cxx file and updated related CMake configurations. - Added comprehensive unit tests for BVH_Box and BVH_BuildQueue to ensure functionality and robustness. - Enhanced various BVH traversal and building classes with new internal structures for better performance and clarity. - Updated destructor implementations to use default specifiers for consistency and clarity.

coderabbitai · 2025-11-20T12:30:39Z

Walkthrough

This PR makes broad BVH changes: adds constexpr/inline/noexcept to many BVH_Box, BVH_Ray, Vec/Trait functions; defaults several destructors; makes THE_NODE_MIN_SIZE constexpr; refactors BVH_BuildQueue to use atomics and removes its .cxx unit; replaces in-place QuickSorter with index-based reorder; adds LUT-based Morton encoder in RadixSorter; adjusts Binned/SweepPlane builders' leaf and SAH logic; introduces non‑templated stack node structs and overflow asserts in traversal; refactors BVH_Tools geometry routines; and adds extensive BVH unit tests.

Pre-merge checks

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 9.70% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarizes the main enhancement objective of improving the BVH implementation with multiple refinements across builders, box structures, and testing.
Description check	✅ Passed	The description is directly related to the changeset, detailing specific updates to BVH components, test additions, and utility improvements that align with the actual file changes.

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b11e01b and 7cc8620.

📒 Files selected for processing (1)

src/FoundationClasses/TKMath/GTests/BVH_Triangulation_Test.cxx (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

src/FoundationClasses/TKMath/GTests/BVH_Triangulation_Test.cxx

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)

GitHub Check: Prepare and Build on macOS with Clang (No PCH)
GitHub Check: Prepare and Build on macOS with Clang (x64)
GitHub Check: Prepare and Build on Ubuntu with Clang (x64)
GitHub Check: Build Documentation
GitHub Check: Prepare and Build on Windows with MSVC (x64)

Tip

📝 Customizable high-level summaries are now available in beta!

You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.

Provide your own instructions using the high_level_summary_instructions setting.
Format the summary however you like (bullet lists, tables, multi-section layouts, contributor stats, etc.).
Use high_level_summary_in_walkthrough to move the summary from the description to the walkthrough section.

Example instruction:

"Divide the high-level summary into five sections:

📝 Description — Summarize the main change in 50–60 words, explaining what was done.

📓 References — List relevant issues, discussions, documentation, or related PRs.

📦 Dependencies & Requirements — Mention any new/updated dependencies, environment variable changes, or configuration updates.

📊 Contributor Summary — Include a Markdown table showing contributions:
| Contributor | Lines Added | Lines Removed | Files Changed |

✔️ Additional Notes — Add any extra reviewer context.
Keep each section concise (under 200 words) and use bullet or numbered lists for clarity."

Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later.

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

src/FoundationClasses/TKMath/BVH/BVH_Tools.hxx (1)
418-419: Restore dimension-agnostic Ray-Box reduction

Line 418 now unconditionally reads aTimeMin[2] and aTimeMax[2]. For N = 2 (which the BVH stack actively supports, see the 2D BVH_LinearBuilder tests in this same PR), this is an out-of-bounds access. For N > 3, the additional axes are ignored, so rays can incorrectly report hits because the enter/leave extrema never account for those dimensions. The previous reduction loop handled all N; we need to keep that general behavior. Please restore a loop-based aggregation so every instantiated dimension stays correct.
-    T aTimeEnter = (std::max)(aTimeMin[0], (std::max)(aTimeMin[1], aTimeMin[2]));
-    T aTimeLeave = (std::min)(aTimeMax[0], (std::min)(aTimeMax[1], aTimeMax[2]));
+    T aTimeEnter = aTimeMin[0];
+    T aTimeLeave = aTimeMax[0];
+    for (int i = 1; i < N; ++i)
+    {
+      aTimeEnter = (std::max)(aTimeEnter, aTimeMin[i]);
+      aTimeLeave = (std::min)(aTimeLeave, aTimeMax[i]);
+    }
src/FoundationClasses/TKMath/BVH/BVH_Box.hxx (1)
206-279: 1D JSON serialization/deserialization appears inconsistent (min printed twice, max never read)

In the n == 1 branches:
DumpJson() prints myMinPoint[0] twice:
OCCT_DUMP_FIELD_VALUE_NUMERICAL(theOStream, myMinPoint[0])
OCCT_DUMP_FIELD_VALUE_NUMERICAL(theOStream, myMinPoint[0])
which likely intends to output min and max.
InitFromJson() reads only a single scalar into myMinPoint[0] and never initializes myMaxPoint[0].
This makes 1D boxes asymmetric with the 2D/3D cases and can leave myMaxPoint[0] stale after JSON round-trips.

Consider updating the 1D branches along these lines:
// DumpJson
- if constexpr (n == 1)
- {
-   OCCT_DUMP_FIELD_VALUE_NUMERICAL(theOStream, myMinPoint[0])
-   OCCT_DUMP_FIELD_VALUE_NUMERICAL(theOStream, myMinPoint[0])
- }
+ if constexpr (n == 1)
+ {
+   OCCT_DUMP_FIELD_VALUE_NUMERICAL(theOStream, myMinPoint[0])
+   OCCT_DUMP_FIELD_VALUE_NUMERICAL(theOStream, myMaxPoint[0])
+ }

// InitFromJson
- if constexpr (n == 1)
- {
-   Standard_Real aValue;
-   OCCT_INIT_FIELD_VALUE_REAL(aStreamStr, aPos, aValue);
-   myMinPoint[0] = (T)aValue;
- }
+ if constexpr (n == 1)
+ {
+   Standard_Real aMin, aMax;
+   OCCT_INIT_FIELD_VALUE_REAL(aStreamStr, aPos, aMin);
+   OCCT_INIT_FIELD_VALUE_REAL(aStreamStr, aPos, aMax);
+   myMinPoint[0] = (T)aMin;
+   myMaxPoint[0] = (T)aMax;
+ }
(or an equivalent macro-based variant matching your existing JSON schema).

🧹 Nitpick comments (12)

src/FoundationClasses/TKMath/Bnd/Bnd_B3.hxx (1)
156-168: Minor cleanup in Enlarge() and possible alignment of compareDist signature

Functionally everything here is correct, but a small refactor would simplify Enlarge() and better mirror the Bnd_B2 implementation:

Avoid repeated RealType casts and keep everything in RealType:
 template <typename RealType>
 inline void Bnd_B3<RealType>::Enlarge(const Standard_Real aDiff) noexcept
 {
-  const Standard_Real aD = std::abs(aDiff);
-  myHSize[0] += RealType(aD);
-  myHSize[1] += RealType(aD);
-  myHSize[2] += RealType(aD);
+  const RealType aD = RealType(std::abs(aDiff));
+  myHSize[0] += aD;
+  myHSize[1] += aD;
+  myHSize[2] += aD;
 }
Optional: now that internal storage uses std::array, compareDist() could take std::array<RealType, 3> (or a reference to a fixed-size C-array) instead of raw RealType[3] parameters for slightly stronger type safety. That would, however, require checking all call sites, so it’s fine to defer if you prefer to keep the current signature.

Also applies to: 266-269, 325-332, 426-438
src/FoundationClasses/TKMath/Bnd/Bnd_B2.hxx (1)

310-315: Enlarge() implementation is consistent and uses RealType directly

Enlarge() now computes aD in RealType and updates myHSize directly, which is the right choice for both Bnd_B2d and Bnd_B2f. This is also consistent with the suggested refactor for Bnd_B3::Enlarge(), so applying that change there would make 2D/3D behavior and implementation style fully aligned.

src/FoundationClasses/TKMath/GTests/BVH_RadixSorter_Test.cxx (1)

114-449: Monotonicity, locality, and adjacency tests provide good behavioral coverage

The powers-of-two, ordering, monotonic X/Y/Z, adjacency (Δ=1/2/4), uniqueness, and locality-preservation tests collectively validate the main qualitative properties of Morton codes, not just specific constants. They look correct for 10‑bit per-axis Z-order and should catch most regressions in the LUT or clamping logic.

src/FoundationClasses/TKMath/BVH/BVH_RadixSorter.hxx (1)

30-81: Morton LUT and EncodeMortonCode implementation look correct

The 256‑entry THE_MORTON_LUT correctly expands 8‑bit inputs by mapping bit i to bit 3*i, and EncodeMortonCode() splits 10‑bit coordinates into low 8 bits plus high 2 bits, placing the latter at bits 24 and 27 via << 24. Offsetting the Y and Z components by 1 and 2 bits matches the standard 3‑axis Morton layout, and for (1023,1023,1023) this yields all 30 low bits set as expected.

Would you consider making EncodeMortonCode constexpr as well (it only uses bitwise operations), to align with the broader constexpr work in this PR, assuming all target compilers handle it cleanly?

src/FoundationClasses/TKMath/GTests/BVH_SpatialMedianBuilder_Test.cxx (1)

169-295: Balanced-split, 2D, main-axis, and BinnedBuilder comparison tests are sensible

The spatial-median split test on uniformly distributed data, 2D build check, and elongated X-axis distribution with theToUseMainAxis = Standard_True all probe important code paths. The CompareWithBinnedBuilder test, which asserts equal length and depth between BVH_SpatialMedianBuilder and BVH_BinnedBuilder<...,2>, is a pragmatic way to ensure they stay behaviorally aligned as long as they share the same core implementation.

If you expect these builders to diverge in strategy in the future, you might later relax this test to compare aggregate properties (e.g., primitive counts, SAH) instead of exact length/depth equality.
src/FoundationClasses/TKMath/GTests/BVH_Tools_Test.cxx (2)
192-212: Include header for std::sqrt to make the dependency explicit

These tests call std::sqrt when normalizing the diagonal ray direction but this file does not include <cmath>. If BVH_Tools.hxx ever stops including <cmath>, these tests will break. It is safer to add an explicit include here:
 #include <BVH_Box.hxx>
 #include <BVH_Tools.hxx>
 #include <Precision.hxx>
+ #include <cmath>
This keeps the test self‑contained and robust to header changes.

Please confirm whether <BVH_Tools.hxx> already includes <cmath> on all supported platforms; if not, the explicit include above is needed.

Also applies to: 246-255

412-415: Clarify intent for squared distances and “behind ray” intersections

Two small points worth tightening:

In BoxBoxSquareDistance2D the comment says // Distance = 2 while the assertion checks 4.0 (the square of the distance). To avoid confusion for future readers, consider updating the comment to explicitly say “distance squared = 4”.

In RayBoxIntersectionBehindRay, the expectation is EXPECT_TRUE(aTimeLeave < 0.0 || !aHit);, which allows both:

“no hit”, or

“reported hit, but entirely at negative times”.

If the intended contract of RayBoxIntersection is “hit only for intersections in front of the ray origin (t ≥ 0)”, you may want to assert EXPECT_FALSE(aHit); instead (and optionally that aTimeLeave < 0.0). As written, the test doesn’t enforce a clear semantic and could mask behaviour you don’t actually want.

Can you confirm what the intended API contract of BVH_Tools::RayBoxIntersection is for boxes lying entirely behind the ray origin? Tightening the assertion accordingly would make the test more meaningful.

Also applies to: 480-494
src/FoundationClasses/TKMath/GTests/BVH_Box_Test.cxx (2)

330-347: Clarify comments for degenerate Area behaviour in 1D/2D tests

The degenerate‑area tests are good, but the comments are slightly misleading:

In FlatBox1D, the comment notes that Area() returns the length (10) for degenerate 3D boxes but the assertion only checks EXPECT_GE(anArea, 0.0);. That’s fine if you don’t want to lock in the exact value; consider rephrasing the comment to avoid implying a strict guarantee when the test doesn’t enforce it.

In Area_DegenerateBox2D, the comment says “2D box with zero area should return perimeter” but EXPECT_NEAR(anArea, 5.0, ...) actually matches “sum of extents” (5 + 0), not the geometric perimeter (2 * (5 + 0) = 10). Updating the comment to e.g. “returns sum of dimensions” would make intent clearer.

These are documentation nits only; the runtime behaviour being asserted looks consistent.

Also applies to: 987-996

1022-1045: Ensure constexpr tests build on all supported compilers/standards

The three Constexpr_* tests use constexpr BVH_Vec3d and constexpr BVH_Box<Standard_Real, 3> plus static_assert on IsValid(). That’s a nice way to lock in compile‑time support, but it does rely on:

BVH_Vec3d being a literal type with constexpr constructors, and

BVH_Box having constexpr constructors and a constexpr IsValid().

Given OCCT’s range of supported toolchains, it’s worth double‑checking that all target compilers (and the chosen C++ standard) actually accept these as constant expressions. If any older toolchain lags in constexpr support, these tests might require guarding with a macro.

Could you confirm that your minimum supported compiler versions and C++ standard guarantee constexpr‑compatibility of these types? If not, a small #if around these tests using an existing OCCT constexpr‑feature macro might be needed.

src/FoundationClasses/TKMath/BVH/BVH_BuildQueue.hxx (1)

34-86: Thread-safe queue design is reasonable; verify relaxed atomics match caller expectations

Locking around myQueue plus atomic mySize / myNbThreads gives a clean separation between structural integrity and counters. The wasBusy protocol in Fetch() correctly tracks transitions between idle and busy threads, and Size()/HasBusyThreads() become cheap reads.

The only subtle point is the use of std::memory_order_relaxed for all loads/stores on both atomics. This is fine if callers use Size() and HasBusyThreads() only as approximate progress/termination hints in a loop that also calls Fetch(), but would be too weak if any code relies on strict ordering between these counters and actual work completion.

If any caller depends on a strong “no more work possible” guarantee from Size()==0 && !HasBusyThreads(), consider upgrading to memory_order_acquire for loads and memory_order_release/memory_order_acq_rel for updates, or documenting that the values are only approximate.

src/FoundationClasses/TKMath/GTests/BVH_Traverse_Test.cxx (2)

208-279: Pair-traverse test doubles align with BVH_PairTraverse contract; minor naming nit

BVH_CountAllPairs and BVH_OverlapDetector implement the BVH_PairTraverse interface as expected:

CountAllPairs never rejects nodes and counts every accepted element pair, which is ideal for validating that the pair traversal visits all pairs exactly once.

OverlapDetector uses BVH_Box::IsOut on leaf bounding boxes to gate Accept calls, matching the intended Outer/Outer pruning behavior added in BVH_PairTraverse::Select.

Note that myRejectCount in BVH_OverlapDetector is incremented on every RejectNode call (regardless of whether the node ends up accepted), and the tests use it only to confirm that RejectNode is being called. If you want the name to be more self-descriptive, consider renaming it (and RejectCount()) to something like myVisitedCount / VisitedCount(); otherwise the current behavior is functionally fine for tests.

285-306: CreateSimpleTriangulationBVH helper is reasonable; consider making box recomputation explicit

The triangulation/BVH helper builds a simple strip of disjoint triangles, then immediately calls BVH_BinnedBuilder::Build with aTriangulation.Box(). That’s a good, controlled geometry for testing traversal behavior.

If BVH_Triangulation requires an explicit MarkDirty() to force box recomputation after vertex/element edits (as seen in later tests), you might want to call aTriangulation.MarkDirty() in this helper before Box() to make the dependency explicit and avoid surprises if default-dirty behavior ever changes.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c479f6e and 0c9f283.

📒 Files selected for processing (34)

src/FoundationClasses/TKMath/BVH/BVH_BinnedBuilder.hxx (2 hunks)
src/FoundationClasses/TKMath/BVH/BVH_Box.hxx (16 hunks)
src/FoundationClasses/TKMath/BVH/BVH_BuildQueue.cxx (0 hunks)
src/FoundationClasses/TKMath/BVH/BVH_BuildQueue.hxx (2 hunks)
src/FoundationClasses/TKMath/BVH/BVH_Constants.hxx (1 hunks)
src/FoundationClasses/TKMath/BVH/BVH_Geometry.hxx (1 hunks)
src/FoundationClasses/TKMath/BVH/BVH_LinearBuilder.hxx (3 hunks)
src/FoundationClasses/TKMath/BVH/BVH_Properties.hxx (1 hunks)
src/FoundationClasses/TKMath/BVH/BVH_QueueBuilder.hxx (1 hunks)
src/FoundationClasses/TKMath/BVH/BVH_QuickSorter.hxx (2 hunks)
src/FoundationClasses/TKMath/BVH/BVH_RadixSorter.hxx (2 hunks)
src/FoundationClasses/TKMath/BVH/BVH_Ray.hxx (2 hunks)
src/FoundationClasses/TKMath/BVH/BVH_SpatialMedianBuilder.hxx (1 hunks)
src/FoundationClasses/TKMath/BVH/BVH_SweepPlaneBuilder.hxx (2 hunks)
src/FoundationClasses/TKMath/BVH/BVH_Tools.hxx (3 hunks)
src/FoundationClasses/TKMath/BVH/BVH_Traverse.hxx (2 hunks)
src/FoundationClasses/TKMath/BVH/BVH_Traverse.lxx (7 hunks)
src/FoundationClasses/TKMath/BVH/BVH_Types.hxx (4 hunks)
src/FoundationClasses/TKMath/BVH/FILES.cmake (0 hunks)
src/FoundationClasses/TKMath/Bnd/Bnd_B2.hxx (18 hunks)
src/FoundationClasses/TKMath/Bnd/Bnd_B3.hxx (17 hunks)
src/FoundationClasses/TKMath/GTests/BVH_BinnedBuilder_Test.cxx (1 hunks)
src/FoundationClasses/TKMath/GTests/BVH_Box_Test.cxx (1 hunks)
src/FoundationClasses/TKMath/GTests/BVH_BuildQueue_Test.cxx (1 hunks)
src/FoundationClasses/TKMath/GTests/BVH_LinearBuilder_Test.cxx (1 hunks)
src/FoundationClasses/TKMath/GTests/BVH_QuickSorter_Test.cxx (1 hunks)
src/FoundationClasses/TKMath/GTests/BVH_RadixSorter_Test.cxx (1 hunks)
src/FoundationClasses/TKMath/GTests/BVH_SpatialMedianBuilder_Test.cxx (1 hunks)
src/FoundationClasses/TKMath/GTests/BVH_SweepPlaneBuilder_Test.cxx (1 hunks)
src/FoundationClasses/TKMath/GTests/BVH_Tools_Test.cxx (1 hunks)
src/FoundationClasses/TKMath/GTests/BVH_Traverse_Test.cxx (1 hunks)
src/FoundationClasses/TKMath/GTests/BVH_Tree_Test.cxx (1 hunks)
src/FoundationClasses/TKMath/GTests/BVH_Triangulation_Test.cxx (1 hunks)
src/FoundationClasses/TKMath/GTests/FILES.cmake (1 hunks)

💤 Files with no reviewable changes (2)

src/FoundationClasses/TKMath/BVH/FILES.cmake
src/FoundationClasses/TKMath/BVH/BVH_BuildQueue.cxx

🧰 Additional context used

🧠 Learnings (3)

📚 Learning: 2025-07-13T22:08:43.235Z

Learnt from: CR
Repo: dpasukhi/OCCT PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-13T22:08:43.235Z
Learning: Applies to src/*/*/GTests/*.cxx : GTest files for a toolkit must be placed in src/Module/Toolkit/GTests/ and registered in FILES.cmake.

Applied to files:

src/FoundationClasses/TKMath/GTests/FILES.cmake
src/FoundationClasses/TKMath/GTests/BVH_Triangulation_Test.cxx
src/FoundationClasses/TKMath/GTests/BVH_BinnedBuilder_Test.cxx
src/FoundationClasses/TKMath/GTests/BVH_LinearBuilder_Test.cxx
src/FoundationClasses/TKMath/GTests/BVH_BuildQueue_Test.cxx
src/FoundationClasses/TKMath/GTests/BVH_Box_Test.cxx
src/FoundationClasses/TKMath/GTests/BVH_Tools_Test.cxx

📚 Learning: 2025-07-13T22:08:43.235Z

Learnt from: CR
Repo: dpasukhi/OCCT PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-13T22:08:43.235Z
Learning: Applies to src/*/*/FILES.cmake : When adding or removing files, you must edit the corresponding src/Module/Toolkit/FILES.cmake file.

Applied to files:

src/FoundationClasses/TKMath/GTests/FILES.cmake

📚 Learning: 2025-07-13T22:08:43.235Z

Learnt from: CR
Repo: dpasukhi/OCCT PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-13T22:08:43.235Z
Learning: Applies to src/*/*/FILES.cmake : After adding a file, ALWAYS update the corresponding src/Module/Toolkit/FILES.cmake file.

Applied to files:

src/FoundationClasses/TKMath/GTests/FILES.cmake

🧬 Code graph analysis (1)

src/FoundationClasses/TKMath/GTests/BVH_BinnedBuilder_Test.cxx (2)

src/FoundationClasses/TKMath/GTests/BVH_LinearBuilder_Test.cxx (12)

aBoxSet (40-40)

aBoxSet (52-52)

aBoxSet (66-66)

aBoxSet (88-88)

aBoxSet (117-117)

aBoxSet (145-145)

aBoxSet (165-165)

aBoxSet (205-205)

aBoxSet (226-226)

aBoxSet (257-257)

aBoxSet (277-277)

aBoxSet (323-323)

src/FoundationClasses/TKMath/GTests/BVH_Box_Test.cxx (6)

aBox (30-30)

aBox (535-535)

aBox (555-555)

aBox (567-567)

aBox (1027-1027)

aBox (1035-1035)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)

GitHub Check: Prepare and Build on Ubuntu with Clang (x64)
GitHub Check: Prepare and Build on macOS with Clang (No PCH)
GitHub Check: Prepare and Build on macOS with Clang (x64)
GitHub Check: Prepare and Build on Windows with MSVC (x64)
GitHub Check: Build Documentation

🔇 Additional comments (43)

src/FoundationClasses/TKMath/Bnd/Bnd_B3.hxx (3)

30-31: Array-based storage and new Center/HSize API look solid

The move to std::array<RealType, 3> for myCenter/myHSize plus the new array-based constructor, setters, and accessors is consistent and keeps the implementation straightforward. Default ctor and Clear() still set the same sentinel “void” state, and the gp-based ctor/mutators are preserved, so existing callers should remain source-compatible while new code can use the array interface.

Also applies to: 40-48, 136-155, 173-176, 180-205, 293-321

41-42: Confirm constexpr viability with std::abs and gp_XYZ accessors

Several members are now constexpr (constructors, IsVoid(), SquareExtent(), IsOut(const gp_XYZ&), IsOut(const Bnd_B3&), IsIn(const Bnd_B3&), and compareDist). Their bodies call std::abs and gp accessors (e.g., gp_XYZ::X()), which must themselves be usable in constant expressions under your minimum C++ standard and supported libcs++/libstdc++ implementations.

Please verify this still compiles cleanly on all supported compilers/standard levels; otherwise it may be safer to:

Either drop constexpr on the affected functions, or

Gate the constexpr qualifiers with configuration macros for newer standards.

Also applies to: 47-52, 68-81, 91-94, 104-107, 127-130, 150-155, 181-184, 190-193, 199-203, 222-223, 266-269, 337-342, 347-360

532-581: Sphere–box intersection logic with isSphereHollow flag matches the documentation

The IsOut(const gp_XYZ& theCenter, Standard_Real theRadius, Standard_Boolean isSphereHollow) implementation cleanly separates the solid-sphere and hollow-sphere cases:

!isSphereHollow path computes standard AABB–sphere intersection (using squared distance to the box).

isSphereHollow path first checks solid intersection, then additionally verifies whether the box is entirely inside the sphere and suppresses intersection in that special case.

This now matches the comment describing isSphereHollow and is consistent with the 2D circle analogue in Bnd_B2.

src/FoundationClasses/TKMath/Bnd/Bnd_B2.hxx (3)

28-29: 2D box switch to std::array and array-based API is coherent

The refactor to std::array<RealType, 2> for myCenter/myHSize, plus the new array-based constructor and Center()/HSize()/setter overloads, is clean and symmetric with Bnd_B3. Default initialization and Clear() preserve the same “void box” sentinel values, so semantics of IsVoid(), Add(), etc., remain unchanged while exposing a more convenient and type-safe API for new callers.

Also applies to: 38-47, 126-145, 161-164, 169-192, 197-205, 278-305

39-42: Check constexpr use with std::abs and gp_XY when targeting older standards

As in Bnd_B3, several members here are now constexpr (Bnd_B2 ctors, IsVoid(), SquareExtent(), IsOut(const gp_XY&), IsOut(const Bnd_B2&), IsIn(const Bnd_B2&), compareDist). Their implementations rely on:

std::abs on floating-point values, and

Methods and operators from gp_XY (e.g., X(), Y(), operator+, operator-).

To avoid surprises on compilers or standard libraries where those aren’t constexpr-enriched, please confirm this builds across all supported toolchains. If any fail, consider either relaxing constexpr on the affected functions or making it conditional on a configuration macro tied to your minimum C++ standard.

Also applies to: 45-52, 66-79, 91-92, 99-102, 117-120, 140-145, 147-154, 169-172, 178-181, 187-191, 197-205, 208-211, 252-255, 260-273, 294-305, 310-315, 320-324, 329-344

147-156: Segment–box intersection refactor with compareDistD improves clarity

The introduction of compareDistD(const gp_XY& aHSize, const gp_XY& aDist) noexcept and its use in IsOut(const gp_XY& theP0, const gp_XY& theP1) makes the segment–box intersection logic easier to follow:

The first phase still checks whether the infinite line intersects the box.

The second phase now clearly expresses the AABB overlap test on the segment’s own bounding box via compareDistD, instead of duplicating the component-wise comparisons inline.

Semantics remain correct with respect to the comments (void box is always “out”, and the method returns Standard_False only when a true intersection exists), so this is a nice cleanup without changing behavior.

Also applies to: 348-387, 584-603

src/FoundationClasses/TKMath/BVH/BVH_Constants.hxx (1)

45-45: Switching THE_NODE_MIN_SIZE to constexpr is fine

Same value, now usable in constant expressions; no behavioral change expected for existing users.

src/FoundationClasses/TKMath/BVH/BVH_Properties.hxx (1)

51-51: Defaulted BVH_Transform destructor is appropriate

Virtual destructor remains, body is now compiler-generated; behavior is unchanged and consistent with other BVH classes.

src/FoundationClasses/TKMath/BVH/BVH_SpatialMedianBuilder.hxx (1)

37-37: Defaulted BVH_SpatialMedianBuilder destructor is fine

Keeps the destructor virtual while removing redundant empty body; aligns with other builders.

src/FoundationClasses/TKMath/GTests/BVH_RadixSorter_Test.cxx (1)

22-112: Value-based Morton code tests match the intended 3D bit-interleaving

The origin/single-bit/interleaving/max-value tests exercise key patterns of the LUT-based encoder and their expectations (e.g., 1, 2, 4, 7, 8, 0x3FFFFFFF) are consistent with a 3D Morton scheme where axis bits occupy positions 3i, 3i+1, 3*i+2. This gives strong regression coverage for the new implementation.

src/FoundationClasses/TKMath/GTests/FILES.cmake (1)

10-21: New BVH GTest files correctly registered in TKMath FILES.cmake

All added BVH_*_Test.cxx sources are listed under OCCT_TKMath_GTests_FILES, matching the new test files and the documented requirement to update FILES.cmake when adding tests. Based on learnings.

src/FoundationClasses/TKMath/BVH/BVH_RadixSorter.hxx (1)

257-277: Voxel clamping and LUT-based encoding integration are sound

The new code computes voxel centers, clamps each axis to [0, 1023] via IntFloor and (std::min)/(std::max), sets Z to 0 for 2D cases, and then calls BVH::EncodeMortonCode once per primitive. This keeps Morton codes within 30 bits and avoids out-of-range LUT access while improving readability over the previous bit‑twiddling inline logic.

src/FoundationClasses/TKMath/BVH/BVH_Types.hxx (2)

191-218: constexpr VecComp::Get is reasonable given the pure accessors

Making VecComp<T,N>::Get constexpr preserves runtime behavior while allowing compile-time evaluation when both the vector and axis are constant. The ternary logic is side‑effect‑free and only depends on x()/y()/z()/w(), so this should be safe as long as the underlying vector accessors are acceptable for constexpr on your lowest supported standard.

304-310: constexpr IntFloor implementation is correct for arithmetic T

IntFloor’s logic (aRes = (Standard_Integer)theValue; return aRes - (aRes > theValue);) correctly handles negative values and now can participate in constant expressions. This is a drop‑in replacement for the previous inline version.

src/FoundationClasses/TKMath/GTests/BVH_SpatialMedianBuilder_Test.cxx (3)

19-31: ComputeSetBox helper is straightforward but relies on BVH_Box’s empty state semantics

The aggregation over theSet->Size() with a default-constructed BVH_Box is clean and reusable. It implicitly assumes that an empty BVH_Box combined with the first primitive box yields a valid scene box, and that passing a default box for an empty set (used in BuildEmptySet) is acceptable to the builders. That matches typical OCCT box behavior but is worth keeping in mind if those semantics ever change.

36-168: Constructor, empty-set, and basic build tests give good safety coverage

The DefaultConstructor/CustomParameters checks, plus BuildEmptySet, BuildSingleTriangle, BuildMultipleTriangles, LeafNodeSizeRespected, and MaxDepthRespected collectively validate default parameters, custom overrides, and the main structural invariants (non-negative length, reasonable depth, and leaf sizes not exceeding the configured limit). These are solid regressions for the spatial-median path.

297-412: Clustered, float-precision, identical-box, and large-dataset tests round out coverage well

The clustered-data, Standard_ShortReal, identical-box, and large-dataset scenarios ensure the builder behaves sensibly across non-uniform, degenerate, and higher-load inputs and that EstimateSAH() remains positive. This is a nice breadth of cases without over-constraining the exact tree shape.

src/FoundationClasses/TKMath/BVH/BVH_Geometry.hxx (1)

110-111: LGTM: Documentation typo corrected.

The spelling correction from "hight-level" to "high-level" improves documentation quality.

src/FoundationClasses/TKMath/BVH/BVH_LinearBuilder.hxx (2)

20-20: LGTM: Added NCollection_Vector header.

The include is required for the NCollection_Vector usage in UpdateBoundTask.

250-286: LGTM: Replaced std::vector with NCollection_Vector.

The migration from std::vector to NCollection_Vector aligns with OCCT conventions. The API changes are straightforward: push_back → Append, empty() → Size() > 0. The constructor NCollection_Vector<BoundData<T, N>>(2) correctly pre-allocates capacity for the expected maximum of 2 child nodes.

src/FoundationClasses/TKMath/BVH/BVH_QueueBuilder.hxx (1)

49-49: LGTM: Modernized destructor declaration.

Using = default is idiomatic C++11 and makes the intent explicit that the default compiler-generated destructor is sufficient.

src/FoundationClasses/TKMath/BVH/BVH_SweepPlaneBuilder.hxx (2)

49-49: LGTM: Improved leaf node size condition.

The change from span-based (aNodeEndPrimitive - aNodeBegPrimitive < leafSize) to count-based (aNodeNbPrimitives <= leafSize) is clearer and eliminates potential off-by-one confusion. This aligns with the PR's stated objective of improving leaf node size conditions across BVH builders.

60-61: LGTM: Removed unused array element.

Changing the lower bound from 0 to 1 correctly eliminates the unused first element, since the sweep loops (lines 79-89) start at index 1. This cleanup is consistent with the summary's note about removing explicit initialization of unused first elements.

src/FoundationClasses/TKMath/BVH/BVH_Ray.hxx (2)

19-20: LGTM: Added required header.

The BVH_Types.hxx include is necessary for type definitions used by BVH_Ray.

33-37: LGTM: Enabled compile-time construction.

The constexpr qualifier allows BVH_Ray instances to be constructed at compile time, aligning with the PR's modernization objectives.

src/FoundationClasses/TKMath/BVH/BVH_BinnedBuilder.hxx (2)

221-227: LGTM: Improved leaf node size condition.

Consistent with BVH_SweepPlaneBuilder, the change from span-based to count-based comparison with <= operator clarifies the leaf node decision logic and eliminates potential off-by-one issues.

274-283: LGTM: Proper SAH cost normalization implemented.

The addition of parent area normalization (lines 282-283) implements the correct Surface Area Heuristic formula where child probabilities are computed as (child_area / parent_area). This was previously commented out (as seen in line 96 of SweepPlaneBuilder) and is now properly enabled, improving the quality of the SAH-based split decisions.

src/FoundationClasses/TKMath/BVH/BVH_Traverse.hxx (2)

241-256: LGTM: Added internal traversal stack structure.

The BVH_NodeInStack structure with its constexpr noexcept constructor provides a lightweight, compile-time-constructible type for stack-based tree traversal. This is an internal implementation detail that doesn't affect the public API.

331-349: LGTM: Added internal paired-node traversal structure.

Similar to the single-tree variant, BVH_PairNodesInStack provides efficient stack management for parallel dual-tree traversal. The constexpr constructor enables compile-time initialization when possible.

src/FoundationClasses/TKMath/BVH/BVH_QuickSorter.hxx (3)

20-23: LGTM: Added required headers.

The includes support the new index-based sorting implementation with OCCT-compatible allocation.

26-27: LGTM: Updated documentation.

The comment accurately reflects the new implementation using std::sort with guaranteed O(n log n) complexity via introsort.

49-88: LGTM: Replaced in-place quicksort with robust index-based introsort.

The refactored implementation offers important improvements:

Guaranteed complexity: std::sort uses introsort with O(n log n) worst-case vs. potential O(n²) for naive quicksort

Correctness: The cycle-following permutation algorithm (lines 78-86) is a well-established technique that applies the computed permutation in O(n) element swaps

Trade-offs to be aware of:

Memory overhead: Two temporary index arrays (O(n) space each) vs. in-place sorting

Allocation cost: Two vector allocations per sort invocation

The cycle-following logic is correct: for each position i, the algorithm follows the cycle (invPerm[i] indicates where element i should go) until element i reaches its final position where invPerm[i] == i.

src/FoundationClasses/TKMath/GTests/BVH_Tree_Test.cxx (1)

20-399: BVH_Tree test coverage and expectations look consistent

The tests exercise the key BVH_Tree behaviours (node creation, primitive ranges, SAH, multi‑dimension support, mutation APIs, and internal buffers) with expectations that are consistent with a typical BVH_Tree implementation. I don’t see correctness issues or unrealistic assumptions here.

src/FoundationClasses/TKMath/GTests/BVH_BinnedBuilder_Test.cxx (1)

20-307: BVH_BinnedBuilder tests look well‑aligned with builder semantics

These tests exercise a broad range of BVH_BinnedBuilder behaviours (parameter wiring, empty/single/multi element builds, SAH sanity, leaf size and depth constraints, 2D/3D and different bin counts, and total primitive coverage) without over‑constraining the implementation. The expectations all look reasonable; I don’t see correctness problems here.

src/FoundationClasses/TKMath/BVH/BVH_Traverse.lxx (2)

27-98: Single-tree traversal stack refactor and overflow checks look sound

Using a local alias for NodeInfoBuffer() plus the non-templated BVH_NodeInStack and explicit Standard_ASSERT_RAISE before aStack[++aHead] gives clearer code and protects against stack overflow. The aHead < BVH_Constants_MaxTreeDepth - 1 condition matches the fixed-size array capacity, and the pop path still guards against underflow. I don’t see functional regressions here.

147-257: Pair-traversal Outer/Outer rejection and pair ordering are consistent with metric semantics

The new RejectNode call for the Outer/Outer (leaf/leaf) case correctly skips element-pair testing when leaf bounding boxes are disjoint, and the metric is not reused later so there’s no hidden dependency. The candidate-pair handling (building aPairs, filtering into aKeptPairs sorted by IsMetricBetter, and pushing the tail with an overflow assert against aMaxNbPairsInStack) is also consistent with the inner/inner and mixed inner/outer cases. Overall, the updated BVH_PairNodesInStack usage and pruning logic look correct and align with the new overlap tests.

src/FoundationClasses/TKMath/BVH/BVH_Box.hxx (3)

128-199: Constexpr/noexcept upgrades on BVH_Box core API look correct

Making the constructors, Clear, IsValid, the corner accessors, Size(), Center(), and axis-wise Center(theAxis) constexpr/noexcept (or inline) is consistent with their semantics and should enable more compile-time use without changing runtime behavior. The member initializations and arithmetic remain unchanged, so this is a safe modernization.

283-383: Axis-wise constexpr overlap/containment tests align with AABB expectations

The rewritten IsOut and Contains overloads (for both boxes and points) use if constexpr (N >= k) branches per axis and early-return on separating axes, which matches standard AABB logic and keeps compilation-time dimension handling explicit. The hasOverlap flag in Contains is still set per axis with short-circuiting on separation, so its final value correctly indicates overall overlap when the function returns. I don’t see behavioral regressions here.

403-562: Helper structs CenterAxis, SurfaceCalculator, and BoxMinMax updates are safe

Marking CenterAxis::Center, SurfaceCalculator::Area, and BoxMinMax::CwiseMin/CwiseMax as static inline in the specializations keeps ODR issues at bay for this header-only code while preserving behavior. The arithmetic and axis handling remain the same, so these are straightforward linkage/optimization tweaks.

src/FoundationClasses/TKMath/GTests/BVH_Traverse_Test.cxx (4)

27-202: Traversal selector test doubles correctly exercise BVH_Traverse hooks

The four selector classes (BVH_CountAllElements, BVH_BoxSelector, BVH_DistanceSelector, and BVH_LimitedSelector) override the relevant BVH_Traverse hooks (RejectNode, Accept, IsMetricBetter, RejectMetric, Stop) in a consistent way:

CountAllElements and BoxSelector provide simple acceptance criteria and just count hits.

DistanceSelector uses squared distance to the node box as the metric for ordering and pruning, with a robust “closest index” bookkeeping.

LimitedSelector’s Stop() condition is based solely on the number of accepted elements, which meshes with the traversal’s Stop() checks.

These look appropriate for validating both the generic traversal flow and the new metric-based pruning behavior.

312-417: Single-tree traversal tests cover key edge cases and new metric logic

The BVH_TraverseTest cases validate:

Normal counting, empty-tree, and null-tree behavior (CountAllElements, EmptyTree, NullTree).

Spatial selection via BVH_BoxSelector for partially, fully, and non-overlapping boxes.

Distance-based pruning (DistanceBasedSelection, MetricBasedPruning) and early termination via BVH_LimitedSelector.

Scalability on a large dataset (LargeDataSet).

These tests exercise the new metric-driven ordering and the Stop()/RejectNode interactions in realistic ways, and the expectations are framed loosely enough (e.g., bounds on counts rather than exact values) to remain stable across BVH builder heuristic tweaks.

436-555: Pair-traverse IsOut and BVH root box tests nicely target recent changes

The BVH_PairTraverseTest cases around IsOutVerification, TriangulationBoxVerification, and BVHRootBoxVerification effectively validate:

BVH_Box::IsOut semantics on non-overlapping boxes.

Triangulation-derived boxes being placed where expected.

BVH root node bounding boxes and leaf/inner classification (aData.x() checks) after BVH_BinnedBuilder changes.

The expectations (e.g., root being a leaf for 3 triangles) are reasonable behavioral invariants for small inputs, so these tests should act as good guards against regressions in future BVH builder adjustments.

557-683: Pair-traverse counting and overlap tests validate Outer/Outer pruning behavior

The remaining BVH_PairTraverseTest cases (CountAllPairs, empty/NULL trees, overlap on the same tree, non-overlapping triangulations, asymmetric sizes, and large datasets) thoroughly exercise:

The correctness of pair enumeration (e.g., 5×5 and 3×7 pair counts).

Proper handling of empty/NULL inputs.

That the new Outer/Outer bounding-box rejection in BVH_PairTraverse::Select correctly prunes disjoint trees while still invoking RejectNode (checked via RejectCount()).

These tests align tightly with the recent PairTraverse changes and should give good confidence in both correctness and robustness at scale.

src/FoundationClasses/TKMath/GTests/BVH_Triangulation_Test.cxx

- Adjusted variable initialization for improved clarity and consistency in the Select3D_SensitivePrimitiveArray class. - Minor formatting changes to enhance code readability.

- Added a default constructor to BVH_Ray for creating an invalid ray at the origin. - Updated BVH_Ray constructor to include noexcept and documentation for clarity. - Refactored BVH_Tools' RayBoxIntersection methods to improve readability and performance, including early exit optimizations. - Introduced comprehensive unit tests for BVH_Ray and enhanced tests for BVH_Tools to ensure robustness and correctness across various scenarios.

…Logic - Simplified Box-Box and Point-Box square distance calculations by replacing repetitive code with loop-based implementations for better maintainability. - Introduced new internal helper methods for setting projection states and computing projections onto edges, enhancing clarity and reducing code duplication. - Updated Point-Triangle projection logic to utilize Voronoi region testing for improved accuracy in determining the nearest point on a triangle.

… Semantics - Updated memory order semantics in BVH_BuildQueue to use acquire/release for size tracking and thread synchronization, ensuring proper visibility across threads. - Refactored EncodeMortonCode in BVH_RadixSorter to use constexpr for better compile-time evaluation and optimization.

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (4)

src/FoundationClasses/TKMath/BVH/BVH_Tools.hxx (2)

129-268: PointTriangleProjection: robust and feature-aware, but degenerate-state semantics may merit clarification

The refactor to Voronoi-region tests (vertex/edge/interior) with helpers SetVertexState, SetEdgeState, and ProjectToEdge is a solid upgrade: it matches standard closest-point-on-triangle algorithms and avoids division by zero through the aNorm / epsilon guard.

One edge case to double-check is the degenerate-triangle branch (Lines 255–260): you set the projection state to BVH_PrjStateInTriangle_VERTEX with node indices (0, 0), but return the triangle centroid (theNode0 + theNode1 + theNode2) / T(3) rather than theNode0 itself. If any downstream code assumes that VERTEX state implies the projected point equals that vertex coordinate, this could be surprising.

If callers only use the state to understand “feature type” and node range, this is fine; otherwise, consider either:

Returning theNode0 when marking VERTEX(0), or

Marking the state as BVH_PrjStateInTriangle_INNER (and documenting that degenerate triangles project to their centroid).

299-312: RayBoxIntersection algorithm is correct; verify entry-time semantics and consider explicit standard headers

The reworked slab-based RayBoxIntersection overloads are standard and look correct:

Parallel-axis handling (theRayDirection[i] == T(0)) early-outs when origin is outside the slab and otherwise leaves the interval unconstrained.

Interval updates via aTimeEnter = max(aTimeEnter, aTMin) / aTimeLeave = min(aTimeLeave, aTMax) and the early-exit when aTimeEnter > aTimeLeave are consistent with canonical implementations.

Final check aTimeLeave < T(0) correctly rejects intersections entirely behind the ray origin.

Two minor points to verify / consider:

Ray starting inside the box
When the origin lies inside the box, aTimeEnter will typically end up negative, and you expose that negative value via theTimeEnter. If existing callers expect theTimeEnter >= 0 for hits (e.g., they clamp to theTimeEnter as a travel distance), it might be safer to clamp theTimeEnter to T(0) when the intersection is accepted. Please confirm that current users either handle negative theTimeEnter or that this behavior matches the previous implementation.

Standard headers for std::numeric_limits, std::min/max, and std::abs
This function (and the degenerate-triangle logic above) now depend on <limits>, <algorithm>, and <cmath>. If those headers are currently pulled in only transitively (e.g., via BVH_Box.hxx), you may want to add explicit includes to keep this header robust to upstream changes.

Also applies to: 333-386

src/FoundationClasses/TKMath/GTests/BVH_Ray_Test.cxx (1)

1-268: BVH_Ray test coverage is strong; constexpr intent could be made more explicit

This GTest suite gives good coverage of BVH_Ray storage semantics across dimensions, signs, magnitudes, and scalar types, and it validates the new default and non-default constructors well.

For ConstexprConstructor (Lines 166–175), consider adding a static_assert on aRay.Origin.x() (or a constexpr lambda) instead of—or in addition to—the runtime EXPECT_EQ. That would make the compile-time requirement explicit and avoid the impression that the runtime expectation alone validates constexpr.

Otherwise, the tests look solid.

src/FoundationClasses/TKMath/BVH/BVH_BuildQueue.hxx (1)

106-110: LGTM! Atomic member declarations enable efficient lock-free reads.

The atomic declarations for myNbThreads and mySize are correct and necessary for the lock-free Size() and HasBusyThreads() methods. The queue and mutex remain properly non-atomic since all queue accesses occur under the mutex.

Minor note: Signed integer overflow is technically undefined behavior. While extremely unlikely in practice, if mySize or myNbThreads could theoretically exceed INT_MAX in very large or long-running BVH builds, consider using unsigned types. This is optional and low-priority.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0c9f283 and cac6751.

📒 Files selected for processing (9)

src/FoundationClasses/TKMath/BVH/BVH_BuildQueue.hxx (2 hunks)
src/FoundationClasses/TKMath/BVH/BVH_RadixSorter.hxx (2 hunks)
src/FoundationClasses/TKMath/BVH/BVH_Ray.hxx (2 hunks)
src/FoundationClasses/TKMath/BVH/BVH_Tools.hxx (6 hunks)
src/FoundationClasses/TKMath/GTests/BVH_Ray_Test.cxx (1 hunks)
src/FoundationClasses/TKMath/GTests/BVH_SweepPlaneBuilder_Test.cxx (1 hunks)
src/FoundationClasses/TKMath/GTests/BVH_Tools_Test.cxx (1 hunks)
src/FoundationClasses/TKMath/GTests/FILES.cmake (1 hunks)
src/Visualization/TKV3d/Select3D/Select3D_SensitivePrimitiveArray.cxx (1 hunks)

✅ Files skipped from review due to trivial changes (1)

src/Visualization/TKV3d/Select3D/Select3D_SensitivePrimitiveArray.cxx

🚧 Files skipped from review as they are similar to previous changes (3)

src/FoundationClasses/TKMath/GTests/BVH_Tools_Test.cxx
src/FoundationClasses/TKMath/BVH/BVH_RadixSorter.hxx
src/FoundationClasses/TKMath/GTests/BVH_SweepPlaneBuilder_Test.cxx

🧰 Additional context used

🧠 Learnings (3)

📚 Learning: 2025-07-13T22:08:43.235Z

Learnt from: CR
Repo: dpasukhi/OCCT PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-13T22:08:43.235Z
Learning: Applies to src/*/*/GTests/*.cxx : GTest files for a toolkit must be placed in src/Module/Toolkit/GTests/ and registered in FILES.cmake.

Applied to files:

src/FoundationClasses/TKMath/GTests/FILES.cmake

📚 Learning: 2025-07-13T22:08:43.235Z

Learnt from: CR
Repo: dpasukhi/OCCT PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-13T22:08:43.235Z
Learning: Applies to src/*/*/FILES.cmake : When adding or removing files, you must edit the corresponding src/Module/Toolkit/FILES.cmake file.

Applied to files:

src/FoundationClasses/TKMath/GTests/FILES.cmake

📚 Learning: 2025-07-13T22:08:43.235Z

Learnt from: CR
Repo: dpasukhi/OCCT PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-13T22:08:43.235Z
Learning: Applies to src/*/*/FILES.cmake : After adding a file, ALWAYS update the corresponding src/Module/Toolkit/FILES.cmake file.

Applied to files:

src/FoundationClasses/TKMath/GTests/FILES.cmake

🧬 Code graph analysis (1)

src/FoundationClasses/TKMath/GTests/BVH_Ray_Test.cxx (1)

src/Draw/TKQADraw/QABugs/QABugs_11.cxx (1)

aOrigin (3112-3112)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)

GitHub Check: Prepare and Build on macOS with Clang (No PCH)
GitHub Check: Prepare and Build on macOS with Clang (x64)
GitHub Check: Prepare and Build on Ubuntu with Clang (x64)
GitHub Check: Build Documentation
GitHub Check: Prepare and Build on Windows with MSVC (x64)

🔇 Additional comments (8)

src/FoundationClasses/TKMath/BVH/BVH_Ray.hxx (1)

19-45: BVH_Ray constexpr/noexcept and default construction look consistent

Including BVH_Types.hxx in the header and making both constructors constexpr/noexcept, plus the zero-initializing default constructor, align well with the new tests and expected usage. Semantics of an "invalid" zero-direction ray are clear and consistent with BVH_Ray_Test.cxx.

src/FoundationClasses/TKMath/BVH/BVH_Tools.hxx (1)

60-72: Distance accumulation changes are correct and clearer

Initializing aDist with T(0) and accumulating d * d in both box-box and point-box distance helpers is numerically correct and improves readability without changing behavior.

Also applies to: 93-105

src/FoundationClasses/TKMath/GTests/FILES.cmake (1)

10-22: BVH GTest sources correctly registered

The new BVH_* test files are properly added to OCCT_TKMath_GTests_FILES, matching the existing pattern and the requirement to register new GTests in the toolkit’s FILES.cmake.

src/FoundationClasses/TKMath/BVH/BVH_BuildQueue.hxx (5)

23-38: LGTM! Atomic infrastructure properly initialized.

The <atomic> include and constructor initialization are correct. Atomic members are properly initialized in the member initializer list, ensuring well-defined starting state for the thread-safe queue.

41-41: LGTM! Defaulted destructor is appropriate.

Since the class uses std::atomic, std::mutex, and NCollection_Sequence—all of which manage their own resources—defaulting the destructor is correct and idiomatic.

45-54: LGTM! Memory ordering correctly implements lock-free reads.

The acquire-release synchronization between Size() and Enqueue() is correct:

Enqueue() uses memory_order_release on mySize increment while still holding the mutex, ensuring the queue modification is visible before the size update.

Size() uses memory_order_acquire, establishing a happens-before relationship with the enqueue operation.

This enables efficient lock-free monitoring of queue size.

94-97: LGTM! Correct synchronization for termination detection.

HasBusyThreads() uses memory_order_acquire, which correctly pairs with the memory_order_release operations in Fetch(). This ensures that when a thread observes HasBusyThreads() == false after finding an empty queue, it can safely conclude that no other threads are processing work that might enqueue new items.

The comments clearly explain the termination detection pattern.

57-88: No issues found. All callers properly initialize the wasBusy parameter.

The verification is comprehensive. The second script confirms that all references to "Fetch" in the BVH module are contained in three files: BVH_BuildQueue.hxx (method definition), BVH_BuildThread.cxx (production usage), and BVH_BuildQueue_Test.cxx (tests). The initial search results already captured all call sites from these files, and every single one properly initializes wasBusy = Standard_False before calling Fetch(). The thread counter state-tracking logic is working correctly as designed.

…lation

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (2)

src/FoundationClasses/TKMath/BVH/BVH_RadixSorter.hxx (2)

30-81: Morton LUT + encoder logic looks sound; consider documenting 10‑bit/1024 assumption more explicitly

The LUT pattern and EncodeMortonCode() bit math look consistent with 10‑bit coordinates expanded to 30 interleaved bits (low 8 bits via LUT at positions 0–21, high 2 bits via << 24 landing at 24 and 27, then shifted for Y/Z). Given RadixSorter::Sort(..., 29, ...) and the theDigit < 24 split, this preserves the previous 0–29 bit layout.

To make future changes safer, I’d suggest (optional):

Add a brief note near THE_MORTON_LUT or EncodeMortonCode explicitly stating that it assumes 10‑bit inputs and a 1024³ grid.

If there is a central constant for grid resolution / Morton bit depth elsewhere, reference or reuse it here so a future change to the grid can’t silently desync the encoder and the radix sort depth.

257-276: Voxel clamping + EncodeMortonCode usage is correct; optional: add debug‑time range assertions

The new path that:

Computes aVoxelF from normalized center,

Floors and clamps each component to [0, aDimension - 1],

Casts to unsigned int and calls BVH::EncodeMortonCode(...)

ensures LUT indices stay within [0, 255] and that (value >> 8) & 0x03 only ever uses the low 2 bits, matching the encoder’s 10‑bit assumption. The 2D case (aNbEffComp == 2) is handled cleanly by forcing Z to 0.

If you want a bit more defensive checking in debug builds, you could (optionally) add an assertion before the EncodeMortonCode call to enforce 0 <= aVoxelX/Y/Z && aVoxelX/Y/Z < aDimension, making the coupling between clamping and encoding explicit.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between cac6751 and b11e01b.

📒 Files selected for processing (1)

src/FoundationClasses/TKMath/BVH/BVH_RadixSorter.hxx (2 hunks)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)

GitHub Check: Prepare and Build on Windows with MSVC (x64)
GitHub Check: Prepare and Build on Ubuntu with Clang (x64)
GitHub Check: Prepare and Build on macOS with Clang (x64)
GitHub Check: Prepare and Build on macOS with Clang (No PCH)
GitHub Check: Build Documentation

…rison

dpasukhi added 2 commits November 20, 2025 10:46

dpasukhi changed the base branch from master to IR November 20, 2025 12:18

coderabbitai bot reviewed Nov 20, 2025

View reviewed changes

src/FoundationClasses/TKMath/GTests/BVH_Triangulation_Test.cxx Show resolved Hide resolved

dpasukhi added 5 commits November 20, 2025 12:38

// formatting

3377840

Refactor Select3D_SensitivePrimitiveArray for Consistency

da5a8c0

- Adjusted variable initialization for improved clarity and consistency in the Select3D_SensitivePrimitiveArray class. - Minor formatting changes to enhance code readability.

dpasukhi changed the base branch from IR to master November 20, 2025 16:57

// formatting

cac6751

coderabbitai bot reviewed Nov 20, 2025

View reviewed changes

Refactor BVH_RadixSorter to Use std::clamp for Voxel Coordinate Calcu…

b11e01b

…lation

coderabbitai bot reviewed Nov 25, 2025

View reviewed changes

Fix precision in BoxForDegenerateTriangle test for y-coordinate compa…

7cc8620

…rison

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comments

Foundation Classes - Enhance BVH Implementation#62

Foundation Classes - Enhance BVH Implementation#62
dpasukhi wants to merge 10 commits intomasterfrom
bvh_testing_improvements

dpasukhi commented Nov 20, 2025

Uh oh!

coderabbitai bot commented Nov 20, 2025 •

edited

Loading

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

coderabbitai bot left a comment

Uh oh!

coderabbitai bot left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Comments

Conversation

dpasukhi commented Nov 20, 2025

Uh oh!

coderabbitai bot commented Nov 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Pre-merge checks

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

coderabbitai bot commented Nov 20, 2025 •

edited

Loading