-
Notifications
You must be signed in to change notification settings - Fork 105
Feature/external lc downsampling #123
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
Conversation
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.
LGTM! I'd double check how the sorting by time works for multi-robot loop closures (otherwise I'm going to have to go fix it), otherwise just minor cleanup
| Eigen::Affine3d pose_discrepancy = to_T_from.inverse() * lc_iter->pose; | ||
| double translation_diff = pose_discrepancy.translation().norm(); | ||
| double rotation_diff = std::acos((pose_discrepancy.rotation().trace() - 1) / 2); | ||
| double rotation_scale = 1; | ||
| double pose_diff = translation_diff + rotation_scale * abs(rotation_diff); |
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'd advocate for pulling this out and putting it somewhere more accessible (I do the same thing in PoseGraphOdom but with the frobenius norm because I was lazy). Also, I'd make the rotation scale a parameter (preferred) or drop it, I'd take a look at how I implement it for PoseGraphForOdom
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 pulled it into a function in this file, I'll let you move it elsewhere if you want to
|
Also: I actually think this is the correct place for this feature, I think I'd do something different on the PGO/PGMO side if I wanted to sparsify the loop closures |
This is a self-contained feature we implemented for active-dsg so that we could downsample the loop closures coming into Hydra. In a perfect world I think this would be handled by pgmo instead of right when we receive the loop closures, but I don't think that's feasible right now.