Skip to content
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

Reimplement Qt3DWindow to work with Qt Webengine #53255

Merged
merged 21 commits into from
Dec 19, 2023
Merged

Conversation

jbp35
Copy link
Contributor

@jbp35 jbp35 commented May 28, 2023

This is a follow-up of: #47873

Qt3DWindow is currently not compatible with Qt Webengine. As mentioned in QT doc, when using Qt::AA_ShareOpenGLContexts (which is required for Qt Webengine) it is strongly recommended to set the default QSurface format before the construction of the Qapplication. Otherwise, the format will not be applied to the global share context and therefore issues may arise with context sharing afterwards.

https://doc.qt.io/qt-6/qsurfaceformat.html#setDefaultFormat

However, Qt3DWindow sets the default QSurface format during its initialization which is causing the following warning message:
image

This PR is a tentative to work around this issue by reimplementing Qt3dWindow. The new implementation lets qgis main.cpp configure the default Qsurface format.

Added benefit of this approach is that it should also make it possible to overlay widgets on top of the 3d viewport which is currently not possible using Qt3dWindow.

@github-actions github-actions bot added this to the 3.32.0 milestone May 28, 2023
Copy link
Member

@wonder-sk wonder-sk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @jbp35 - thanks for the pull request! I have been looking into these issues in the last couple of days, and I am have been heading towards a similar solution but I am still testing various options. I have left a couple of comments.

Also a bunch of general questions:

  • what operating system(s) have you been testing on?
  • what Qt versions have you tried? Qt5 only, or also Qt6?
  • what do you test to verify the PR has fixed the issues? (e.g. open a qgis 3d window, then open a webengine window, and they both work with no crash?)

Are you doing this to get PyQt5.QtWebEngineWidgets working? Because for me, with similar implementation, import of this PyQt5 module would still fail with an import error:

QtWebEngineWidgets must be imported before a QCoreApplication instance is created

And it seems that getting this python module working may be a bit more tricky, because in QGIS, python gets initialized much later than when QApplication gets instantiated.

src/3d/qgs3dwindow.cpp Show resolved Hide resolved
src/3d/qgs3dwindow.cpp Outdated Show resolved Hide resolved
src/3d/qgs3dwindow.cpp Show resolved Hide resolved
src/3d/qgs3dwindow.cpp Show resolved Hide resolved
src/3d/qgsoffscreen3dengine.cpp Outdated Show resolved Hide resolved
src/3d/qgsoffscreen3dengine.cpp Show resolved Hide resolved
src/app/main.cpp Show resolved Hide resolved
@jbp35
Copy link
Contributor Author

jbp35 commented May 29, 2023

Hi, thanks for the comments! Regarding testing, I just wanted to share the overall approach before to spend too much time on it, so there is still a lot more to be done. I only tested on windows with QT5 so far, using a python script to create a QT WebEngine window.

In order to get PyQt5.QtWebEngineWidgets to work, you have to update manually PyQtWebEngine (only version 5.15.6 will work). This version does not exit if shared opengl context is enabled.
You may also have to upgrade python3-pyqt5-sip if you get another error message “RuntimeError: the sip module implements API v12.0 to v12.8 but the PyQt5.QtCore module requires API v12.9” after upgrading PyQtWebEngine.

Here is the python script I am using to create the QtWebEngine window:

from PyQt5.QtWebEngineWidgets import QWebEngineView
from qgis.PyQt import QtWidgets
from qgis.PyQt.QtCore import QUrl

dlg = QtWidgets.QDialog()
view = QWebEngineView()
layout = QtWidgets.QHBoxLayout()
layout.setContentsMargins(0, 0, 0, 0)
layout.addWidget(view)
dlg.setLayout(layout)

url = QUrl("https://www.whatsmybrowser.org/")
view.load(url)
view.show()

dlg.show()

@wonder-sk
Copy link
Member

Regarding testing, I just wanted to share the overall approach before to spend too much time on it, so there is still a lot more to be done.

Ah ok, cool! The overall approach seems fine to me, so yeah, let's try to address the ugly details :-)

In order to get PyQt5.QtWebEngineWidgets to work, you have to update manually PyQtWebEngine (only version 5.15.6 will work). This version does not exit if shared opengl context is enabled.

I see... I am on ubuntu 22.04, and I can see now that my python3-pyqt5 is at 5.15.6, but python3-pyqt5.qtwebengine is versioned separately and it is at 5.15.5. And indeed, there should be a fix related to AA_ShareOpenGLContexts in the latest version. Great to see this got addressed.

For the time being I will stick to the existing PyQt5.QtWebEngineWidgets from my system, and use C++ code to test QtWebEngine inside QGIS... I am mainly concerned about having QGIS 3D running smoothly, even if QtWebEngine is used (for Qt6 we will probably need to move to QtWebEngine and finally ditch QtWebKit)

@jbp35 jbp35 requested a review from wonder-sk June 5, 2023 17:40
@github-actions github-actions bot added the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Jun 20, 2023
@github-actions github-actions bot closed this Jun 27, 2023
@wonder-sk wonder-sk reopened this Jul 6, 2023
@github-actions github-actions bot removed the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Jul 6, 2023
@github-actions github-actions bot added the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Jul 21, 2023
@github-actions github-actions bot closed this Jul 29, 2023
@wonder-sk wonder-sk reopened this Jul 30, 2023
@wonder-sk wonder-sk removed the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Oct 9, 2023
@qgis qgis deleted a comment from github-actions bot Oct 20, 2023
@qgis qgis deleted a comment from github-actions bot Oct 20, 2023
@qgis qgis deleted a comment from github-actions bot Oct 20, 2023
@qgis qgis deleted a comment from github-actions bot Oct 20, 2023
@nyalldawson nyalldawson added Frozen Feature freeze - Do not merge! and removed Freeze Exempt Feature Freeze exemption granted labels Oct 23, 2023
Copy link

github-actions bot commented Dec 17, 2023

🪟 Windows builds ready!

Windows builds of this PR are available for testing here. Debug symbols for this build are available here.

(Built from commit 8952595)

Copy link

Tests failed for Qt 6

One or more tests failed using the build from commit db47e93

pointcloud_3d_singlecolor (testPointCloudSingleColor)

pointcloud_3d_singlecolor

Test failed at testPointCloudSingleColor at tests/src/3d/testqgspointcloud3drendering.cpp:272

pointcloud_3d_colorramp (testPointCloudAttributeByRamp)

pointcloud_3d_colorramp

Test failed at testPointCloudAttributeByRamp at tests/src/3d/testqgspointcloud3drendering.cpp:312

pointcloud_3d_classification (testPointCloudClassification)

pointcloud_3d_classification

Test failed at testPointCloudClassification at tests/src/3d/testqgspointcloud3drendering.cpp:349

The full test report (included comparison of rendered vs expected images) can be found here.

Further documentation on the QGIS test infrastructure can be found in the Developer's Guide.

@wonder-sk
Copy link
Member

Finally managed to test this with both Qt5 and Qt6 on linux - and I can confirm things seems to be working fine even if there is a qt3d window and webengine window displayed at the same time, and without any scary warnings/errors from Qt. Well done @jbp35 and sorry for the long delay from me!

I have added some minor sip and test fixes on the branch, and if all tests pass I think we should be good to merge this.

@wonder-sk wonder-sk modified the milestones: 3.32.0, 3.36.0 Dec 18, 2023
@wonder-sk wonder-sk removed the Frozen Feature freeze - Do not merge! label Dec 18, 2023
@jbp35
Copy link
Contributor Author

jbp35 commented Dec 18, 2023

Finally managed to test this with both Qt5 and Qt6 on linux - and I can confirm things seems to be working fine even if there is a qt3d window and webengine window displayed at the same time, and without any scary warnings/errors from Qt. Well done @jbp35 and sorry for the long delay from me!

I have added some minor sip and test fixes on the branch, and if all tests pass I think we should be good to merge this.

@wonder-sk Thank you for the review!

One doubt on my side, should we also bump dependencies for PyQtWebEngine (>= 5.15.6) and python3-pyqt5-sip (>=12.9) to avoid that users get an error when trying to run a plugin using QtWebEngineWidgets?

@wonder-sk
Copy link
Member

