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

sync MoveIt 1 master #47

Merged
merged 10 commits into from
Mar 19, 2019
Merged

sync MoveIt 1 master #47

merged 10 commits into from
Mar 19, 2019

Conversation

rhaschke
Copy link
Contributor

No description provided.

davetcoleman and others added 10 commits March 5, 2019 16:38
Make it clear that future migration notes go in a new section
* Fix alignment of Eigen transforms

With AVX support, Eigen requires a 32-byte alignment,
instead of the default 16-byte alignment.

* use constexpr
* Make hidden __ methods static and move to end

* Remove unused import

* Use the apply_planning_scene service to add objects

* "X != None" --> "X is not None"

* Add arguments asynchronous and service_timeout

* Fix PEP style warning (except for line length)
PR2 robot requires a different path then Fanuc and Panda.
* Conform class name to `CamelCase`

* Conform member method name to `camelBack`

* Exceptions to method name

* Conform local variable name to `lower_case` part 1

* Conform local variable name to `lower_case` part 2

* Conform local variable name to `lower_case` part 3

* Conform local variable name to `lower_case` part 4

* Local static variable to `lower_case`

* Local variable manual fix

* Exceptions to local variable name

* Conform static const variable name to `UPPER_CASE`

* Conform global variable name to `UPPER_CASE`

* Conform static const member variable to `UPPER_CASE`

* clang-format

* Travis: mandatory clang-tidy-check

* Catch up most recent changes

* Update .clang-tidy

* fixup! Conform static const variable name to `UPPER_CASE`
…en velocity (moveit#684)

* New isValidVelocityMove() for checking time between two waypoints given velocity

* Move isValidVelocityMove() to JointModelGroup

* Update moveit_core/robot_model/src/joint_model_group.cpp
…oveGroup::plan() (moveit#790)

* MoveGroupCommander.plan() now returns [success, trajectory, planning_time, error_code]
@vmayoral
Copy link
Contributor

Thanks @rhaschke, I can look into this but before doing so, can we please address #36 (comment). It'll end up being more work for us otherwise.

Copy link
Member

@davetcoleman davetcoleman left a comment

Choose a reason for hiding this comment

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

Merging now as everything in this PR has already been reviewed extensively in the moveit1 repo.

@davetcoleman
Copy link
Member

@vmayoral please open PR for your changes requested in #36

@davetcoleman davetcoleman merged commit 40cf17d into moveit:master Mar 19, 2019
@vmayoral
Copy link
Contributor

Merging now as everything in this PR has already been reviewed extensively in the moveit1 repo.

IMHO this is a bad policy and we should reconsider and set up some guidelines for syncing. The fact that they were reviewed for ROS1 doesn't mean we should accept it right away. As seen in previous PRs that were ROS 2-related, these changes won't even compile right away and will cause issues with the CI (see #52 for the last sync we did).

Note this has caused conflicts at #10 or #41 to name a few. Solving them will take time time.

@rhaschke
Copy link
Contributor Author

I don't exactly understand, what you are complaining about, @vmayoral. Of course, we should carefully review these sync PRs and - as long as we don't yet have a working CI - one should compile them locally for approval. I hope that @davetcoleman has done this before merging. I don't yet have a ROS2 infrastructure, so I cannot help on this. I stated that fact several times.

As seen in previous PRs that were ROS 2-related, these changes won't even compile right away.

That's a pity indeed. That's why a top-priority should be to get Travis CI working again.

Note this has caused conflicts at #10 or #41 to name a few. Solving them will take time.

If there is heavy parallel development, it is usually unavoidable that we will face merge conflicts. This cannot be used as an argument to not merge certain PRs.

@vmayoral
Copy link
Contributor

That's a pity indeed. That's why a top-priority should be to get Travis CI working again.

Well @rhaschke, that's my complaint. Syncing shouldn't go forward before looking at these commits first from a ROS 2 perspective. Accepted PRs should go through some sort of alignment with the ongoing development (in ROS 2). Otherwise it will break master and slow us down. We should go through some sort of CI mechanism before accepting PRs, otherwise our own development (and all related PRs) get compromised over and over. I had to rebase and solve conflicts in 8+ PRs the last time we accepted a sync to make it compile.

We indeed need CI. We enabled a simple CI in our own at our fork a couple of weeks ago and submitted a PR to "enable it". #49 could somehow still do but I understand that something much better is being cooked by @mlautman and the guys at PickNik (certainly desirable).

I don't yet have a ROS2 infrastructure, so I cannot help on this. I stated that fact several times.

Let me know if there's something we can do to help here.

vmayoral added a commit to AcutronicRobotics/moveit2 that referenced this pull request Mar 24, 2019
vmayoral added a commit to AcutronicRobotics/moveit2 that referenced this pull request Mar 24, 2019
JafarAbdi pushed a commit to JafarAbdi/moveit2 that referenced this pull request Oct 28, 2019
…nterface_objects

Port Common_planning_interface_objects to ROS2
MikeWrock pushed a commit to MikeWrock/moveit2 that referenced this pull request Aug 15, 2022
MikeWrock pushed a commit to MikeWrock/moveit2 that referenced this pull request Aug 15, 2022
MikeWrock pushed a commit to MikeWrock/moveit2 that referenced this pull request Aug 15, 2022
* updated cmake to colcon/ament

* file cleanup and commented out unconfigured build_depend in package.xml

* more cleanup

* removed subdirectory moveit_cpp

* Switch to travis-ci.com in readme links

* fix to travis status in readme

* Update conf.py to generate CNAME

* updated to meta.keys() (moveit#45)

* fix travis for custom domain (moveit#47)

* updated cmake with moveit_package, removed empty cmake, added vcs file

* package.xml cleanup

* updated readme with source build instructions

* updated travis to foxy

* updated repos to include moveit2.repos

* fixed typo

* ament format

* more ament formatting

* removed code coverage ci test added moveit.repos

* updated cmake to colcon/ament

* file cleanup and commented out unconfigured build_depend in package.xml

* more cleanup

* removed subdirectory moveit_cpp

* updated cmake with moveit_package, removed empty cmake, added vcs file

* package.xml cleanup

* updated readme with source build instructions

* updated travis to foxy

* updated repos to include moveit2.repos

* fixed typo

* ament format

* more ament formatting

* removed code coverage ci test added moveit.repos

* fixed travis and repose files, rebased on main

* re-added fix travis for custom domain

Co-authored-by: Alex Goldman <[email protected]>
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.

7 participants