ros-industrial / ros_industrial_issues

Repository for tracking common ROS-Industrial issuses.
3 stars 4 forks source link

Proposal: remove rviz run dependency from robot support packages #50

Open gavanderhoorn opened 6 years ago

gavanderhoorn commented 6 years ago

rviz is currently a run_depend (or exec_depend) of many robot support packages (here for fanuc_m10ia_support fi). This is because of the robot_state_visualize and test launch files which start RViz to show the current robot state after having started the appropriate robot drivers.

While this is a nice convenience, it makes it impossible / harder to deploy the robot support packages onto resource constraint systems: installing RViz in the base ros:indigo Docker image fi adds over 150MB of additional packages. On more barren installs / images (ubuntu:trusty fi) this pulls in multiples of that).

fanuc_m10ia_support itself is only 1MB, and merely provides urdfs, meshes and some convenience launch files.

My proposal is to remove the explicit dependency on rviz and by doing so reduce the footprint of robot support packages significantly.

gavanderhoorn commented 6 years ago

Note that we can keep the robot_state_visualize and test launch files. Not having the dependency on RViz will mean that those cannot be used until it is installed.

For new users - who may not understand why the launch file is not 'working' - this should not be a problem: they typically install either ros-$distro-desktop or ros-$distro-desktop-full, which will already have RViz installed.

New users will typically also want to start with the supplied MoveIt configuration packages. As those also already have a dependency on RViz, it should get pulled in at that point as well.

gavanderhoorn commented 6 years ago

Note also that the urdfs and meshes provided by support packages are used by MoveIt and other packages for motion planning purposes. MoveIt can be run on headless systems that don't have any of the packages installed that rviz would want to pull in, which is one of the motivations for this ticket.

mathias-luedtke commented 6 years ago

:+1: I totally agree with you.

For improving the user experience we could prepare a meta-package to pull in the (visualization) packages that we excluded intentionally from support packages. Of course, this does not make sense if it's just rviz.

BrettHemes commented 6 years ago

:+1:

jrgnicho commented 6 years ago

:+1:

stwirth commented 6 years ago

:+1:

Levi-Armstrong commented 6 years ago

:+1:

gavanderhoorn commented 6 years ago

Thanks for the input.

Note btw that this is exactly the kind of ticket that the +1 button on the top right was introduced for ;)

130s commented 6 years ago

+1 I didn't even know such kind of requirement exists. Just curious if you could share the idea, what kind of OS/hardware do you have in mind to run the packages you mentioned?

Probably minor caveat of this is that roslaunch_add_file_check might always fail.

gavanderhoorn commented 6 years ago

I didn't even know such kind of requirement exists.

rviz has been a dependency since the time the test_*.launch and robot_state_visualize_*.launch files were introduced, which was somewhere around 2013/2014.

Reasoning was that new users (which we had a lot at that time) do not always immediately understand what they should do, so making sure that all launch files worked out-of-the-box took priority over modularity and separation-of-concerns.

Just curious if you could share the idea, what kind of OS/hardware do you have in mind to run the packages you mentioned?

A simple example would be a headless industrial PC: although it probably would have enough resources to run RViz (cpu and memory, obviously not gpu), it doesn't need to have all those dependencies installed. It's just a waste of space.

A second example could be a RPi (or similar SBC, BBB fi): while relatively powerful in recent editions, it makes little sense to have RViz there, especially if the SBC is only used to run the driver (and base move_group fi). This happens more often than you probably realise: vendors of mobile bases for industrial robots (such as Clearpath) do not equip their products with any graphics hardware most of the time :)

Probably minor caveat of this is that roslaunch_add_file_check might always fail.

yes, that is a good point. The checks we have will have to either be allowed to fail, or removed completely.

mathias-luedtke commented 6 years ago

Probably minor caveat of this is that roslaunch_add_file_check might always fail.

yes, that is a good point. The checks we have will have to either be allowed to fail, or removed completely.

Looking at the existing launch files the robot-specific part seems to be duplicated. So we could just factor it out und test this part.

gavanderhoorn commented 4 years ago

