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

Integrate Ogre-Next 3.x.x built from source #468

Open
wants to merge 10 commits into
base: gz-cmake4
Choose a base branch
from

Conversation

sumir0
Copy link

@sumir0 sumir0 commented Feb 9, 2025

🎉 New feature

Related to Integrating Ogre-Next 3.0's atmosphere component

Summary

This PR allows Ogre-Next version>=3.0.0 to be found by using gz_find_package (for example: gz_find_package(GzOGRENext VERSION 3).

The code was tested on OGRE-Next version 2.3.3 (installed via a package manager) and version 3.0.0 (built from source using -D OGRE_USE_NEW_PROJECT_NAME=ON) with OGRE (classic) installed alongside.

The code is heavily based on the cmake/FindGzOGRE2.cmake file and represents a reduced and slightly modified version of that file.

Test it

  • Install pkg-config. To do that I typed pacman -S pkg-config in a terminal. For Ubuntu a similar package probably is available.
  • Install Ogre-Next
  • Follow the instructions in the examples/find_ogre_next/README.md file.

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.

@sumir0
Copy link
Author

sumir0 commented Feb 10, 2025

As it is related to #457, should I try to merge cmake/FindGzOGRE2.cmake and cmake/FindGzOgre3.cmake into something like cmake/FindGzOgreNext.cmake?

PS. Sorry for the commits related to a typo and obsolete documetation. For some strange reason I didn't notice it when I was reviewing my code.

@sumir0 sumir0 requested a review from ahcorde February 10, 2025 14:28
@traversaro
Copy link
Contributor

As mentioned in #457 (comment) since OgreNext 3.0 it should be sufficient to just call find_package(OgreNext CONFIG) and all the required variables and imported target should be defined without any additional complexity, just in case did you checked if that works?

@j-rivero
Copy link
Contributor

Thanks @sumir0 for the PR ! The support for OGRE-3.x and gazebosim/gz-sim#2753 is really cool.

As it is related to #457, should I try to merge cmake/FindGzOGRE2.cmake and cmake/FindGzOgre3.cmake into something like cmake/FindGzOgreNext.cmake?

This is indeed a good idea. Even better would be that we can use the the CMake Module shipped directly by upstream as @traversaro mentioned. If you could give it a try, would be great.

PS. Sorry for the commits related to a typo and obsolete documetation. For some strange reason I didn't notice it when I was reviewing my code.

No worries!

@sumir0
Copy link
Author

sumir0 commented Feb 10, 2025

I checked find_package(OGRE) on my setup (OGRE-Next v3.0.0 built from source) with -D CMAKE_MODULE_PATH=/usr/local/lib/OGRE/cmake. It finds OGRE (classic) when it is installed alongside. It does find OGRE-Next when OGRE (classic) is not installed. But it doesn't find the Atmosphere component which we will, probably, need in future.

Output with OGRE (classic) installed alongside:

-- Looking for OGRE...
-- OGRE_PREFIX_WATCH changed.
-- Found PkgConfig: /usr/bin/pkg-config (found version "2.3.0")
-- Checking for module 'OGRE-Next'
--   Package 'OGRE-Next' not found
-- Checking for module 'OGRE'
--   Found OGRE, version 14.3.2
-- Found Ogre  (..)
-- Found OGRE: optimized;/usr/lib/libOgreMain.so;debug;/usr/local/lib/libOgreMain_d.so
-- Looking for OGRE_Paging...
-- Found OGRE_Paging: optimized;/usr/lib/libOgrePaging.so;debug;/usr/lib/libOgrePaging.so
-- Looking for OGRE_Terrain...
-- Found OGRE_Terrain: optimized;/usr/lib/libOgreTerrain.so;debug;/usr/lib/libOgreTerrain.so
-- Looking for OGRE_Property...
-- Found OGRE_Property: optimized;/usr/lib/libOgreProperty.so;debug;/usr/lib/libOgreProperty.so
-- Looking for OGRE_Volume...
-- Found OGRE_Volume: optimized;/usr/lib/libOgreVolume.so;debug;/usr/lib/libOgreVolume.so
-- Looking for OGRE_Overlay...
-- Found OGRE_Overlay: optimized;/usr/lib/libOgreOverlay.so;debug;/usr/local/lib/libOgreOverlay_d.so
-- Looking for OGRE_HlmsPbs...
-- Could not locate OGRE_HlmsPbs
-- Looking for OGRE_HlmsUnlit...
-- Could not locate OGRE_HlmsUnlit

Output with only OGRE-Next installed (with the Atmosphere component) from source:

-- Looking for OGRE...
-- OGRE_PREFIX_WATCH changed.
-- Found PkgConfig: /usr/bin/pkg-config (found version "2.3.0")
-- Checking for module 'OGRE-Next'
--   Package 'OGRE-Next' not found
-- Checking for module 'OGRE'
--   Package 'OGRE' not found
-- Found Ogre Eris (3.0.0)
-- Found OGRE: /usr/local/lib/libOgreMain_d.so
-- Looking for OGRE_Paging...
-- Could not locate OGRE_Paging
-- Looking for OGRE_Terrain...
-- Could not locate OGRE_Terrain
-- Looking for OGRE_Property...
-- Could not locate OGRE_Property
-- Looking for OGRE_Volume...
-- Could not locate OGRE_Volume
-- Looking for OGRE_Overlay...
-- Found OGRE_Overlay: /usr/local/lib/libOgreOverlay_d.so
-- Looking for OGRE_HlmsPbs...
-- Found OGRE_HlmsPbs: /usr/local/lib/libOgreHlmsPbs_d.so
-- Looking for OGRE_HlmsUnlit...
-- Found OGRE_HlmsUnlit: /usr/local/lib/libOgreHlmsUnlit_d.so

Thank you to all of you! I will convert this PR to draft for now. I will think about possibility of contributing to OGRE-Next, because I need the Atmosphere component for gazebosim/gz-sim#2753. Feel free to comment, any (good) advice would be appreciated.

@sumir0 sumir0 marked this pull request as draft February 10, 2025 16:25
@sumir0
Copy link
Author

sumir0 commented Feb 13, 2025

Summary of my investigation/thinking/review:

@sumir0
Copy link
Author

sumir0 commented Feb 13, 2025

Added more than one example because they can be used in case we will need automated tests for cmake/FindGzOGRENext.cmake. Furthermore, from the examples it can be seen on which versions the code was tested by me. But I have no problem with reducing the number of examples, don't hesitate if you think some examples are redundant.

@sumir0
Copy link
Author

sumir0 commented Feb 13, 2025

Is the chosen branch correct one to merge into or should I change it?

@sumir0 sumir0 marked this pull request as ready for review February 13, 2025 07:27
@sumir0
Copy link
Author

sumir0 commented Feb 13, 2025

Don't know if it was good or bad to sync the fork. Let me know if it was a bad decision.

@traversaro
Copy link
Contributor

Just to clarify, my suggestion is to use find_package(OgreNext CONFIG), not find_package(Ogre CONFIG).

@traversaro
Copy link
Contributor

Just to clarify, my suggestion is to use find_package(OgreNext CONFIG), not find_package(Ogre CONFIG).

Sorry, I realized I was missing a point. Is there any problem on just requiring that OgreNext is compiled by enabling the option OGRE_USE_NEW_PROJECT_NAME ? From what I understand some version of this option is already enabled in the existing packaged version of OgreNext that we support in gz-* projects.

@sumir0
Copy link
Author

sumir0 commented Feb 14, 2025

Sorry, I realized I was missing a point.

It's no trouble!

Is there any problem on just requiring that OgreNext is compiled by enabling the option OGRE_USE_NEW_PROJECT_NAME ?

Unfortunately, even in that case find_package(OGRE) is finding not only OGRE-Next but also OGRE and sets some variables pointing to OGRE. As far as I understand, it happens because of the logic in FindOGRE.cmake.

Take a note of what OGRE_PREFIX_GUESSES represents.

set(OGRE_PREFIX_GUESSES
    /opt/ogre-next
    /opt/OGRE-Next
    /usr/lib${LIB_SUFFIX}/ogre-next
    /usr/lib${LIB_SUFFIX}/OGRE-Next
    /usr/local/lib${LIB_SUFFIX}/ogre-next
    /usr/local/lib${LIB_SUFFIX}/OGRE-Next
    $ENV{HOME}/ogre-next
    $ENV{HOME}/OGRE-Next
    /opt/ogre
    /opt/OGRE
    /usr/lib${LIB_SUFFIX}/ogre
    /usr/lib${LIB_SUFFIX}/OGRE
    /usr/local/lib${LIB_SUFFIX}/ogre
    /usr/local/lib${LIB_SUFFIX}/OGRE
    $ENV{HOME}/ogre
    $ENV{HOME}/OGRE
  )

Also, take a note of what OGRE_PREFIX_PATH represents. ${OGRE_HOME}, ${OGRE_SDK}, ${ENV_OGRE_HOME}, ${ENV_OGRE_SDK} are not set by default. But even if we will set them to some value, it will not affect the logic in the way we want. I hope I will be able to show it to you further.

set(OGRE_PREFIX_PATH
  ${OGRE_HOME} ${OGRE_SDK} ${ENV_OGRE_HOME} ${ENV_OGRE_SDK}
  ${OGRE_PREFIX_GUESSES}
)

This is the macro used to create paths which will be searched later. In the FindOGRE.cmake this macro is called with ${PREFIX} == OGRE.

# Construct search paths for includes and libraries from a PREFIX_PATH
macro(create_search_paths PREFIX)
  foreach(dir ${${PREFIX}_PREFIX_PATH})
    set(${PREFIX}_INC_SEARCH_PATH ${${PREFIX}_INC_SEARCH_PATH}
      ${dir}/include ${dir}/Include ${dir}/include/${PREFIX} ${dir}/Headers)
    set(${PREFIX}_LIB_SEARCH_PATH ${${PREFIX}_LIB_SEARCH_PATH}
      ${dir}/lib ${dir}/Lib ${dir}/lib/${PREFIX} ${dir}/Libs)
    set(${PREFIX}_BIN_SEARCH_PATH ${${PREFIX}_BIN_SEARCH_PATH}
      ${dir}/bin)
  endforeach(dir)
  if(ANDROID)
	set(${PREFIX}_LIB_SEARCH_PATH ${${PREFIX}_LIB_SEARCH_PATH} ${OGRE_DEPENDENCIES_DIR}/lib/${ANDROID_ABI})
  endif()
  set(${PREFIX}_FRAMEWORK_SEARCH_PATH ${${PREFIX}_PREFIX_PATH})
endmacro(create_search_paths)

Later on, the include paths are searched as follows.

find_path(OGRE_CONFIG_INCLUDE_DIR NAMES OgreBuildSettings.h HINTS ${OGRE_INC_SEARCH_PATH} ${OGRE_FRAMEWORK_INCLUDES} ${OGRE_PKGC_INCLUDE_DIRS} PATH_SUFFIXES "OGRE")
find_path(OGRE_INCLUDE_DIR NAMES OgreRoot.h HINTS ${OGRE_CONFIG_INCLUDE_DIR} ${OGRE_INC_SEARCH_PATH} ${OGRE_FRAMEWORK_INCLUDES} ${OGRE_PKGC_INCLUDE_DIRS} PATH_SUFFIXES "OGRE")

And the version is determined via OgrePrerequisites.h.

# determine Ogre version
file(READ ${OGRE_INCLUDE_DIR}/OgrePrerequisites.h OGRE_TEMP_VERSION_CONTENT)
get_preprocessor_entry(OGRE_TEMP_VERSION_CONTENT OGRE_VERSION_MAJOR OGRE_VERSION_MAJOR)
get_preprocessor_entry(OGRE_TEMP_VERSION_CONTENT OGRE_VERSION_MINOR OGRE_VERSION_MINOR)
get_preprocessor_entry(OGRE_TEMP_VERSION_CONTENT OGRE_VERSION_PATCH OGRE_VERSION_PATCH)
get_preprocessor_entry(OGRE_TEMP_VERSION_CONTENT OGRE_VERSION_NAME OGRE_VERSION_NAME)
set(OGRE_VERSION "${OGRE_VERSION_MAJOR}.${OGRE_VERSION_MINOR}.${OGRE_VERSION_PATCH}")
pkg_message(OGRE "Found Ogre ${OGRE_VERSION_NAME} (${OGRE_VERSION})")

So far:

  • ${OGRE_INCLUDE_DIR}/OgrePrerequisites.h depends on OGRE_INCLUDE_DIR
  • OGRE_INCLUDE_DIR depends on OGRE_CONFIG_INCLUDE_DIR
  • OGRE_CONFIG_INCLUDE_DIR depends on OGRE_INC_SEARCH_PATH
  • OGRE_INC_SEARCH_PATH depends on create_search_paths and OGRE_PREFIX_PATH
    • create_search_paths does not generate a path to /usr/include/OGRE-Next/ (in which OgrePrerequisites.h is located) but generates only the next paths: ${dir}/include ${dir}/Include ${dir}/include/${PREFIX} ${dir}/Headers, where dir depends on OGRE_PREFIX_PATH
    • OGRE_PREFIX_PATH depends on OGRE_PREFIX_GUESSES and OGRE_HOME
      • OGRE_PREFIX_GUESSES does not result in generating /usr/include/OGRE-Next path
      • We can set OGRE_HOME to some value, but create_search_paths will not generate a path ending with OGRE-Next neither.

As a consequence, find_package(OGRE) sets OGRE_INCLUDE_DIRS to point to OGRE (not OGRE-Next) include paths at the moment. But at the same time find_package(OGRE) seems to set OGRE_LIBRARIES to a correct value. Though, find_package(OGRE) mixes components (especially, render systems). I don't know yet if found OGRE components are an issue, but find_package(OGRE) points to /lib/OGRE/RenderSystem* and not to /lib/OGRE-Next/RenderSystem*.

In conclusion, if OGRE-Next built with OGRE_USE_NEW_PROJECT_NAME=ON and OGRE are installed sidy-by-side find_package(OGRE) does not point to the correct include directories of the OGRE-Next library at the moment. Therefore, I suggest not to use find_package(OGRE) for now. But of course, I may have missed something, in which case let me know about that!

@traversaro
Copy link
Contributor

Unfortunately, even in that case find_package(OGRE) is finding not only OGRE-Next but also OGRE and sets some variables pointing to OGRE.

But in that case you should use find_package(OgreNext) to find the CMake files of ogre-next 3.*, why do you want to use find_package(OGRE) ?

@sumir0
Copy link
Author

sumir0 commented Feb 14, 2025

But in that case you should use find_package(OgreNext) to find the CMake files of ogre-next 3.*, why do you want to use find_package(OGRE) ?

I am sorry, it seems I didn't understand you for some reason.

I am not an expert in CMake (yet). Please, tell me if I have understood something wrong.

Let PackageName be a name of some package. To find this package via find_package(PackageName) at least one of the following files needs to be present:

  • FindPackageName.cmake
  • PackageNameConfig.cmake
  • packagename-config.cmake

Hence, to find_package(OgreNext) one of FindOgreNext.cmake, OgreNextCondif.cmake, ogrenext-config.cmake files needs to be present. Alas, I haven't managed to find any of these or similar files (using PackageName == OGRENext and PackageName == OGRE-Next) nor in my filesystem (with OGRE-Next 2.3.3 and 3.0.0 installed), nor in the OGRE-Next upstream.

There is no mention of find_package(OgreNext) in #457 and OGRECave/ogre-next#470, only find_package(OGRE) is mentioned.

Just in case I tested find_package(OgreNext), find_package(OGRENext), and find_package(OGRE-Next). Sadly, but it didn't work.

I apologize, I don't understand how it is supposed to work.

@traversaro
Copy link
Contributor

You are perfectly right, I had a distinct memory that Ogre-Next was installing CMake config files, but apparently it was totally wrong, sorry for the noise!

@sumir0
Copy link
Author

sumir0 commented Feb 15, 2025

Nothing to worry about! I believe, here we all just want to make this world better by making code better.

@j-rivero
Copy link
Contributor

j-rivero commented Feb 17, 2025

Thanks @sumir0 for the clarification. So let's continue the review of this implementation.

If we are targeting the gz-cmake4 branch, then we can not remove the FindOGRE2.cmake module. I agree on transitioning the name to OGRENext but we need a friendly path forward. Something like:

  • We can add FindOGRENext to deal with version >=3.0.0. Add an error and point to FindOGRE2.cmake if version is lower than 3.
  • We can leave the FindOGRE2.cmake to handle the 2.x series and add a warning to it if a version >=3.0.0 is used and point to the new module.

Maintaining both Modules is not the ideal solution but if we want to maintain compatibility we can not remove the FindOGRE2.cmake file and transforming it to use FindOGRENext internally will require quite an effort to export all the OGRE2_ variables that the current module is using.

"OGRE-Next with major version ${GzOGRE2_FIND_VERSION_MAJOR} was requested to be found but OGRE-Next>=3.0.0 versions are not supported by GzOGRE2. Please, use instead GzOGRENext."
)

