search

ros2 / rviz

ROS 3D Robot Visualizer
BSD 3-Clause Clear License
296 stars 212 forks source link

Using ogre-1.12-dev instead of vendoring #876

Open ijnek opened 2 years ago

ijnek commented 2 years ago

Quoting @wjwwood from #723, the reason for having rviz_ogre_vendor was:

I think that we vendored it because the versions below it didn't work for us, the ones in Ubuntu, and later versions had bugs too. I imagine this has been resolved by now, at least partially. But I know that the version used by rviz in ROS 1 and ignition/gazebo didn't work for us.

One reason we didn't have the conditional "find it upstream first" is that the cmake files that are provided by ogre were broken in some way and so we have a lot of custom logic for loading them as imported targets. I don't know if this logic can be used if the libraries are being provided separately. But maybe they can and we just didn't do that because it was extra work that we didn't have time to implement.

Now, the version that is built in rviz_ogre_vendor is libogre-1.12 which I believe was not available when rviz was ported to ROS2, but is available on Ubuntu focal onwards.

Would it be reasonable to add the libogre-1.12-dev key to rosdep and add it as a dependency to try and remove rviz_ogre_vendor? Or are there other concerns preventing us from using it?

Ps. I did manage to get RViz2 up and running using libogre-1.12-dev from ubuntu packages as shown in screenshot below

image

clalancette commented 2 years ago

We would love to switch to using the Ubuntu version of the libogre package. The problems to be solved to get there:

  1. There are a bunch of deprecation warnings when building against this newer libogre package. We had some discussion about it in https://github.com/ros2/rviz/pull/755 , and I made some further progress in https://github.com/ros2/rviz/tree/clalancette/ogre-1.12.10-upgrade , but it still needs more work.
  2. We still have to leave the vendor package in place, as we still need to build the vendored package for Windows.
  3. Someone needs to test out most of the RViz functionality with the new libogre1.12 and ensure that it works properly.

@ijnek If you are willing to pick up that work and complete it, we would be happy to review it. Thanks!

wjwwood commented 2 years ago

Yeah, what @clalancette said. We'll have to keep the vendor package regardless.

But upgrading to a newer version in the vendor package and then changing it to use the version from Ubuntu/Debian/REHL when they're found would be a fine step.

ijnek commented 2 years ago

@clalancette @wjwwood Thanks for the input, I was originally looking into upgrading Ogre up to one of the most recent versions to fix the line strip marker displaying stangely, (fixed in Ogre13.4.1 - https://github.com/ros-visualization/rviz/issues/1287#issuecomment-1151238688)

(The large jump in version number from Ogre1.12 to Ogre13.4.1 is due to a version schema change from Ogre.)

To fix that issue, staying with a vendor package but with a newer version would be necessary, but I don't know if that bug alone outweighs the advantages of simplifying things by using binary packages if available.

Also, what are your thoughts on moving rviz_ogre_vendor out of rviz's repo to an ogre_vendor repo?

ijnek commented 2 years ago

To start off, I'm going to continue the upgrade to 1.12.10, then look into using the non-vendor version.

clalancette commented 2 years ago

Also, what are your thoughts on moving rviz_ogre_vendor out of rviz's repo to an ogre_vendor repo?

I don't think that is strictly necessary. As we have it now, it is kind of "hidden" from the rest of the system, so only RViz can (easily) use it. The only time that would be a problem is if some program wanted to both embed RViz, and embed another library that uses the system version of OGRE. While I can imagine such programs, we've never yet had a request for that. Regardless, I think the easiest way to fix that particular problem is to use the system version of OGRE.

To start off, I'm going to continue the upgrade to 1.12.10, then look into using the non-vendor version.

That sounds good. I definitely would like to get onto 1.12.10, and then we can discuss if we want to go further forward than that.

ijnek commented 2 years ago

Now that the upgrade to 1.12.10 is done, I was looking at the option of switching rviz_ogre_vendor to binary ogre-1.12 packages if possible. I've listed ogre1.12.10 binaries across different platforms in this Draft PR (https://github.com/ros/rosdistro/pull/34085), but it seems like 1.12.10 is only supported on Debian bullseye and Ubuntu Jammy, and not found in the others.

The lack of binary packages across platforms makes me wonder if switching to binary platform-specific packages aren't worth it.

Two advantages I see of not switching to binary packages are:

What do you think @clalancette ?

clalancette commented 2 years ago

What do you think @clalancette ?

In short, what I'd prefer to do here is what we do for several other vendored packages, which is to use the binary package where it is available (Ubuntu Jammy, currently), and build it from source on the platforms that don't have the correct version. This will allow us to nicely integrate into the platform when we can, and fall back to our own private copy when we can't.

Note that the rviz_ogre_vendor CMakeLists.txt doesn't currently do this, so we'll have to change/augment it to do that.

ijnek commented 2 years ago

Thanks for informing me about what is usually done, I wasn't too familiar so it helps.

In that case, I'm going to mark ros/rosdistro#34085 as ready to be reviewed, and try and get rviz to use that version over the vendor as per your suggestion.

ijnek commented 2 years ago

https://github.com/ros/rosdistro/pull/34085 has been merged, I'll proceed with trying toget rviz to use that version.

clalancette commented 2 years ago

ros/rosdistro#34085 has been merged, I'll proceed with trying toget rviz to use that version.

Thanks!

felixf4xu commented 2 months ago

It should be easy to switch, but when I check the code, why https://github.com/ros2/rviz/blob/rolling/rviz_ogre_vendor/rviz_ogre_vendor-extras.cmake.in is so complicated?