2.6 debug #1040

cedricp · 2025-04-25T14:14:10Z

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:

I've followed the contributing guidelines to the best of my understanding
I've read and understood the contributing guidelines
I've formatted my code according to Natron's code style
I've searched the pull requests tracker to ensure that this PR is not a duplicate

PR Description

What type of PR is this? (Check one of the boxes below)

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Improvement (non-breaking change which does not add functionality nor fixes a bug but improves Natron in some way)
Breaking change (fix or feature that would cause existing functionality to change)
My change requires a change to the documentation
- I have updated the documentation accordingly

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

devernay · 2025-05-20T08:15:53Z

App/CMakeLists.txt

-    list(APPEND Natron_SOURCES ../Natron.rc)
-endif()
 add_executable(Natron ${Natron_SOURCES})
+if(WIN32)


I can't review the WIN32 stuff, @acolwell can you give it a look?

Also it would be nice to have two separate PRs (one for the CMake stuff, one for the OpenGL stuff)

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.

devernay

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)

devernay · 2025-05-20T08:32:56Z

Gui/CurveWidget.cpp

    // always running in the main thread
    assert( qApp && qApp->thread() == QThread::currentThread() );
    appPTR->initializeOpenGLFunctionsOnce();
+    makeCurrent();


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.

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.

I agree, I thought it was a good idea to push what I found to help maintainers debug the code.

devernay · 2025-05-20T08:33:27Z

Gui/DopeSheetView.cpp

        return;
    }

+    makeCurrent();


same as above, no need to call makeCurrent, according to the doc

You're right, I think it's something I left while debugging

devernay · 2025-05-20T08:34:13Z

Engine/AppManager.cpp

    _imp->renderingContextPool.reset( new GPUContextPool() );
    initializeOpenGLFunctionsOnce(true);

+


Please avoid formatting changes in PRs (or make a formatting-only PR)

devernay · 2025-05-20T08:36:28Z

Engine/AppManager.cpp

        std::cout << "argv[" << i << "] = " << StrUtils::utf16_to_utf8( std::wstring(_imp->commandLineArgsWide[i]) ) << std::endl;
    }
 #endif
+    // This should fix GL widgets when undocked


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?

githubuser0xFFFF/Qt-Advanced-Docking-System#397 (comment)

devernay · 2025-05-20T08:39:25Z

tools/utils/sourceList.py

    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]


windows-specific. @acolwell can you check?

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.

This is the error I get without it :

[build] CMake Error at Engine/CMakeLists.txt:69 (add_library): [build] Syntax error in cmake code when parsing string [build] [build] C:\Users\cedric\Documents\sources\Natron\build_vscode\Engine\Qt5\NatronEngine\natronengine_module_wrapper.cpp [build] [build] Invalid character escape '\U'.

acolwell · 2025-05-20T17:23:35Z

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)

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.

cedricp · 2025-05-30T08:51:08Z

I made some tests about the [un]docking issue, I tried to remove the Qt patch as told here :
githubuser0xFFFF/Qt-Advanced-Docking-System#294 (comment)
but that did not work. I spent some hours to debug it without success. An interesting thig is this message in the terminal when I dock the floating widget in the main app one :

qt.qpa.backingstore: composeAndFlush: makeCurrent() failed
qt.qpa.backingstore: composeAndFlush: makeCurrent() failed
qt.qpa.backingstore: composeAndFlush: makeCurrent() failed
qt.qpa.backingstore: composeAndFlush: makeCurrent() failed
....

cedricp · 2025-05-30T10:17:51Z

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)

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.

I tried to connect the context "aboutToBeDestroyed" to a cleanup slot as explained in Qt doc, but the cleanup slot is never called when I dock/undock.... So I guess it's not going to solve anything

EDIT : "aboutToDestroy" is triggered but I found a way to solve the issue by just adding

QCoreApplication::setAttribute(Qt::AA_DontCreateNativeWidgetSiblings);

after

QCoreApplication::setAttribute(Qt::AA_ShareOpenGLContexts);

I don't know if it's a good fix or not, but it works. Tested on Win10 and Win11
I can now use Natron flawlessly :)

rodlie · 2025-06-03T08:04:39Z

App/CMakeLists.txt

 install(FILES ../Gui/Resources/Images/natronProjectIcon_linux.png
    DESTINATION "${CMAKE_INSTALL_DATADIR}/pixmaps")
+
+IF(WIN32 AND DEPLOYQT_FOUND)


Don't force this, make it optional.

@cedricp

Fixes Windows specific code in CMake files by using the proper platform variable for Windows. This is a slightly modified version of @cedricp changes proposed in NatronGitHub#1040.

@cedricp

Fixes Windows specific code in CMake files by using the proper platform variable for Windows. This is a slightly modified version of @cedricp changes proposed in #1040.

cedricp added 2 commits April 25, 2025 16:07

Build libs install

28f1264

Attempt to fix undocking issue

ea2e028

devernay reviewed May 20, 2025

View reviewed changes

devernay requested changes May 20, 2025

View reviewed changes

devernay requested a review from acolwell May 20, 2025 08:41

Add viewer dithering

0615c75

rodlie reviewed Jun 3, 2025

View reviewed changes

acolwell mentioned this pull request Jul 2, 2025

Replace WINDOWS with WIN32 in CMakeLists.txt #1059

Merged

10 tasks

		_imp->renderingContextPool.reset( new GPUContextPool() );
		initializeOpenGLFunctionsOnce(true);

2.6 debug #1040

Are you sure you want to change the base?

2.6 debug #1040

Uh oh!

Conversation

cedricp commented Apr 25, 2025

PR Description

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devernay left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

acolwell commented May 20, 2025

Uh oh!

cedricp commented May 30, 2025

Uh oh!

cedricp commented May 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cedricp commented May 30, 2025 •

edited

Loading