Skip to content

Draft: Initial debug utils naming implementation #1419

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

AnyOldName3
Copy link
Contributor

Adds names to some objects (pipelines, shader stages, and pipeline layouts so far) via VK_EXT_debug_utils. This means that in tools like RenderDoc, they get names instead of numbers, and in the logs from validation errors or the API dump layer, they get names instead of pointers, making it possible to have some idea of what was being drawn without needing RenderDoc.

It's a draft because the performance impact even when it's off isn't zero. I'm not sure anything can be done about that other than gating it off behind a #define, and therefore a CMake option. Even having names in the scenegraph without telling Vulkan about them isn't free as it means a vsg::Auxiliary needs to exist for the object, meaning extra memory usage (although vsg::Auxiliary uses the system allocator, so hopefully won't end up wasting space in hot cache lines).

For now, it's still potentially useful to temporarily merge this branch while investigating a problem and then discard the changes once it's been solved, but I hope we can come up with something more maintainable.

The built-in shader sets are altered because they've been given names. A counterpart PR to vsgExamples will be opened shortly.

@robertosfield
Copy link
Collaborator

Had a quick look through and first reaction is jikes that intrusive.

Second reaction is, would vsg::GpuAnnoation not be useful and be something would at least interact with any additional features related to shader debug utils.

@AnyOldName3
Copy link
Contributor Author

I needed to label the objects I've labelled to fix the problem I was working on. vsg::GpuAnnotation won't tell people which shaderset with which defines a pipeline layout was intended to be used for and which shaderset you're trying to use it with, but this can.

When Vulkan objects are created, it's similarly invasive to the use of debugObjectLabel in the OSG in that it needs doing everywhere the objects are created, but it's a little more verbose because we've got to set up a struct instead of just pass three parameters to a function, and there's no getName() in vsg::Object like there was in osg::Object, so the concept needs to be invented specifically for this feature.

As for generating names, in the OSG, the objects a programmer would interact with and be able to assign names to were typically 1:1 with OpenGL objects, at least within the same context, so a name could be set and directly used. Here, though, a lot of Vulkan objects of different types, with several instances of each type, end up being created behind the scenes for one thing the programmer explicitly sets up, so names need transferring to the implicitly-generated objects, and annotating with extra information to identify different variants.

It could be made less visibly bulky by adding setDebugName and a templated recordDebugName to vsg::Object, but that would really just be syntax sugar rather than a meaningful change to how things worked. It'd still need to call recordDebugName everywhere debug names are currently recorded, and names would still need generating for implicit objects.

I'm aware that something being intrusive in the OSG isn't a good justification for it to be intrusive here, hence lots of things being pared back, but give Vulkan objects a name when they're created means they need to have a name and it needs assigning when they're created, so something's got to give them a name and the functions that creates them needs to assign it. In a world where RenderDoc considers supporting applications that emit validation errors out of scope, there's no better way to know what an object is than by giving it a meaningful name.

Obviously, it's a lot harder to identify what's broken with a message like

ERROR: [Vulkan] Validation Error: [ VUID-vkCmdDraw-None-08600 ] Object 0: handle = 0xaaa64600000018b7, type = VK_OBJECT_TYPE_PIPELINE; Object 1: handle = 0xec25c90000000093, type = VK_OBJECT_TYPE_PIPELINE_LAYOUT; | MessageID = 0x4768cf39 | vkCmdDraw(): The VkPipeline 0xaaa64600000018b7[] (created with VkPipelineLayout 0xafc92d00000018b4[]) statically uses descriptor set 1, but all sets 0 to 1 are not compatible with the pipeline layout bound with vkCmdBindDescriptorSets (VkPipelineLayout 0xec25c90000000093[])

than it is with

