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

Material can specify defines for the shader #6865

Merged
merged 6 commits into from
Aug 2, 2024
Merged

Conversation

mvaligursky
Copy link
Contributor

@mvaligursky mvaligursky commented Jul 30, 2024

Fixes #2962

New public API

Material.setDefine(name, value);

This allows to set values to user defines on a shader. The define is then added to both vertex and fragment shader, allowing a user defined behaviour.

Usage:

// set define on the material. This will force all shaders to be recompiled with this define.
material.setDefine('TEST1', true);
material.setDefine('TEST2', true);

// inside the vertex and fragment shaders, lines like this will be automatically added:
#define TEST1
#define TEST2

// allowing user to test against it
#ifdef TEST1
   // do something
#end

StandardMaterial, ShaderMaterial, GSplat materials and LitMaterial support this.

Examples of shader customisations:

Screenshot 2024-07-30 at 14 44 56 Screenshot 2024-07-30 at 14 45 17

Copy link
Member

@slimbuck slimbuck left a comment

Choose a reason for hiding this comment

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

I don't really understand why you're exposing defines without values?

@mvaligursky
Copy link
Contributor Author

I don't really understand why you're exposing defines without values?

We don't need values, in shader code we only test if it exists.
And additionally, our preprocessor does not handle expressions in defines either currently.

@slimbuck
Copy link
Member

slimbuck commented Aug 1, 2024

What do you mean by "we don't need values in shader code"?

As you know, defines are very useful as compile time parameters, we use them everywhere.

Here is an example: https://github.com/playcanvas/engine/blob/main/src/scene/shader-lib/chunks/common/frag/reproject.js#L14C8-L14C15

The alternatives (hard-coded value or uniform) are both worse options.

@mvaligursky
Copy link
Contributor Author

I see your point, but these are more like a boolean switches for shader features.
We simply do not support generic values / expression in preprocessor. Other than a numerical value.
We can add it later, but this PR uses existing functionality.

@slimbuck
Copy link
Member

slimbuck commented Aug 1, 2024

So my is question is why particularly limit it this way?

Adding support later is possible, but that would (presumably) break public API.

I'm also still curious what doesn't work with defines, given we have lots of this in the engine: https://github.com/playcanvas/engine/blob/main/src/scene/shader-lib/chunks/lit/frag/shadowPCSS.js#L10

@mvaligursky
Copy link
Contributor Author

for example this does not work:

#define XX 3
#if XX == 3
#endif

we do not handle expressions inside ifdef, we only test for the existance.

And if defines would have values, it'd be expected that they can be used this way.

We could add this without breaking API in the future though. setDefine could accept other values than boolean.

@slimbuck
Copy link
Member

slimbuck commented Aug 1, 2024

#define XX 3
#if XX == 3
#endif

What would happen in the case above? WebGL2 shader works because of glsl preprocessor, but WebGPU fails to compile?

@mvaligursky
Copy link
Contributor Author

Yep, the preprocessor would assert and fail here:

Debug.assert(correct, `Resolving expression like this is not supported: ${expression}`);

@mvaligursky
Copy link
Contributor Author

actually, even WebGL would fail here, we preprocess WebGL shader the same way.

@slimbuck
Copy link
Member

slimbuck commented Aug 1, 2024

Ok thanks for explaining. This limitation doesn't change or mitigate the usefulness of define values as constants IMO. And the error would help stop developers from trying it quite early on.

So unless it requires major restructuring work or similar, I'd still vote to support arbitrary define values in material, otherwise it seems like a half feature.

@willeastcott
Copy link
Contributor

Yeah, I think I agree with @slimbuck on that.

@mvaligursky
Copy link
Contributor Author

I'll add it tomorrow, but I'm not sure it's that useful for shaders used by the forward renderer. For example we do not have a single user case like this currently in whole standard material shader generator. This is more useful for postprocessing and similar shader, for which ShaderMaterial is not applicable.

The boolean only format would allow us to easily expose these to the Inspector in the Editor as boolean toggles for material features. Generic data (that user needs to type in) is less ideal here in the Inspector.

@mvaligursky
Copy link
Contributor Author

Added support for define values. As discussed, what can be done with the values is limited.

@mvaligursky mvaligursky requested a review from slimbuck August 2, 2024 09:43
@mvaligursky mvaligursky requested a review from slimbuck August 2, 2024 11:21
@mvaligursky mvaligursky merged commit 86e79ae into main Aug 2, 2024
8 checks passed
@mvaligursky mvaligursky deleted the mv-shader-defines branch August 2, 2024 12:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: graphics Graphics related issue enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request] Add shader recompile mechanism
3 participants