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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert OGRE-Next custom support and back to use IgnOGRE2 module #605

Merged
merged 9 commits into from
Apr 13, 2022

Conversation

j-rivero
Copy link
Contributor

@j-rivero j-rivero commented Apr 8, 2022

馃帀 New feature

The PR mostly removes the support introduced in #577 to workaround the problems with Ubuntu Jammy using ogre-next package. The support is implemented in gazebosim/gz-cmake#224 that should work transparently for ign-rendering.

The PR tries to look for OGRE2 twice, one with PlanarRelections component (needed for Jammy) and the other one without PlanarReflections. The issues is being worked in #597

Test it

Colcon workspace with gazebosim/gz-cmake#224 and this PR should be enough.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

ign-cmake support to deal with Ubuntu Jammy's ogre-next package is under
review and will make the custom workaround in the CMake code of this
project not needed anymore. This commit partially reverts the changes
introduced in f043b4f.

Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org>
Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org>
Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org>
@j-rivero j-rivero requested a review from iche033 as a code owner April 8, 2022 19:05
@github-actions github-actions bot added the 馃彲 fortress Ignition Fortress label Apr 8, 2022
@osrf-triage osrf-triage added this to Inbox in Core development Apr 8, 2022
@chapulina chapulina self-requested a review April 8, 2022 19:39
@chapulina chapulina moved this from Inbox to In progress in Core development Apr 8, 2022
@chapulina chapulina moved this from In progress to In review in Core development Apr 8, 2022
@j-rivero
Copy link
Contributor Author

j-rivero commented Apr 8, 2022

I think that we are going to need a new release of ign-cmake to be able to check CI.

CMakeLists.txt Outdated
PRIVATE_FOR ogre2
QUIET)

if (NOT OGRE2_FOUND)
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe check to see if ${OGRE2-PlanarReflections} equals "OGRE2-PlanarReflections-NOTFOUND" here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

uh much better, thanks Ian 3fd7e1d.

Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org>
@j-rivero
Copy link
Contributor Author

I think that CI is going to require new ignition cmake to work. We need to bump the ign-cmake version required here too.

@j-rivero
Copy link
Contributor Author

@osrf-jenkins run tests please

Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org>
@j-rivero
Copy link
Contributor Author

Ummm, we have a problem that is hard to solve or workaround in ign-rendering. The two calls for ign_find_package is completely buggy. We would need gazebosim/gz-cmake#231.

Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org>
@codecov
Copy link

codecov bot commented Apr 12, 2022

Codecov Report

Merging #605 (619c9b6) into ign-rendering6 (ab9af78) will increase coverage by 25.65%.
The diff coverage is n/a.

@@                 Coverage Diff                 @@
##           ign-rendering6     #605       +/-   ##
===================================================
+ Coverage           54.34%   80.00%   +25.65%     
===================================================
  Files                 198        1      -197     
  Lines               20225       15    -20210     
===================================================
- Hits                10991       12    -10979     
+ Misses               9234        3     -9231     
Impacted Files Coverage 螖
include/ignition/rendering/base/BaseStorage.hh
include/ignition/rendering/base/BaseMesh.hh
ogre2/src/Ogre2Capsule.cc
ogre/src/OgreNode.cc
ogre2/src/Ogre2AxisVisual.cc
ogre/src/OgreRenderEngine.cc
ogre2/src/Ogre2Heightmap.cc
include/ignition/rendering/Visual.hh
ogre/src/OgreLidarVisual.cc
include/ignition/rendering/GpuRays.hh
... and 189 more

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 ab9af78...619c9b6. Read the comment docs.

@j-rivero j-rivero requested a review from iche033 April 13, 2022 11:44
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

Nice! I see this on the Bionic and Focal builds:

  -- Looking for OGRE using the name: OGRE2
  --   + component HlmsPbs: found
  --   + component HlmsUnlit: found
  --   + component Overlay: found
  --   ! component PlanarReflections: not found!
  --   ! OGRE2 not found
  -- Looking for OGRE using the name: OGRE-Next
  --   ! OGRE-Next not found
  -- Looking for IgnOGRE2 - not found
  
  -- PlanarReflections component was not found. Try looking without it:
  -- Looking for OGRE using the name: OGRE2
  --   + component HlmsPbs: found
  --   + component HlmsUnlit: found
  --   + component Overlay: found
  -- Looking for IgnOGRE2 - found

It looks like Jammy CI is not on ign-rendering6 anymore and was reverted with a commit directly to the branch:

92bbff3

We should make sure this patch works with Jammy. Can we add back that CI here?

@j-rivero
Copy link
Contributor Author

j-rivero commented Apr 13, 2022

We should make sure this patch works with Jammy. Can we add back that CI here?

here we go 619c9b6

It looks like Jammy CI is not on ign-rendering6 anymore and was reverted with a commit directly to the branch:

my bad sorry, wrong branch.

Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

Nice! I see Jammy finds it now:

   -- Looking for OGRE using the name: OGRE2
  --   ! OGRE2 not found
  -- Looking for OGRE using the name: OGRE-Next
  --   + component HlmsPbs: found
  --   + component HlmsUnlit: found
  --   + component Overlay: found
  --   + component PlanarReflections: found
  -- Looking for IgnOGRE2 - found

馃殺 it!

@j-rivero j-rivero merged commit 65f0eff into ign-rendering6 Apr 13, 2022
@j-rivero j-rivero deleted the jrivero/6_revert_ogrenext_workaround branch April 13, 2022 23:23
Core development automation moved this from In review to Done Apr 13, 2022
@osrf-triage
Copy link

This pull request has been mentioned on Gazebo Community. There might be relevant details there:

https://community.gazebosim.org/t/new-releases-2022-04-27-fortress-citadel/1389/1

@j-rivero j-rivero removed this from Done in Core development May 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
馃彲 fortress Ignition Fortress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants