Skip to content

BACKENDS: Merge 3D graphics managers with the 2D ones #6651

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

Merged
merged 47 commits into from
Jun 1, 2025

Conversation

lephilousophe
Copy link
Member

This (big) PR removes the graphics3d folder inherited from ResidualVM and merges the needed parts inside the OpenGLGraphicsManager and its platform specific descendants.
Several fixes which were revealed during my testing are also included in the PR even touching the now removed 3D graphics manager.

I tried to test as much combinations as I could between SDL1.2/2, FBO, shaders, OpenGL/ES2, antialiasing and bit-depth support.
The main tests have been done on Linux, with various versions of Mesa (up to 3.5, limited to OpenGL 1.2 support), 16-bits Xorg server and SDL1.2 and 2.
A quick test on SDL3 has been done (and uncovered an unrelated bug).
Android and Windows 95 and XP also got tested, although the rendering of these MS platforms may not perfect due to the MS OpenGL buggy implementation.
For iOS, only the build has been tested.
libretro port has not been tested at all. It should "magically" gain 3D engines support.

Some things need to be done:

  • testing on iOS
  • testing more SDL platforms
  • testing more engines than Playground3d

@@ -96,8 +96,6 @@ class OSystem_iOS7 : public ModularGraphicsBackend, public EventsBaseBackend {
void setFeatureState(Feature f, bool enable) override;
bool getFeatureState(Feature f) override;

bool setGraphicsMode(int mode, uint flags) override;

TouchMode getCurrentTouchMode() const { return _currentTouchMode; };
Copy link
Contributor

Choose a reason for hiding this comment

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

; after functions

Copy link
Member Author

Choose a reason for hiding this comment

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

I prefer not to modify this one to not blur the code changes.
The PR is already big enough. :)

@larsamannen
Copy link
Contributor

I've tested Grim Fandango in iOS and it works. I've tested a couple of shaders which works. One shader, crt/hyllian, didn't work as expected. Perhaps not all shaders will work with the 3d engine. However, when exiting with that shader scummVM crashes because of _gameScreen being NULL when setting up the new 2D graphicManager. It only happens with that shader. Perhaps you can test that specific shader on Linux and see if the same thing happens.
But good work!

@lephilousophe
Copy link
Member Author

However, when exiting with that shader scummVM crashes because of _gameScreen being NULL when setting up the new 2D graphicManager.

I did some fixes but never got this bug when testing with the shader. I hope it's really fixed anyway.

@lotharsm
Copy link
Member

I tested this on Windows 11 24H2 with MSYS2, using TWP as a game target. It works great, and shader support is there as well. Great job!

@lephilousophe
Copy link
Member Author

I added two more fixes to the PR:

  • Playground3d now gets a mouse cursor in all modes. The default arrow cursor provided by CursorMan is used.
  • TWP now gets the (system) mouse cursor when ImGui is enabled. This fits the behaviour of the other ImGui enabled games where the system cursor is always enabled.

@lephilousophe
Copy link
Member Author

Hum my last addition fails the build because the setting I added is only supported on SDL 2.32+.
It also seems that this may not work on some platforms (like MacOS).

@lephilousophe
Copy link
Member Author

This is now fixed differently (in TWP instead of globally).

@@ -112,6 +111,37 @@ OSystem_iOS7::~OSystem_iOS7() {
delete _graphicsManager;
}

