-
Notifications
You must be signed in to change notification settings - Fork 557
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
Port moveit_core/utils to ROS 2 #68
Conversation
See #62 (comment) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes look good! I would only vote to remove xmlrpc_casts.cpp
completely as explained below.
moveit_core/utils/CMakeLists.txt
Outdated
@@ -2,34 +2,37 @@ set(MOVEIT_LIB_NAME moveit_utils) | |||
|
|||
add_library(${MOVEIT_LIB_NAME} | |||
src/lexical_casts.cpp | |||
src/xmlrpc_casts.cpp | |||
# src/xmlrpc_casts.cpp # TODO: solve the conflicts with xmlrpc dependencies |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think xmlrpc_casts.cpp
will not be used anymore since XML-RPC is not supported in ROS 2.
parseDouble()
and isArray()
should be redundant in the future but we have to check whereisStruct()
is used and if we need to reimplement it somehow.
Let's remove xmlrpc_casts.cpp
and add TODOs everywhere where it is used.
These parts will need to be changed anyway since the same missing XML-RPC dependencies are used.
…ML-RPC is not supported in ROS 2
@henningkayser can you review it? |
@henningkayser friendly ping |
@ahcorde removing these files won't be enough since the functionality needs to be reimplemented eventually. Those files that
Please fix related includes and add a descriptions and TODOs for all function calls that need to be replaced somehow. We should keep a list of all those references, either in a new issue or maybe inside the CMakeLists.txt of the utils package. Ideally we would replace the implementation right now but I believe that we still need to have some further discussions on how we handle parameter config files. |
we decided to remove We have replaced all these calls in the code with the corresponding new ROS 2.0 code. I think it's better to discuss if we can include new abstractions for getting parameters than maintaining this old ROS code. |
Yes, that's why I suggested to remove
I wasn't aware that this was already implemented. Is it still under review? If yes I would suggest adding related descriptions and would +1 this PR |
You can open this (huge) PR and have a look to moveit_planners/ompl/ompl_interface/src/ompl_interface.cpp. You will see the changes. Most of these changes are still pending of been ported. But You will find a TODO (anasarrak) or (ahcorde). The parameters which were ported are the most relevant. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One small question. +1 after the comment has been resolved
Initial iteration was made on a different "master" status