nba-emu / NanoBoyAdvance

A cycle-accurate Nintendo Game Boy Advance emulator.
GNU General Public License v3.0
955 stars 53 forks source link

Qt: Replace QOpenGLWidget with QWidget #366

Closed GranMinigun closed 3 months ago

GranMinigun commented 4 months ago

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 commented 4 months ago

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 commented 3 months ago

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 commented 3 months ago

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

GranMinigun commented 3 months ago

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.