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

Support for Qt6 #2296

Closed
wants to merge 7 commits into from
Closed

Support for Qt6 #2296

wants to merge 7 commits into from

Conversation

bjeurissen
Copy link
Member

Not sure if this should go to master.

Today homebrew upgraded my qt installation to qt6, so I tried rebuilding mrtrix3 against it, which did not work out-of-the-box.

I believe that all of the changes here are backward compatible with Qt5. Most of the things I had to change were things that were already deprecated in Qt5.

I tested this on:

  • MacOS Big Sur (arm64) with Qt 6.0.2
  • Ubuntu 20.04.2 LTS (x86_64) with Qt 5.12.8

One caveat is the configure script:

  • for Qt6 it needs to add module openglwidgets to the project file (as in this branch)
  • for Qt5 it needs to add module opengl to the project file (as in master)
    but we detect the Qt-version by building a binary using this project file...

What would be the preferred solution here?

@bjeurissen
Copy link
Member Author

bjeurissen commented Mar 5, 2021

Not convinced that this will work with older Qt versions.

The issue appears to be that:
-QGLWidget no longer exists in Qt6, but is fully replaced with QOpenGLWidget
-Both QGLWidget and QOpenGLWidget are available in Qt>=5.4, and we seem to be currently mixing both.

whereas in this branch I have replaced all instances of QGLWidget by QOpenGLWidget, so this will probably only work in Qt>=5.4 and will break both Qt4 and older Qt5 versions.

EDIT: Given that we require Qt>=5.5 now, this is probably not an issue...

@jdtournier
Copy link
Member

Thanks Ben! It'll take me a while to get through that...

Just one comment though:

-Both QGLWidget and QOpenGLWidget are available in Qt>=5.4, and we seem to be currently mixing both.

I'm pretty sure we're using one or the other, depending on the Qt version - but we shouldn't be mixing both at the same time, right...?!? There's a version check based on QT_VERSION (e.g. this line), and after that it should all be based on one version or the other. Given it switches to QOpenGLWidget if Qt >= 5.4, I would have thought it would cover Qt6 already...

@bjeurissen
Copy link
Member Author

bjeurissen commented Mar 6, 2021

-Both QGLWidget and QOpenGLWidget are available in Qt>=5.4, and we seem to be currently mixing both.

I'm pretty sure we're using one or the other, depending on the Qt version - but we shouldn't be mixing both at the same time, right...?!? There's a version check based on QT_VERSION (e.g. this line), and after that it should all be based on one version or the other. Given it switches to QOpenGLWidget if Qt >= 5.4, I would have thought it would cover Qt6 already...

I think src/gui/opengl/gl.h is indeed fine to that respect, but there were some other files that were including and using QGLWidget regardless of the Qt version (see the list of Files changed). This worked fine in Qt5 because QGLWidget was still there (but deprecated), but fails on Qt6 where QGLWidget is removed.

EDIT: An alternative is to make all those additional files also use QGLWidget or QOpenGLWidget depending on the Qt version, but since we have Qt>=5.5 in our dependency list, which supports QOpenGLWidget, I think it would be cleaner to remove all use of QGLWidget.

@bjeurissen
Copy link
Member Author

bjeurissen commented Mar 8, 2021

Noticed that the Tool widgets don't work properly when docked when using Qt6. You can see them being rendered for a split second, but then the entire widget becomes empty and no interaction whatsoever is possible (e.g. undocking or changing between different Tools by clicking the vertical tabs). Everything works fine when using mrview with MRViewDockFloating: 1, but as soon as you dock the widget, it becomes empty and one cannot interact with it.

@Lestropie
Copy link
Member

Have not followed along here or even looked at Qt6, but want to explicitly link to #2157, and make sure it's recognised that those changes were made on dev whereas this PR is currently targeting master. The existing changes on master are only to the documentation (#2153).

While there's a strong argument for adding support for Qt6 as a hotfix on master, if embedded within that is an increase in minimum version for Qt5, that probably shouldn't go directly to master; though if it were to change immediately prior to a macro version increment it'd be fine.

@bjeurissen
Copy link
Member Author

Thanks Rob, I did not realize that the 5.5 requirement was only on master in the documentation. I think it makes a lot of sense to move Qt6 support to dev given the overlap with the Qt5 requirement work.

@bjeurissen
Copy link
Member Author

Moved here: #2297

@bjeurissen bjeurissen closed this Mar 9, 2021
@bjeurissen bjeurissen deleted the qt6 branch March 9, 2021 09:09
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.

3 participants