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

Qt: Replace QOpenGLWidget with QWidget #366

Merged
merged 6 commits into from
Mar 16, 2024
Merged

Conversation

GranMinigun
Copy link
Contributor

This PR switches QOpenGLWidget with QWidget and QOpenGLContext for context management and graphics output. Also, GLEW was replaced with glad, as otherwise Qt would complain about function redefinitions.

Prerequisite for #332.

RFC. The changes seem to work just fine on Linux (both X11 and Wayland), but I have a couple of questions.

First of all, there are two ways to integrate glad: by including the pre-generated sources (like currently done in this PR), and (since 2.0.5) by generating sources via CMake during the build process. The latter option keeps the source tree cleaner, but introduces a build-time dependency on either system-provided glad generator or Python (for use with FetchContent-obtained glad). I'm leaning towards the latter option, in line with other external dependencies, but would like to get your preference.

Second, right now there's no handling of Qt creating <3.3 context. That's the case with QOpenGLWidget implementation as well. It's easy to check which context version you got and show a message box, but I'm not sure what to do afterwards: for instance, calling QCoreApplication::exit() doesn't work until exec() is called. Best case, users will get a black screen, as shaders won't compile. Worst case, it can segfault due to calling null functions.

Lastly, what is the convention on commit messages naming, specifically prefixes?

@fleroviux
Copy link
Member

Seems to work on macOS. On Windows 10 I am currently experiencing a segmentation fault when launching the executable:
image

First of all, there are two ways to integrate glad: by including the pre-generated sources (like currently done in this PR), and (since 2.0.5) by generating sources via CMake during the build process. The latter option keeps the source tree cleaner, but introduces a build-time dependency on either system-provided glad generator or Python (for use with FetchContent-obtained glad). I'm leaning towards the latter option, in line with other external dependencies, but would like to get your preference.

I would also prefer avoiding pre-generated sources to keep things consistent and avoid bloating the repository. That is if the sources can be generated on the fly reasonably quickly (mainly relevant for CI I would guess)

Second, right now there's no handling of Qt creating <3.3 context. That's the case with QOpenGLWidget implementation as well. It's easy to check which context version you got and show a message box, but I'm not sure what to do afterwards: for instance, calling QCoreApplication::exit() doesn't work until exec() is called. Best case, users will get a black screen, as shaders won't compile. Worst case, it can segfault due to calling null functions.

A warning message box + black screen or crash would likely already be a bit better than what we have right now (which would just show a black screen or crash).

Lastly, what is the convention on commit messages naming, specifically prefixes?

For regarding src/platform/core I am using Platform: Core: as prefix and for changes regarding src/platform/qt I've lately been using just Qt: but also Platform: Qt: in the past. For code in src/nba it depends on the component. It could be Core: for the core class for example, or PPU: for the picture processing unit.

@GranMinigun GranMinigun force-pushed the qwidget branch 15 times, most recently from e91fc2c to 139c998 Compare March 12, 2024 15:07
@GranMinigun GranMinigun force-pushed the qwidget branch 2 times, most recently from e40c204 to 32b9c06 Compare March 14, 2024 15:53
@GranMinigun
Copy link
Contributor Author

On Windows 10 I am currently experiencing a segmentation fault when launching the executable:

Fixed. Didn't expect it to fire events so early.

I would also prefer avoiding pre-generated sources to keep things consistent and avoid bloating the repository. That is if the sources can be generated on the fly reasonably quickly (mainly relevant for CI I would guess)

It is quick. Had to wrestle with CMake so it finds the correct Python installation, pardon the CI spam. Still need someone to verify updated building instructions for macOS and FreeBSD.

The last commit implements OpenGL context version checking. If it's not new enough (I'm looking at you, Intel HD 3000 on Windows), an error message is shown, and the program closes. Couldn't think of a better way than to introduce yet another initialization function, but perhaps some more stuff could be put into it in the future.

If there are no objections on the current overall design, then I'll move the PR out of draft.

@fleroviux
Copy link
Member

No objections on my part. Just retested on Windows 10, seems to be fixed. Good job!

@GranMinigun
Copy link
Contributor Author

GranMinigun commented Mar 16, 2024

Tested with both Qt 5 and 6 on Linux (X.org and Wayland sessions), but only with Qt 5 on Windows 10. Checked the error message by running with MESA_GL_VERSION_OVERRIDE environment variable set. I think I fixed everything that I initially broke?

Oh, by the way, maybe check whether the changes affect #253.

@GranMinigun GranMinigun marked this pull request as ready for review March 16, 2024 23:23
@fleroviux fleroviux merged commit 00110ea into nba-emu:master Mar 16, 2024
4 checks passed
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.

2 participants