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

Protect dart-gui-osg and check more rigorously for the presence of OSG #898

Merged
merged 3 commits into from
Aug 7, 2017

Conversation

mxgrey
Copy link
Member

@mxgrey mxgrey commented Aug 4, 2017

This PR has two related (but independent) purposes:

  1. Prevent dart-gui-osg from being built when OSG is not detected. This is done by wrapping the second half of dart/gui/osg/CMakeLists.txt in an if-block. This is done by returning immediately if OpenSceneGraph cmake variables cannot be found. This was not needed in the past because the call to add_subdirectory(osg) used to be wrapped in an if-statement.

  2. More rigorously check whether OSG exists. In the process of fixing this, I discovered that the version of FindOpenSceneGraph.cmake that is provided with cmake-3.5 will set OPENSCENEGRAPH_FOUND to true if it finds libopenthreads, even if libopenscenegraph isn't actually installed. I believe this is a bug on the part of either CMake or OpenSceneGraph, because this behavior contradicts the variable's description provided by FindOpenSceneGraph.cmake, but I found a workaround by checking both OPENSCENEGRAPH_FOUND and OSG_FOUND. The latter seems to correctly return false when OpenSceneGraph is not available.

Note that the vast majority of the diff is due to white space from indenting the content inside the if-statement.

@mxgrey
Copy link
Member Author

mxgrey commented Aug 4, 2017

PR #899 by @scpeters made me realize that CMake offers a return() function. That allows us to eliminate the HAVE_OPENSCENEGRAPH variable altogether and make the diff much nicer. I'm going to shamelessly steal his approach.

@@ -8,11 +8,10 @@ if(DART_BUILD_GUI_OSG)

find_package(OpenSceneGraph 3.0 QUIET
COMPONENTS osg osgViewer osgManipulator osgGA osgDB)
if(OPENSCENEGRAPH_FOUND)
if(OPENSCENEGRAPH_FOUND AND OSG_FOUND)
Copy link
Member

@jslee02 jslee02 Aug 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are both of these variables set to true when OSG is found? It seems OPENSCENEGRAPH_FOUND is defined only when FindOpenSceneGraph.cmake is used, while OSG_FOUND is defined only when FindOSG.cmake is used. Shouldn't this to be OPENSCENEGRAPH_FOUND OR OSG_FOUND? Also, if FindOSG.cmake is used, it looks like we need to use OSG_LIBRARY instead of OPENSCENEGRAPH_LIBRARIES. Am I missing something here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, you already mentioned about this in the post. Sorry, wasn't paying sufficient attention. 😓 I think it would be much great if we add a simple comment why we check both of them.

@scpeters
Copy link
Collaborator

scpeters commented Aug 4, 2017

I only noticed the return() function because it was already used at the top of that file:

@codecov
Copy link

codecov bot commented Aug 4, 2017

Codecov Report

Merging #898 into master will increase coverage by 0.07%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           master    #898      +/-   ##
=========================================
+ Coverage   50.63%   50.7%   +0.07%     
=========================================
  Files         299     299              
  Lines       23411   23476      +65     
  Branches     3016    3030      +14     
=========================================
+ Hits        11854   11904      +50     
- Misses      10226   10241      +15     
  Partials     1331    1331
Impacted Files Coverage Δ
dart/utils/urdf/DartLoader.cpp 63.66% <0%> (+2.92%) ⬆️
dart/collision/CollisionFilter.cpp 58.82% <0%> (+6.97%) ⬆️

@mxgrey mxgrey merged commit 71e31c9 into master Aug 7, 2017
@jslee02 jslee02 deleted the guard_gui-osg branch August 7, 2017 21:53
jslee02 added a commit that referenced this pull request Aug 7, 2017
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.

3 participants