Added Effort plugin #990

ahcorde · 2023-06-07T13:13:54Z

Added Effort plugin

Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>

clalancette

First pass review.

rviz_default_plugins/include/rviz_default_plugins/displays/effort/effort_display.hpp

rviz_default_plugins/plugins_description.xml

rviz_default_plugins/src/rviz_default_plugins/displays/effort/effort_display.cpp

rviz_rendering/src/rviz_rendering/objects/effort_visual.cpp

rviz_rendering/test/rviz_rendering/objects/effort_visual_test.cpp

Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>

clalancette

I left a few more comments; these all should be pretty easy to fix. Once that is done I'm happy with this.

clalancette · 2023-06-14T16:33:41Z

rviz_default_plugins/src/rviz_default_plugins/displays/effort/effort_display.cpp

+    if (joint->type == urdf::Joint::REVOLUTE || joint->type == 2) {
+      std::string joint_name = it->first;
+      urdf::JointLimitsSharedPtr limit = joint->limits;
+      joints_[joint_name] = createJoint(joint_name);


At this point, createJoint is a one-liner, so I'll suggest we just inline it here:

Suggested change

joints_[joint_name] = createJoint(joint_name);

joints_[joint_name] = new JointInfo(joint_name, joints_category_);

and remove the the createJoint method.

clalancette · 2023-06-14T16:35:35Z

rviz_default_plugins/include/rviz_default_plugins/displays/effort/effort_display.hpp

+  // data gets popped from the front (oldest) and pushed to the back (newest)
+  std::deque<std::shared_ptr<rviz_rendering::EffortVisual>> visuals_;
+
+  typedef std::map<std::string, JointInfo *> M_JointInfo;


I just had a thought about this. If we instead made this:

Suggested change

typedef std::map<std::string, JointInfo *> M_JointInfo;

typedef std::map<std::string, std::unique_ptr<JointInfo>> M_JointInfo;

Then we wouldn't need to explicitly free it in the destructor. We'd also have to change where we allocate it to std::make_unique, but I think that is probably worth it.

clalancette · 2023-06-14T16:36:07Z

rviz_default_plugins/src/rviz_default_plugins/displays/effort/effort_display.cpp

+  robot_description_ = msg.data;
+  robot_model_ = std::shared_ptr<urdf::Model>(new urdf::Model());
+  if (!robot_model_->initString(robot_description_)) {
+    // ROS_ERROR("Unable to parse URDF description!");


Remove this commented-out code.

clalancette · 2023-06-14T16:36:30Z

rviz_default_plugins/src/rviz_default_plugins/displays/effort/effort_display.cpp

+
+void EffortDisplay::processMessage(sensor_msgs::msg::JointState::ConstSharedPtr msg)
+{
+  // Robot model might not be loaded already


Suggested change

// Robot model might not be loaded already

// Robot model might not be loaded yet

clalancette · 2023-06-14T16:37:13Z

rviz_default_plugins/src/rviz_default_plugins/displays/effort/effort_display.cpp

+{
+  // Robot model might not be loaded already
+  if (!robot_model_) {
+    std::cerr << "Robot model might not be loaded yet" << '\n';


My previous comment about this still seems to be open:

"an unconditional print here seems problematic. It seems like 1) the user might miss this on the console, and 2) we might spam the user early on with a lot of these messages. Could we instead update the Status field somehow?"

(though maybe this isn't possible)

clalancette · 2023-06-14T16:38:18Z

rviz_default_plugins/src/rviz_default_plugins/displays/effort/effort_display.cpp

+  if (visuals_.size() == static_cast<size_t>(history_length_property_->getInt())) {
+    visual = visuals_.front();
+  } else {
+    visual.reset(new rviz_rendering::EffortVisual(context_->getSceneManager(), scene_node_));


I think std::make_shared is generally preferred for memory layout and other reasons:

Suggested change

visual.reset(new rviz_rendering::EffortVisual(context_->getSceneManager(), scene_node_));

visual = std::make_shared<rviz_rendering::EffortVisual>(context_->getSceneManager(), scene_node_);

Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>

ahcorde · 2023-06-19T09:04:59Z

Linux
Linux-aarch64
Windows

Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>

ahcorde · 2023-06-19T11:17:13Z

Windows

Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>

ahcorde · 2023-06-20T09:25:14Z

Linux
Linux-aarch64
Windows

clalancette · 2023-06-20T13:51:06Z

So the code here is looking good to me.

The thing I'm wondering about is whether we should actually add this to the default set of RViz plugins. That is, we already have a large set, and it already takes quite a long time to compile this package. Is this plugin important/universal enough that we should include it in the default set, or can we distribute it in a separate package instead?

ahcorde · 2023-06-21T10:10:08Z

So the code here is looking good to me.

The thing I'm wondering about is whether we should actually add this to the default set of RViz plugins. That is, we already have a large set, and it already takes quite a long time to compile this package. Is this plugin important/universal enough that we should include it in the default set, or can we distribute it in a separate package instead?

@clalancette I can add here two arguments:

Out of the box experience: When people are trying to migrate from ROS 1 to ROS 2, they will expect to have the same behaviour without any additional step.
Feature parity: With these 3 open PRs we will reach feature parity (finally).

ahcorde · 2023-06-26T14:46:51Z

friendly ping @clalancette

clalancette · 2023-06-26T19:27:24Z

So here is the issue that I'm struggling with this.

First, we've gone many years without these plugins, and haven't had a lot of call for them. That doesn't mean there has been zero call for them, or that they don't have value, but these PRs are the first time I've heard of them requested. That makes me think that they aren't necessarily widely used, and hence could be in a separate package. That would also make it so that these plugins aren't release-critical, and thus keep from adding additional burden on the core maintainers.

Second, our build and test times are already absurd. Here are some numbers:

On Linux, it currently takes us 2 hours and 26 minutes to run a nightly full build and test. Of that, 59 minutes are spent compiling. And of that, the largest compile time is for rviz_default_plugins at 11 minutes.
On Windows, the situation is far more dire. It currently takes us 5 hours and 44 minutes to run a nightly full build and test. Of that, 2 hours 45 minutes are spent compiling. And of that, the largest compile time is for rviz_default_plugins at 32 minutes.

This combination of factors makes me think that these plugins should be released into a separate package for manipulation. Maybe rviz_manipulation_plugins or something like that. It could then be released into humble, iron, and rolling, and be available as normal.

All of that said, I could be convinced otherwise. I'd also like to hear from @ros2/team here for other opinions.

sloretz · 2023-06-26T20:53:54Z

[...] And of that, the largest compile time is for rviz_default_plugins at 32 minutes.

I agree rviz_default_plugins is really slow to build. By how much does adding this plugin increase the compile time? If it increases it significantly then I'd see that as justification to put it in a different package.

ahcorde · 2023-06-29T10:19:42Z

I created this branch ahcorde/rolling/rviz_default_plugins_parity which includes the remaining rviz default plugins to get the feature parity. Let's see how much increase the compilation time.

Windows

ahcorde · 2023-06-30T10:33:01Z

@clalancette in this last build the time to compile rviz_default_plugins with these three new plugins is

Finished <<< rviz_default_plugins [22min 55s]

What do you think? can we move forward ?

clalancette · 2023-07-05T19:35:06Z

What do you think? can we move forward ?

Looking at the build time in your example branch, it is actually substantially less than the current nightly CI times (22 minutes for your CI vs 32 minutes for nightly CI, and they were running on the same AWS instance). That seems counter-intuitive; we are adding code, so it should be at least the same amount of time, if not longer. Do you have any idea why that might be?

codebot · 2023-07-05T19:37:01Z

FWIW, I think it would be great to have this plugin included in the default set. It's very useful for sanity-checking things when debugging.

ahcorde · 2023-07-05T20:52:40Z

What do you think? can we move forward ?

Looking at the build time in your example branch, it is actually substantially less than the current nightly CI times (22 minutes for your CI vs 32 minutes for nightly CI, and they were running on the same AWS instance). That seems counter-intuitive; we are adding code, so it should be at least the same amount of time, if not longer. Do you have any idea why that might be?

I don´t have a answer for this, but maybe @j-rivero or @nuclearsandwich have one.

ahcorde · 2023-07-07T09:00:29Z

I was discussing this with @j-rivero offline, and these are his thoughts:

we need to check if both build are running with the same paremeters
changes in the CMakeLists.txt seems trivial, he can not explain any reduction in time.
There are failing test in CI but I think that are all related to linting. If there is a test failure in something time-consuming that could explain why the CI is running in less time but does not seem to be the case.

Last comment is not relevant because we are just comparing compilation times. I will check that the parameters are the same.

ahcorde · 2023-07-07T09:04:57Z

The only difference is the CI_CMAKE_BUILD_TYPE which is setted to Release. Launching again the job

Windows

nuclearsandwich · 2023-07-07T16:12:07Z

I don´t have a answer for this, but maybe @j-rivero or @nuclearsandwich have one.

One thing that jumps out at me is the scoped build argument in the CI job

 build_args: --event-handlers console_cohesion+ console_package_list+ --cmake-args -DINSTALL_EXAMPLES=OFF -DSECURITY=ON -DAPPEND_PROJECT_NAME_TO_INCLUDEDIR=ON --packages-above-and-dependencies rviz_default_plugins rviz_rendering rviz_common,

With fewer packages being built there's probably more capacity for the rviz_default_plugins build. To get a true apples-to-apples comparison you would need to build the same set of packages as we do in the nightlies.

…t_plugin

ahcorde · 2023-07-10T12:34:55Z

Linux
Linux-aarch64
Windows

clalancette

I've left two small things to fix, then I think I'm happy with this.

rviz_default_plugins/CMakeLists.txt

rviz_default_plugins/test/rviz_default_plugins/publishers/effort_publisher.hpp

Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>

ahcorde · 2023-07-11T07:35:17Z

Linux
Linux-aarch64
Windows

ahcorde · 2023-07-13T13:32:42Z

https://github.com/Mergifyio backport humble iron

mergify · 2023-07-13T13:32:49Z

backport humble iron

✅ Backports have been created

#1010 Added Effort plugin (backport #990) has been created for branch humble
#1011 Added Effort plugin (backport #990) has been created for branch iron

* Added Effort plugin Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com> (cherry picked from commit e3b56ed)

* Added Effort plugin Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com> (cherry picked from commit e3b56ed) Co-authored-by: Alejandro Hernández Cordero <alejandro@openrobotics.org>

Added Effort plugin

5c45447

Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>

ahcorde requested review from sloretz and adityapande-1995 June 7, 2023 13:13

ahcorde self-assigned this Jun 7, 2023

clalancette requested changes Jun 7, 2023

View reviewed changes

Added feedback

7571bb4

Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>

ahcorde requested a review from clalancette June 14, 2023 15:22

clalancette requested changes Jun 14, 2023

View reviewed changes

Added feedback

fa366ff

Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>

ahcorde requested a review from clalancette June 15, 2023 22:58

update readme

7615bbb

Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>

Fixed windows failured and warnnings

720cf07

Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>

ahcorde added 3 commits June 19, 2023 15:31

Fixed M_PI

0b70e8e

Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>

Fixed windows visibility

d72b08a

Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>

Fixed warnings

ec25e56

Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>

Merge remote-tracking branch 'origin/ros2' into ahcorde/rolling/effor…

262249e

…t_plugin

clalancette requested changes Jul 10, 2023

View reviewed changes

rviz_default_plugins/CMakeLists.txt Outdated Show resolved Hide resolved

rviz_default_plugins/test/rviz_default_plugins/publishers/effort_publisher.hpp Outdated Show resolved Hide resolved

Added feedback

55b8b1d

Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>

ahcorde requested a review from clalancette July 11, 2023 07:35

clalancette approved these changes Jul 11, 2023

View reviewed changes

clalancette merged commit e3b56ed into rolling Jul 11, 2023

delete-merged-branch bot deleted the ahcorde/rolling/effort_plugin branch July 11, 2023 12:45

mergify bot mentioned this pull request Jul 13, 2023

Added Effort plugin (backport #990) #1010

Merged

mergify bot pushed a commit that referenced this pull request Jul 13, 2023

Added Effort plugin (#990)

edf0628

* Added Effort plugin Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com> (cherry picked from commit e3b56ed)

mergify bot pushed a commit that referenced this pull request Jul 13, 2023

Added Effort plugin (#990)

8b23505

* Added Effort plugin Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com> (cherry picked from commit e3b56ed)

mergify bot mentioned this pull request Jul 13, 2023

Added Effort plugin (backport #990) #1011

Merged

clalancette mentioned this pull request Jul 14, 2023

Migrate EffortDisplay #95

Closed

roncapat mentioned this pull request Nov 6, 2023

Effort display seg fault when no effort limit is specified in URDF #1083

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added Effort plugin #990

Added Effort plugin #990

ahcorde commented Jun 7, 2023

clalancette left a comment

clalancette left a comment

clalancette Jun 14, 2023

clalancette Jun 14, 2023

clalancette Jun 14, 2023

clalancette Jun 14, 2023

clalancette Jun 14, 2023

clalancette Jun 14, 2023

ahcorde commented Jun 19, 2023

ahcorde commented Jun 19, 2023 •

edited

Loading

ahcorde commented Jun 20, 2023

clalancette commented Jun 20, 2023

ahcorde commented Jun 21, 2023

ahcorde commented Jun 26, 2023

clalancette commented Jun 26, 2023

sloretz commented Jun 26, 2023

ahcorde commented Jun 29, 2023 •

edited

Loading

ahcorde commented Jun 30, 2023

clalancette commented Jul 5, 2023

codebot commented Jul 5, 2023

ahcorde commented Jul 5, 2023

ahcorde commented Jul 7, 2023

ahcorde commented Jul 7, 2023

nuclearsandwich commented Jul 7, 2023

ahcorde commented Jul 10, 2023 •

edited by clalancette

Loading

clalancette left a comment

ahcorde commented Jul 11, 2023

ahcorde commented Jul 13, 2023

mergify bot commented Jul 13, 2023 •

edited

Loading

	joints_[joint_name] = createJoint(joint_name);
	joints_[joint_name] = new JointInfo(joint_name, joints_category_);

	typedef std::map<std::string, JointInfo *> M_JointInfo;
	typedef std::map<std::string, std::unique_ptr<JointInfo>> M_JointInfo;

	// Robot model might not be loaded already
	// Robot model might not be loaded yet

	visual.reset(new rviz_rendering::EffortVisual(context_->getSceneManager(), scene_node_));
	visual = std::make_shared<rviz_rendering::EffortVisual>(context_->getSceneManager(), scene_node_);

Added Effort plugin #990

Added Effort plugin #990

Conversation

ahcorde commented Jun 7, 2023

clalancette left a comment

Choose a reason for hiding this comment

clalancette left a comment

Choose a reason for hiding this comment

clalancette Jun 14, 2023

Choose a reason for hiding this comment

clalancette Jun 14, 2023

Choose a reason for hiding this comment

clalancette Jun 14, 2023

Choose a reason for hiding this comment

clalancette Jun 14, 2023

Choose a reason for hiding this comment

clalancette Jun 14, 2023

Choose a reason for hiding this comment

clalancette Jun 14, 2023

Choose a reason for hiding this comment

ahcorde commented Jun 19, 2023

ahcorde commented Jun 19, 2023 • edited Loading

ahcorde commented Jun 20, 2023

clalancette commented Jun 20, 2023

ahcorde commented Jun 21, 2023

ahcorde commented Jun 26, 2023

clalancette commented Jun 26, 2023

sloretz commented Jun 26, 2023

ahcorde commented Jun 29, 2023 • edited Loading

ahcorde commented Jun 30, 2023

clalancette commented Jul 5, 2023

codebot commented Jul 5, 2023

ahcorde commented Jul 5, 2023

ahcorde commented Jul 7, 2023

ahcorde commented Jul 7, 2023

nuclearsandwich commented Jul 7, 2023

ahcorde commented Jul 10, 2023 • edited by clalancette Loading

clalancette left a comment

Choose a reason for hiding this comment

ahcorde commented Jul 11, 2023

ahcorde commented Jul 13, 2023

mergify bot commented Jul 13, 2023 • edited Loading

✅ Backports have been created

ahcorde commented Jun 19, 2023 •

edited

Loading

ahcorde commented Jun 29, 2023 •

edited

Loading

ahcorde commented Jul 10, 2023 •

edited by clalancette

Loading

mergify bot commented Jul 13, 2023 •

edited

Loading