2.6 debug #1040
2.6 debug #1040
Conversation
| add_executable(Natron ${Natron_SOURCES}) | ||
| if(WIN32) |
There was a problem hiding this comment.
I can't review the WIN32 stuff, @acolwell can you give it a look?
There was a problem hiding this comment.
Also it would be nice to have two separate PRs (one for the CMake stuff, one for the OpenGL stuff)
There was a problem hiding this comment.
I agree. The CMake stuff should be in its own PR so it can be landed quickly. I mentioned this in the previous incarnation of these changes.
There was a problem hiding this comment.
When doing changes to several functional components (here, build system vs OpenGL), Please do several PRs.
However, I think the OpenGL changes will just be one line, because the extra makeCurrent calls can be removed, according to the doc, so I'm OK to keep it here. I would just like to have a proper reference to an explanation for AA_ShareOpenGLContexts.
I found this one, but there may be a better one: githubuser0xFFFF/Qt-Advanced-Docking-System#397 (comment)
| @@ -164,6 +164,7 @@ CurveWidget::initializeGL() | |||
| // always running in the main thread | |||
| assert( qApp && qApp->thread() == QThread::currentThread() ); | |||
| appPTR->initializeOpenGLFunctionsOnce(); | |||
| makeCurrent(); | |||
There was a problem hiding this comment.
I don't understand why you call makeCurrent here.
From the doc: There is no need to call makeCurrent() because this has already been done when this function is called.
There was a problem hiding this comment.
So these calls are needed because initializeOpenGLFunctionsOnce() can end up creating a context, making it current, and then does not restore the old context. I believe we should fix that behavior instead of papering over it here and below. I started looking into that but haven't got a full solution yet. I also am a little skeptical about the need to init all the GL functions in these various places. ISTM that this should happen very early in startup instead of here, but I'm not as familiar with this code yet.
I suspect making something like my ScopedGLContext on Windows cross-platform would make resolving this and any other similar situation we encounter simpler.
| @@ -3171,6 +3171,8 @@ DopeSheetView::initializeGL() | |||
| return; | |||
| } | |||
|
|
|||
| makeCurrent(); | |||
There was a problem hiding this comment.
same as above, no need to call makeCurrent, according to the doc
| @@ -323,6 +325,7 @@ AppManager::loadFromArgs(const CLArgs& cl) | |||
| _imp->renderingContextPool.reset( new GPUContextPool() ); | |||
| initializeOpenGLFunctionsOnce(true); | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
Please avoid formatting changes in PRs (or make a formatting-only PR)
| @@ -315,6 +315,8 @@ AppManager::loadFromArgs(const CLArgs& cl) | |||
| std::cout << "argv[" << i << "] = " << StrUtils::utf16_to_utf8( std::wstring(_imp->commandLineArgsWide[i]) ) << std::endl; | |||
| } | |||
| #endif | |||
| // This should fix GL widgets when undocked | |||
There was a problem hiding this comment.
Nice to add a comment, but it would be nice to have a link to the "why". I found this one, maybe you have a better one?
| @@ -15,7 +15,7 @@ def list_typesystem_cpp_sources(typesystem, out): | |||
| sources = [f"{package.lower()}_module_wrapper.cpp"] | |||
| sources.extend([f"{typename.lower()}_wrapper.cpp" for typename in types]) | |||
|
|
|||
| return [os.path.normpath(os.path.join(out, package, f)) for f in sources] | |||
| return [os.path.normpath(os.path.join(out, package, f)).replace("\\", "/") for f in sources] | |||
There was a problem hiding this comment.
windows-specific. @acolwell can you check?
There was a problem hiding this comment.
This one confuses me. I don't quite understand why we are getting native path separators all of a sudden. While the change is trivial and probably ok, it would be nice to understand why this is needed now when it wasn't before.
So I believe using AA_ShareOpenGLContext masks the issue and potentially might cause resources to hang around longer than we might want. Take a look at https://doc.qt.io/qt-5/qopenglwidget.html#resource-initialization-and-cleanup the 2 notes in that section and the paragraph after them seem particularly important. I believe a more robust solution would be to implement the cleanup logic mentioned in that section to all classes that are dockable. It would be a more invasive change, but I think it would be closer to the behavior Qt expects to be implemented. I've been meaning to create a prototype but haven't gotten around to it yet. |
Thanks for submitting a pull request! Please provide enough information so that others can review your pull request. Additionally, make sure you've done all of these things:
PR Description
What type of PR is this? (Check one of the boxes below)
What does this pull request do?
Replace last PR
Added libraries and Python modules to cmake install directory
Fixes OpenGL context when a panel is undocked. Redocking is still an issue, though
Have you tested your changes (if applicable)? If so, how?
Yes