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

VideoCommon: Add names for textures and shaders #10269

Merged
merged 1 commit into from Jan 3, 2022

Conversation

Pokechu22
Copy link
Contributor

This continues the work started in #10065. Names are now present for all shaders other than the generated pixel/vertex shaders used for emulation and for all textures except the ones used in emulation (as there are a lot of these, and I haven't thought of a good way to create unique names for them). Names are also not created for shaders created from a binary, but that only applies to shaders used for emulation.

@iwubcode
Copy link
Contributor

iwubcode commented Dec 10, 2021

Very nice work @Pokechu22 . I'd also like to expose a way to do event regions: https://renderdoc.org/docs/how/how_annotate_capture.html#application-provided-marker-regions but haven't had time to implement that yet.

Do note there is a bug in d3d12's compute shader implementation that causes a crash. I have a fix for that, I need to push it up (haven't updated to VS2022 yet).

@iwubcode
Copy link
Contributor

all textures except the ones used in emulation (as there are a lot of these, and I haven't thought of a good way to create unique names for them)

Do you mean the ones in the texture cache? I'd suggest using GetFullName() which is the common name texture pack authors are used to seeing . Though not sure how much of a perf impact that'd have (?)

@Pokechu22
Copy link
Contributor Author

GetFullName() would be a good choice (the possible performance issue (if there is one) could be worked around by only calling it if g_ActiveConfig.bEnableValidationLayer is set, since that also enables other debug functions). However, what we have is an instance of TextureConfig, not TextureInfo, so GetFullName() isn't available.


For what it's worth, I've also made the following change:

Patch
diff --git a/Source/Core/VideoCommon/ShaderCache.cpp b/Source/Core/VideoCommon/ShaderCache.cpp
index 257650713c..27edf8d3dc 100644
--- a/Source/Core/VideoCommon/ShaderCache.cpp
+++ b/Source/Core/VideoCommon/ShaderCache.cpp
@@ -422,8 +422,7 @@ std::unique_ptr<AbstractShader> ShaderCache::CompileVertexShader(const VertexSha
 {
   const ShaderCode source_code =
       GenerateVertexShaderCode(m_api_type, m_host_config, uid.GetUidData());
-  return g_renderer->CreateShaderFromSource(ShaderStage::Vertex, source_code.GetBuffer(),
-                                            "Dolphin vertex shader");
+  return g_renderer->CreateShaderFromSource(ShaderStage::Vertex, source_code.GetBuffer());
 }

 std::unique_ptr<AbstractShader>
@@ -431,16 +430,14 @@ ShaderCache::CompileVertexUberShader(const UberShader::VertexShaderUid& uid) con
 {
   const ShaderCode source_code =
       UberShader::GenVertexShader(m_api_type, m_host_config, uid.GetUidData());
-  return g_renderer->CreateShaderFromSource(ShaderStage::Vertex, source_code.GetBuffer(),
-                                            "Dolphin vertex UberShader");
+  return g_renderer->CreateShaderFromSource(ShaderStage::Vertex, source_code.GetBuffer());
 }

 std::unique_ptr<AbstractShader> ShaderCache::CompilePixelShader(const PixelShaderUid& uid) const
 {
   const ShaderCode source_code =
       GeneratePixelShaderCode(m_api_type, m_host_config, uid.GetUidData());
-  return g_renderer->CreateShaderFromSource(ShaderStage::Pixel, source_code.GetBuffer(),
-                                            "Dolphin pixel shader");
+  return g_renderer->CreateShaderFromSource(ShaderStage::Pixel, source_code.GetBuffer());
 }

 std::unique_ptr<AbstractShader>
@@ -448,8 +445,7 @@ ShaderCache::CompilePixelUberShader(const UberShader::PixelShaderUid& uid) const
 {
   const ShaderCode source_code =
       UberShader::GenPixelShader(m_api_type, m_host_config, uid.GetUidData());
-  return g_renderer->CreateShaderFromSource(ShaderStage::Pixel, source_code.GetBuffer(),
-                                            "Dolphin pixel UberShader");
+  return g_renderer->CreateShaderFromSource(ShaderStage::Pixel, source_code.GetBuffer());
 }

 const AbstractShader* ShaderCache::InsertVertexShader(const VertexShaderUid& uid,

This is because giving names for those results in RenderDoc always using them, but this results in ambiguity, as the names are always the same (all of them show up as "Dolphin vertex shader"); not supplying a name results in "Shader Module 440" or similar showing up, with a unique number. So, until detailed names are available for those generated shaders, it's probably best not to apply any name.

@iwubcode
Copy link
Contributor

iwubcode commented Dec 10, 2021

GetFullName() would be a good choice (the possible performance issue (if there is one) could be worked around by only calling it if g_ActiveConfig.bEnableValidationLayer is set, since that also enables other debug functions). However, what we have is an instance of TextureConfig, not TextureInfo, so GetFullName() isn't available.

We do have a TextureInfo as it is passed into GetTexture() so it could be used here: https://github.com/dolphin-emu/dolphin/blob/master/Source/Core/VideoCommon/TextureCacheBase.cpp#L1550 if passed down. There are other places like the reinterpret and the palette where we are only given the config or the cache entry pointer, so we'd have to save that off somehow but at least for "normal" textures, it could be done rather easily. But is definitely a bigger task and can be left for another PR

This is because giving names for those results in RenderDoc always using them, but this results in ambiguity, as the names are always the same (all of them show up as "Dolphin vertex shader"); not supplying a name results in "Shader Module 440" or similar showing up, with a unique number. So, until detailed names are available for those generated shaders, it's probably best not to apply any name.

Could we use uid possibly? It's fine leaving it as, not sure how useful the context of knowing it was a vertex/pixel/uber shader was (vertex/pixel is something you can get from just looking at the pipeline).

@Pokechu22
Copy link
Contributor Author

Could we use uid possibly?

That's definitely possible, and I've already done something similar for geometry shaders... it's just that the UID has a lot of information in it, so it would result in a very large name (the names created here are already somewhat long (I think the EFB to VRAM or maybe EFB to RAM one ends up being multiple lines (though in a large font)), and including everything in the UID would result in a stupid-long name probably). It might be fine to just use the number of texgens and such (the information that ends up in the comment at the top of the shader), but I think it'd be better to address this in a later PR.

@Pokechu22 Pokechu22 force-pushed the graphics-debug-details branch 2 times, most recently from f10c95c to 1282aa6 Compare December 22, 2021 23:53
@leoetlino leoetlino merged commit f6883a0 into dolphin-emu:master Jan 3, 2022
10 checks passed
@brujo5
Copy link

brujo5 commented Jan 3, 2022

android build crash when launch game with opengl and vulkan since this version. thx

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants