-
Notifications
You must be signed in to change notification settings - Fork 424
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
Allow renderer choice to be stored in user configuration (and add automatic fallbacks) #5682
Conversation
This will handle the case where the user has chosen a renderer which will not work with their system.
60066d9
to
8476f98
Compare
8476f98
to
e90a35b
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.
Looking pretty solid to me. I've pushed a number of minor changes to further solidify the logic and get this to a complete state. @peppy check and see whether you're on board with it.
@smoogipoo would appreciate another pair of eyes on this one.
Would have preferred review rather than direct commits for changes. |
Fair enough, will keep that in mind. |
I've refactored this heavily to allow the game-side select to work... Will probably just push to this branch since it changes so much. |
@frenzibyte please re-review for readability. see usage at https://github.com/ppy/osu/compare/master...peppy:osu:add-renderer-selection?expand=1 if curious. |
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.
Bit of a preliminary review for now
osu.Framework/Platform/GameHost.cs
Outdated
case RuntimeInfo.Platform.Linux: | ||
case RuntimeInfo.Platform.Android: | ||
yield return RendererType.Vulkan; |
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.
I suggest we don't use Vulkan on Linux yet. Since I'm a Linux user I'll be debugging it, but it is broken at runtime.
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.
@smoogipoo the way i have things setup right now, if a renderer isn't in the preference list it won't be displayed at all. do you think we should still allow it to be displayed in settings (this can be done quite easily, just want to check your opinion first).
osu.Framework/Platform/GameHost.cs
Outdated
.Append(RendererType.OpenGL) | ||
.Append(RendererType.OpenGLLegacy); |
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.
I'd probably reverse the order of these for now since OpenGL is pretty broken until veldrid/veldrid#481 and I don't know how deep the effects of it go beyond the intro (e.g. I don't if there's some case where sliders also won't render as a result/etc).
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.
Yeah for sure. Also potentially d3d before vulkan on windows
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.
These aren't in the preferred renderers, so order doesn't actually matter (it's basically for display order).
Veldrid GL isn't preferred at all for now.
osu.Framework/Platform/GameHost.cs
Outdated
yield return RendererType.Vulkan; | ||
yield return RendererType.Direct3D11; |
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.
I don't think we should default to Vulkan on Windows yet. For me it looks to be deadlocking in an infinite allocate loop on a 3060ti, and I'm not sure where yet (somewhere inside Veldrid) because I haven't really been testing Vulkan on Windows.
D3D11, while still not yet perfect and will experience glitchyness, is at least a little bit more stable and I will be continuing my work there before Vulkan because I don't know how Vulkan will behave with Optimus for example.
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.
Yep, agree here too.
Co-authored-by: Salman Ahmed <[email protected]>
RFC
The goal here is to provide both renderer fallback logic, but also expose the renderer to the user in a way they can interact with it from game settings (or via config file for recovery, should that ever be required).
Example with user set configuration that is invalid:
Legacy example: