Add odometry covariance to ros node #283
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
nachovizzo
changed the title
nachovizzo
changed the base branch from
main
to
AUTO-2216_expose_ros_parameters_to_yaml
|
If you guys agree, I remembered that I computed before the actual covariance of the pose estimate. There is a bit of math to work, but it is not much and I can probably get it done during the next week. I already used this covariance and it seems to be usable at least for loop closure detection. If we are all in for this, we can directly do it "more right" (which doesn't mean correct) instead of putting a constant. @nachovizzo @benemer opinions? |
|
@tizianoGuadagnino I think this would be amazing. Yes. The question is, would it make sense to have the 2 options:
|
…_add_odometry_covariance_to_ros_node
|
I agree with @tizianoGuadagnino to compute some covariance estimate. @nachovizzo do you mean having the computed covariances by default and optionally you can set them yourself? I favor this over having hardcoded default values because otherwise, the user might wonder how they are picked. |
Yes, similar to the adaptive threshold configuration. Something like (pseudo-code) |
|
In any case, I was thinking that we can merge this (after reviewing dependant PR) to move forward and open a ticket for improving this in the future. I believe that even having a hardcoded value is better than not having anything, as this allow to quickly play around with packages like |
benemer
approved these changes
Mar 3, 2024
…y_covariance_to_ros_node
…_add_odometry_covariance_to_ros_node
|
added a ticket so once we merge this we don't let the auto-covariance computation fly away from our minds #296 |
tizianoGuadagnino
approved these changes
Mar 18, 2024
tizianoGuadagnino
deleted the
AUTO-2207_add_odometry_covariance_to_ros_node
branch
nachovizzo
added a commit
that referenced
this pull request
Mar 25, 2024
* Move ros params from launch file to YAML * Reuse arguments for debug clouds * Rename odometry_node to kiss_icp_node odometry is way to generic * Rename node agin * Voxel size is optional paramter * Reads better like this * New parameter proposal * Add odometry covariance to ros node (#283) * Add fixed covariance to odometry msg * Add default value just in case * pre-commit * Expose number of icp iterations in ros (#292) * Expose registration params to ROS node * More merge conflicts * Default to multithread
tizianoGuadagnino
pushed a commit
to 02alexander/kiss-icp
that referenced
this pull request
Mar 25, 2024
* Move ros params from launch file to YAML * Reuse arguments for debug clouds * Rename odometry_node to kiss_icp_node odometry is way to generic * Rename node agin * Voxel size is optional paramter * Reads better like this * New parameter proposal * Add odometry covariance to ros node (PRBonn#283) * Add fixed covariance to odometry msg * Add default value just in case * pre-commit * Expose number of icp iterations in ros (PRBonn#292) * Expose registration params to ROS node * More merge conflicts * Default to multithread
tizianoGuadagnino
added a commit
that referenced
this pull request
Mar 25, 2024
* Fix bug where point would match with random point * Use closest neightbor distance * Switch order * Refactor ROS parametrization (#282) * Move ros params from launch file to YAML * Reuse arguments for debug clouds * Rename odometry_node to kiss_icp_node odometry is way to generic * Rename node agin * Voxel size is optional paramter * Reads better like this * New parameter proposal * Add odometry covariance to ros node (#283) * Add fixed covariance to odometry msg * Add default value just in case * pre-commit * Expose number of icp iterations in ros (#292) * Expose registration params to ROS node * More merge conflicts * Default to multithread * Revert "Refactor ROS parametrization (#282)" This reverts commit 7b8b095. * Fix and improve ROS visualization (#285) * Fix this madness Simplify implementation by debugging in an ego-centric way always. Since the KISS-ICP internal map is on the global coordinates, fetch the last ego-centric pose and apply it to the map. Seen from the cloud_frame_id (which is the sensor frame) everything should always work in terms of visualization, no matter the multi-sensor setup. * Now is safe to disable this by default * Simplify, borrow the header from the input pointcloud msg This actually makes the visualization closer to the Python visualizer * Disable path/odom visualization by default In the case where we are not computing the poses in an egocentric world (base_frame != "") and we are not publishing to the TF tree, then the visualization wouldn't make sense. Therefore, disable it by default * Changed my mind If someone doesn't have that particular frame defined, then the visualization won't work. Leave this default * Move responsability of handling tf frames out of Registration (#288) * Move responsability of handling tf frames out of Registration Since with this new changes PublishOdometry is the only member that requieres to know the user specified target-frame, it is not necesary to handle all this bits. This makes the implementation cleaner, easier to read and reduces the lines of code. Now RegisterFrame is a simple callback as few months ago one can read in seconds. * typo * Easier to read * We need this for LookupTransform * Remove unused variable * Revert "Remove unused variable" This reverts commit 424ee90. * Remove unnecessary check * Remove unnecessary exposed utility function from ROS API With this change this function is not exposed (which was never the intention to) to the header. This way we can also "hide" this into a private unnamed namesapces and benefit from inlining the simple function into the translation unit * Revert "Remove unnecessary exposed utility function from ROS API" This reverts commit 23cd7ef. * Revert "Remove unnecessary check" This reverts commit d1dcb48. * merge conflicts :0 * Remove unnecessary exposed utility function from ROS API (#289) * Move responsability of handling tf frames out of Registration Since with this new changes PublishOdometry is the only member that requieres to know the user specified target-frame, it is not necesary to handle all this bits. This makes the implementation cleaner, easier to read and reduces the lines of code. Now RegisterFrame is a simple callback as few months ago one can read in seconds. * typo * Easier to read * We need this for LookupTransform * Remove unused variable * Revert "Remove unused variable" This reverts commit 424ee90. * Remove unnecessary check * Remove unnecessary exposed utility function from ROS API With this change this function is not exposed (which was never the intention to) to the header. This way we can also "hide" this into a private unnamed namesapces and benefit from inlining the simple function into the translation unit * Revert "Remove unnecessary exposed utility function from ROS API" This reverts commit 23cd7ef. * Remove unnecessary exposed utility function from ROS API With this change this function is not exposed (which was never the intention to) to the header. This way we can also "hide" this into a private unnamed namesapces and benefit from inlining the simple function into the translation unit * Revert "Remove unnecessary check" This reverts commit d1dcb48. --------- Co-authored-by: tizianoGuadagnino <[email protected]> * too many merges * Merge Rviz and Python colors * Just make the default construction more clear --------- Co-authored-by: tizianoGuadagnino <[email protected]> * Revert "Fix and improve ROS visualization (#285)" This reverts commit 20797de. --------- Co-authored-by: tizianoGuadagnino <[email protected]> Co-authored-by: Benedikt Mersch <[email protected]> Co-authored-by: Ignacio Vizzo <[email protected]>
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.
Depends: #282
Adding a default value for the covariance matrix in the odometry msg.
Far from being ideal, at least now users can tweak this value. If we later realize that it's better to have a full matrix, like in other open source systems then we might upgrade later. For now, I put the same value on the translation/rotational part. The user can only modify 3 values at the same time (Cxx == Cyy == Czz), and (Crr, Cpp, Cyawyaw) accordingly.
Note: By briefly inspecting the current forks of KISS-ICP, most of the cases people are just forking to add this exact bit