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

wglwidget: thin layer on top of qglwidget or qopenglwindow based widget #10959

Closed
wants to merge 2 commits into from

Conversation

m0dB
Copy link
Contributor

@m0dB m0dB commented Oct 13, 2022

This PR replaces the use of QGLWidget with a new class WGLWidget.

The new cmake option QOPENGL determines how WGLWidget is implemented:

  • with QOPENGL=OFF: WGLWidget is a class derived from QGLWidget, with code that was duplicated in derived classes such as WSpinny, WVuMeterGL, the waveform widget and in waveform widget factory, moved WGLWidget methods. This is a refactoring that should behave exactly the same as before.

  • with QOPENGL=ON: WGLWidget is class derived from QWidget that has an container widget that contains an QOpenGLWindow. As far as I have tested, on macOS rendering with QOpenGLWindow is significantly faster than QGLWidget.

The two implementations use the same API, wit the intention of keeping all use of these classes and the derived classes identical, independent of the implementation used. There are some exceptions, where implementation specific code is needed (e.g. forwarding events to the WWaveformViewer), this is done inside #ifdef MIXXX_USE_QOPENGL blocks.

In addition to this, all use of either the old Qt QGL.. or the new Qt QOpenGL.. classes is also handled with the MIXXX_USE_QOPENGL preprocessor define (defined by cmake)

Finally, I added another WVuMeter implementation, one that uses QOpenGL code and uses textures and shaders to render the pixmaps instead of QPainter.

Note that I have a third WGLWidget implementation, which uses QOpenGLWidget, but on macOS this has the same problems as QGLWidget (bad framerate, laggy interaction), so I can't really test it. It would be good if a linux or windows dev code take over. This will allow us to completely replace the deprecated QGL... classes with QOpenGL... classes.

@github-actions github-actions bot added the ui label Oct 13, 2022
@m0dB m0dB changed the title wglwidget: thin layer qglwidget and working towards qopengl-based drop-in replacement wglwidget: thin layer on top of qglwidget Oct 13, 2022
@github-actions github-actions bot added the build label Oct 13, 2022
Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

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

Thank you for the good mergable step by step approach.
There is a Qt 6 guard missing to fix the Arch build. And a comment nit-pick.

/// deferred until Qt calls QGLWidget::initializeGL and cannot be done in the
/// GLWaveformWidgetAbstract is a WaveformWidgetAbstract & WGLWidget. It may
/// have a GLWaveformRenderer member which requires initialization that must be
/// deferred until Qt calls WGLWidget::initializeGL and cannot be done in the
Copy link
Member

Choose a reason for hiding this comment

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

Is this comment correct now or calls QT QGLWidget::initializeGL? This sentence is anyway weird because QT does not call magically something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will need to investigate this. But there were several widgets that derived from QGLWidget + WaveformWidgetAbstract, and other widgets that derived from GLWaveformWidgetAbstract, which derived from QGLWidget + WaveformWidgetAbstract, which made no sense to me. The only difference between the two approaches was this additional GLWaveformRenderer member in GLWaveformWidgetAbstract and how it is called from initializeGL.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will have another look to understand the intention and try to improve the comment or maybe get rid of the construction altogether if i think it's not needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will move the wglwidgetqwidget.cpp file in CMakeLists.txt so it's not added when building with Qt6.

@m0dB m0dB marked this pull request as draft October 13, 2022 16:54
Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you. Is the draft state sill correct? You may squash the two last commits.

@m0dB
Copy link
Contributor Author

m0dB commented Oct 14, 2022

Still draft, there are some more changes I want to include.

@m0dB m0dB force-pushed the wglwidget branch 3 times, most recently from 176b279 to d243084 Compare October 15, 2022 15:49
@m0dB m0dB changed the title wglwidget: thin layer on top of qglwidget wglwidget: thin layer on top of qglwidget or qopenglwindow based widget Oct 15, 2022
@m0dB
Copy link
Contributor Author

m0dB commented Oct 15, 2022

@daschuer in the end I included my QOpenGLWindow based WGLWidget in this PR. The only thing remaining is that I think this should use a buildenv with Qt 5.15.x. Are you planning to do that for main?

@github-actions
Copy link

This PR is marked as stale because it has been open 90 days with no activity.

@github-actions github-actions bot added the stale Stale issues that haven't been updated for a long time. label Jan 20, 2023
@JoergAtGithub
Copy link
Member

@daschuer in the end I included my QOpenGLWindow based WGLWidget in this PR. The only thing remaining is that I think this should use a buildenv with Qt 5.15.x. Are you planning to do that for main?

Is this really neccessary to merge this PR? Shouldn't it work with the actual buildenv as well?

@daschuer
Copy link
Member

You can find an QT 5.15.7 macOS environment here:
https://github.com/daschuer/vcpkg/actions/runs/3842608943

This should not be a requirement for merging.

@github-actions github-actions bot removed the stale Stale issues that haven't been updated for a long time. label Jan 22, 2023
@github-actions
Copy link

This PR is marked as stale because it has been open 90 days with no activity.

@github-actions github-actions bot added the stale Stale issues that haven't been updated for a long time. label Apr 23, 2023
@JoergAtGithub
Copy link
Member

I think this can be closed too?

@m0dB
Copy link
Contributor Author

m0dB commented Jul 15, 2023

closed as superseded by #10989

@m0dB m0dB closed this Jul 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build stale Stale issues that haven't been updated for a long time. ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants