Call reset on new plan, not new goal

[mintar]

The documentation on reset() says:

called at the beginning of every new navigation, i.e. when we get a new global plan

The reset() functions were called in setGoalPose(), i.e. when the user gives a new goal. However, when the local planner fails and the global planner replans, setGoalPose() is not called, but setPlan() is. This PR makes sure that reset() is also called in this case.

[DLu]

So this is a case where I forgot to update the documentation as the interfaces evolved. It should have said new global goal, which was only detected when the global plan's last endpoint shifted. I changed it to make it more explicit.

Do you have a use case where you want things to be reset per-global-plan?

[mintar]

I changed it to make it more explicit.

Where? I guess not pushed yet?

Do you have a use case where you want things to be reset per-global-plan?

Hmmmm... I thought I did: I wrote a Critic that scores the distance to an intermediate goal, and remembers which intermediate goal poses have already been reached to stop oscillations between two intermediate goals. This list of reached goals is cleared on reset(). Since it's somehow related to the current plan, I was surprised that reset() wasn't called.

Upon reflection, it's even better to reset this list per-goal, not per-plan: If I set the global planner to continually replan, it would otherwise reset my list too often. So I'm closing this issue now.

[DLu]

Sorry, dangling reference. It here refers to me the nav_core2 interface. I believe I wrote that comment before there was an explicit setGoal function in the local planner interface. I'll update the comment accordingly soon.

[DLu]

Following up on the comments here

@mintar My thinking is that the critics already have access to the global plan via the prepare method. Do you basically want a version of RotateToGoal critic that resets when there is a new global plan?

[mintar]

My thinking is that the critics already have access to the global plan via the prepare method.

The trouble with that is, that the prepare method is called with the transformed global plan:

https://github.com/locusrobotics/robot_navigation/blob/578479ee255b5281572c9b6333bd5acecf5dd5be/dwb_local_planner/src/dwb_local_planner.cpp#L220

Thus, the critic will be called with a different global plan on each iteration, and it is not trivial to figure out when there is an actual new global plan was send. It would require to align the transformed plan from the previous iteration with the current one and calculate the difference between them, and that's not even guaranteed to work if the two plans are similar within the current cost map window. It seemed much cleaner to me to explicitly call reset() on each new plan.

Do you basically want a version of RotateToGoal critic that resets when there is a new global plan?

Yes. That would fix the bug I described in this comment, but I suspect there might be more such bugs in other critics as well.

The current implementation is that reset() is only called when a global plan is triggered by a new global goal, as opposed to being triggered by replanning. The new behavior propsed here is to always call reset() on a new global plan, no matter what triggered it.

Can you come up with an example where the current behavior is more useful than the proposed one? I.e., where the critics should behave differently based on whether replanning or a new goal triggered the new global plan?