Coming back to this: https://github.com/ros/joint_state_publisher/pull/31 has given this new priority, as UI infrastructure was being pulled in by joint_state_publisher anyway, so having rviz as a dependency was not really a problem.

Now that the JSP has been fixed, the only dependency pulling in 800MB+ of UI infrastructure when installing a support package on an otherwise bare system is rviz.

I'm not entirely sure how we'd present a nice error to users that may expect things to just work.

ipa-nhg commented 3 years ago

assigned to @jeritgeorge

gavanderhoorn commented 3 years ago

Has there been any discussion on how to approach this?

Simply removing the dependency would be the easiest, but would also probably result in surprised users.

jeritgeorge commented 3 years ago

I was thinking one way to just present some sort of error message is doing something like creating a node for a rostopic publisher and subscriber in the launch file that would print out a message stating that rviz is not installed by default and is required. Though I can't figure out a way to easily do something like check if the package is installed and if not print the message.

gavanderhoorn commented 3 years ago

launch supports eval(..), which could perhaps be used with find(..) to figure out whether a package is installed. I'm not sure what happens if you try to find(..) something and it fails.

jeritgeorge commented 3 years ago

I tried using eval(..) with find(..) which does work if it finds the package, but if find(..) does fail, it just prints out the package wasn't found and the launch file crashes. A simpler solution could be to print out a message stating something like "rviz isn't a dependency and if it isn't found, then it will need to manually installed." and have it print out regardless.

gavanderhoorn commented 3 years ago

A simpler solution could be to print out a message stating something like "rviz isn't a dependency and if it isn't found, then it will need to manually installed." and have it print out regardless.

you mean, unconditionally?

That would be really confusing I feel.

The nr of times we have people opening issues because they perceive warnings to be fatal errors is certainly non-negligible.

gavanderhoorn commented 3 years ago

We could consider creating a 'wrapper script', which we start instead of RViz proper.

That script checks whether RViz is installed, if not: print warning and hang (we can't exit as the rviz node is often a required node and if we exit everything just shuts down and we might as well not check and let roslaunch just fail).

If it is installed, simply start RViz and pass on all args the wrapper script was given (by roslaunch).

It doesn't even need to be a node. I've started plain Python scripts with something like Is it possible to launch non ros applications with roslaunch?.

C++ might be nicer as it uses less resources than a Python script. But a first prototype could be written using Python.


Edit: unfortunate roslaunch doesn't support something like xacro's optional attribute. That would have made this trivial to implement. optional="true" as a complement to required="true" would even make a lot of sense.

jeritgeorge commented 3 years ago

Would having a compiled C++ script in a simple support package be unnecessarily complex? If not, I can go ahead and try to implement this.

gavanderhoorn commented 3 years ago

No that's basically what I was suggesting.

So instead of

<launch>
  <node name="rviz" pkg="rviz" type="rviz" args="..." required="true" />
</launch>

we'd have:

<launch>
  <node name="rviz" pkg="my_rviz_wrapper" type="is_rviz_installed" args="<the_same_args_as_normal>" required="true" />
</launch>

Let's treat it as a prototype though. It could be were either over-engineering this or UX is horrible which we figure out after it's all implemented.

PS: checking whether RViz is installed is probably easiest to do with rospack (which has a C++ library).


Edit: we could also use a launch-prefix. That way, the node element would stay exactly as it is, but it would get an additional launch-prefix="$(find my_rviz_wrapper)/is_rviz_installed" (or something similar).

This won't work, as roslaunch will first try to locate RViz and fail. The launch-prefix won't affect this.

gavanderhoorn commented 3 years ago

Could even make it generic, and not check for RViz specifically, but a binary in a ROS package. Then it could be used to make whatever node optional -- in lieu of support for this in roslaunch proper.

gavanderhoorn commented 3 years ago

@jeritgeorge: would you be able/willing to complete implementing the discussed approach? Otherwise please unassign yourself from this issue.

jeritgeorge commented 3 years ago

Sorry, yes, I just haven't had any time these past few days, though I should have time to try implementing today.