[costmap_filters] Add ZoneParameterFilter — config-driven per-zone parameter overrides

[aki1770-del]

Basic Info

Info	Please fill out this column
Ticket(s) this addresses	Closes #6080
Primary OS tested on	Ubuntu 24.04 (Rolling, Docker ros:rolling base)
Robotic platform tested on	Simulation / unit tests only
Does this PR contain AI generated software?	Yes — AI-assisted, with the human author (aki1770-del) reviewing every change before commit.
Was this PR description generated by AI software?	No

---

Description of contribution in a few bullet points

• Adds a new costmap filter, ZoneParameterFilter, that mutates a configurable set of nav2 parameters on configurable target nodes based on the mask value at the robot's pose. Closes Feature: Add SurfaceConditionFilter costmap filter plugin #6080.
• Generalizes the original SurfaceConditionFilter proposal per Steve's reframe in the issue thread: one filter, N parameters per state, no use-case in the C++.
• State 0 is the special reset state. Each non-zero state value (1–255 admitted; effective range bounded by the int8_t mask transport) maps to parameter overrides via YAML.
• Two configurable knobs: on_param_set_failure (warn/throw), and an optional state_event_topic that publishes a UInt8 on every state transition.
• All parameter sets are issued asynchronously via AsyncParametersClient; the hot path (process()) never calls wait_for_service. Pending futures are drained at the start of the next process() call.

Description of documentation updates required from your changes

• New parameters introduced (state_ids, target_nodes, state_<N>.<node>.<param>, nominal_defaults.<node>.<param>, state_event_topic, on_param_set_failure) need a configuration page on docs.nav2.org. Companion PR opened at docs(costmap-filters): add Zone Parameter Filter configuration page docs.nav2.org#907.
• Plugin should be added to the costmap_filters plugin page index.
• No migration guide entry needed — additive new plugin, no breaking changes.

Description of how this change was tested

• colcon test --packages-select nav2_costmap_2d --ctest-args -R zone_parameter_filter on rolling, in a Docker container built from the upstream Dockerfile.
• 13 gtest cases pass (1.4 s total): 4 architectural sanity checks plus 9 lifecycle cases. The lifecycle cases include explicit regression guards for two specific items I noticed when reading PR [binary filter] change boolean ros params #3796 (the closest prior art): a non-blocking service_is_ready() check in the hot path, and the lifetime of pending futures across process() calls.
• Local ament_uncrustify, ament_cpplint, and pre-commit all clean before push. CI status visible on this PR.

# Example YAML (the plugin in use)
local_costmap:
  ros__parameters:
    plugins: ["voxel_layer", "inflation_layer"]
    filters: ["zone_parameter_filter"]
    zone_parameter_filter:
      plugin: "nav2_costmap_2d::ZoneParameterFilter"
      enabled: true
      filter_info_topic: "/zone_filter_info"
      state_event_topic: "/zone_filter_state"
      on_param_set_failure: "warn"
      state_ids: [1, 2]
      target_nodes: [controller_server, local_costmap]
      state_1:
        controller_server:
          FollowPath.max_vel_x: 0.5
        local_costmap:
          inflation_layer.inflation_radius: 0.8
      state_2:
        controller_server:
          FollowPath.max_vel_x: 0.2
        local_costmap:
          inflation_layer.inflation_radius: 1.2
      nominal_defaults:
        controller_server:
          FollowPath.max_vel_x: 1.0
        local_costmap:
          inflation_layer.inflation_radius: 0.55

Notes for review

A few design decisions worth flagging:

1. State 0 reset uses YAML-declared nominal_defaults, not auto-capture. I tried auto-capture first — fire get_parameters then set_parameters on the same AsyncParametersClient and rely on FIFO. That doesn't actually work because the two calls go through separate underlying services::Client instances, so server-side ordering between them isn't guaranteed and a late get response would capture the overridden value. Declarative nominal_defaults is race-free, deterministic, and matches the "config-driven" preference you stated in Feature: Add SurfaceConditionFilter costmap filter plugin #6080.

2. Explicit target_nodes list is required. The user lists every node the filter will touch. The state-override parser then does longest-prefix-match against that list, so nested-namespace targets parse unambiguously. Self-documenting and catches typos at config-load.

3. AsyncParametersClient is per-target-node, not per-parameter. Per your first review on [binary filter] change boolean ros params #3796.

4. Hot path never calls wait_for_service. Per [binary filter] change boolean ros params #3796 review item 2. There's a unit test (ServiceNotReadyInHotPathDoesNotBlock) that asserts process() returns under 500 ms when the target service is not present, as a regression guard.

Out of scope for this PR

These are reasonable v2 items once this lands:

• Pre-built mask generation tools (user supplies their own publisher)
• Per-zone parameter blending across overlapping filters
• Auto-discovery of which nav2 nodes own which parameters
• Hot-reload of the state map (config change requires plugin re-init)

