added loop_closure_enabled_, to enable/disable loop closure constrains #36
Conversation
| loop_closure_constraints_enabled_); | ||
| ROS_INFO_STREAM_COND( | ||
| verbose_, | ||
| "Loop Closure registration constraints: " |
There was a problem hiding this comment.
Strictly speaking, the loop closures are added to the pose graph as relative pose constraints. Registration constraints are added automatically for all overlapping submaps, so implicitly loop closures will also lead to registration constraints - but only if they cause submaps to overlap.
Could you change this INFO message to something like "Loop closure constraints: "? Just to reduce the change of other users getting confused.
| pose_graph_interface_.addLoopClosureMeasurement(submap_id_A, submap_id_B, | ||
| T_AB); | ||
|
|
||
| // Check if loop closure constraints is enabled |
There was a problem hiding this comment.
With the suggested implementation of this callback, the loop closure constraints would be shown in Rviz despite not being used in the optimization.
This could be confusing to other users. It'd be cleaner if the first block of code at the top of this method was:
if (!loop_closure_constraints_enabled_) {
ROS_WARN_THROTTLE(1.0, "Received loop closure constraint, but loop_closure_constraints_enabled is set to false. Constraint will be ignored.");
return;
}
So that if loop closures are disabled, they're ignored entirely - such that the visuals and optimization stay consistent.
Alternatively, you could start the method with:
if (!loop_closure_constraints_enabled_) {
ROS_WARN_THROTTLE(1.0, "Received loop closure constraint, but loop_closure_constraints_enabled is set to false. Constraint will be ignored by the optimization but still shown in Rviz.");
}
But there'd still be a high chance that people don't see the warning message (e.g. when running it on a robot, or when there are many other messages being published to the same log), so unless there's a strong reason not to, I'd rather go with the first option.
No description provided.