-
Notifications
You must be signed in to change notification settings - Fork 311
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
[SofaHAPI] Fixes for HAPI #1189
Conversation
It's good for me, @guparan could you just check the CMake part? |
endif() | ||
# endif() | ||
|
||
configure_file(config.h.in "${CMAKE_BINARY_DIR}/include/SofaHAPI/config.h") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this plugin calls sofa_create_package you can simply add config.h.in to HEADER_FILES and it will be configured automatically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok thanks, I didn't know that. Done that and au passage I also upgraded the call to the new function sofa_generate_package
5082d95
|
||
endif() | ||
# endif() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's remove this entire block 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done a23dbb5
cmake/Modules/FindHAPI.cmake
Outdated
@@ -11,6 +11,7 @@ GET_FILENAME_COMPONENT(module_file_path ${CMAKE_CURRENT_LIST_FILE} PATH ) | |||
# Look for the header file. | |||
FIND_PATH(HAPI_INCLUDE_DIR NAMES HAPI/HAPI.h | |||
PATHS $ENV{H3D_ROOT}/../HAPI/include | |||
$ENV{hapi_ROOT}/include |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why an environment variable and not a CMake variable?
I think it would be more user friendly to have HAPI_ROOT in CMake cache.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know. Here is the problem, I'm using cmake 3.15 and they have a new policy for the <package>_ROOT
variables:
CMP0074
-------
``find_package()`` uses ``<PackageName>_ROOT`` variables.
In CMake 3.12 and above the ``find_package(<PackageName>)`` command now
searches prefixes specified by the ``<PackageName>_ROOT`` CMake
variable and the ``<PackageName>_ROOT`` environment variable.
Package roots are maintained as a stack so nested calls to all ``find_*``
commands inside find modules also search the roots as prefixes. This policy
provides compatibility with projects that have not been updated to avoid using
``<PackageName>_ROOT`` variables for other purposes.
The ``OLD`` behavior for this policy is to ignore ``<PackageName>_ROOT``
variables. The ``NEW`` behavior for this policy is to use
``<PackageName>_ROOT`` variables.
This policy was introduced in CMake version 3.12. CMake version
3.15.0 warns when the policy is not set and uses ``OLD`` behavior.
Use the ``cmake_policy()`` command to set it to ``OLD`` or ``NEW``
explicitly.
.. note::
The ``OLD`` behavior of a policy is
``deprecated by definition``
and may be removed in a future version of CMake.
So if I use HAPI_ROOT
with cmake default settings it will be ignored.
Is it ok for you if I had to the CMakeLists.txt of HAPI this:
if(${CMAKE_VERSION} VERSION_GREATER "3.11.4")
message(STATUS "policy")
cmake_policy(SET CMP0074 NEW)
endif()
It worked for me and it should stay local to the HAPI module as its CMakeLists is a separate project. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with your suggestion 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@simogasp your suggestion looks great, let us know when you can complete this! cheers!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the changes are ready, sorry for the delay
Hey @simogasp thank you again for your contribution on the HAPI plugin! Do not hesitate to join us in Paris to present your work related to simulations at the SOFA Week 2019! Even more if you have some haptics demo! |
Hi, sorry, I've been away from my pc for some days, I'll finalize the PR tomorrow. |
Here's a bunch of fixes for the HAPI plugin:
config.h
was not generated --> added automatic generation to the cmake and fix the template to use cmake variable replacementthere is some broken code in CMakeLists, not sure what was used for but it's not even a valid cmake code (broken if clause). For the moment I just commented out, I'm open to advices...
updated the code with recent member renaming in main sofa code (
indice
-->d_indice
)fix for
findH3D.cmake
to find the module without relying on non-default installation path (i.e. if you install H3D into a given directory, that is the root path from whichinclude
andlib
can be easily found )fixes for
findHAPI.cmake
--> usinghapi_ROOT
to easily find the module (instead of relying on the mutual position of HAPI and H3DUtils)This PR:
Reviewers will merge only if all these checks are true.