-
Notifications
You must be signed in to change notification settings - Fork 24
Pose filtering #655
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Pose filtering #655
Conversation
Andeshog
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
part1
| pose_sub_topic: "/aruco_detector/board" | ||
| pose_array_pub_topic: "/filtered_pose_array" | ||
| landmark_pub_topic: "/filtered_landmarks" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason these topics are not part of the topic list in orca.yaml, like the rest of the packages in vortex-auv?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This package is not supposed to be used in production. Only for tuning/testing and debugging. The plan is to have the landmark server import the /lib part of this package. So this package will mainly be used to tune for various different obejcts etc.
| /** | ||
| * @brief Get the list of currently maintained tracks. | ||
| * @return const reference to internal track vector | ||
| */ | ||
| const std::vector<Track>& get_tracks() { return tracks_; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would also add const at the end to signal that the function does not modify
const std::vector<Track>& get_tracks() const;| // Internal bookkeeping | ||
| int track_id_counter_ = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would use brace init for consistency
| void setup_debug_publishers(); | ||
| void publish_meas_debug(); | ||
| void publish_state_debug(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
picky, but nice to group methods/member variables
| output='screen', | ||
| parameters=[config, {'use_sim_time': True}], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is use_sim_time default to true? Do we even have a \clock topic in sim?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just used this for testing with a rosbag and then tf2 is very quick to complain about timestamps. I can add a comment explaining its use case
|
|
||
| Eigen::Matrix<double, 3, Eigen::Dynamic> Z(3, measurements.size()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Z? Perhaps a more descriptive name? 😅
| if (q.w() < 0.0) { | ||
| q.coeffs() *= -1.0; | ||
| } | ||
|
|
||
| double norm_v = q.vec().norm(); | ||
|
|
||
| if (norm_v < 1e-6) { | ||
| return 2.0 * q.vec(); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for these "magic operations" maybe a comment would be nice
| initial_position_std_ = config.initial_position_std; | ||
| initial_orientation_std_ = config.initial_orientation_std; | ||
| ipda_config_ = config.ipda; | ||
| existence_config_ = config.existence; | ||
| max_angle_gate_threshold_ = config.max_angle_gate_threshold; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps have the config as a member instead of the individual fields?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it might become more cluttered if i do that as the code is now. The most structured solution would probably be to refactor and separate out a position filter similar to the orientation filter class. I will work on that later if I have time.
I should probably add some dynamic reconfiguration of ros parameters to aid in the tuning process and that might reveal a more intuitive struct setup.
Pose filtering ros package plus track manager library.
Mainly used for testing and development.
Intended use case is to have the landmark server include the library and do the landmark filtering