GPU: Do not allow GPU Shader builder when USD is enabled.
ClosedPublic
Actions

Authored by Jeroen Bakker (jbakker) on Jul 11 2022, 1:53 PM.

Details

Reviewers

Ray Molenkamp (LazyDodo)

Commits

rB76d86142367e: GPU: Do not allow GPU Shader builder when USD is enabled.

Summary

Linking GPU shader builder requires stubs for many functions of the USD library.
We don't want to rely on other modules to update the stubs for a tool that
is only used by GPU developers.

This patch raises an error when both WITH_GPU_SHADER_BUILDER and WITH_USD are
enabled. This reduces the maintenance of updating the stubs when USD API changes.

Diff Detail

Repository

rB Blender

Branch

temp-gpu-shader-with-usd-error (branched from master)

Build Status

Buildable 22936
Build 22936: arc lint + arc unit

Event Timeline

Jeroen Bakker (jbakker) requested review of this revision.Jul 11 2022, 1:53 PM

Jeroen Bakker (jbakker) created this revision.

Harbormaster completed remote builds in B22936: Diff 53510.Jul 11 2022, 1:53 PM

Jeroen Bakker (jbakker) added a reviewer: Ray Molenkamp (LazyDodo).Jul 11 2022, 1:54 PM

Hi Ray,

There are several ways to solve this issue.

Add the USD API stubs
Automatically disable USD. I don't like as we are doing something that a user/developer should have control over.
Raise an error when compilation is known to fail. (this patch)

Just a checkup if the solution is fine and wording is correct. Build scripts aren't my specialism :-)

While i don't mind given it's an internal tool for GPU devs and on top of that one that is off by default, I can't help pondering why this is depending on USD at all, linking half of the blender core in a shader compiler feels.....strange, I suspect some architectural changes can/should be done here.

Also please validate if i understand correctly, WITH_GPU_SHADER_BUILDER runs all our glsl though the opengl shader compiler of whatever gpu is configured for opengl at build time, no? If so, wouldn't WITH_BUILDTIME_GPU_SHADER_VALIDATION (or something like it, not married to this name) be a more suitable name for this option?

This revision is now accepted and ready to land.Jul 11 2022, 2:59 PM

Thanks for the quick feedback.
Yes it is a built time only option to validate that we didn't break other shaders along the way. In future this will also go through shadercompiler to validate vulkan. Will rename the option to be more specific.
Yes, having more looser components would help here. Will check with @Sybren A. Stüvel (sybren) if this is a known issue or could be 'solved'.

Closed by commit rB76d86142367e: GPU: Do not allow GPU Shader builder when USD is enabled. (authored by Jeroen Bakker (jbakker)). · Explain WhyJul 11 2022, 4:12 PM

This revision was automatically updated to reflect the committed changes.

Jeroen Bakker (jbakker) added a commit: rB76d86142367e: GPU: Do not allow GPU Shader builder when USD is enabled..

Honestly sounds more like the shader compiler needs to be less needy in its requirements rather than sybren needing to make changes to USD, didn't check but feels like the shader compiler has a dependency on blenkernel which is likely be better off without

Checked with @Clément Foucault (fclem), we will rename it to WITH_BUILDTIME_GPU_SHADER_COMPILER.
Mainly that this tool will also be used to compile SpirV shaders not only validation.
It is currently not known if the buildtime compilation will eventually become a hard requirement.

For a dev only tool, i'm perfectly happy with wonky options being enforced like you can't have USD, if we want to turn this on for the general public, this is unacceptable however, you guys have to trim the fat, all other helper tools are essentially standalone (with maybe a png dep) this one dragging in half of blender for reasons unknown needs a closer look before making it mandatory.

Revision Contents
Changeset List

Path

Size

CMakeLists.txt

6 lines

Diff 53510

View Options

GPU: Do not allow GPU Shader builder when USD is enabled.ClosedPublicActions