Add and cleanup RobotState doxygen documentation #1846

davetcoleman · 2020-01-21T14:40:18Z

@jliukkonen asked the difference between getGlobalLinkTransform() and getFrameTransform() so I cleaned up some stuff while verifying my recollection of the difference was correct.

moveit_core/robot_state/include/moveit/robot_state/robot_state.h

jliukkonen

Thank you for clarifying the terminology and comments. This at least helped me understand what the functions do / try to achieve.

henningkayser · 2020-01-21T15:37:15Z

This seems to be overlapping with #1843

davetcoleman · 2020-01-21T15:58:58Z

Accidentally replaces https://github.com/ros-planning/moveit/pull/1843/files

my bad @AndyZe - I've integrated your changes!

davetcoleman · 2020-01-21T16:02:54Z

Please re-review, due to lots of changes

rhaschke

+1 if the requested changes are made.

moveit_core/robot_state/include/moveit/robot_state/robot_state.h

rhaschke · 2020-01-21T16:20:08Z

moveit_core/robot_state/include/moveit/robot_state/robot_state.h

@@ -1311,27 +1298,39 @@ as the new values that correspond to the group */
  /** \brief Update the state after setting a particular link to the input global transform pose.*/
  void updateStateWithLinkAt(const LinkModel* link, const Eigen::Isometry3d& transform, bool backward = false);

