Skip to content

Conversation

@GoldenZephyr
Copy link
Contributor

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.

@GoldenZephyr GoldenZephyr changed the base branch from main to develop November 19, 2025 22:53
Copy link
Collaborator

@nathanhhughes nathanhhughes left a 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

Comment on lines 175 to 179
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);
Copy link
Collaborator

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

Copy link
Contributor Author

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

@nathanhhughes
Copy link
Collaborator

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

@GoldenZephyr GoldenZephyr merged commit b9f7161 into develop Nov 20, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants