floooh / sokol

minimal cross-platform standalone C headers
https://floooh.github.io/sokol-html5
zlib License
6.53k stars 468 forks source link

[sokol_gfx] Assign labels as debug names to D3D11 resources #1025

Closed jakubtomsu closed 2 months ago

jakubtomsu commented 2 months ago

This change assigns the "label" field form resource desc structs to D3D11 resource objects, using SetPrivateData with WKPDID_D3DDebugObjectName. This code is enabled only when SOKOL_DEBUG is defined.

Note: this requires sokol_gfx to also link with dxguid.lib.

Note: some resources have multiple "sub-resources", e.g. a shader has a vertex and a fragment shader resource. In those cases the label is the same for all sub-resources.

This is useful especially in RenderDoc (docs%3B)) and in D3D11 debugging messages.

floooh commented 2 months ago

Good change, but I want to give it a whirl before merging.

We should try a change though which I think/hope allows us to get rid of the dxguid dependency:

In sokol_app.h I'm defining any required COM IIDs inline like this:

https://github.com/floooh/sokol/blob/91ec4b5fa95b1e3e3b2e21cfeac85d03cdc9a8ef/sokol_app.h#L6317-L6319

Maybe we can do the same inline definition for the WKPDID_D3DDebugObjectName ID.

I seem to remember that linking with dxguid caused problems in some scenarios (like compiling via Zig which uses the mingw2 headers instead of the Windows SDK).

Also another change request regarding this block:

#if defined(__cplusplus)
#define _sg_d3d11_SetPrivateData(self, guid, DataSize, pData) (self)->SetPrivateData(guid, DataSize, pData)
#else
#define _sg_d3d11_SetPrivateData(self, guid, DataSize, pData) (self)->lpVtbl->SetPrivateData(self, &guid, DataSize, pData)
#endif

Please create a wrapper function like this instead (all D3D API functions called by sokol-gfx have such a wrapper:

https://github.com/floooh/sokol/blob/91ec4b5fa95b1e3e3b2e21cfeac85d03cdc9a8ef/sokol_gfx.h#L9354-L9360

That's all for now :)

floooh commented 2 months ago

PS: did you also check that calling SetPrivateData with a nullptr works in case no label is defined?

jakubtomsu commented 2 months ago

Oh yeah, defining the GUIIDs inline is a very good idea, I didn't notice the ones that were already there, that's why I linked dxguid.

Please create a wrapper function like this instead

The reason why I didn't create a function like this is because SetPrivateData is called on many different types, all child of ID3D11Device. And while the polymorphism works in C++, the C version needs to explicitly use the VTable. So I made a macro similar to _sg_d3d11_Release or _sg_d3d11_AddRef. I'm not aware of any way to make this work in C, apart from writing the function for each resource type. Should I do that instead?

did you also check that calling SetPrivateData with a nullptr works in case no label is defined?

Oops, good catch! Somehow I completely forgot about that, since I always set the labels in my own code. But looking at the docs it should be fine.

floooh commented 2 months ago

The reason why I didn't create a function like this is because SetPrivateData is called on many different types

Ah right, in that case the macro totally makes sense. But can you add a little explanation comment above the macro? Because such things tend to come up after a while again ;)

But looking at the docs it should be fine.

Agreed, looks like nullptr is valid.

jakubtomsu commented 2 months ago

Alright, I added the GUID constant and comments in the last commit

floooh commented 2 months ago

Last changes look good.

I just noticed a problem though I overlooked at first, those strlen calls on potential nullptrs:

(UINT)strlen(desc->label)

..AFAIK strlen on a nullptr is UB.

My suggestion would be to change the _sg_d3d11_SetPrivateData macro into something simpler and remove any redundancies, e.g. for the C++ version:

#define _sg_d3d11_setlabel(self, label) (self)->SetPrivateData(_sg_win32_refguid(_sg_d3d11_WKPDID_D3DDebugObjectName), label ? (UINT)strlen(label) : 0, label)

...calling this would then look a bit simpler:

_sg_d3d11_setlabel(buf->d3d11.buf, desc->label);
jakubtomsu commented 2 months ago

Yeah the setlabel macro is a very good idea, it makes it quite a lot simpler. I didn't know about the strlen UB, thanks for letting me know.

floooh commented 2 months ago

Giving the PR a whirl now...

floooh commented 2 months ago

Nice, it also works in the Visual Studio graphics debugger:

image

floooh commented 2 months ago

And merged. I also added a blurb to the changelog.