-  const Eigen::Isometry3d& getGlobalLinkTransform(const std::string& link_name)
-  {
-    return getGlobalLinkTransform(robot_model_->getLinkModel(link_name));


Why did you removed the inlining for these 1-liner functions? This (slightly) degrades performance.

The optimizer decided what gets inlined, modern compilers do not care if its in the header or not (unless its a template)

Perhaps insert inline to force inlining?

inline isn't needed for modern compilers, either. they will choose on their own what to inline or not.

The compiler cannot inline this function if it's not declared in the header. How should another compilation unit know what to inline? See http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines.html#Rf-inline

i've added inline to header functions.

mamoll · 2020-01-21T18:45:56Z

moveit_core/robot_state/include/moveit/robot_state/robot_state.h

@@ -1311,27 +1298,39 @@ as the new values that correspond to the group */
  /** \brief Update the state after setting a particular link to the input global transform pose.*/
  void updateStateWithLinkAt(const LinkModel* link, const Eigen::Isometry3d& transform, bool backward = false);

-  const Eigen::Isometry3d& getGlobalLinkTransform(const std::string& link_name)
-  {
-    return getGlobalLinkTransform(robot_model_->getLinkModel(link_name));


Perhaps insert inline to force inlining?

mamoll · 2020-01-21T19:15:14Z

moveit_core/robot_state/include/moveit/robot_state/robot_state.h

+   *   but also other frame sources (attached objects, subframes).
+   *
+   *   As opposed to the visual links in |getGlobalLinkTransform|, this function returns
+   *   the collision link transform used for collision checking.


This helps explain when to use which one. I find this distinction still a bit confusing. Is this the difference:

global transform for link: transform for given link in root coordinate frame of robot model

collision body transform: transform for given collision body in given link's frame

Your high-level description of the transforms returned by the two families methods is the same. I'd reword them a bit to clarify.

They are worded the same because they behave the same. I don't want to spend any more time on this documentation that no one else has bothered to do in the last 8 years. I need to get actual work done.

codecov-io · 2020-01-21T20:00:09Z

Codecov Report

Merging #1846 into master will decrease coverage by 0.16%.
The diff coverage is 80%.

@@            Coverage Diff            @@
##           master   #1846      +/-   ##
=========================================
- Coverage   48.46%   48.3%   -0.17%     
=========================================
  Files         298     299       +1     
  Lines       23305   23401      +96     
=========================================
+ Hits        11295   11303       +8     
- Misses      12010   12098      +88

Impacted Files	Coverage Δ
...ros/planning/plan_execution/src/plan_execution.cpp	`12.91% <0%> (ø)`	⬆️
moveit_core/robot_state/src/robot_state.cpp	`44.02% <100%> (ø)`	⬆️
...ler_manager/src/moveit_fake_controller_manager.cpp	`64.21% <100%> (+0.76%)`	⬆️
...bot_state/include/moveit/robot_state/robot_state.h	`90.9% <75%> (ø)`	⬆️
...veit_experimental/moveit_jog_arm/src/jog_calcs.cpp	`62.37% <0%> (-4.18%)`	⬇️
moveit_core/robot_model/src/joint_model.cpp	`60.95% <0%> (-3.81%)`	⬇️
...ccupancy_map_monitor/src/occupancy_map_monitor.cpp	`29.65% <0%> (-0.69%)`	⬇️
...nning_scene_monitor/src/planning_scene_monitor.cpp	`56.03% <0%> (-0.62%)`	⬇️
...rimental/moveit_jog_arm/src/jog_interface_base.cpp	`83.33% <0%> (-0.19%)`	⬇️
...veit_jog_arm/include/moveit_jog_arm/jog_arm_data.h	`100% <0%> (ø)`	⬆️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 546d6e0...87adfa5. Read the comment docs.

davetcoleman · 2020-01-21T22:33:28Z

This is ready to be merged.

rhaschke · 2020-01-22T11:21:12Z

This is ready to be merged.

I don't agree. I would like to insist on keeping the inlining.
You are right that the compilers automatically decide when to inline and when not - even with an inline statement provided. But they can only do so if the function definition is accessible in the header.

- ExecutableTrajectory has a new member controller_names_ - PlanExecution includes that list when pushing trajctory parts to the TrajectoryExecutionManager

When reading controller configurations, "default" is ignored as opposed to what is currently written in the documentation. MoveIt{Fake|Simple}ControllerManager now maintains map of controller name and ControllerState where "default" is initialized as "false" or whatever is given in the configuration. MoveIt{Fake|Simple}ControllerManager::getControllerState() now returns the state that is held in that map instead of a default one.

davetcoleman · 2020-01-22T16:14:30Z

Okay, I've restored all the functions to be inline in .h file (without inline tag though, which is how it was originally).

davetcoleman requested review from rhaschke and jliukkonen January 21, 2020 14:40

davetcoleman requested a review from mlautman as a code owner January 21, 2020 14:40

davetcoleman force-pushed the document_robot_state branch 2 times, most recently from b4be3d6 to d9312bd Compare January 21, 2020 15:10

mamoll reviewed Jan 21, 2020

View reviewed changes

moveit_core/robot_state/include/moveit/robot_state/robot_state.h Outdated Show resolved Hide resolved

moveit_core/robot_state/include/moveit/robot_state/robot_state.h Outdated Show resolved Hide resolved

jliukkonen approved these changes Jan 21, 2020

View reviewed changes

AndyZe mentioned this pull request Jan 21, 2020

Add a comment for getGlobalLinkTransform #1843

Closed

davetcoleman mentioned this pull request Jan 21, 2020

Fix command frame transform computation #1842

Merged

davetcoleman force-pushed the document_robot_state branch from d9312bd to 368a561 Compare January 21, 2020 16:02

rhaschke requested changes Jan 21, 2020

View reviewed changes

mamoll requested changes Jan 21, 2020

View reviewed changes

llach and others added 3 commits January 22, 2020 11:13

forward controller names to TrajectoryExecutionManager

efc36e0

- ExecutableTrajectory has a new member controller_names_ - PlanExecution includes that list when pushing trajctory parts to the TrajectoryExecutionManager

Add and cleanup RobotState doxygen documentation

9cfd27a

davetcoleman force-pushed the document_robot_state branch from 8c08a4b to 9cfd27a Compare January 22, 2020 16:13

davetcoleman requested review from mikeferguson and v4hn as code owners January 22, 2020 16:13

rhaschke added 2 commits January 22, 2020 20:45

Fix wording in modified comments

975800b

fix typos

87adfa5

rhaschke approved these changes Jan 22, 2020

View reviewed changes

rhaschke merged commit 38fa392 into moveit:master Jan 22, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add and cleanup RobotState doxygen documentation #1846

Add and cleanup RobotState doxygen documentation #1846

davetcoleman commented Jan 21, 2020

jliukkonen left a comment

henningkayser commented Jan 21, 2020

davetcoleman commented Jan 21, 2020

davetcoleman commented Jan 21, 2020

rhaschke left a comment

rhaschke Jan 21, 2020

davetcoleman Jan 21, 2020

mamoll Jan 21, 2020

davetcoleman Jan 21, 2020

rhaschke Jan 22, 2020

davetcoleman Jan 22, 2020

mamoll Jan 21, 2020

mamoll Jan 21, 2020

davetcoleman Jan 21, 2020

codecov-io commented Jan 21, 2020 •

edited

Loading

davetcoleman commented Jan 21, 2020

rhaschke commented Jan 22, 2020

davetcoleman commented Jan 22, 2020

Add and cleanup RobotState doxygen documentation #1846

Add and cleanup RobotState doxygen documentation #1846

Conversation

davetcoleman commented Jan 21, 2020

jliukkonen left a comment

Choose a reason for hiding this comment

henningkayser commented Jan 21, 2020

davetcoleman commented Jan 21, 2020

davetcoleman commented Jan 21, 2020

rhaschke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jan 21, 2020 • edited Loading

Codecov Report

davetcoleman commented Jan 21, 2020

rhaschke commented Jan 22, 2020

davetcoleman commented Jan 22, 2020

codecov-io commented Jan 21, 2020 •

edited

Loading