PhysX icon indicating copy to clipboard operation
PhysX copied to clipboard

Add new CMakeLists.txt to provide PhysXConfig.cmake

Open phcerdan opened this issue 4 years ago • 19 comments

This allows to find_package(PhysX) and facilitate integration in third party CMake projects.

  • Provide PhysXConfig.cmake and exported targets file for build and install tree.
  • Other projects can use find_package(PhysX) where PhysX_DIR can be a build tree or an install tree.
  • The implementation only adds two new CMakeLists.txt that do not collide with the existing build procedure of Nvidia. Instead of using python and the generate_projects scripts, now it solely uses CMake in a standard way.
  • This allows PhysX to be used with modern standards of CMake, making it compatible with FetchContent (add_subdirectory) and ExternalProject (find_package) commands.
  • Snippets and Samples have not been integrated into the new build procedure.
  • But added a simpler project to show find_package(PhysX) usage.
  • The original build procedure is maintained compatible for easier integration with future upstream changes from NVidia.

How to build:

mkdir PhysX;
git clone https://github.com/phcerdan/PhysX src
mkdir build; cd build;
cmake -GNinja -DCMAKE_BUILD_TYPE:STRING=release -DCMAKE_INSTALL_PREFIX:PATH=/tmp/physx ../src
ninja install

Example project using PhysX (example added)

find_package(PhysX REQUIRED)
add_executable(main main.cpp)
target_link_libraries(main PRIVATE PhysX::PhysXCommon PhysX::PhysXExtensions)

You can also use FetchContent, or ExternalProjects for handling PhysX automatically.

When building your project, just provide PhysX_DIR to where the PhysXConfig.cmake is located (it could be from a build or an install tree)

cmake -GNinja -DCMAKE_BUILD_TYPE:STRING=RelWithDebInfo -DPhysX_DIR:PATH=/tmp/physx/PhysX/bin/cmake/physx ../src

Fix #208

phcerdan avatar Nov 12 '19 19:11 phcerdan

If anybody wants to use CMake FetchContent or external projects, I have set the default branch in my fork to be this PR: https://github.com/phcerdan/PhysX

phcerdan avatar Nov 12 '19 19:11 phcerdan

Another thing, is PhysX_DIR required also to find the project from the install tree? If yes, probable the most idiomatic way to handle this case would be adding the install prefix to CMAKE_INSTALL_PREFIX.

diegoferigo avatar Nov 13 '19 08:11 diegoferigo

Another thing, is PhysX_DIR required also to find the project from the install tree? If yes, probable the most idiomatic way to handle this case would be adding the install prefix to CMAKE_INSTALL_PREFIX.

I don't think that's the CMake idiomatic way. PhysX_DIR is set by the consumer to point wherever PhysXConfig.cmake is, in the build or install tree.

  • In the install tree is in: PhysX/bin/cmake/physx/PhysXConfig.cmake. Having the install config file in the bin/cmake (or lib/cmake) folder is idiomatic in CMake.

  • In the build folder is in the top source folder named: sdk_source_bin (name is maintained from upstream).

phcerdan avatar Nov 13 '19 09:11 phcerdan

Another thing, is PhysX_DIR required also to find the project from the install tree? If yes, probable the most idiomatic way to handle this case would be adding the install prefix to CMAKE_INSTALL_PREFIX.

I don't think that's the CMake idiomatic way. PhysX_DIR is set by the consumer to point wherever PhysXConfig.cmake is, in the build or install tree. - In the install tree is in: PhysX/bin/cmake/physx/PhysXConfig.cmake. Having the install config file in the bin/cmake (or lib/cmake) folder is idiomatic in CMake. - In the build folder is in the top source folder named: sdk_source_bin (name is maintained from upstream).

Sorry, I didn't notice that the installed tree had the additional PhysX folder. Without this scope, my comment would apply and as you said, it would be the most idiomatic way to handle install trees.

diegoferigo avatar Nov 13 '19 21:11 diegoferigo

The alias is missing then: try without the PhysX:: namespace? Sorry, I am away from the computer to check it properly.

EDIT: Your test project is untestable in Linux.

phcerdan avatar Aug 07 '20 18:08 phcerdan

The alias is missing then: try without the PhysX:: namespace?

That was it! Thanks for sticking with me on this one, this is great. :D

Moving forwards, is this an error in the docs, or the CMake files in this PR? I still can't figure out where the namespace is meant to come from, or even these names.

alanjfs avatar Aug 08 '20 07:08 alanjfs

There was one thing I ran into that maybe could be handled by this PR.

cmake .. -DCMAKE_BUILD_TYPE=Release
...
Error copying directory /PhysX/physx/bin/linux.clang/Release to /some/install/path

Presumably because I passed Release rather than release.

cmake .. -DCMAKE_BUILD_TYPE=release
...
Success

Bit of a tricky one to debug, and maybe not something handled here?

