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

Track File Dependencies in ShaderStage #1032

Merged

Conversation

solid-angle
Copy link
Contributor

Changes
This change allows an application to retrieve file dependencies from generated shaders caused by shader includes (via ShaderStage::addInclude) and SourceCodeNode source blocks taken from a file.

We add ShaderStage::getIncludes, ShaderStage::addSourceDependency, and ShaderStage::getSourceDependencies. SourceCodeNode is modified to call addSourceDependency.

Testing
A small unit test is added to verify getSourceDependencies returns valid files. We didn't want to make the test hardcode any dependencies as that would make it break when other parts of the library changed the set of dependencies.

Motivation
Use case: an application may want to cache generated or compiled shaders. In order to do a dependency check on the cache, it needs access to the set of input files that went into generation. There was no way to retrieve these two types of dependencies using the existing API.

This change allows shader generator code to look at file dependencies in ShaderStages.
This is done by adding the file path of a SourceCodeNode to the ShaderStage's set of dependencies whenever a function definition is emitted.
Also, ShaderStage is changed to allow returning the set of include files that it requires.
Includes a unit test to check that a material's shader dependencies actually exist as files.
@solid-angle solid-angle marked this pull request as ready for review July 18, 2022 17:12
@kwokcb
Copy link
Contributor

kwokcb commented Jul 18, 2022

Hi @solid-angle,

A few notes:

  • First that source code file existence is checked within the generation "context", inside SourceCodeNode::initialize().
    This would seem to be a more suitable place for caching on a GenContext as opposed to when code is being emitted to the Stage.
  • Second I don't know what lifetime you want for this but a generator and context can be reused for > 1 generation
    so some "clearing" mechanism is required.

@solid-angle
Copy link
Contributor Author

Well I really believe these dependencies are associated with a particular generated shader stage, so I'm not sure having the API on the context makes sense - I was trying to mirror how includes were handled, and keep the changes minimal.

I think what makes this somewhat confusing is ShaderStage handles caching includes but GenContext handles caching source files. Perhaps it would make most sense to separate caching from dependency tracking, make GenContext responsible for caching both includes and source files, and track dependencies on ShaderStage.

I'm open to reworking this but I think that inconsistency needs to be resolved.

@kwokcb
Copy link
Contributor

kwokcb commented Jul 18, 2022

Thanks for the clarification and info. I didn't look at include scope at the stage level. Adding source caching at the Stage level makes sense and is consistent then.

Copy link
Member

@jstone-lucasfilm jstone-lucasfilm left a comment

Choose a reason for hiding this comment

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

This looks like a good improvement, @solid-angle, and I had just one question about the data type of the new method argument and stored data.

Copy link
Member

@jstone-lucasfilm jstone-lucasfilm left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks @solid-angle!

@jstone-lucasfilm jstone-lucasfilm merged commit a255e89 into AcademySoftwareFoundation:main Jul 26, 2022
Michaelredaa pushed a commit to Michaelredaa/MaterialX that referenced this pull request Oct 21, 2023
This change allows an application to retrieve file dependencies from generated shaders caused by shader includes (via ShaderStage::addInclude) and SourceCodeNode source blocks taken from a file.

We add ShaderStage::getIncludes, ShaderStage::addSourceDependency, and ShaderStage::getSourceDependencies. SourceCodeNode is modified to call addSourceDependency.
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