-
Notifications
You must be signed in to change notification settings - Fork 317
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
[SofaFramework] Isolate OpenGL code into a single module (Sofa.GL) #1649
Conversation
[ci-build][with-all-tests] |
{ | ||
|
||
class SOFA_HELPER_API Capture | ||
class SOFA_SOFA_GL_API Capture |
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.
normal SOFA_SOFA_*?
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.
40 times
add_library(${PROJECT_NAME} SHARED ${HEADER_FILES} ${SOURCE_FILES}) | ||
target_link_libraries(${PROJECT_NAME} PUBLIC SofaHelper SofaDefaultType) | ||
|
||
target_compile_definitions(${PROJECT_NAME} PRIVATE SOFA_BUILD_SOFA_GL) # To remove once sofa_add_targets_to_package remove the dot in the generated definition |
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.
@guparan just not to be forgotten for the update of sofa_add_targets_to_package
@@ -0,0 +1,106 @@ | |||
cmake_minimum_required(VERSION 3.12) | |||
project(Sofa.GL LANGUAGES CXX) |
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.
shouldn't we define versions?
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.
Package version follows Sofa's version; you can see that this is set in the macro with (... PACKAGE_VERSION ${Sofa_VERSION}... )
[ci-build][force-full-build] |
…compat from helper::gl
[ci-build][with-all-tests] |
[ci-build][force-full-build][with-all-tests] |
[ci-build][force-full-build][with-all-tests] |
foreach(TARGET ${SOFAFRAMEWORK_TARGETS}) | ||
message("Adding ${TARGET}") |
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.
Was this for debugging purposes?
|
||
// |
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.
Remove?
MacOS CI stalled for some reason. But the CI passed just before and the subsequent changes were minimal, so it should be safe to merge. |
Fix #1645 (needs #1646 , #1644 , #1650 )
TL;DR
OpenGL code in the core is now enclosed in the new module Sofa.GL.
If you need to call OpenGL code, you will need to find_package() it and add it to target_link_library().
Description
OpenGL is spread a bit everywhere in the core, and it is difficult to bypass it if you dont need it (typically for batch mode or headless system).
The macro SOFA_NO_OPENGL is a really Quick and Dirty solution to this problem.
For example, people include sofa/helper/system/gl.h (or any gl-related header), even without needing OpenGL actually.
And in this header, SOFA_NO_OPENGL does include (or not) the good gl.h header according to your OS. So if you enable this header, you still include gl.h, which does nothing.
This is quite nonsensical.
Moreover, if the macro is not well handled in CMake, you may have some weird things happening.
Solution
This PR regroups all OpenGL related code in the core in a single package, needing only SofaHelper and SofaDefaulttype.
If you wish to be able to call OpenGL things, just find_package() it and link it to your library
You can even conditionnally test if OpenGL is present or not with
if needed (to compile specific components in your module for example).
And even more, if you want to compile specific parts of your code with OpenGL (not a good idea but still), you can use this like an external dependency.
Example with ColourPickingVisitor in SofaGuiCommon.
CMakeLists.txt:
config.h.in
ColourPickingVisitor.cpp
Compatibility with actual code and breaking things
This new module follows the new targeted architecture for Sofa(NG), hence the Sofa.GL syntax.
All classes/functions are now in the namespace sofa::gl and in the folder sofa/gl; e.g Texture,
sofa/gl/Texture.h
To avoid breaking (a lot of) code, a compatibility layer has been implemented, so the existing code still works:
and will either:
So for 99.999% of code, all you have to do is to link against Sofa.GL (or even nothing if you code is already linking against modules linking now against Sofa.GL like SofaOpenGlVisual)
Real breaking code
ONE function has been removed from core::visual::VisualParam :
helper::gl::Framebuffer getFramebuffer()
which was used to get the current bound framebuffer in OpenGL (getting a Sofa utility class Framebuffer).
Two reasons why it has been removed:
for example.
Notes
The compilation of Sofa.GL is itself enabled with the CMake variable SOFA_NO_OPENGL (weird name but I wanted to keep the same "reasoning" for now)
Use of SOFA_NO_OPENGL macro in the code has been removed from the Sofa codebase only, not the plugins (in a future PR)
😅
By submitting this pull request, I acknowledge that
I have read, understand, and agree SOFA Developer Certificate of Origin (DCO).
Reviewers will merge this pull-request only if