Opengl refactor#3367
Draft
Lestropie wants to merge 2 commits into
Draft
Conversation
Replace the vendored, auto-generated gl_core_3_3 OpenGL loader with a thin gl:: compatibility facade backed by Qt's per-context QOpenGLFunctions_3_3_Core. The gl:: namespace, call syntax, and enumerant constants are preserved verbatim so existing call sites are unchanged, while function dispatch now binds to the OpenGL context current on the calling thread rather than a single process-global table. Qt5 and Qt6 are both supported, with the version-specific function acquisition isolated to a single helper. The Shader wrapper classes gain the same context-checking member as the other GL resource wrappers, and a debug trap asserts that GL calls occur with a current context. The obsolete loader files are removed entirely. Session prompts: 1. > Investigate the interface between MRtrix3 and OpenGL for the GUI commands mrview and shview. Cross-reference with accepted modern best practise for compilation of OpenGL 3.3+ programs in C++, such as window management, input device interfacing, and extension loaders. Summarise (without yet generating a comprehensive plan) the most suitable third-party libraries or code refactoring strategies to bring the MRtrix3 implementation up-to-date. 2. > Is it necessary to drop Qt5 support to move to QOpenGLFunctions_3_3_Core? Or is the rationale for dropping Qt5 support as part of refactoring merely opportunistic if refactoring is taking place? 3. > Elaborate on how replacing "gl_core_3_3" with "QOpenGLFunctions_3_3_Core" "makes function dispatch context-cprrect". Would this entirely obviate the use of GL::assert_context_is_current()? 4. > Draft a comprehensive plan for refactoring. Tabulate the mapping from existing functions and classes that are MRtrix-specific or from gl_core_3_3.* to QOpenGLFunctions_3_3_Core. Preserve Qt5 & Qt6 compatibility. Ensure that files cpp/gui/opengl/gl_core_3_3.* can be removed entirely. Preserve MRtrix assertions on context even if redundant. Audit usage of Context::Grab and makeCurrent() and ensure that it is included in all appropriate locations. Generated-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| assert(context != nullptr && // | ||
| "OpenGL call issued with no current context: missing " | ||
| "MR::GUI::GL::Context::Grab or QOpenGLWidget::makeCurrent()"); | ||
| thread_local QOpenGLContext *cached_context = nullptr; |
There was a problem hiding this comment.
warning: variable 'cached_context' provides global access to a non-const object; consider making the pointed-to data 'const' [cppcoreguidelines-avoid-non-const-global-variables]
thread_local QOpenGLContext *cached_context = nullptr;
^| "OpenGL call issued with no current context: missing " | ||
| "MR::GUI::GL::Context::Grab or QOpenGLWidget::makeCurrent()"); | ||
| thread_local QOpenGLContext *cached_context = nullptr; | ||
| thread_local QOpenGLFunctions_3_3_Core *cached_functions = nullptr; |
There was a problem hiding this comment.
warning: variable 'cached_functions' provides global access to a non-const object; consider making the pointed-to data 'const' [cppcoreguidelines-avoid-non-const-global-variables]
thread_local QOpenGLFunctions_3_3_Core *cached_functions = nullptr;
^| // QOpenGLFunctions_3_3_Core, so &QOpenGLFunctions_3_3_Core::gl<Name> is | ||
| // unambiguous. To add a function, append a single MRTRIX_GL_FN(<Name>) line; | ||
| // if Qt ever overloads one of these the build fails loudly here. | ||
| #define MRTRIX_GL_FN(name) \ |
There was a problem hiding this comment.
warning: function-like macro 'MRTRIX_GL_FN' used; consider a 'constexpr' template function [cppcoreguidelines-macro-usage]
#define MRTRIX_GL_FN(name) \
^| // unambiguous. To add a function, append a single MRTRIX_GL_FN(<Name>) line; | ||
| // if Qt ever overloads one of these the build fails loudly here. | ||
| #define MRTRIX_GL_FN(name) \ | ||
| constexpr auto name = &::MR::GUI::GL::detail::Forward<decltype(&QOpenGLFunctions_3_3_Core::gl##name), \ |
There was a problem hiding this comment.
warning: macro argument should be enclosed in parentheses [bugprone-macro-parentheses]
Suggested change
| constexpr auto name = &::MR::GUI::GL::detail::Forward<decltype(&QOpenGLFunctions_3_3_Core::gl##name), \ | |
| constexpr auto (name) = &::MR::GUI::GL::detail::Forward<decltype(&QOpenGLFunctions_3_3_Core::gl##name), \ |
|
|
||
| protected: | ||
| GLuint index_; | ||
| Context::Checker check_context; |
There was a problem hiding this comment.
warning: member variable 'check_context' has protected visibility [cppcoreguidelines-non-private-member-variables-in-classes]
Context::Checker check_context;
^|
|
||
| protected: | ||
| GLuint index_; | ||
| Context::Checker check_context; |
There was a problem hiding this comment.
warning: member variable 'check_context' has protected visibility [cppcoreguidelines-non-private-member-variables-in-classes]
Context::Checker check_context;
^
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I was trying to deal with issues arising from
cpp/gui/opengl/gl_core_3_3.(cpp|h)due to name violations andclang-tidyparsing issues (#3349), and I recalled that these files have an unusual (ambiguous?) origin story and have been the topic of various discussions (though wasn't able to find anything suitable to link). Certainly the topic of mass refactoring has come up, eg. to Vulcan.I thought I'd test the prospect of something not quite so intrusive, asking Claude about modern best practise for OpenGL in C++. Its primary criticism was that given the (justified) use of Qt, there should be no need to use a bespoke loader:
QOpenGLFunctions_3_3_Coreshould possess all of the required functionality. Moreover, we've historically fought a continuous fight regarding grabbing and ensuring consistency in the relevant context when dealing with multiple GL windows / widgets; this is supposedly intrinsically addressed by use of the Qt loader, since the context is stored per object and therefore embedded into each function call.Posting here as draft PR because it's far from the top priority right now, but wanted it recorded so can come back to it later. Also so that if any
mrviewfault is reproduced this branch can be tested. Compiles and runs on both Qt5 and Qt6 for me, but haven't tested GUI extensively.