Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Pilz blending times... the right way #2961

Merged
merged 3 commits into from
Aug 11, 2024

Conversation

sea-bass
Copy link
Contributor

@sea-bass sea-bass commented Aug 10, 2024

Description

Found the issue!

Turns out that Pilz's appendWithStrictTimeIncrease() function had a single if-statement that encompassed two conditions:

  1. If trajectory is empty
  2. If the end state of one segment is not equal to the start state of the second segment

In both cases, the whole trajectory segment was appended with a dt=0.

The problem is that in case 1, it's appropriate to add a dt=0 (as the trajectory is empty, the first point should be at t=0). In case 2, we needed to apply a non-zero sample time offset dictated by the actual dt of that trajectory's first waypoint.

Closes #2945

Checklist

  • Required by CI: Code is auto formatted using clang-format
  • Extend the tutorials / documentation reference
  • Document API changes relevant to the user in the MIGRATION.md notes
  • Create tests, which fail without this PR reference
  • Include a screenshot if changing a GUI
  • While waiting for someone to review your request, please help review another open pull request to support the maintainers

@sea-bass
Copy link
Contributor Author

sea-bass commented Aug 10, 2024

@ct2034 want to give this a sanity check? (or maybe it's been too long...)

Copy link
Contributor

@stephanie-eng stephanie-eng left a comment

Choose a reason for hiding this comment

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

LGTM!

Could you verify that this prevents the duplicate timestamp issue you saw in the tutorial? I could never reproduce it for whatever reason, so would be good to know if it works for you.

@sea-bass
Copy link
Contributor Author

Could you verify that this prevents the duplicate timestamp issue you saw in the tutorial? I could never reproduce it for whatever reason, so would be good to know if it works for you.

Oh, it for sure resolves this issue. It's how I've been testing it.

In fact, the previous PR squashed duplicate time points, but this one actually handles it correctly and ensures the two points are spaced accordingly.

@sea-bass sea-bass added this pull request to the merge queue Aug 11, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 11, 2024
@sea-bass sea-bass added this pull request to the merge queue Aug 11, 2024
Merged via the queue into main with commit 4fad0d0 Aug 11, 2024
10 of 13 checks passed
@sea-bass sea-bass deleted the fix-pilz-blend-time-duplication branch August 11, 2024 15:13
@sea-bass sea-bass added backport-humble Mergify label that triggers a PR backport to Humble backport-iron Mergify label that triggers a PR backport to Iron and removed backport-humble Mergify label that triggers a PR backport to Humble backport-iron Mergify label that triggers a PR backport to Iron labels Aug 12, 2024
@sea-bass sea-bass added backport-humble Mergify label that triggers a PR backport to Humble backport-iron Mergify label that triggers a PR backport to Iron and removed backport-humble Mergify label that triggers a PR backport to Humble backport-iron Mergify label that triggers a PR backport to Iron labels Sep 14, 2024
mergify bot pushed a commit that referenced this pull request Sep 14, 2024
* Fix Pilz blending times... the right way

* Remove more debugs

* Format

(cherry picked from commit 4fad0d0)

# Conflicts:
#	moveit_planners/pilz_industrial_motion_planner/src/command_list_manager.cpp
#	moveit_planners/pilz_industrial_motion_planner/src/trajectory_blender_transition_window.cpp
mergify bot pushed a commit that referenced this pull request Sep 14, 2024
* Fix Pilz blending times... the right way

* Remove more debugs

* Format

(cherry picked from commit 4fad0d0)

# Conflicts:
#	moveit_planners/pilz_industrial_motion_planner/src/command_list_manager.cpp
#	moveit_planners/pilz_industrial_motion_planner/src/trajectory_blender_transition_window.cpp
sea-bass pushed a commit that referenced this pull request Sep 15, 2024
sjahr pushed a commit that referenced this pull request Oct 29, 2024
Co-authored-by: Sebastian Castro <4603398+sea-bass@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-humble Mergify label that triggers a PR backport to Humble backport-iron Mergify label that triggers a PR backport to Iron
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix duplication of Pilz motion sequence trajectory points... the right way
3 participants