-
-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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: Use Silk.NET Bindings #6788
base: master
Are you sure you want to change the base?
Conversation
I don't quite see the benefits of replacing OpenTK now. It's working fine and it's not like we are held back by it in any way. Is there a special reason you did this? Did you want to do something that wasn't possible with OpenTK? |
I initially started looking into this as I was interested in trying Silk Windowing over SPB (Less for us to maintain, and would resolve some of the jank/confusion of using SPB as talked about with yam on the Discord). The bindings are mostly pretty similar, but there are benefits to the Silk bindings.
|
public static bool SupportsQuadsCheck(GL api) | ||
{ | ||
api.GetError(); // Clear any existing error. | ||
#pragma warning disable CS0618 // Type or member is obsolete |
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.
This function should be rewritten not to use deprecated OpenGL calls, as they will prevent Renderdoc from performing a capture. This was an issue with the previous HwCapabilities
implementation too, but due to the Lazy nature, it would only get triggered if a game requested a draw with quads. As all caps are now init'd at the start of the Renderer, this function gets called regardless of whether the game requests quads or not.
Another route would be to remove the use of GL_QUAD
and GL_QUAD_STRIP
entirely and just use the existing vertex remapping draw calls when they are requested (similar to Vulkan backend).
I'm personally more in favour of that approach as it gives us control over the conversion of quads to triangles, and were not subject to potential poor driver implementation of this deprecated feature.
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 there is a way to make it not use deprecated features. We'd have to remove it and assume that quads are never supported. I think ideally it should just check if renderdoc is attached, and disable it in this case.
There is also alpha test which I believe is deprecated too, but I think we always assume it is supported right now.
Tested with: Balatro, Hat in Time, Deltarune, Kirby & Forgotten Land, MK8D, TOTK (broken cause asahi), P5R, Arceus, Scott Pilgrim, Undertale, Mania, Wonder, and ACNH. |
Currently crashing on Windonws in GTK and Avalonia when emulation is stopped. You might have to bind the context with INativeContext that is how I was able to stop it IIRC, but I will have to confirm later. Also am not sure if threaded rendering is working correctly, because the context is passed before the background context is created. Solving this might solve GTK crash also. EDIT: Will post logs later. |
# Conflicts: # src/Ryujinx.Graphics.OpenGL/OpenGLRenderer.cs
Silk.NET bindings are rigorously maintained, backed by the .NET Foundation, and take full advantage of the latest .NET features.
By comparison, OpenTK is rather slow on the uptake and lacks much of the serious backing Silk benefits from.
This PR migrates our OpenGL backend from the OpenTK bindings to Silk.NET. The migration should result in no user-facing changes (maybe a performance uplift if we're lucky).
Changes / Differences in OpenTK and Silk.NET bindings:
HwCapabilities
implementation has been rewritten to follow the Vulkan backend's model as the Silk bindings perform API calls on a givenGL
instance, not in a static context.OpenGLRenderer
class, I've only supplied the currentGL
instance. We may want to change everything to take theOpenGLRenderer
for consistency's sake, but I'll leave that up to the reviewer.uint
.uint
instead ofint
.Legacy
name space. This includes enums likeGL_CLAMP
andGL_QUAD_STRIP
. Ryujinx does use these deprecated APIs in some places (as they're still present in Maxwell IR), so we have to use the Legacy namespace instead of the regular one (using both causes horrible redefinitions).