fix: Set disabled plugins to current even with no bounds to update

[tonynajjar]

---

Basic Info

Info	Please fill out this column
Ticket(s) this addresses	#5862 (comment)
Primary OS tested on	Ubuntu
Robotic platform tested on	Dexory sim
Does this PR contain AI generated software?	No
Was this PR description generated by AI software?	Out of respect for maintainers, AI for human-to-human communications are banned

---

Description of contribution in a few bullet points

• Set disabled plugins to current even with no bounds to update.

Description of documentation updates required from your changes

Description of how this change was tested

---

Future work that may be required in bullet points

For Maintainers:

• Check that any new parameters added are updated in docs.nav2.org
• Check that any significant change is added to the migration guide
• Check that any new features OR changes to existing behaviors are reflected in the tuning guide
• Check that any new functions have Doxygen added
• Check that any new features have test coverage
• Check that any new plugins is added to the plugins page
• If BT Node, Additionally: add to BT's XML index of nodes for groot, BT package's readme table, and BT library lists
• Should this be backported to current distributions? If so, tag with backport-*.

[tonynajjar]

Now that I look at this again, I think there might be another potential bug:

1- We disable obstacle layer
2- No other plugin updates the bounds
3- in layered_costmap we skip resetting + updating the map because no layers grew the bounds
4- The disabled obstacle layer is set to current

Unless I'm missing something, I think the artifacts of the disabled obstacle layer would never be cleared with this scenarios.

The inflation_layer doesn't have this problem because of need_reinflation_ flag - It is set when disabling the layer and cause updateBounds to return the full costmap bounds.

It feels like this should be something generic: if a plugin is disabled, it should set the full costmap bounds for the next cycle to cleanup its artifacts.

@SteveMacenski can you mentally check my logic?

[codecov]

Codecov Report

❌ Patch coverage is 90.47619% with 2 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
nav2_costmap_2d/src/layered_costmap.cpp	90.47%	2 Missing ⚠️

Files with missing lines	Coverage Δ
nav2_costmap_2d/src/layered_costmap.cpp	96.61% <90.47%> (ø)

... and 17 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[SteveMacenski]

3- in layered_costmap we skip resetting + updating the map because no layers grew the bounds

Where is this? Also, if we have no layers that increase the bounds (i.e. no sensor processing layers), why would we need to update the map? There's no new information to update. The inflation has no work because there's no new data either.

I think the artifacts of the disabled obstacle layer would never be cleared with this scenarios.

How does this logic differ when we do have other layers that increase bounds?

Should we think that disabling the layer means we should clear all its previously contributed data? I don't think that's immediately obvious that that is the intended behavior. How does it all get cleared with other sensor processing layers (just in bounds) & is that expected either?

It feels to be like this should be something generic: if a plugin is disabled, it should set the full costmap bounds for the next cycle to cleanup its artifacts.

If the expected behavior is to clear out its contributions, sure. But that also is a pretty big computational hit for large costmaps, no?

[tonynajjar]

Where is this?

  if (xn < x0 || yn < y0) {
    return;
  }

This is the logic that would return early if no bounds are expanded, skipping combined_costmap_.resetMap(x0, y0, xn, yn); and (*plugin)->updateCosts(combined_costmap_, x0, y0, xn, yn);

if we have no layers that increase the bounds (i.e. no sensor processing layers), why would we need to update the map?

I'd imagine the disabled layers would set the full bounds to clear out its artifacts

Should we think that disabling the layer means we should clear all its previously contributed data? I don't think that's immediately obvious that that is the intended behavior
If the expected behavior is to clear out its contributions, sure.

Hmm, my instinct was yes but maybe it's not obvious for others like you said. In that case....... clear_on_disable param? 😄

But that also is a pretty big computational hit for large costmaps, no?

It is but I mean you gotta do what you gotta do to clear artifacts, I don't see a way around it. Alternative one would call ClearEntireCostmap which is also expensive.

As a good next step, we experimentally confirm that what I'm saying is true, that is, that disabling a layer does not clear its artifacts - no guarantee that I get to this soon but this PR is independent anyway; if this gets merged I'll create a follow up ticket

[BriceRenaudeau]

First test works great, thanks.

For the other questions, I also assume that the "disable/enable" parameter should work as a "pause/resume" and not a clear. In our case, we want to clear it, so we just call the clear_region service.

In my opinion, the current behavior is correct.

[tonynajjar]

I also assume that the "disable/enable" parameter should work as a "pause/resume"

Ok if you both think so then let's not change that default behavior, a clear_on_disable param might still makes sense but I wouldn't implement it now. Let's just focus on the PR at hand then

[SteveMacenski]

So do we agree this is good to merge?

[tonynajjar]

Good to go from my side