Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix need for downstream dependency on OGRE, #733. #734

Closed
wants to merge 1 commit into from

Conversation

hersh
Copy link
Contributor

@hersh hersh commented Feb 27, 2014

This seems to fix #733 for me. See also #730.

@wjwwood
Copy link
Member

wjwwood commented Feb 27, 2014

I don't yet understand how this is not just redundant, I am building OGRE 1.9 now to test it.

@hersh
Copy link
Contributor Author

hersh commented Feb 27, 2014

For me with hydro on ubuntu precise with vanilla OGRE install, this was necessary for the downstream projects to #include ogre files correctly. I don't know enough about catkin to say anything about its redundancy, just that it fixes the problem for me.

@wjwwood
Copy link
Member

wjwwood commented Feb 27, 2014

Rviz is using pkg-config to find OGRE-OV, adding it to the DEPENDS section of catkin_package is almost certainly not the right fix by itself.

@hersh
Copy link
Contributor Author

hersh commented Feb 27, 2014

Ah, well... at least it contains the fix within rviz and doesn't make downstream packages think about it. Fwiw, the "vanilla" OGRE install I mentioned above is 1.7.4.

@cottsay
Copy link
Member

cottsay commented Feb 27, 2014

It looks like Ogre 1.8 has a FindOGRE.cmake packaged with it on Fedora 19 and 20, which is why adding it to DEPENDS fixes this for Fedora. OGRE.pc is also present.

What I can't explain (yet) is why adding the include directories to the catkin_package macro doesn't take care of this (https://github.com/ros-visualization/rviz/blob/hydro-devel/CMakeLists.txt#L108), which @wjwwood mentioned in #730 (comment)

@wjwwood
Copy link
Member

wjwwood commented Feb 27, 2014

He added OGRE_OV to DEPENDS which is neither a pkg-config file nor any cmake module or config.

@wjwwood
Copy link
Member

wjwwood commented Feb 27, 2014

The assertion is that rviz 1.10.13 does not properly pass along the include directories for ogre, even with ogre 1.7.4 (default in precise/raring etc...).

So I built rviz 1.10.13 from source and modified it like this:

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 2da54b4..f1f410f 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -101,6 +101,8 @@ find_package(Eigen REQUIRED)

 catkin_python_setup()

+message("OGRE_OV_INCLUDE_DIRS: ${OGRE_OV_INCLUDE_DIRS}")
+
 catkin_package(
   INCLUDE_DIRS
     src

The out of which looks like this:

OGRE_OV_INCLUDE_DIRS: /usr/include/OGRE

Inspecting the rvizConfig.cmake in the devel space after only running cmake I see:

$ cat devel/share/rviz/cmake/rvizConfig.cmake  | grep _include_dirs
  set(_include_dirs "/home/william/devel/rviz/src;/usr/include/eigen3;/usr/include/OGRE;/usr/include;/opt/ros/hydro/include;/opt/ros/hydro/include;/opt/ros/hydro/include")
  foreach(idir ${_include_dirs})

Then after building and installing rviz I looked at the rvizConfig.cmake in the install space:

 cat install/share/rviz/cmake/rvizConfig.cmake  | grep _include_dirs
  set(_include_dirs "include")
  foreach(idir ${_include_dirs})

So then I applied the patch from this pull request, and ran cmake again (after cleaning the entire build folder), checking each of the above again:

OGRE_OV_INCLUDE_DIRS: /usr/include/OGRE
$ cat ./devel/share/rviz/cmake/rvizConfig.cmake | grep _include_dirs
  set(_include_dirs "/home/william/devel/rviz/src;/usr/include/eigen3;/usr/include/OGRE;/usr/include;/opt/ros/hydro/include;/opt/ros/hydro/include;/opt/ros/hydro/include;/usr/include/OGRE")
  foreach(idir ${_include_dirs})
$ cat ./install/share/rviz/cmake/rvizConfig.cmake | grep _include_dirs
  set(_include_dirs "include;/usr/include/OGRE")
  foreach(idir ${_include_dirs})

@dirk-thomas

@wjwwood
Copy link
Member

wjwwood commented Feb 27, 2014

Also, the variable OGRE_OV_INCLUDE_DIRS is being passed to catkin_package's INCLUDE_DIRS option in both cases:

https://github.com/ros-visualization/rviz/blob/hydro-devel/CMakeLists.txt#L108

@wjwwood
Copy link
Member

wjwwood commented Feb 27, 2014

This commit introduced the problem for downstream things:

c7b8d44

But, it looks like this should be fixed in catkin, @dirk-thomas is making a pull request now. But this pull request is, in my opinion, not the correct solution.

@dirk-thomas
Copy link
Contributor

+1 for not merging this.

Instead catkin should allow passing absolute paths via catkin_package(INCLUDE_DIRS ...). See pull request for that: ros/catkin#600

@bchretien
Copy link
Contributor

Works perfectly with ros/catkin#600 on Arch Linux with Ogre 1.9. Thanks @dirk-thomas @wjwwood etc.

@wjwwood
Copy link
Member

wjwwood commented Feb 28, 2014

The farm seems to be green again, so I think ros/catkin#600 should have fixed this issue without any need to change rviz. I will close this, please comment here if you disagree.

@wjwwood wjwwood closed this Feb 28, 2014
@wjwwood wjwwood deleted the fix-733 branch February 28, 2014 19:26
130s pushed a commit to 130s/rviz that referenced this pull request Aug 21, 2024
* Remove window_close.png

Signed-off-by: Rebecca Butler <[email protected]>

* Rename failed_display.png -> close.png

Signed-off-by: Rebecca Butler <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants