-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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: Check the list of supported AA modes instead of hardcoding #11699
OpenGL: Check the list of supported AA modes instead of hardcoding #11699
Conversation
f85f975
to
ccafc6a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested on my AMD card, could see AA options in drop-down. OGL logic LGTM.
One thing that I'm uncertain about is how this interacts with Android - I didn't need to change any android code for the context creation part, which seems odd. Is Android still hardcoding the list of AA modes, or how does it work? |
Yes, it's currently hardcoded. |
Ah: dolphin/Source/Android/app/src/main/res/values/arrays.xml Lines 280 to 292 in 62ff2f1
And this applies even for other backends, not just opengl. So, it's something that probably can be fixed later in a separate PR. (It looks like MSAA/SSAA is also not exposed?) |
void VideoBackend::InitBackendInfo() | ||
void VideoBackend::InitBackendInfo(const WindowSystemInfo& wsi) | ||
{ | ||
{ | ||
std::unique_ptr<GLContext> temp_gl_context = | ||
GLContext::Create(wsi, g_Config.stereo_mode == StereoMode::QuadBuffer, true, false, | ||
Config::Get(Config::GFX_PREFER_GLES)); | ||
|
||
FillBackendInfo(temp_gl_context.get()); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two issues here:
- There's no reason for this to be an indented block. I think I must have done that for scoping reasons initially and then moved the code that cared about it away afterwards.
- This breaks if
temp_gl_context
isnullptr
, which is the case on my hacky WSL setup.VideoBackend::Initialize
checks for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great point about number 2, sorry I didn't catch that, too used to leveraging exceptions and didn't consider Create()
returning nullptr.
I would have thought somewhere in GLExtensions::Init
it would check for nullptr but doesn't look like it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More annoyingly, we don't convey this to the user in any way - VideoBackend::Initialize
returning false gives "Failed to initialize video backend!" on starting, but there's no equivalent for VideoBackend::FillBackendInfo
returning false, and it'd be nice to clarify that the context creation failed versus other things (and GLContext::Create
failing because it couldn't find a matching ifdef versus context->Initialize
failing).
ccafc6a
to
ea7356b
Compare
Additionally, issue 13198 mentioned that only 4x shows up on Mesa and selecting it did not behave correctly. It turns out that this is something I can reproduce in my jank WSL environment; for both Vulkan and OpenGL, the options in the dropdown are None, 4x MSAA, and 4x SSAA, but selecting 4x MSAA and 4x SSAA does not work. Mesa reports only 4x as the value (it doesn't include 1x or 2x). Additionally, selecting 2x before gave the same result as 4x. I've fixed it so that 4x only works properly. |
@alyssarosenzweig Could you take a look at this if you get a chance? You probably won't be able to run it without combining it with #11628. |
ea7356b
to
18ebad7
Compare
Not really sure what to look at specifically. I ain't no Dolphin dev 🐈 |
Certainly +1 on what the PR claims it does, I haven't reviewed if that's what it actually does 🐱 |
I'm mainly interested in your thoughts on the new logic in OGLConfig.cpp added by e05644d. |
5008859
to
9ba743e
Compare
It seems plausible but I'm not going spec diving to review with more certainty. |
Well, since it seems we're not getting any more detailed reviews here... we should probably just merge this. Can you rebase? |
Mesa (llvmpipe) only reports 4x MSAA, and doesn't report 2x (or 1x, but we implicitly add that). The old logic did not handle this correctly, causing selecting 4x to fail and fall back to None. This also removes VideoUtils::GetAvailableAntialiasingModes, and thus VideoUtils entirely, as its only other function was removed in 1f74653.
9ba743e
to
ac48b2e
Compare
Previously we hardcoded 1x, 2x, 4x, and 8x as the only valid modes; however, per issue 13198 8x is not always supported, and some hardware supports more (my Intel GPU supports 16x and my NVIDIA GPU supports 32x). Now we actually check the GPU for what ones are supported. See #11628 for additional context.
To do this, we also need to create a temporary OpenGL context. All of the other backends created a temporary context already; it's just that OpenGL requires a window to do so while nothing else does.