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

OPENGL: Add support for ARGB8888 pixel format on little endian systems #2775

Open
wants to merge 1 commit into
base: master
from

Conversation

@lowtraxx
Copy link
Contributor

@lowtraxx lowtraxx commented Feb 14, 2021

Should also fix display issues for AGS titles on other platforms using mainly OpenGL.

@deepcode-ci-bot
Copy link

@deepcode-ci-bot deepcode-ci-bot bot commented Feb 14, 2021

Congratulations 🎉. DeepCode analyzed your code in 2.095 seconds and we found no issues. Enjoy a moment of no bugs ☀️.

👉 View analysis in DeepCode’s Dashboard | Configure the bot

Copy link
Member

@criezy criezy left a comment

The change for GLES seems OK.

However with the changes here we get a crash when trying to use OpenGL on desktop as it fails to create the texture. You also need to handle this new pixel format in getGLPixelFormat(). If I got my format right, this would look like:

#ifdef SCUMM_LITTLE_ENDIAN
	} else if (pixelFormat == Graphics::PixelFormat(4, 8, 8, 8, 8, 24, 16, 8, 0)) { // RGBA8888
		glIntFormat = GL_RGBA;
		glFormat = GL_RGBA;
		glType = GL_UNSIGNED_INT_8_8_8_8;
		return true;
+	} else if (pixelFormat == Graphics::PixelFormat(4, 8, 8, 8, 8, 16, 8, 0, 24)) { // ARGB8888
+		glIntFormat = GL_RGBA;
+		glFormat = GL_BGRA;
+		glType = GL_UNSIGNED_INT_8_8_8_8_REV;
+		return true;
#endif
@lowtraxx lowtraxx force-pushed the lowtraxx:bugfix_ags_opengl branch 2 times, most recently from 86374c9 to 23485c4 Feb 15, 2021
@lowtraxx lowtraxx requested a review from criezy Feb 15, 2021
@lowtraxx
Copy link
Contributor Author

@lowtraxx lowtraxx commented Feb 15, 2021

@criezy I amended your changes and tested on MacOS as well as Linux. Should work now.

Copy link
Member

@criezy criezy left a comment

The commit message does not quite follow our conventions:

  • The subsystem should be OPENGL and not AGS
  • The first line should be quite short with additional information as a separate paragraph below.
  • The description is no longer quite right since the commit is more generic than just adding the fake texture.

Here is an example:

OPENGL: Add support for ARGB8888 pixel format

This format is used by the AGS engine and is thus required to play AGS games.
On GLES devices this is handled using a fake texture as the format is not
supported natively.

Also rereading the changes I am worried it might nor work on big endian GLES systems.
And I am realising we forgot the big endian case for OpenGL as well (in getGLPixelFormat()).

@@ -530,6 +530,46 @@ void TextureRGBA8888Swap::updateGLTexture() {
// Do generic handling of updating the texture.
Texture::updateGLTexture();
}

TextureARGB8888::TextureARGB8888()
: FakeTexture(GL_RGBA, GL_RGBA, GL_UNSIGNED_BYTE, Graphics::PixelFormat(4, 8, 8, 8, 8, 0, 8, 16, 24)) // ABGR8888

This comment has been minimized.

@criezy

criezy Feb 15, 2021
Member

I am worrying that some big endian GLES systems might not support this format, which may be why we have a fake texture for it. So maybe when SCUMM_BIG_ENDIAN is defined (or when SCUMM_LITTLE_ENDIAN is not defined) you should be converting to RGBA8888 rather than ABGR8888.

This comment has been minimized.

@criezy

criezy Feb 15, 2021
Member

I actually forgot that you only added the format in getSupportedFormats() when SCUMM_LITTLE_ENDIAN is defined. This means we probably cannot play AGS games with OpenGL/GLES currently on big endian systems, but at least that also means that getGLPixelFormat() and the TextureARGB8888 are consistent with it.

It would be nice to extend the changes to the big endian case, but if you cannot test that case and are not confortable with doing it, you could just mention in the commit message that this is done only for little endian systems.

This format is used by the AGS engine and is thus required to play AGS games.
On GLES devices this is handled using a fake texture as the format is not
supported natively. Currently only supported on little endian systems.
@lowtraxx lowtraxx force-pushed the lowtraxx:bugfix_ags_opengl branch from 23485c4 to 86f4102 Feb 15, 2021
@lowtraxx lowtraxx changed the title AGS: New FakeTexture for ARGB8888. Fixes Android display issues. OPENGL: Add support for ARGB8888 pixel format on little endian systems Feb 15, 2021
@criezy
criezy approved these changes Feb 16, 2021
Copy link
Member

@criezy criezy left a comment

This seems good now.
I will keep this open for a few more days until I have time to look at the big endian side of things.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants