Module parameters proposal, take 2#164
Conversation
|
Should we make it less verbose by changing it to I took a quick look, and my initial comments are We should also disallow the following cases, or have explicit rules for them
Not a problem yet:
|
mighdoll
left a comment
There was a problem hiding this comment.
Perhaps we should also add numeric @param const values in this revision? even w/o using them in conditions, that gets us out of the nonstandard constants:: we're doing now..
|
I think it might be worth making a table or a flow diagram about override vs. param vs. const... rough table idea:
(later we'd a column if param is settable from import statements.) rough diagram idea: |
|
I note that |
|
@stefnotch Thinking about it, I would like to amend the proposal: "all consts initialized with a simple literal can be used in @mighdoll Ok to add numeric params in this revision. |
|
Presumably, And presumably, |
|
Very relevant here: Section "4.2.6 Global Generic Type Parameters" from the Slang paper https://kilthub.cmu.edu/articles/thesis/Slang_A_Shader_Compilation_System_for_Extensible_Real-Time_Shading/16826602?file=31117213 I do encourage reading that section. A rough summary of it is: Slang has generic entry points. They let you specialize a shader. float3 entryPoint<M : IMaterial, L : ILightingEnv>(
Camera camera,
M material,
L lights,
SurfaceGeometry geometry)
{
float3 viewDir = normalize(camera.P - geometry.P);
M.Pattern bxdf = material.evalPattern(geometry);
return lights.illuminate(bxdf, viewDir);
}but there's a lot of existing HLSL code.
Some of it was hard to convert. So they added a mechanism for that. type_param M : IMaterial;
type_param L : ILightingEnv;
ParameterBlock<M> gMaterial;
ParameterBlock<L> gLights;
ParameterBlock<Camera> gCamera;
float4 main(SurfaceGeometry geom)
{
float3 viewDir = normalize(gCamera.P - geometry.P);
M.BxDF bxdf = gMaterial.evalPattern(geometry);
return gLights.illuminate(surface, viewDir);
}
And then they link to section 7.2.3 where they explain why the
The problem there is that one runs into "Confusing circular dependency scenarios" when one starts doing simple kinds of reflection. To create a shader with The param const equivalent is param const X: u32;
const Y = X + 1;and then trying to read the value of In HLSL/Slang, this problem is much worse, since they have methods directly in their struct definitions. So here, I couldn't use reflection to read type_param T;
struct D
{
float doSomething()
{
T t; ...
}
}; |
|
@stefnotch it took me a while to understanding but your slang feedback is interesting. It doesn't affect the current proposal, since param consts are currently not overridable from shader code. But it's something that we'll maybe want to do in the future 🤔 |
|
param const in dependencies seem to be trickier than I thought. TL;DR: we need at least re-exports to finish this proposal. Possibly we need to allow overriding param consts from shader code too.
|
I agree that packages will need a way to control what's published to the host from their imported libraries. I'd thought perhaps
I think we had earlier discussed having the ability to allow importers to re-set const values. I agree that we want app shaders to have that ability to set param const values from library declared param consts. Potentially we could use that same syntax to allow alternate root shaders in the app package to set param consts delared on another app package module.
Do we need to expose module paths in the host interface to address param consts at all? We could require that param consts be
How 'bout if we start by declaring this an error. We can consider relaxing that restriction later (without causing any breakage if an error case now becomes a new feature).
In this case perhaps we'd reflect Y as a const of type u32 with a value of 'runtime-injected'. I note that we already have a variant of this problem due to conditions, for the same reason. For sure, flexible runtime control makes reflection more complicated. |
That absolutely does work, even if it has a few annoying implications. (Using a param const is a breaking change from a semver perspective despite the const having the same type, ...) It's probably also the best we can do, given the current design of param const. If however, we went back to what existing programming languages have, we don't have such problems.
|
|
For this example @stefnotch: I don't see how that's an issue. If I guess if you have an |
|
re @mighdoll
I'm not super satisfied with the error. I think duplicating the package might be a better solution. We haven't closed the question of dependency unification yet (#21). Apparently we settled on that a while ago in #21 (comment). Note, we only have to effectively duplicate packages, i.e. it should behave as if it was duplicated, but identical declarations can be unified to reduce code generated.
yes, this is my preferred approach too. I can update the proposal to reflect that. |
|
re @stefnotch Do you think we should postpone this proposal until we settle on a design for generics? Maybe, a good generics design would fulfill most/all use-cases of conditional compilation. And cond-comp is error-prone and hard to reason about. Param consts do look and feel like ML module constructors, and it was the design I had in mind in the original proposal (#34), but now there are 2 important differences:
|
|
I think we can defer it until we have some generics prototypes. Unless there are some pressing use-cases that this would unblock? |
|
It would be nice to have it before bevy adoption of WESL. Because
Otherwise I'm happy to defer. |
const injection is pretty valuable. It's used extensively in Lygia, and I think will be important for wgsl-test too (nearly ready!). Anecdotally, it's usually the first thing people ask about when I mention enhancing wgsl, and it was quite common when we looked at wgsl shaders on github. Our current workaround is to use the So I think it would be valuable if we can land a param const design. Though not if we'd want to obsolete it a few months later. I don't want to fall into the trap of having to fully design all the things before we can make progress on any of them, but let's look ahead so we don't zig zag too much. I think when last we discussed, the sense was that param const a useful addition to typeclasses/context classes/generics, and conceptually compatible with ml style solutions (though syntax might change).. |
|
Okay, if we need it, then let's see how to unblock this:
|
Does that mean that param consts in non-root modules are effectively consts for the time being? Should we just forbid them?
can defer I believe. |
|
Makes sense to me to do it in 3 steps.
I think so, until we get to step 2. For shader libraries like Lygia that use conditions, there's benefit in allowing the conditions to be set from shader code, not just host code as now. That only works if condition variables are global, across modules and packages. Condition variables are global now, so I guess we can stay with that for now and then tighten up scoping with publishing/public. For wesl-test or wesl-play, having param consts for injectable values only at the root level is pretty ok. Those are intended to be short self contained shaders. So step 1 is still useful.
Libraries like Lygia will want injectable consts, both injecting from host code and from app shaders. Here's a Lygia example: // Smaller = nicer blur, larger = faster
#define SAMPLEDOF_RAD_SCALE .5I think we are pretty close on a step 2 design too, so hopefully we can get that in relatively soon. With step 3, the story will be nice and orthogonal: with conditions and consts unified, usable in apps and libraries, and settable from both host and shader code. |
supersedes #146.
closed #117.
Greatly simplified as commented in #146 (comment).
@param constcreate overridable module parameters@if(replaces conditional features)I also added
@elifand@elsein the spec (#117)Rendered