Skip to content

Conversation

@davidd-lunarg
Copy link
Contributor

Each shader identifier should be globally unique so it isn't necessary to track shader IDs per state object.

Below are supporting statements from the DXR spec:

Shader identifier
"A shader identifier is an opaque data blob of 32 bytes that uniquely identifies (within the current device / process) one of the raytracing shaders"

"An application might create the same shader multiple times. This could be the same code but with same or different export names, potentially across separate raytracing pipelines or collections of code (described later). In this case the seemingly identical shaders may or may not return the same identifier depending on the implementation. Regardless, execution behavior will be consistent with the specified shader code."

Regarding differing shader IDs on duplicate shaders:

Update v1.24 05/30/2024 - "In GetShaderIdentifier removed the sentence: "The data itself globally identifies the shader, so even if the shader appears in a different state object (with same associations like any root signatures) it will have the same identifier.". This contradicted the text in Shader identifier that allows for duplicate shaders to have the same or different identifier depending on implementation."

The spec indicates that any shader with associated root signatures
should have a unique, global shader ID.
@ci-tester-lunarg
Copy link
Collaborator

CI gfxreconstruct build queued with queue ID 632841.

@ci-tester-lunarg
Copy link
Collaborator

CI gfxreconstruct build # 8703 running.

@ci-tester-lunarg
Copy link
Collaborator

CI gfxreconstruct build # 8703 passed.

@davidd-lunarg davidd-lunarg marked this pull request as ready for review January 28, 2026 21:14
@davidd-lunarg davidd-lunarg requested a review from a team as a code owner January 28, 2026 21:14
Copy link
Contributor

@MarkY-LunarG MarkY-LunarG left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good from a code standpoint, but I don't know if the functionality is correct, so I'll leave that for someone else to look over. It appears to do the same thing as before with the only exception being the new warning.

Maybe @bradgrantham-lunarg or @locke-lunarg


struct ShaderIdAssociationInfo
{
std::set<format::HandleId> state_object_ids;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you saving this? It doesn't appear used.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is saved for reference, to allow developers to determine which state object(s) contained this shader. For example, useful for debugging.

@davidd-lunarg
Copy link
Contributor Author

Thanks for taking a look.

It appears to do the same thing as before with the only exception being the new warning.

The functional difference of this PR is contained in the first commit Use a global map to track Shader ID - LRS data. Previously the shader_id_lrs_map was stored and looked up per state object. However the IDs are global as stated in the spec, and this lead to issues where a state object can be created from other state objects (e.g., AddToStateObject or from an existing collection subobject) that weren't being tracked correctly. Tracking globally avoids the complication of building up the map between dependent state objects.

The second commit is for preserving information about which shader IDs came from which state objects. This doesn't have a functional difference on replay--it is for verification and debugging purposes. The shader ID to LRS mapping can be complicated to resolve, and the small amount of additional work to track this meta info should be useful to have around in the future.

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.

3 participants