-
Notifications
You must be signed in to change notification settings - Fork 419
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
Add OpenGL Core renderer and use as default #5655
Conversation
iOS startup crash: osu.Framework.Graphics.OpenGL.Shaders.GLShader+PartCompilationFailedException: A osu.Framework.Graphics.OpenGL.Shaders.GLShaderPart failed to compile: sh_Texture2D.vs:
ERROR: 0:1: '' : version '330' is not supported
ERROR: 0:1: '' : syntax error: #version
at osu.Framework.Graphics.OpenGL.Shaders.GLShaderPart.Compile() in /Users/dean/Projects/osu-framework/osu.Framework/Graphics/OpenGL/Shaders/GLShaderPart.cs:line 151
at osu.Framework.Graphics.OpenGL.Shaders.GLShader.CompileInternal() in /Users/dean/Projects/osu-framework/osu.Framework/Graphics/OpenGL/Shaders/GLShader.cs:line 124
at osu.Framework.Graphics.OpenGL.Shaders.GLShader.compile() in /Users/dean/Projects/osu-framework/osu.Framework/Graphics/OpenGL/Shaders/GLShader.cs:line 62
at osu.Framework.Threading.ScheduledDelegate.InvokeTask() in /Users/dean/Projects/osu-framework/osu.Framework/Threading/ScheduledDelegate.cs:line 106
at osu.Framework.Threading.ScheduledDelegate.RunTaskInternal() in /Users/dean/Projects/osu-framework/osu.Framework/Threading/ScheduledDelegate.cs:line 92
at osu.Framework.Threading.ScheduledDelegate.RunTask() in /Users/dean/Projects/osu-framework/osu.Framework/Threading/ScheduledDelegate.cs:line 76
at osu.Framework.Graphics.Rendering.Renderer.BeginFrame(Vector2 windowSize) in /Users/dean/Projects/osu-framework/osu.Framework/Graphics/Rendering/Renderer.cs:line 185
at osu.Framework.Graphics.OpenGL.GLRenderer.BeginFrame(Vector2 windowSize) in /Users/dean/Projects/osu-framework/osu.Framework/Graphics/OpenGL/GLRenderer.cs:line 86
at osu.Framework.Graphics.Rendering.Renderer.osu.Framework.Graphics.Rendering.IRenderer.BeginFrame(Vector2 windowSize) in /Users/dean/Projects/osu-framework/osu.Framework/Graphics/Rendering/Renderer.cs:line 1067
at osu.Framework.Platform.GameHost.DrawFrame() in /Users/dean/Projects/osu-framework/osu.Framework/Platform/GameHost.cs:line 482
at osu.Framework.Threading.GameThread.processFrame() in /Users/dean/Projects/osu-framework/osu.Framework/Threading/GameThread.cs:line 451 Android: osu.Framework.Graphics.OpenGL.Shaders.GLShader+PartCompilationFailedException: A osu.Framework.Graphics.OpenGL.Shaders.GLShaderPart failed to compile: sh_Texture2D.vs:
0:60: L0001: Storage qualifier not allowed
at osu.Framework.Graphics.OpenGL.Shaders.GLShaderPart.Compile()
at osu.Framework.Graphics.OpenGL.Shaders.GLShader.CompileInternal()
at osu.Framework.Graphics.OpenGL.Shaders.GLShader.compile()
at osu.Framework.Threading.ScheduledDelegate.InvokeTask()
at osu.Framework.Threading.ScheduledDelegate.RunTaskInternal()
at osu.Framework.Threading.ScheduledDelegate.RunTask()
at osu.Framework.Graphics.Rendering.Renderer.BeginFrame(Vector2 windowSize)
at osu.Framework.Graphics.OpenGL.GLRenderer.BeginFrame(Vector2 windowSize)
at osu.Framework.Graphics.OpenGLCore.GLCoreRenderer.BeginFrame(Vector2 windowSize)
at osu.Framework.Graphics.Rendering.Renderer.osu.Framework.Graphics.Rendering.IRenderer.BeginFrame(Vector2 windowSize)
at osu.Framework.Platform.GameHost.DrawFrame()
at osu.Framework.Threading.GameThread.processFrame()
--- End of stack trace from previous location ---
at osu.Framework.Platform.GameHost.<>c__DisplayClass133_0.<abortExecutionFromException>b__0()
at osu.Framework.Threading.ScheduledDelegate.InvokeTask()
at osu.Framework.Threading.ScheduledDelegate.RunTaskInternal()
at osu.Framework.Threading.Scheduler.Update()
at osu.Framework.Threading.GameThread.processFrame()
at osu.Framework.Threading.GameThread.RunSingleFrame()
at osu.Framework.Platform.ThreadRunner.RunMainLoop()
at osu.Framework.Platform.GameHost.windowUpdate()
at osu.Framework.Platform.GameHost.<Run>b__155_2(Object _, FrameEventArgs _)
at osuTK.GameViewBase.OnUpdateFrame(FrameEventArgs e)
at osuTK.Android.AndroidGameView.UpdateFrameInternal(FrameEventArgs e)
at osuTK.Android.AndroidGameView.RunIteration(CancellationToken token)
at osuTK.Android.AndroidGameView.<>c__DisplayClass58_0.<StartThread>b__2(Object _)
at Android.App.SyncContext.<>c__DisplayClass3_0.<Send>b__0()
at Java.Lang.Thread.RunnableImplementor.Run()
at Java.Lang.IRunnableInvoker.n_Run(IntPtr jnienv, IntPtr native__this)
at Android.Runtime.JNINativeWrapper.Wrap_JniMarshal_PP_V(_JniMarshal_PP_V callback, IntPtr jnienv, IntPtr klazz) |
I'm not entirely sure how make iOS use the core profile since it still uses osuTK 😅 But GL <-> Veldrid needs a compatibility layer anyway (basically some shader macros) so I'll add one in for compat profile until we figure out how to get iOS on the core, or switch away from osuTK (will be required anyway to use Metal). |
FWIW android runs framework visual tests fine with this patch applied: diff --git a/osu.Framework/Resources/Shaders/sh_Precision_Internal.h b/osu.Framework/Resources/Shaders/sh_Precision_Internal.h
index 265b627a6..df6e57da5 100644
--- a/osu.Framework/Resources/Shaders/sh_Precision_Internal.h
+++ b/osu.Framework/Resources/Shaders/sh_Precision_Internal.h
@@ -1,6 +1,6 @@
// This file is automatically included in every shader
-#version 330 core
+#version 300 es
#ifndef GL_ES
#define lowp
iOS not so much. |
{ | ||
ref var currentVertex = ref getMemory().Span[vertexIndex]; | ||
|
||
bool isNewVertex = !currentVertex.Vertex.Equals(vertex) || currentVertex.BackbufferDrawDepth != Renderer.BackbufferDrawDepth; |
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.
Maybe a bit late / the wrong PR, but we should probably be using BackBuffer
in naming.
Confirmed latest commit is working on iOS / android (framework visual tests). |
I'm really not sure why it isn't in this case. My naive guess is that currently in |
I've changed this to use environment variables for the graphics renderer/surface selection for two reasons: 1) I don't want to update every project to pass parameters via |
Are we still blocked on android here? |
Yeah, Android needs a workaround for the library missing exception. Simplest way would be by copying the native library from the upstream repo and placing it inside |
I've took a look at the approach I mentioned above, but it turns out I can't test it well because I apparently can't reproduce the issue in the first place (testing with emulator). The commit is c3cca77 if anyone wants to test it or cherry-pick it here. I've copied the ARM ones directly from the NuGet package folder, but the x86 one was generated manually since the package was providing x86_64 instead. |
Using This is on actual hardware once again. |
From testing with emulator, no native libraries get updated in the output folder unless |
Tested on android hardware to work with 8e090c3. |
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.
Took a slightly quick skim on all commits in this PR and caught some things, mostly minor/quality notes (minus #5655 (comment)).
|
||
int blockBindingIndex = 0; | ||
int textureIndex = 0; | ||
|
||
for (int i = 0; i < uniformCount; i++) | ||
foreach (ResourceLayoutDescription layout in crossCompileResult.Reflection.ResourceLayouts) |
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.
With this change, Uniforms
is no longer being populated, causing TestSceneTextureUnit
to crash. Uniforms
is likely to be removed any time now, but not sure how the crashing test scene would be fixed.
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.
Uniforms
is to be removed, yeah. As for the test scene, it can't really be done that way anymore. You'd have to bind each subsequent texture to unit 0 and draw with that unit.
I think there's no reason for that test scene to exist as TestSceneVideo
already does multiple texturing. I've removed it.
protected override IRenderer CreateRenderer() | ||
{ | ||
if (FrameworkEnvironment.PREFERRED_GRAPHICS_RENDERER != null || FrameworkEnvironment.PREFERRED_GRAPHICS_SURFACE != null) | ||
return base.CreateRenderer(); | ||
|
||
return new WindowsGLRenderer(this); | ||
} |
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 looks like it needs a minor refactor. CreateRenderer
should be changed to CreateDefaultRenderer
and there should be another factory method that would first check the environment variables and then fall back to the GameHost
's default renderer such as the one here. Can be left for another PR though if it embeds too much changes.
That being said, if a user sets environment variables to OpenGL, GLRenderer
would be used instead of WindowsGLRenderer
, which breaks fullscreen capability detection method, wouldn't 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.
Yeah I'm not sure that it's incorrect the way it is right now. Potentially this could also switch on the "gl"/"opengl"/"wingl" renderer types and handle it that way? But I think it's fine for renderer=gl to bypass WindowsGLRenderer
for the time being.
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.
Let's leave this as a follow-up change if required.
|
||
blurParametersBuffer.Data = blurParametersBuffer.Data with |
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'm not sold on the readability of with
...
More details for others unfamiliar with this new expression.
Also correct me if I'm wrong but this looks to be initialising every property in this case, so I think using new
would suffice right?
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.
Also see at least one code inspection where this is to be applied.
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.
Yes, new
would suffice here. with
is only really that helpful in Renderer
itself, where there's a massive buffer that's changed by many methods. I think new
is actually usable for most cases other than that one...
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.
Let's go with it for now. I think it makes sense to use with
in that case because it will handle a property potentially being added to the record in the future more correctly.
This is in as good a state as it will be I think. Have done multiple review passes and tested quite extensively on mobile and macOS. I intend to do some CPU/alloc regression testing on the weekend to make sure nothing has changed too much, but let's get this in as it is and work from there. |
Split out from/improves upon #5647
Abstract
In moving to the new Veldrid renderer, we need to move to using the OpenGL core profile. The primary reason is macOS (on M1 in particular) which does not support the
ARB_uniform_buffer_object
extension, so we couldn't fallback to OpenGL if anything goes wrong with Metal. This extension is mainlined in the OpenGL core profile.This has been done by using SPIRV-Cross, which is included with Veldrid, to cross-compile from "Vulkan GLSL" to GLSL. Quite a few changes to shaders are required as a result, not in the least helped by the fact that we've been using a long-deprecated version of GLSL until now, but at least the shaders will be forward compatible with Vulkan/Metal/HLSL/etc...
Breaking shader changes have been documented below.
Testing
Tested:
vNext
The OpenGL Core profile is now used
Shaders now follow "Vulkan GLSL", which is mostly documented in the spec:
GL_KHR_vulkan_glsl
. The primary differences are documented below:Vertex shader members
The location increases for each additional member in the respective
in
/out
lists.Fragment shader members
The location increases for each additional member, and must match the location of the respective member in the vertex shader.
Fragment shader outputs
Fragment shaders are required to define an output variable.
Uniform members
Free-floating "global" uniforms are not supported. They must be placed in "uniform blocks".
Notes:
std140
should always be used.set
increases for each unique uniform block.binding
is a special required value which indicates where in the program the block should be bound. osu!framework manages this for you and should be 0 for uniform blocks.Furthermore, this means that uniform blocks have become a first-class citizen in osu!framework. The code to make use of the above looks like this:
Runtime validation is present to notify of incorrect alignment/packing.
Uniform textures/samplers
The combined sampler of GLSL is not supported.
Notes:
std140
cannot be applied here unlike for uniform blocks.set
increases along with all other uniform blocks/textures.Sampling textures