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

Port kinematics_base submodule moveit_core #8

Merged
merged 21 commits into from
Apr 22, 2019

Conversation

vmayoral
Copy link
Contributor

No description provided.

vmayoral added a commit to AcutronicRobotics/moveit2 that referenced this pull request Feb 24, 2019
vmayoral added a commit to AcutronicRobotics/moveit2 that referenced this pull request Feb 24, 2019
@vmayoral
Copy link
Contributor Author

@mlautman and @davetcoleman please have a look again at this PR.

I've opted for not using LOGGER for now since as is, it'll need to be defined in the header causing conflicts with other submodules. Alternatively, we can change a bit the distribution of the code and push the implementation of searchPositionIK and other functions (now implemented in the header file) to the cpp file. Let me know if this approach is acceptable and I'll make the changes.

vmayoral added a commit to AcutronicRobotics/moveit2 that referenced this pull request Mar 16, 2019
Follows from moveit#8 (comment)
taking into account that recommendations do currently fail
to compile given the structure of RCLCPP macros
Follows from moveit#8 (comment)
taking into account that recommendations do currently fail
to compile given the structure of RCLCPP macros
@vmayoral
Copy link
Contributor Author

@davetcoleman and @mlautman, consider again this PR

if (pnh.hasParam(param))
auto parameters_lookup = std::make_shared<rclcpp::SyncParametersClient>(node);

auto groupname_param = parameters_lookup->get_parameters({ group_name_ + "/" + param });
Copy link
Member

Choose a reason for hiding this comment

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

auto is nice where the type is obvious but in cases like this it's more confusing then helpful.
Especially since the following for loops are using auto as well this type should be made explicit.

Copy link
Member

Choose a reason for hiding this comment

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

Also, the SyncParametersClient class contains the functions has_param() and get_parameter() which can be used to replace hasParam() and param() directly. This way we don't have to loop and compare the results as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Partially complied with your request @henningkayser at b2d436f.

Please make a suggestion if you require further changes.

Copy link
Member

Choose a reason for hiding this comment

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

get_parameters() is meant to be used for multiple parameters but here it's always called with only a single entry. In this case it's much more straightforward to query every parameter like in the sample code below:

    bool param_success = node.get_parameter<T>(group_name + "/" + param, val);

No need to create a parameter vector or loop over the results and also we can use the node directly and don't need to cast it to rclcpp::SyncParametersClient.

Since lookupParam() does in fact query multiple parameters in different locations we can absolutely use Node::get_parameters() but in that case including all parameter names:

val = default_val;
std::vector<std::string> param_names = {
  group_name_ + "/" + param, param,
  "robot_description_kinematics/" + group_name_ + "/" + param,
  robot_description_kinematics/" + param
};
std::vector<rclcpp::parameter::ParameterVariant> param_results =
  node.getParameters(param_names);
bool param_success = !param_results.empty();
if (param_success)
    val = param_results[0].get_value<T>();
return param_success;

I actually prefer the second implementation.
Please also note that your current implementation calls value_to_string() on all parameters but lookupParam() is a template function that supports all parameter types. The get_value() function takes care of using the correct type.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

@anasarrak this looks much better. However, each if/else block can be implemented in a single line using node->get_parameter_or(param, val, default_val).

moveit_core/kinematics_base/src/kinematics_base.cpp Outdated Show resolved Hide resolved
moveit_core/kinematics_base/src/kinematics_base.cpp Outdated Show resolved Hide resolved
moveit_core/kinematics_base/src/kinematics_base.cpp Outdated Show resolved Hide resolved
moveit_core/kinematics_base/src/kinematics_base.cpp Outdated Show resolved Hide resolved
Copy link
Member

@henningkayser henningkayser left a comment

Choose a reason for hiding this comment

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

Please advice if we're good to go here

To me it looks like #8 (comment) wasn't really addressed. The new implementation of lookupParam() is overly complicated and should be implemented following either one of the approaches discussed in the comments.

@henningkayser
Copy link
Member

@vmayoral I took the freedom and reimplemented the lookupParam() function as I requested and also resolved the merge conflicts. I think this is ready to be merged now.

{
val = pnh.param(param, default_val);
val = param_results.get_value<T>();
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a vector. I think you need to get the parameter at first index right? @henningkayser

Copy link
Contributor

Choose a reason for hiding this comment

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

nit picky but I think this should be the order:

 { 
    param,
    group_name_ + "/" + param,  
    "robot_description_kinematics/" + param,
    "robot_description_kinematics/" + group_name_ + "/" + param
}

Copy link
Member

Choose a reason for hiding this comment

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

This is a vector. I think you need to get the parameter at first index right? @henningkayser

@mlautman You're absolutely right. Fixed!

nit picky but I think this should be the order:

 { 
    param,
    group_name_ + "/" + param,  
    "robot_description_kinematics/" + param,
    "robot_description_kinematics/" + group_name_ + "/" + param
}

I kept the old order and don't think we should change it.
After all the specification (with group name) should override the default (without group name), no?

Copy link
Contributor

Choose a reason for hiding this comment

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

fair enough. My nit was aesthetic

@mlautman mlautman dismissed stale reviews from henningkayser and v4hn April 22, 2019 17:45

stale

@mlautman
Copy link
Contributor

@henningkayser lgtm

@henningkayser henningkayser merged commit f5483c5 into moveit:master Apr 22, 2019
AndyZe pushed a commit to AndyZe/moveit2 that referenced this pull request Aug 14, 2021
* Register ruckig smoothing as a plugin via xml. Fix bad constructor in AddRuckigTrajectorySmoothing

* Add add_ruckig_traj_smoothing.cpp to CMakeLists.txt
AndyZe pushed a commit to AndyZe/moveit2 that referenced this pull request Aug 21, 2021
* Register ruckig smoothing as a plugin via xml. Fix bad constructor in AddRuckigTrajectorySmoothing

* Add add_ruckig_traj_smoothing.cpp to CMakeLists.txt
AndyZe pushed a commit to AndyZe/moveit2 that referenced this pull request Sep 15, 2021
* Register ruckig smoothing as a plugin via xml. Fix bad constructor in AddRuckigTrajectorySmoothing

* Add add_ruckig_traj_smoothing.cpp to CMakeLists.txt
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.

8 participants