ERROR: [Vulkan] Validation Error: [ VUID-vkCmdDraw-None-08600 ] Object 0: handle = 0xed822f00000001e9, name = VSG built-in text ShaderSet with defines: BILLBOARD GPU_LAYOUT (ED822F00000001E9), type = VK_OBJECT_TYPE_PIPELINE; Object 1: handle = 0xec25c90000000093, name = Layout for [Application name redacted] PBR ShaderSet (EC25C90000000093), type = VK_OBJECT_TYPE_PIPELINE_LAYOUT; | MessageID = 0x4768cf39 | vkCmdDraw(): The VkPipeline 0xed822f00000001e9[VSG built-in text ShaderSet with defines: BILLBOARD GPU_LAYOUT (ED822F00000001E9)] (created with VkPipelineLayout 0xe0a4d800000001e6[Layout for VSG built-in text ShaderSet with defines: BILLBOARD GPU_LAYOUT (E0A4D800000001E6)]) statically uses descriptor set 1, but all sets 0 to 1 are not compatible with the pipeline layout bound with vkCmdBindDescriptorSets (VkPipelineLayout 0xec25c90000000093[Layout for [Application name redacted] PBR ShaderSet (EC25C90000000093)])

Whether or not this, or something derived from it, ends up merged, I'm going to keep using it in situations where it'll tell me things I need to know, but it'll be a lot less hassle if it's just a window traits option or CMake flag than if I have to reapply this patch and resolve any conflicts that appeared since the last time I used it.

@robertosfield
Copy link
Collaborator

I have just done another code review. I'm currently pondering about adding name fields to ShaderModel/ShaderSet/PipelineLayout, which can be used for general developer housekeeping as well provide an input for debug utils is enabled. I will let this topic sit for a little longer to see how I feel given more time. I have plenty of other pressing work to get on with so tackle these and come back to this one. I think it's worthwhile coming up with some form of solution so don't think a temporary is required.

@AnyOldName3
Copy link
Contributor Author

I think it's likely that at some point an application will have a bug that warrants labelling something else, especially vertex buffers or textures with the file they were loaded from (or even the node for formats like glTF that let things have names), and maybe render passes. Some of that can already be done with vsg::GpuAnnotation, e.g. by adding vsg::InstrumentationNode as a parent of a loaded model to set its filename, but if someone's trying to fix a bug, we know there's a bug, so can't necessarily trust everything to work, and it's easier to detect if a vertex buffer's somehow ended up in the wrong place if the buffer itself has been labelled rather than relying on it being identified by the context it's used within.

As anything in VkObjectType can be labelled, it's probably worth taking the time to design a solution that works in lots of situations. I wouldn't have given this the Draft: prefix if I thought it was already ready for mainline VSG.

@robertosfield
Copy link
Collaborator

Good point about making the functionality as general purpose as we can, potentially we might be able to simply the changes/usage in the process.

Looking at the docs:
https://registry.khronos.org/vulkan/specs/latest/man/html/VkDebugUtilsObjectNameInfoEXT.html
https://registry.khronos.org/vulkan/specs/latest/man/html/VkObjectType.html

We'd need the vkObjectType as well objectHandle from the VSG/Vulkan integration and the name to associate the Vulkan object.

Perhaps GpuAnnotation could have a feature to traverse a subgraph and find the Vulkan objects, their type and then set up names for them, such as getting a user value. This way the functionality wouldn't need to be intrusive to the wider VSG codebase, and only applied when users need to annotate for the purposes of using API output/RenderDoc.

@AnyOldName3
Copy link
Contributor Author

Possibly. The concerns I'd have with that kind of approach are:

  • We don't keep enough data around to figure out a good name for most objects. Once the GraphicsPipelineConfigurator has set up a subgraph, there's no pointer to the shaderset that was used etc., so there'd need to be some bookkeeping to leave enough of a paper trail to figure out which names to use for which objects. That could mean keeping the invasive name-generating parts when creating VSG objects but removing the invasive Vulkan-object-labelling parts when compiling them.
  • While it'd work for draw calls after everything's set up, if someone's investigating a problem that happens in how something's initially set up, labels won't get retroactively applied to calls that have already been logged. Seeing a pipeline get created with layout: 0xc5a5c666 and having a call much later giving 0xc5a5c666 a name is less immediately useful than seeing it created with layout: 0xc5a5c666 (Layout made for <some other shaderset>).
  • With the right kind of bug, we might end up with one value for a field set when compiling an object and another value set when the visitor applied labels, so things could end up labelled incorrectly.

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

Successfully merging this pull request may close these issues.

2 participants