[hugogo1998]

Description

Path::getPathSegment() currently scans from the beginning for every lookup. During time parameterization, queries usually move forward, so the same segments are visited repeatedly.

This change stores the last segment index and resumes forward queries from there. Backward queries restart at the beginning. The cached index is atomic so concurrent const queries remain safe.

Results

Mean time for a full time-parameterization pass:

Waypoints	Current	This change	Speedup
20	7.83 ms	6.42 ms	1.21x
40	10.59 ms	5.98 ms	1.76x
80	18.59 ms	6.73 ms	2.76x
160	60.71 ms	7.92 ms	7.64x

Testing

Added coverage for forward and backward queries, segment boundaries, copied paths, and concurrent queries.

Built moveit_core and ran the targeted test in moveit/moveit2:humble-ci: 18/18 gtests passed; colcon reported 19 tests, 0 errors, 0 failures, 0 skipped.

Checklist

• Required by CI: Code is auto formatted using clang-format
• Extend tutorials / documentation — not applicable; no user-facing change
• Document API changes in MIGRATION.md — not applicable; no API change
• Create tests — added targeted cache coverage
• Include a screenshot if changing a GUI — not applicable
• While waiting for the PR to be reviewed, review another open PR

Summary by CodeRabbit

• Performance
  • Repeated path-segment lookups are faster, especially for queries that move forward through a path. Queries continue to return consistent results when moving backward or crossing segment boundaries, and concurrent path queries preserve expected results. These improvements reduce lookup overhead during time-optimal trajectory generation without changing the results users receive.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: defaults
• Review profile: CHILL
• Plan: Advanced
• Run ID: b36fee25-bb61-400f-b6d7-7573b0807783

📥 Commits

Reviewing files that changed from the base of the PR and between 42847bb and 2d08b6e.

📒 Files selected for processing: 1

• moveit_core/trajectory_processing/test/test_time_optimal_trajectory_generation.cpp

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.

📝 Walkthrough

Path now stores segments in indexed storage and caches the current segment index atomically. Tests compare cached and uncached queries across traversal order, segment boundaries, copied paths, and concurrent access.

Changes: Path-segment cache

Layer / File(s)	Summary
Indexed path-segment cache moveit_core/trajectory_processing/include/moveit/trajectory_processing/time_optimal_trajectory_generation.hpp, moveit_core/trajectory_processing/src/time_optimal_trajectory_generation.cpp	Path stores segments in a std::vector and tracks the cached segment with an atomic index. getPathSegment uses indexed traversal and resets the cache for backward queries.
Cache behavior validation moveit_core/trajectory_processing/test/test_time_optimal_trajectory_generation.cpp	Tests compare cached and uncached configuration, tangent, and curvature queries across traversal orders, segment boundaries, copied paths, and concurrent access.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Refactor

Suggested reviewers: marioprats

Merge Risk: ⚪ Minimal · up to 2d08b

No concrete merge-blocking issue is established by the reviewed change.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 1 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly and concisely describes the main change: caching the last-accessed segment in Path::getPathSegment.
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[hugogo1998]

Hi @davetcoleman, could you take a look at this PR when you get a chance? Please let me know if I overlooked anything. Thanks.

[marioprats]

Looks good, thank you!

[codecov]

Codecov Report

❌ Patch coverage is 98.11321% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 49.02%. Comparing base (a9004a4) to head (2d08b6e).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...g/test/test_time_optimal_trajectory_generation.cpp	97.83%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3800      +/-   ##
==========================================
+ Coverage   48.92%   49.02%   +0.11%     
==========================================
  Files         735      735              
  Lines       62684    62729      +45     
  Branches     7617     7624       +7     
==========================================
+ Hits        30663    30748      +85     
+ Misses      32021    31981      -40     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

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

[hugogo1998]

Just fixed the CI error, please take a look again when you have time, thanks @marioprats