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

[Don’t Merge] Sync with MoveIt1 #2212

Draft
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

afrixs
Copy link

@afrixs afrixs commented May 30, 2023

Description

Syncing the following commits

* [2022-09-09][b3b363f7b] | Limit Cartesian speed for link(s) (#2856) {{Michael Görner}}
* [2022-09-09][87038c3e2] | Update panda_moveit_config to noetic-devel in .rosinstall files {{Robert Haschke}}
* [2022-09-09][dadd5063e] | MSA templates: replace hard-coded package name {{Robert Haschke}}
* [2022-09-09][63660ef21] | Merge PR #3093: extended ACM editing in MSA {{Robert Haschke}}
* [2022-09-09][1245f1513] | Merge PR #3197: Improve computeCartesianPath() {{Robert Haschke}}

Note: Since structure of MSA has changed in MoveIt2, I had to guess a correct placement of methods which work with SRDFWriter. I moved them into moveit_setup::srdf_setup::DefaultCollisions.

Also I had to re-add trajectory_processing::updateTrajectory method which had resided (in moveit1) in a deprecated and deleted trajectory_processing::IterativeParabolicTimeParametrization but is used by LimitMaxCartesianLinkSpeed planning adapter. The new placement for the method is in trajectory_tools.h

Note: a sync PR must be merged into moveit_msgs as well to make this build. I'll create it tomorrow as soon as I get write access to our company's fork. Edited: the PR already exists: moveit/moveit_msgs#159

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

rhaschke and others added 15 commits August 30, 2022 21:10
... as revealed by asan. Seems to be a bug in libyamlcpp.
- Directly use SRDFWriter as data source for data models
- LinkPairMap only used temporarily in generateCollisionTable()
- Use wip_srdf_ as work-in-progress copy of config_data_->srdf_
  to allow reverting
Move common code into method selectedSections()
When switching from matrix to linear view, the checkbox state was ignored.
- Run costly mapToSource() only if actually needed
- Signal dataChanged() on success only
* Make `IterativeParabolicTimeParameterization::updateTrajectory()` public, static
* Add Cartesian speed limitation in `cartesian_path_service` and new planning request adapter
  `default_planner_request_adapters/LimitMaxCartesianLinkSpeed`
* Provide setters in MoveGroupInterface (both C++ and Python)

Co-authored-by: Benjamin Scholz <ben.scholz@posteo.net>
Co-authored-by: Thies Oelerich <thies.oelerich@iwu.fraunhofer.de>
Co-authored-by: Gauthier Hentz <64833674+gautz@users.noreply.github.com>
Co-authored-by: Robert Haschke <rhaschke@techfak.uni-bielefeld.de>
afrixs pushed a commit to Spinbotics-s-r-o/moveit2 that referenced this pull request Jun 13, 2023
@afrixs
Copy link
Author

afrixs commented Jun 14, 2023

friendly ping

@Abishalini
Copy link
Contributor

Abishalini commented Jun 26, 2023

Thank you for your efforts. I am reviewing this. In the meantime, can you rebase your PR with main?

We prefer not using the Update Branch button on the web UI so that the following commits don't pollute the git log when this is merged to main
image

@afrixs afrixs force-pushed the pr-sync-cart-speed branch from a5d3e4d to da36544 Compare June 28, 2023 08:03
afrixs pushed a commit to Spinbotics-s-r-o/moveit2 that referenced this pull request Jun 28, 2023
@afrixs
Copy link
Author

afrixs commented Jun 28, 2023

@Abishalini thank you for reviewing.

In the meantime, can you rebase your PR with main?

I think I'll need your help here... I removed the Update branch commits by force-pushing my previous commit, but I have problems with the rebase part. Could you please add command-by-command instructions on how to do it? I don't know all the techniques that can be used to achieve something in git/github, but what I tried resulted just in a long manual merging process (the Moveit1 sync had many conflicts, as ACM structure has totally changed in Moveit2), which is temporally impossible to do every few days when the main branch updates:

  • Doing the rebase by this guide reintroduces all the conflicts I have already resolved and totally leaves out my merge commits (175bc04 and da36544).
  • Moveit2 syncing guide says I shouldn't rebase at all, but I should repeat the whole sync process and resolve conflicts "easily by checking out the changes from the outdated sync branch". But I can't imagine a way of checking out (or somehow merging) changes from a different branch during an unresolved merge. Also, the new sync process would need a new branch, which means a new PR with all the waiting/approval/etc. included

@afrixs afrixs force-pushed the pr-sync-cart-speed branch from b2fa17e to 3b06891 Compare June 28, 2023 09:53
@Abishalini
Copy link
Contributor

Hi @afrixs ,
I apologize for the delay in the review.
To rebase with main and preserve the commits from MoveIt1

git checkout main
git pull origin main
git checkout <sync-branch>
git reset --hard main
git merge b3b363f7b

Unfortunately, this would require you to resolve all the merge conflicts again.

You could also create a clone of this branch

git checkout <sync-branch>
git checkout -b <sync-branch-clone>
### Go through all the commands listed in the block above
git checkout <sync-branch-clone> <file-name-which-has-merge-conflicts>

This way you can easily transfer your work and need not manually fix every conflict.

This review took more time since the changes are not trivial. Once this PR is close to ready, we will make sure not to merge any other PRs in the meantime so that this PR does not need to be rebased again.

@henningkayser henningkayser marked this pull request as draft August 29, 2023 14:24
@mergify
Copy link

mergify bot commented Oct 22, 2023

This pull request is in conflict. Could you fix it @afrixs?

@Abishalini Abishalini mentioned this pull request Oct 31, 2023
1 task
Copy link

github-actions bot commented Dec 7, 2023

This PR is stale because it has been open for 45 days with no activity. Please tag a maintainer for help on completing this PR, or close it if you think it has become obsolete.

@github-actions github-actions bot added the stale Inactive issues and PRs are marked as stale and may be closed automatically. label Dec 7, 2023
@github-actions github-actions bot removed the stale Inactive issues and PRs are marked as stale and may be closed automatically. label Dec 5, 2024
Copy link

mergify bot commented Dec 5, 2024

This pull request is in conflict. Could you fix it @afrixs?

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.

4 participants