Optionally enable an embedded copy of DART 6.10 Open Robotics fork

[j-rivero]

🦟 Bug fix

Summary

There are problems affecting ground vehicles simulation when ign-physics is using DART version 6.9, which is the case of the Ubuntu Focal platform where DART comes from Ubuntu repositories.

Some context about how DART is used in ign-physics:

• The only public header requiring DART headers is ignition/physics/dartsim/World.hh (#include <dart/simulation/World.hpp>).
  • The PR does not handle compilations using this header (by now).
• The only library I've detected with public DART symbols exposed is libignition-physics3-dartsim-plugin.so.3.2.0.
  • The PR assumes that we can break the ABI on plugins.
• The goal is to be able to respect current DART version installed in the user system, particularly DART coming from main distribution repository.
  • Should be possible to install dart 6.9 packages and having ign-physics using DART 6.10 with no further configuration by the user at execution time.

The PR implements the following:

1. Embed a DART copy of Open Robotics fork, version 6.10 commit gazebo-forks/dart@71c7cfb
2. Add an option named USE_VENDOR_DART to build against the internal copy instead of system DART.
   • Automatically enable this option if system is Ubuntu 20.04 and user did not provide an explicit value
3. Install dart6.10 libraries needed in the engine directory to avoid conflicts with dart system libraries and use RPATH/RUNPATH to make ign-physics libraries to find them automatically.

Checklist

• Signed all commits for DCO
• Added tests (adapt test to work with changes actually)
• Updated documentation (as needed)
• Updated migration guide (as needed)
• codecheck passed (See contributing)
• All tests passed (See test coverage)

Note to maintainers: Remember to use Squash-Merge

[codecov]

Codecov Report

Merging #274 (f69753f) into ign-physics3 (1a58f73) will not change coverage.
The diff coverage is n/a.

@@              Coverage Diff              @@
##           ign-physics3     #274   +/-   ##
=============================================
  Coverage         74.63%   74.63%           
=============================================
  Files               115      115           
  Lines              4703     4703           
=============================================
  Hits               3510     3510           
  Misses             1193     1193           

---

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 1a58f73...f69753f. Read the comment docs.

[azeey]

I wonder if it would be possible to solve our problem without having to vendor a copy of DART in ign-physics. Here's what I'm thinking:

• For binary distributions of ign-physics, we statically link it against DART. This will solve the issue with inter-operating with ROS.
• For source builds, we stipulate that users have to either build our fork of DART or add our PPA. If users are building from source, I don't think it would be too much to ask that they enable our PPA. Maybe we can add some cmake logic that detects if ign-physics is being built with the upstream version of DART and emit a warning saying some features will not be supported.

[chapulina]

For binary distributions of ign-physics, we statically link it against DART.

That sounds promising.

For source builds, we stipulate that users have to either build our fork of DART or add our PPA

A while back we decided that we wanted to support the upstream DART PPA, as well as the DART version shipped with Ubuntu. I think that it's reasonable to support the version on Ubuntu because it's stable, but upstream is less stable, so it may be more difficult to stay on top of it.

Another thing to keep in mind is that the end goal is to melt our fork (#49) 🍴

In summary, I don't think it's unreasonable to require our fork for now, as long as we can satisfy the 1st item.

[azeey]

For binary distributions of ign-physics, we statically link it against DART.

I gave it a try and I ran into a couple of issues

1. Multiple definition of symbols Joint::FORCE, Joint::Passive, etc from https://github.com/ignition-forks/dart/blob/11876d5516aa95c38d4837a6dcb2817b692cff51/dart/dynamics/Joint.cpp#L49-L56 I removed the offending lines and it built successfully. Despite the comment "These declarations are needed for linking to work", I didn't have any problem linking dynamically or statically.
2. When static linking in ign-physics, some tests fail because global variables are duplicated. This happens only in tests that directly use DART's data structures, and thus, link against DART directly. For example, UNIT_JointFeatures_TEST fails because two instances of a the global variable gContactSurfaceHandlers exist: one in libignition-physics-dartsim.so and another in the UNIT_JointFeatures_TEST binary. I don't know how we can fix this, but as I mentioned, it would only affect tests. It would not affect any downstream applications. As @j-rivero mentioned, the public header ignition/physics/dartsim/World.hh uses DART headers. If this header gets used by a downstream application, it would be problematic, but it is only meant to be used internally for unit tests.

[mxgrey]

As @j-rivero mentioned, the public header ignition/physics/dartsim/World.hh uses DART headers. If this header gets used by a downstream application, it would be problematic, but it is only meant to be used internally for unit tests.

If we intend for dartsim/World.hh to only be used for testing, I suggest moving it into the src/ directory. It was originally put in the public header directory on purpose to showcase that ignition-physics can even expose the internal API of specific physics engines if a user ever wants that. But if safe/correct usage relies on the assumption that users won't touch that header, then we may as well remove it from the public API and only expose it internally.

[peci1]

I think that it's reasonable to support the version on Ubuntu

If it's the usual ancient-and-never-updating version as is the example of gazebo classic in Bionic, then I'd say supporting this library could bring in a lot of issues if even bugs are not fixed in the ubuntu version (or if pushing the fix takes 3 years as with e.g. urdfdom).

[chapulina]

If it's the usual ancient-and-never-updating version as is the example of gazebo classic in Bionic, then I'd say supporting this library could bring in a lot of issues if even bugs are not fixed in the ubuntu version (or if pushing the fix takes 3 years as with e.g. urdfdom).

Yeah it's the same situation. What I meant is that it may be desirable to make sure ign-physics can be compiled against the DART version upstream, in case users rely on that stability. Ideally we wouldn't need to maintain a fork. But the more time goes by, the less it seems like we'll be able to get rid of our fork. Maybe for Ubuntu Jammy...

[chapulina]

@j-rivero , what's the status of this PR. It's targeted at the Dome branch. Should we table this idea?

[peci1]

• For binary distributions of ign-physics, we statically link it against DART. This will solve the issue with inter-operating with ROS.

Hmm, how would ROS interfere with ign-physics? IIUC, new Gazebo never runs a ROS library in its own process (contrary to Classic with gazebo_ros_api_plugin). So there doesn't seem to be any potential for conflict. Or am I missing something? Maybe with the newly added Python support, or if somebody uses ign-physics without Gazebo in a ROS library, but I guess those are niche use-cases (and Python support is not available in ign-physics5 at all).

So I guess static linking is not required and the RPATH solution with vendored DART should work.

Better than vendoring DART like this, I'd suggest creating a packages.osrfoundation.org DEB of DART that would install into a non-system location, and making sure ign-physics (or ign-cmake) can find it and RPATH it.

What I meant is that it may be desirable to make sure ign-physics can be compiled against the DART version upstream, in case users rely on that stability.

I agree it'd be good to allow users building against distribution DART. However, different from current behavior, things that are actually broken by that should show loud and clear errors in the log (e.g. the tracked vehicle plugin could check whether DART has the support, and if not, issue an error message; similarly, when there is a joint velocity limit, Gazebo should issue an error that with DART 6.9, it will not be respected).