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

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

Merged
merged 4 commits into from
Apr 13, 2024

Conversation

jakubtomsu
Copy link
Contributor

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) and in D3D11 debugging messages.

@floooh
Copy link
Owner

floooh commented Apr 12, 2024

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:

sokol/sokol_app.h

Lines 6317 to 6319 in 91ec4b5

static const IID _sapp_IID_ID3D11Texture2D = { 0x6f15aaf2,0xd208,0x4e89, {0x9a,0xb4,0x48,0x95,0x35,0xd3,0x4f,0x9c} };
static const IID _sapp_IID_IDXGIDevice1 = { 0x77db970f,0x6276,0x48ba, {0xba,0x28,0x07,0x01,0x43,0xb4,0x39,0x2c} };
static const IID _sapp_IID_IDXGIFactory = { 0x7b7166ec,0x21c7,0x44ae, {0xb2,0x1a,0xc9,0xae,0x32,0x1a,0xe3,0x69} };

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:

sokol/sokol_gfx.h

Lines 9354 to 9360 in 91ec4b5

static inline HRESULT _sg_d3d11_CreateBuffer(ID3D11Device* self, const D3D11_BUFFER_DESC* pDesc, const D3D11_SUBRESOURCE_DATA* pInitialData, ID3D11Buffer** ppBuffer) {
#if defined(__cplusplus)
return self->CreateBuffer(pDesc, pInitialData, ppBuffer);
#else
return self->lpVtbl->CreateBuffer(self, pDesc, pInitialData, ppBuffer);
#endif
}

That's all for now :)

@floooh
Copy link
Owner

floooh commented Apr 12, 2024

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

@jakubtomsu
Copy link
Contributor Author

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
Copy link
Owner

floooh commented Apr 12, 2024

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.

Also added `_sg_win32_refguid` instead of taking a pointer directly in the macro...
@jakubtomsu
Copy link
Contributor Author

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

@floooh
Copy link
Owner

floooh commented Apr 12, 2024

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
Copy link
Contributor Author

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
Copy link
Owner

floooh commented Apr 13, 2024

Giving the PR a whirl now...

@floooh
Copy link
Owner

floooh commented Apr 13, 2024

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

image

floooh added a commit that referenced this pull request Apr 13, 2024
@floooh floooh merged commit 3226b66 into floooh:master Apr 13, 2024
24 checks passed
@floooh
Copy link
Owner

floooh commented Apr 13, 2024

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

floooh added a commit that referenced this pull request Apr 13, 2024
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