#if defined(USE_OPENGL_GAME) || defined(USE_OPENGL_SHADERS)
Common::Array<uint> OSystem_iOS7::getSupportedAntiAliasingLevels() const {
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to share this code across all platforms when framebufferObjectMultisampleSupported is in use?

Copy link
Member Author

Choose a reason for hiding this comment

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

I suppose we could but I didn't find an elegant way for it.

@ccawley2011
Copy link
Member

How easy would it be to transplant the changes from PR #4633 on top of this PR?

@lephilousophe
Copy link
Member Author

I am not sure about it.
The framebuffer changes are not needed anymore, I would say.
The backend part is already done as OpenGLGraphicsManager supports GLES1.
All the shader aliasing has been deleted in the time (because not everything is aliased).

What would remain is fixing enter3D and leave3D to save and restore the state in a GLES1 compliant way.
And, adapt Stark, obviously.

One point that bothers me is: do you have any platform to test this? Because, I don't.

@neuromancer
Copy link
Contributor

If you start one of the dark side demos, you can see a bug that affects all the freescape games when using this code, there is some issue with the viewport handling:
scummvm-darkside-demo-00002

(pressing "space" a couple of times will take you to the game, which looks good otherwise).

For reference, it should look like this:

scummvm-darkside-demo-00003

@spleen1981
Copy link
Contributor

libretro port has not been tested at all. It should "magically" gain 3D engines support.

I've tested the libretro port on Win11, linux and Android with TWP and Grim Fandango, it looks like is working very good... great job!

@ccawley2011
Copy link
Member

One point that bothers me is: do you have any platform to test this? Because, I don't.

I'd been using Mesa while working on the PR, and I should be able to test on Android as well.

@lephilousophe
Copy link
Member Author

Well, I think we should postpone this to later.
The plan here is to have something fully functional.
GLES is not supported on Android currently so, if there is a need for GLES support, there should be more work to do.

This allows to get the real SDL error message.
Move the definitions where they belong and avoid a runtime check for the
GL renderer.
Also remove the now unused common parts
This fixes a segfault when starting in 16-bits rendering mode.
This allows the engine to react to screen changes which happened when it
was unable to track them.
Now that there is one backend for all 2D and 3D, the state must be
checked.
Shader pipeline expects the framebuffer to be alive when cleaning up.
A SIGSEGV can happen when notifyContextCreate is called twice in a row.
This should never happen, but better be safe.
This function was never used before so there is no consequences.
This is only supported on SDL2.
On SDL1.2, the window is completely managed by SDL and we cannot know
when it would get destroyed.
kFeatureFullscreenToggleKeepsContext was a feature only used internally
by the SDL backend. Remove it and replace it by a virtual function.
Make the common code agnostic of the 3D rendering mode.
3D renderer initializes context using functions from this optional part
of OpenGL 1.2.
This fixes a segfault on Windows XP OpenGL.
The data returned by SDL_GetPreferredLocales is allocated in one block.
And indicates to the user that the shader failed to apply.
With the SDL 3D manager the system cursor was used.
This is not the case anymore.
The system cursor is never displayed because we are in relative mode.
@lephilousophe
Copy link
Member Author

If you start one of the dark side demos, you can see a bug that affects all the freescape games when using this code, there is some issue with the viewport handling: scummvm-darkside-demo-00002

(pressing "space" a couple of times will take you to the game, which looks good otherwise).

For reference, it should look like this:

scummvm-darkside-demo-00003

Thank you for testing and for the diagnostic, it's now fixed.
Blitting the framebuffer when AA is enabled depends on the scissor test which was set by Freescape.
Disabling the scissor test before the blit makes it work.

@lephilousophe
Copy link
Member Author

Last call for testing: I plan to merge this on tomorrow.

@lephilousophe
Copy link
Member Author

Merging now (and watching the buildbot failures).

@lephilousophe lephilousophe merged commit b096e30 into scummvm:master Jun 1, 2025
14 of 15 checks passed
@dwatteau
Copy link
Contributor

dwatteau commented Jun 1, 2025

I did a quick test on OSXPPC, which relies on SDL1.2 (it was running commit 53f1375, i.e. before the latest PR changes)

I've tested Grim/Monkey4, mostly (and also a bit of SCUMM/AGS in case of any regression). I didn't see any obvious issue ✅

FWIW, this was on an iBook G4 running OSX 10.4, so it's limited to OpenGL 1.3. I won't have access to my other OSX Leopard machines (with OpenGL 2.0 support) before a few weeks, though.

So yeah, for this port this looks fine. I hope to find some time this summer for more extensive testing there.

Output on this machine:

Debuglevel (from command line): 6
Using SDL Video Driver "Quartz"
OpenGL: GL context initialized
OpenGL version: 1.3 ATI-1.4.18
OpenGL vendor: ATI Technologies Inc.
OpenGL renderer: ATI Radeon 9200 OpenGL Engine
OpenGL: version 1.3
OpenGL: GLSL version string: 
OpenGL: GLSL version: 0
OpenGL: Max texture size: 2048
OpenGL: NPOT texture support: 0
OpenGL: Imaging support: 1
OpenGL: Shader support: 0
OpenGL: Shader support for engines: 0
OpenGL: Multitexture support: 1
OpenGL: FBO support: 0
OpenGL: Multisample FBO support: 0
OpenGL: Multisample max number: -1
OpenGL: Packed pixels support: 1
OpenGL: Packed depth stencil support: 0
OpenGL: Unpack subimage support: 1
OpenGL: OpenGL ES depth 24 support: 0
OpenGL: Texture edge clamping support: 1
OpenGL: Texture border clamping support: 1
OpenGL: Texture mirror repeat support: 1
OpenGL: Texture max level support: 1
OpenGL: Texture lookup precision: -1

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.

8 participants