alanjfs avatar Aug 08 '20 07:08 alanjfs

Chatting with the author of another project providing a CMake module, he says missing namespaces is a common CMake-thing, and pointed at his workaround:

add_library(Magnum::MeshTools ALIAS MagnumMeshTools)

https://github.com/mosra/magnum/blob/bd44e2ab7cc0a00a4ad3d4fc0f7bcf24cd05c697/src/Magnum/MeshTools/CMakeLists.txt#L160-L161

I figure, it's either that, or update the documentation to not include the namespace?

alanjfs avatar Aug 08 '20 08:08 alanjfs

I have added an alias for all the targets with the namespace PhysX:: on it.

phcerdan avatar Aug 08 '20 09:08 phcerdan

Presumably because I passed Release rather than release.

I have commented on this up in this discussion: https://github.com/NVIDIAGameWorks/PhysX/pull/222#discussion_r345645091

phcerdan avatar Aug 08 '20 09:08 phcerdan

I have commented on this up in this discussion: #222 (comment)

Thanks, that makes sense.

I wonder, would it not be possible to convert whatever was passed by the user to lowercase, such that at least Release and Debug would work as expected? It looks like the failure happens during a CMake copy.

alanjfs avatar Aug 09 '20 17:08 alanjfs

I'm back! :)

Regarding the debug, profile, checked, and release build configurations of PhysX, I can't figure out how to "map" PhysX's checked configuration to my CMake project's Release configuration.

From what I gather, the only way to influence how PhysX is build when add_subdirectory(dir/to/physx) is to set CMAKE_BUILD_TYPE=checked . That would get PhysX building, but then break the parent project which doesn't know about checked.

What I'd like to do is..

if(CMAKE_BUILD_TYPE STREQUAL "Release")
  set(PHYSX_BUILD_TYPE "checked")
else()
  set(PHYSX_BUILD_TYPE "profile")
endif()

Is that possible?

alanjfs avatar Aug 24 '20 13:08 alanjfs

@alanjfs I would love to, I asked a few months ago this question in the CMake discourse, but it wasn't clear to me. https://discourse.cmake.org/t/mapping-cmake-build-type-and-cmake-configuration-types-between-project-subdirectories/192

I hacked a way to restore the original CMAKE_BUILD_TYPE for your add_subdirectory case, please test. Please remember that this is somehow of a quimera to bring CMake here. There are old CMakeLists.txt files (probably contributed by a third-party), that I am guessing are untouchable, and the new CMakeLists to bring all the goodies to find_package(physx) and easier interaction with the C++ ecosystem.

A complete new rewrite of all the CMakeLists.txt files would be way easier than the current mixing between new and old CMakeLists.txt. But, hey, let's try. No word of any maintainer, for good or bad so far.

I would be happy to modernize all the CMakeLists.txt, but I have no idea about NVIDIA constraints about not breaking code with 3rd parties. This PR tries to not break anything and bring utilities, but for example, the non-standard CMAKE_BUILD_TYPES are a pain.

Please try the last commit that tries to hack on it. Remember that using find_package should work (instead of add_subdirectory in your project).

phcerdan avatar Aug 24 '20 14:08 phcerdan

Thanks for this, I'll try!

I'm not sure how to actually determine whether the right config is built though, other than by looking at whether the library file sizes differ :S Do you have any idea?

Remember that using find_package should work (instead of add_subdirectory in your project).

find_package does work, but I haven't figured out how to build PhysX alongside my project using find_package. It seems like add_subdirectory is the only option?

alanjfs avatar Aug 25 '20 07:08 alanjfs

You can use ExternalProject_Add to build all your dependencies and your own project, it's what in CMake is called a Superbuild. This Superbuild project builds all your dependencies, and lastly your project also with ExternalProject_Add. Your project would need to worry on building only itself, using find_package to find the external dependencies (that you just built/managed in the Superproject). If you think about it, add_subdirectory is like your project is owning the dependencies, it doesn't allow to find libraries in your system.

ExternalProject_Add has the mapping ability with: CMAKE_MAP_IMPORTED_CONFIG_<CONFIG> that would solve this non-standard CMAKE_BUILD_TYPE of Physx. Nevertheless, give the last commit a try and let me know.

phcerdan avatar Aug 25 '20 07:08 phcerdan

I hadn't heard of ExternalProject_Add before, that does sound like the right approach, I will experiment. Thanks a lot @phcerdan!

alanjfs avatar Aug 25 '20 07:08 alanjfs

I'm assuming this never got integrated ?

xamado avatar Jul 08 '22 22:07 xamado

I'm assuming this never got integrated ?

You are right :eyes:

phcerdan avatar Jul 08 '22 22:07 phcerdan

I'm assuming this never got integrated ?

You are right 👀

:disappointed:

xamado avatar Jul 09 '22 00:07 xamado