if (NOT (DEFINED GzOGRE2_FIND_VERSION_MAJOR AND DEFINED
Copy link
Author

Choose a reason for hiding this comment

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

By using DEFINED we allow GzOGRE2_FIND_VERSION_MAJOR==0 or GzOGRE2_FIND_VERSION_MINOR==0 to not produce the warning "find_package(GzOGRE2) must be called with a VERSION argument with a minimum of major and minor version".

if (${VERSION_MAJOR} VERSION_GREATER "2")
message(WARNING ${MESSAGE_STRING})
message(WARNING
"Keep in mind FindGzOGRE2.cmake will be removed and FindGzOGRENext.cmake will support OGRE-Next 2.x.x versions in the next major release")
Copy link
Author

@sumir0 sumir0 Feb 19, 2025

Choose a reason for hiding this comment

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

I have specified the next major release will move FindGzOGRE2.cmake functionality to FindGzOGRENext.cmake. But it is, actually, open for discussion.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point. To avoid migrations for existing consumers of OGRE2 I would vote for maintaining the module as it is and just moving users to the new one under the new name with the expected move of things from 2.x to 3.x series.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the suggestion. Though, I don't know if the move from 2.x to 3.x series is planned in the next major release (gz-cmake5). Therefore, probably, I need to change the message to something like: Keep in mind FindGzOGRE2.cmake is planned to be removed and FindGzOGRENext.cmake is planned to support OGRE-Next>=2.0.0 soon. Or do you mean the move of things from 2.x to 3.x series is, actually, expected (planned) in the next major release (gz-cmake5)?

For now I will change the message. I assume, we are planning to remove FindGzOGRE2.cmake at some point in time. Sorry, I didn't clearly understand if it is the case.

@sumir0
Copy link
Author

sumir0 commented Feb 19, 2025

Please note, even though cmake/FindGzOGRENext.cmake is based on cmake/FindGzOGRE2.cmake it is, however, a reduced version. Essential parts that have been reduced are:

  • fix_*_jammy_bug macros were removed because Ubuntu Noble is already out (don't have resources right now to test if the bugs were fixed);
  • else branch of if (PkgConfig_FOUND) was removed because OGRE-Next has a .pc file;
  • else branch of if (${GZ_OGRE_NEXT_PROJECT_NAME}_FOUND) after gz_pkg_check_modules_quiet(${GZ_OGRE_NEXT_PROJECT_NAME} ${OGRE_NEXT_INSTALL_PATH} NO_CMAKE_ENVIRONMENT_PATH QUIET) was modified to not attempt to find OGRE-Next built from source because OGRE-Next built from source also has a .pc file and can be found by gz_pkg_check_modules_quiet.

@sumir0
Copy link
Author

sumir0 commented Feb 19, 2025

Memo for the future:
in case cmake/FindGzOGRE2.cmake will be removed examples/find_ogre2 should also be removed.

@sumir0
Copy link
Author

sumir0 commented Feb 19, 2025

I think the code is ready for the next iteration of the review. Please ask you questions if you have any. I will try to explain anything that is not clear.

if (${VERSION_MAJOR} VERSION_GREATER "2")
message(WARNING ${MESSAGE_STRING})
message(WARNING
"Keep in mind FindGzOGRE2.cmake will be removed and FindGzOGRENext.cmake will support OGRE-Next 2.x.x versions in the next major release")
Copy link
Contributor

Choose a reason for hiding this comment

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

Good point. To avoid migrations for existing consumers of OGRE2 I would vote for maintaining the module as it is and just moving users to the new one under the new name with the expected move of things from 2.x to 3.x series.

# check if ogre-next>=3.0.0 was found
check_possible_ogre_next_3_or_later(
OGRE2_VERSION_MAJOR
"OGRE-Next with major version ${OGRE2_VERSION_MAJOR} was found but OGRE-Next>=3.0.0 is not supported by GzOGRE2. Please, use instead GzOGRENext or specify a path to OGRE-Next<3.0.0 via an environment varialbe: PKG_CONFIG_PATH=<path-to-a-directory-containing-a-pkgconfig-file-for-OGRE-Next>."
Copy link
Contributor

Choose a reason for hiding this comment

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

or specify a path to OGRE-Next<3.0.0 via an environment varialbe: PKG_CONFIG_PATH=."

I think that we can skip this part about pkg-config if it is not related to CMake or gz-cmake.

Copy link
Contributor

@j-rivero j-rivero left a comment

Choose a reason for hiding this comment

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

CI gz_cmake-pr-clowin: requires #476 to work correctly.

Please note, even though cmake/FindGzOGRENext.cmake is based on cmake/FindGzOGRE2.cmake it is, however, a reduced version. Essential parts that have been reduced are:

Thanks that is great !

Sorry does not meant to approve it yet although we are close :)

@j-rivero j-rivero self-requested a review February 20, 2025 15:09
# for additional details see
# https://github.com/gazebosim/gz-cmake/pull/468#issuecomment-2662882691
message(WARNING
"Keep in mind FindGzOGRE2.cmake is planned to be removed and FindGzOGRENext.cmake is planned to support OGRE-Next>=2.0.0 soon")
Copy link
Author

Choose a reason for hiding this comment

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

I realized we, probably, should warn about the deprecation not only when GzOGRE2 used for OGRE-Next>=3.0.0 but in any case it is used. Therefore, I put this message here for now. But, as always, it is open for discussion.

Copy link
Author

Choose a reason for hiding this comment

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

I am duplicating the link to #468 (comment) here in case someone will not notice it in the conversation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry but maybe I was not clear with my previous comment on migration path and deprecation. I think that we should not modify the existing behavior for people using FindGzOGRE2 and leave the module without any warning unless users try to use it to get 3.x. The Module will be removed when we consider that look for OGRE 2.x has no longer any sense.

We can drop this warning.

Copy link
Contributor

@j-rivero j-rivero left a comment

Choose a reason for hiding this comment

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

Thanks so much @sumir0 for staying until the end.

I've confirmed that it works as expected compiling ogre3 from source. This is ready to go.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏛️ ionic Gazebo Ionic 🪵 jetty Gazebo Jetty
Projects
Status: In review
Development

Successfully merging this pull request may close these issues.

4 participants