Future work that may be required in bullet points

• If the filter is widely adopted, the nominal_defaults declaration burden could be reduced by a sync get-at-init mode (with documented caveats about target-service availability at configure-time).
• Add a complementary multiple_states_with_overlapping_params test for the state-N to state-M transition matrix.

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-*.

[codecov]

Codecov Report

❌ Patch coverage is 93.01075% with 13 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
.../plugins/costmap_filters/zone_parameter_filter.cpp	93.01%	13 Missing ⚠️

Files with missing lines	Coverage Δ
.../plugins/costmap_filters/zone_parameter_filter.cpp	93.01% <93.01%> (ø)

... and 15 files with indirect coverage changes

🚀 New features to boost your workflow:

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

[aki1770-del]

@SteveMacenski quick note — the four #6080 design pointers (0–255 values, state 0 reset, config-driven map, warn/throw on unknown) are all honored. The state-0 reset uses YAML-declared nominal_defaults rather than auto-capture for the FIFO-not-guaranteed reason called out in the PR body. Open to revisit if any of that diverges from intent.

[SteveMacenski]

Also check the code coverage metrics, there's enough missing that needs to be added.

I did a coarse first review, so lets chat on a few subjects! There's alot of AI generated TMI comments throughout the code that should be removed or reduced. Some parts of this seem dramatically overcomplicated.

Overall, its a really good first stab.

[SteveMacenski]

Change to protected and put all methods before all variables in Nav2 styling.

All methods must have doxygen as well.

[SteveMacenski]

Please remove this and other TMI comments

[SteveMacenski]

I don't understand the "Generalises..." and onwards. THis is not related to the speed filter and it should not reference at ticket

[SteveMacenski]

What is going on here & does it need to be this way? Seems like this and the funciton where its used are overcomplicated and convoluted

[SteveMacenski]

remove

[SteveMacenski]

Always throw, this is a critical issue the robot should stop for

[SteveMacenski]

Set a reasonable default

[SteveMacenski]

Should we not make these on initialization so we don't own the discovery at runtime and building for is service ready? If we parse the states, we should be able to create our param clients at startup / on dynamic parameter changes of the states.

[SteveMacenski]

PS this will block indefinitely

[aki1770-del]

Thank you for the review.

Agreed on the inline feedback below. The changes will be shipped as paired commits with the docs PR (#907), one commit per theme so they trace cleanly to the inline comments:

• protected: + methods-before-variables + doxygen on all helpers (header reorder)
• TMI comment scrub (~25 deleted; specifically the comments citing #3796 and the ones quoting prior reviews back to you)
• state_event_topic defaults to "zone_filter_state"
• on_unknown_state parameter removed entirely; throw on unknown mask cell value (config-class fault, agreed)
• Param-client construction moved from issueAsyncSetParameters to end of loadStateConfig (~30-LOC refactor). The "dynamic parameter changes of the states" hint goes to a follow-up — it expands scope past what's in this PR.
• Probe call (the indefinitely-blocking one) removed; redundant with init-time clients above.
• Coverage: ~17 new TEST_F cases targeting the warn/error branches in loadStateConfig, transformPose failure, multi-transition sequences, multi-target-node batching, and the new init-time-client invariant. Final test count 13 → 30; coverage delta ~30pp.

Two items to flag for your read:

1. on_param_set_failure — the proposal is to default to throw while keeping the parameter, rather than collapsing to throw-always. The other filters in costmap_filters/ (speed_filter.cpp:215-223, keepout_filter.cpp:278-283, binary_filter.cpp:204-213) handle transient RPC and tf2 faults uniformly with throttled-warn-and-continue. Collapsing all such transient classes to a stack-killing exception introduces a second-order failure mode where a single network blip takes the nav stack down. Throw-by-default matches the safety call; the configurable escape preserves the existing convention for transient classes. If a fully-bundled remove (matching unknown_state) is preferred, that is also acceptable.

2. matchTargetNode longest-prefix on target_nodes — the matching itself is being kept. Dotted target-node namespaces are real in this configuration shape (["a", "a.b"] patterns surface when two layers share a parent), and YAML-flat keys like state_1.a.b.foo need disambiguation between routing to a.b (longer) versus a with parameter path b.foo. The colon-encoded round-trip in the same path was the accidental complexity — it will be replaced with structured {target_node, param} storage in the same commit per the "dramatically overcomplicated" framing.

[SteveMacenski]

I don't see any pushed commits to review 🙃

[aki1770-del]

The C.2.a fix is in commit e61099a, which changes the behavior to always throw on unknown mask state, removes the on_unknown_state parameter, and inverts Test 8 to EXPECT_THROW.

The remaining items from the original reply will be addressed next, batched with the paired docs PR commits. The warn-default defense item (PR description item 3) was retracted since the C.2.a fix makes it unnecessary.