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

Android: Expose color space settings #12095

Merged
merged 2 commits into from Aug 30, 2023
Merged

Conversation

t895
Copy link
Contributor

@t895 t895 commented Aug 12, 2023

As per @Filoppi 's request

HDR settings are not included because HDR support is not present in the app yet.

@Filoppi
Copy link
Contributor

Filoppi commented Aug 12, 2023

Good stuff.
I checked how color managment works in Android:
https://source.android.com/docs/core/display/color-mgmt
that doc confirms that Dolphin is already color managed when using Vulkan on Android (see VK_COLOR_SPACE_SRGB_NONLINEAR_KHR), as in, the colors sent by the emulator to the OS will be interpreted as sRGB (gamut and gamma) and mapped to whatever color space the display is using, so they will always be accurate. I don't think OpenGL ES is color managed because we don't specify the output as sRGB.
This implies there's no need to expose the custom gamma settings like it's needed on Windows/PC (Windows is not color managed at all atm).
All stuff regarding these two options can be removed from this PR:

color_correction.bSDRDisplayGammaSRGB = Config::Get(Config::GFX_CC_SDR_DISPLAY_GAMMA_SRGB);
color_correction.fSDRDisplayCustomGamma = Config::Get(Config::GFX_CC_SDR_DISPLAY_CUSTOM_GAMMA);

GFX_CC_SDR_DISPLAY_GAMMA_SRGB can be left at default (true) as it already is, because the OS will always interpret the image as sRGB.
The "Correct Gamma" option is the only one that should stay, and possibly, as an extra, the "Game Gamma" setting, but I wouldn't bother if you don't want.

@t895 t895 force-pushed the color-space-settings branch 3 times, most recently from de9ebac to 65768f3 Compare August 25, 2023 19:29
@t895
Copy link
Contributor Author

t895 commented Aug 25, 2023

Everything should be in order now

}

abstract val selectedValue: Int
abstract val selectedValue: Any
Copy link
Member

Choose a reason for hiding this comment

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

Using Any feels kind of ugly. What do you think of using an algebraic data type (using sealed)? Then it would be clearer what types can be returned, and you would be able to use when in place of if (item is FloatSliderSetting).

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 like that a lot actually. Pushing that change now.

Copy link
Member

Choose a reason for hiding this comment

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

What you implemented isn't what I suggested, but I suppose what you implemented works too.

We did support float settings before but we never showed anything past the decimal place before.
@Filoppi
Copy link
Contributor

Filoppi commented Aug 29, 2023

LGTM

@t895 t895 merged commit 5e5887a into dolphin-emu:master Aug 30, 2023
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants