Slow PackageMap creation possible with ROS_PACKAGE_PATH

[IanTheEngineer]

Initially communicated to me by @calderpg-tri stating the following (edited for issue clarity):

While using

drake::multibody::PackageMap package_map;
package_map.PopulateFromEnvironment("ROS_PACKAGE_PATH");

to enable model loading from ROS packages, certain file structures can produce long package map creation times. In a typical user system, exemplified by

$ echo $ROS_PACKAGE_PATH
/home/user/catkin_workspace/src:/opt/ros/noetic/share

this case is basically instant. However, on certain systems that 1) are using catkin/ament build, ROS_PACKAGE_PATH can be huge, with a separate entry for each package 2) have packages which contain large datasets with equally-deep directory hierarchies this takes several minutes in the worst case.

In those cases, a user can typical prevent directory traversal on a folder by folder basis for ROS tooling. We suspected that CrawlForPackages may not be working as intended (specifically the termination on finding a package.xml). This was confirmed by speaking with some folks with Open Robotics: termination conditions for crawling ROS_PACKAGE_PATH include 1) If a directory contains a CATKIN_IGNORE (ROS1) or AMENT_IGNORE (ROS2), stop traversing recursively deep. 2) If a directory contains a package.xml, stop traversing recursively deep.

This request is for drake::multibody::PackageMap::Populate* functions to respect the typical ROS_PACKAGE_PATH termination conditions during directory traversal.

[IanTheEngineer]

@cottsay is planning to investigate this issue.

[jwnimmer-tri]

The trick here is that the current functions' contract does not allow them to stop under those conditions, they promise to search the full tree.

To add the stopping logic, we'll either need to add new ROS-specific functions with the updated semantics, or else add some kind of flag argument to the current functions to specify whether or not they should use the stopping logic. If we like, we can default the flag to the current behavior for now, deprecate that default, and then in a few months change the default to the the stoppy one. We could then later even deprecate and remove the option to do the full search.

I do think this issue is real and we should fix it, but we'll just need to be careful to provide a transition plan.

[EricCousineau-TRI]

From meeting today, additional suggestions:

• Should have concrete benchmarks / indications (performance, bad shadowing from test packages)
• Consider identifying / documenting existing behavior (beyond just code) - ideally upstream
• Should acknowledge additional affordances (e.g. catkin_build cmdline), and either use them or say "nah, too complicated"