Skip to content

Refactor collect path planner for clarity while preserving behavior#2492

Open
Squid5678 wants to merge 3 commits intoros2from
update-collect
Open

Refactor collect path planner for clarity while preserving behavior#2492
Squid5678 wants to merge 3 commits intoros2from
update-collect

Conversation

@Squid5678
Copy link
Copy Markdown
Contributor

@Squid5678 Squid5678 commented Feb 18, 2026

Disclaimer: this was generated by Codex.

Summary

This PR refactors CollectPathPlanner to make the planner easier to reason about and maintain, while preserving the same overall collection behavior and decision flow.

The previous implementation had tightly coupled logic in plan() and mixed state-management concerns across long code paths. This update keeps the core operational model intact (CoarseApproach -> Intercept -> Dampen -> FineApproach) but makes the control flow explicit and easier to debug.

What Changed

  • Replaced legacy unscoped state enum values with enum class State for stronger typing and clearer transitions.
  • Extracted repeated setup logic from plan() into focused helpers:
    • update_ball_filters(...)
    • update_approach_direction(...)
  • Kept state transitions centralized in process_state_transition(...) and retained the same threshold-driven behavior.
  • Removed dead/unused fields and declarations that added cognitive overhead without affecting runtime behavior.
  • Cleaned up includes and removed dependencies no longer needed by the header/implementation.
  • Preserved all existing state handlers (coarse_approach, intercept, dampen, fine_approach, invalid) and their core planning math.

Design Decisions and Rationale

  • Keep the existing FSM shape: The current collect behavior relies on distinct stages with different constraints and goals. Preserving this structure avoids behavioral surprises while improving readability.
  • Isolate per-frame signal updates: Ball velocity smoothing and approach-direction selection are now separated from global planning flow. This makes tuning/filter adjustments local and reduces accidental coupling.
  • Use scoped enum states: Prevents accidental cross-use of enum values and makes switch/case handling less error-prone.
  • Prefer deletion of dead complexity: Unused path/cache wiring was removed to reduce maintenance burden and make runtime behavior more obvious from the code.

Behavior Expectations

Expected runtime behavior remains the same at a high level:

  • Slow ball: transition toward controlled collection (FineApproach).
  • Moving ball: transition through intercept/dampen path before final collection.
  • Existing thresholds/constraints continue to govern transitions and trajectory generation.

Files Changed

  • src/rj_planning/include/rj_planning/planners/collect_path_planner.hpp
  • src/rj_planning/src/planners/collect_path_planner.cpp

Testing

  • Manual code review focused on state transitions, trajectory stamping flow, and obstacle/path planning handoff.

Copy link
Copy Markdown
Contributor

@shourikb shourikb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we change the way the States are defined? What are the actual changes made?

From what I can tell it's only removing path_coarse_target and adds helpers.

// approach
COARSE_APPROACH,
enum class State {
// From start of subbehavior to the start of the slow part of the approach
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the sake of consistency, states should be in all capital letters


switch (current_state_) {
// Moves from the current location to the slow point of approach
case COARSE_APPROACH:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also not sure why this changed from before, more consistent with previous design to keep it how it was

// Force the path to use the same target if it doesn't move too much
if (!path_coarse_target_initialized_ ||
(path_coarse_target_ - target_slow_pos).mag() >
(collect::PARAM_approach_dist_target - collect::PARAM_dist_cutoff_to_control) / 2) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the only change made?

automated style fixes

Co-authored-by: Squid5678 <Squid5678@users.noreply.github.com>
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.

2 participants