Simplify chunked octree traversals #84
Merged
Conversation
This file contains 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
The initial implementation did not properly distinguish pointers-to-const (i.e. `const T*`) from const pointers (i.e. `T* const`), resulting in complicated code and incorrect handling of some edge cases. The new version's code is cleaner and shorter, and more closely aligns with standard C++ syntax: * pointer-to-const: `ChunkedNdtreeNodePtr<const YourChunkType>` * const pointer: `const ChunkedNdtreeNodePtr<YourChunkType>` * const pointer-to-const: `const ChunkedNdtreeNodePtr<const YourChunkType>`
Instead of first calling hasChild and then retrieving a reference with *getChild, this change directly uses the pointer returned by getChild, avoiding a double lookup and improving efficiency. With C++17's if condition init-statements, the code remains clean and easy to read.
victorreijgwart
force-pushed
the
feature/simplify_chunked_octree_traversals
branch
from
7c9a2fc to
3de0323
Compare
LionelOtt
reviewed
Nov 22, 2024
| const auto num_queries = index_view.shape(0); | ||
| // Create the raw results array and wrap it in a Python capsule that | ||
| // deallocates it when all references to it expire | ||
| auto* results = new float[num_queries]; |
There was a problem hiding this comment.
I assume that's nanobind that wants to have raw memory fun rather than raii things?
There was a problem hiding this comment.
Yes. I was also a bit surprised. Their docs seem to suggest this approach, and I couldn't find an RAII-based alternative.
|
I read through it, seems like a lot of clean-up and unification and typing changes but overall it's not too complicated to follow in my view. |
victorreijgwart
force-pushed
the
feature/simplify_chunked_octree_traversals
branch
from
5be9501 to
75f4edf
Compare
This reverts commit 75f4edf, as we believe its changes are not yet ready for release. Tracy is configured through CMake options, part of which are converted to C++ compile definitions. Since they affect both Tracy's binary and headers, the same options/definitions must be set when compiling Tracy's system library, wavemap C++ library (CMake), and its ROS1 interface (catkin). A complicating factor is that catkin ignores standard CMake features like inheriting a linked library's PUBLIC definitions. We will reintroduce Tracy-support once we find a cleaner, less error-prone way to integrate it into the project.
|
Thanks @LionelOtt. I'll update the Changelogs and document the API changes, and then we can merge it. |
victorreijgwart
deleted the
feature/simplify_chunked_octree_traversals
branch
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.
Description
Release 2.0.0 introduced experimental
NodePtrandNodeRefclasses, enabling traversal of chunked octrees as if they were regular octrees. This PR finalizes their implementation, uses them to simplify map operations and measurement integrators, and extends theQueryAcceleratorto support chunked octrees. Benchmarks confirm these changes improve code clarity while slightly enhancing performance.Type of change
Detailed Summary
Prior to this PR, handling chunked octrees involved explicit reasoning about chunks, relative node positions, and parent-child chunk boundaries. This approach made the code harder to read, maintain, and debug.
Release 2.0.0 introduced
NodePtrandNodeRef, inspired by STL’sstd::vector<bool>::bitref, to emulate the semantics of standard octree nodes for chunked octrees. However, their implementation was experimental, and the performance impact compared to directly interacting with chunks was uncertain.This PR:
NodePtrandNodeRef.QueryAcceleratorto supportHashedChunkedWaveletOctrees.Extensive benchmarks show that these changes not only simplify the codebase but also slightly improve performance.
In addition, several older code sections were cleaned up. While most changes are internal, notable updates affecting users are documented below.
API Changes
List any changes to wavemap's APIs to help users update their code. Write "None" if there are no changes.
C++ API:
Additions
QueryAcceleratornow also supportsHashedChunkedWaveletOctreesChunkedNdtree::NodeRefRefactoring
using ... = ...) nested in the Map classes have been moved or renamedPython API:
Additions
ROS1 Interface:
Refactoring
TfTransformernow returns the transform by value, and indicates lookup failures by returning std::nulloptReview Notes
@LionelOtt A general review would be great. Let me know if some parts of the code are difficult to follow or if the purpose of certain methods or classes is unclear. Then I'll add more comments and docstrings.
Testing
Automated Tests
The
NodePtrandNodeRefclasses are thoroughly tested via thendtreedata structure, map, and measurement integrator test suites. Additionally, theQueryAcceleratortest suite has been extended to coverQueryAccelerator<HashedChunkedWaveletOctree>.Manual Tests
The changes were manually tested by rerunning the Demos on an AMD laptop and an Intel desktop.
Benchmarks (To be completed by maintainers)
Newer College 20cm
Newer College 5cm
Pantopic Flat 5cm
Pantopic Flat 2cm
Note that we leave out the plots comparing the accuracy-over-distance of
proposedandv2.1.2, as they fully overlap. The plots forv2.1.2can be seen in PR #83.Checklist
General
Documentation (where applicable)