I will try to add a summary of things here:

  • Qt WebEngine needs QtWebEngine::initialize() to be called before QApplication is created - what it really does is that it sets Qt::AA_ShareOpenGLContexts
  • that flag was added in set Qt::AA_ShareOpenGLContexts on all platforms #47873 to allow plugins to use WebEngine from PyQt, but then soon afterwards reverted in Disable shared opengl contexts #49888 because it does not play well with existing Qt 3D code. User could get ugly warnings (Warning: Setting a new default format with a different version or profile after the global shared context is created may cause issues with context sharing.) and/or crashes (?)
  • the problem here is that Qt3DWindow constructor calls QSurfaceFormat::setDefaultFormat() which triggers the warning and potentially causes further issues - there is QTBUG-60614 about that, but not much happened towards its resolution
  • the solution is to:
    1. set default QSurfaceFormat in a way that both WebEngine and QGIS 3D are happy
    2. enable shared opengl contexts
    3. create QApplication and then do not change anything anymore related to surface format
  • we need to replace Qt3DWindow by a custom QWindow implementation that does not call QSurfaceFormat::setDefaultFormat() and simply uses whatever default format has been set - fortunately this is not a lot of code - the new Qgs3DWindow class does that for us
  • we need to pick some "global" OpenGL version... Understanding all the OpenGL versions, profiles, their support on different systems and various forward/backward compatibility issues is a bit of nightmare (at least for me)
    • it looks like WebEngine needs OpenGL 3.2 Core profile, but it can live with higher OpenGL version too
    • QGIS 3D does not have requirements for a very recent OpenGL version - Qt3DWindow would ask for 4.3 Core profile.
    • on Mac, Apple stopped supporting newer OpenGL version longer time ago, it is stuck at 4.1 Core (https://support.apple.com/en-us/101525)
    • in the end, we set surface format to OpenGL 4.3, and 4.1 on Mac

Sorry if some bits are not 100% correct - there are various things here I do not fully understand (OpenGL contexts + their sharing in Qt apps, WebEngine / Qt3D specifics of how OpenGL contexts are used).

@wonder-sk
Copy link
Member

One doubt on my side, should we also bump dependencies for PyQtWebEngine (>= 5.15.6) and python3-pyqt5-sip (>=12.9) to avoid that users get an error when trying to run a plugin using QtWebEngineWidgets?

@jbp35 I have not really looked into that, I was primarily concerned about getting things work on C++ side of things. If there are some bumps of dependencies needed, that's something to sort out with QGIS packagers. Do you have some reference to what got fixed in those package versions?

@jbp35
Copy link
Contributor Author

jbp35 commented Dec 18, 2023

One doubt on my side, should we also bump dependencies for PyQtWebEngine (>= 5.15.6) and python3-pyqt5-sip (>=12.9) to avoid that users get an error when trying to run a plugin using QtWebEngineWidgets?

@jbp35 I have not really looked into that, I was primarily concerned about getting things work on C++ side of things. If there are some bumps of dependencies needed, that's something to sort out with QGIS packagers. Do you have some reference to what got fixed in those package versions?

PyQt5-WebEngine >= 5.15.6
Here is the explanation for the need to bump the dependency :
#49512 (comment)

This is needed for QtWebEngineWidgets to work using python.

PyQt6-WebEngine >= 6.2-maint
This would also need to be done for PyQt6-WebEngine (minimum version 6.2-maint)

Changelog for PyQt6:
https://www.riverbankcomputing.com/static/Downloads/PyQt6-WebEngine/ChangeLog-6.6.0.dev2310250952

2022-03-15  Phil Thompson  <phil@riverbankcomputing.com>
NEWS, PyQt6-WebEngine.msp:
Allow Qt.AA_ShareOpenGLContexts to be specified before a
QCoreApplication is created to allow QtWebEngineWidgets to be
imported later.
[060f78886515] <6.2-maint>

This is needed for QtWebEngineWidgets to work using python.

python3-pyqt5-sip >=12.9

I am not so sure that this is needed - it may just be that I was importing from PyQt5 instead of qgis.PyQt in my script above but I am not able to test this right now.

@wonder-sk
Copy link
Member

I'm still not sure if I understand why PyQt5-WebEngine needs to be upgraded... with this PR, AA_ShareOpenGLContexts flag gets set before QApplication is created... so isn't that issue solved already?

@jbp35
Copy link
Contributor Author

jbp35 commented Dec 18, 2023

I'm still not sure if I understand why PyQt5-WebEngine needs to be upgraded... with this PR, AA_ShareOpenGLContexts flag gets set before QApplication is created... so isn't that issue solved already?

When using WebEngine with python, PyQt5-WebEngine is intialized after the creation of QgsApplication.

In old versions, PyQt5-WebEngine checks if a QApplication already exists during initialization and throw an error even if `AA_ShareOpenGLContexts' is enabled.

In versions >= 5.16.6, it does not throw an error if `AA_ShareOpenGLContexts' is enabled and continue initialisation.

@jbp35
Copy link
Contributor Author

jbp35 commented Dec 18, 2023

I will try to add a summary of things here:

  • Qt WebEngine needs QtWebEngine::initialize() to be called before QApplication is created - what it really does is that it sets Qt::AA_ShareOpenGLContexts

  • that flag was added in set Qt::AA_ShareOpenGLContexts on all platforms #47873 to allow plugins to use WebEngine from PyQt, but then soon afterwards reverted in Disable shared opengl contexts #49888 because it does not play well with existing Qt 3D code. User could get ugly warnings (Warning: Setting a new default format with a different version or profile after the global shared context is created may cause issues with context sharing.) and/or crashes (?)

  • the problem here is that Qt3DWindow constructor calls QSurfaceFormat::setDefaultFormat() which triggers the warning and potentially causes further issues - there is QTBUG-60614 about that, but not much happened towards its resolution

  • the solution is to:

    1. set default QSurfaceFormat in a way that both WebEngine and QGIS 3D are happy
    2. enable shared opengl contexts
    3. create QApplication and then do not change anything anymore related to surface format
  • we need to replace Qt3DWindow by a custom QWindow implementation that does not call QSurfaceFormat::setDefaultFormat() and simply uses whatever default format has been set - fortunately this is not a lot of code - the new Qgs3DWindow class does that for us

  • we need to pick some "global" OpenGL version... Understanding all the OpenGL versions, profiles, their support on different systems and various forward/backward compatibility issues is a bit of nightmare (at least for me)

    • it looks like WebEngine needs OpenGL 3.2 Core profile, but it can live with higher OpenGL version too
    • QGIS 3D does not have requirements for a very recent OpenGL version - Qt3DWindow would ask for 4.3 Core profile.
    • on Mac, Apple stopped supporting newer OpenGL version longer time ago, it is stuck at 4.1 Core (https://support.apple.com/en-us/101525)
    • in the end, we set surface format to OpenGL 4.3, and 4.1 on Mac

Sorry if some bits are not 100% correct - there are various things here I do not fully understand (OpenGL contexts + their sharing in Qt apps, WebEngine / Qt3D specifics of how OpenGL contexts are used).

This is a good summary.

Regarding https://bugreports.qt.io/browse/QTBUG-60614, I think the logic is that Qt3DWindow is just a helper class to get started quickly with qt 3d (part of QT 3d Extras) and that it is meant to be reimplemented to suit project needs.. it is therefore unlikely that they will ever fix this and writing a custom class is the way to go.

@wonder-sk
Copy link
Member

In old versions, PyQt5-WebEngine checks if a QApplication already exists during initialization and throw an error even if `AA_ShareOpenGLContexts' is enabled.

In versions >= 5.16.6, it does not throw an error if `AA_ShareOpenGLContexts' is enabled and continue initialisation.

Ah ok, that makes sense - thanks for the clarification!

To get the python packages upgraded, you will probably need to get in touch with package maintainers for various platform and ask them to do the upgrade - unfortunately there is no "official" registry of what package versions should be used with qgis installers...

@wonder-sk wonder-sk merged commit 1d1c556 into qgis:master Dec 19, 2023
28 checks passed
@jbp35
Copy link
Contributor Author

jbp35 commented Dec 19, 2023

In old versions, PyQt5-WebEngine checks if a QApplication already exists during initialization and throw an error even if AA_ShareOpenGLContexts' is enabled. In versions >= 5.16.6, it does not throw an error if AA_ShareOpenGLContexts' is enabled and continue initialisation.

Ah ok, that makes sense - thanks for the clarification!

To get the python packages upgraded, you will probably need to get in touch with package maintainers for various platform and ask them to do the upgrade - unfortunately there is no "official" registry of what package versions should be used with qgis installers...

Alright, for reference I already opened a ticket for osgeo4W a few months ago:
https://trac.osgeo.org/osgeo4w/ticket/788

@jbp35 jbp35 deleted the qgs3dwindow branch December 21, 2023 11:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants