Auto 2257 fix double construction kiss icp cpp #294
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
Since we can't guarantee how the pipeline is going to be constructed at runtime is better to force the user to wrap it in a pointer type to avoid having a default-constructed object that "should" be modified later on
This way we guarantee that it's constructed once the `config_` has been properly populated
benemer
approved these changes
Mar 12, 2024
tizianoGuadagnino
approved these changes
Mar 18, 2024
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.
NOTE: I sneaked some small changes alongside this PR to avoid stacking 20 other PRs. I tried to be more verbose commit-wise (don't just review the full diff) and also provided a description here
Testing #252 I realized that there was a small buggy problem in the construction of the C++ pipeline:
Since we were using a stack KISS-ICP pipeline, we were constructing twice, once at compile time with default parameters and 2nd at runtime with the runtime ROS params
Besides this being ugly led to a horrible bug, that luckily I captured early, which was the fact that I couldn’t control the number of threads (static const g) since it was default constructed to multithreaded and ignored at runtime 🤦♂️
Changes
kiss_icp::pipeline::KissICPdefault constructor 15e20a2. Python system is not affected, nothing is affectedodometry_way too generic 8c38132Minor changes
Since I was forced to review the horror I wrote last year, I also:
config_to be an unnecessary class member of the odometry server 250708a, and update the utils accordingly at 533047c