Alpha test.
We're currently in feature freeze, let alone API freeze, but I'm submitting this now in the hopes of getting early feedback.
The new interface doesn't have documentation; I'd like to add this, but I think there are some open questions that may require rewriting the interface.
Possibly alpha test function should be a compile option (while the ref should remain a parameter). This is unfortunately not very local, though.
Specifying alpha ref as an immediate constant is probably not very efficient, and it's certainly not how it's done for GLSL in wined3d. However, specifying it as a uniform will take a reroll of the vkd3d_shader_parameter API, which currently has no space to fit a whole buffer in its unions, and can be dealt with independently anyway.
The code is implemented in the spirv backend rather than in common vsir, despite the fact that GLSL will need similar handling. This is because it uses the vkd3d_shader_parameter API, and we cannot handle things like VKD3D_SHADER_PARAMETER_TYPE_SPECIALIZATION_CONSTANT in vsir.
Merge request reports
Activity
+enum vkd3d_shader_parameter_alpha_test_func +{ + VKD3D_SHADER_PARAMETER_ALPHA_TEST_NEVER = 1, + VKD3D_SHADER_PARAMETER_ALPHA_TEST_LESS, + VKD3D_SHADER_PARAMETER_ALPHA_TEST_EQUAL, + VKD3D_SHADER_PARAMETER_ALPHA_TEST_LESS_EQUAL, + VKD3D_SHADER_PARAMETER_ALPHA_TEST_GREATER, + VKD3D_SHADER_PARAMETER_ALPHA_TEST_NOT_EQUAL, + VKD3D_SHADER_PARAMETER_ALPHA_TEST_GREATER_EQUAL, + VKD3D_SHADER_PARAMETER_ALPHA_TEST_ALWAYS, +};
That's enum vkd3d_shader_comparison_func.
Specifying alpha ref as an immediate constant is probably not very efficient, and it's certainly not how it's done for GLSL in wined3d. However, specifying it as a uniform will take a reroll of the vkd3d_shader_parameter API, which currently has no space to fit a whole buffer in its unions, and can be dealt with independently anyway.
The code is implemented in the spirv backend rather than in common vsir, despite the fact that GLSL will need similar handling. This is because it uses the vkd3d_shader_parameter API, and we cannot handle things like VKD3D_SHADER_PARAMETER_TYPE_SPECIALIZATION_CONSTANT in vsir.
It may not be a bad thing to revisit the shader parameter interface, but I don't think that's necessarily a requirement here. For example, imagine an interface like this:
struct vkd3d_shader_d3dbc_source_info { ... enum vkd3d_shader_comparison_func alpha_test; unsigned int alpha_ref_cbv_id; unsigned int alpha_ref_offset; ... };
And that trivially translates to compilation options as well. As always, there are a number of variants we could do as well. E.g., we could use a vkd3d_shader_descriptor_binding structure above instead of a CBV id.
Also, is there a reason we couldn't handle shader parameters in vsir? I imagine it'd take some effort, but I don't think I see a fundamental blocker?
It may not be a bad thing to revisit the shader parameter interface, but I don't think that's necessarily a requirement here. For example, imagine an interface like this:
struct vkd3d_shader_d3dbc_source_info { ... enum vkd3d_shader_comparison_func alpha_test; unsigned int alpha_ref_cbv_id; unsigned int alpha_ref_offset; ... };
And that trivially translates to compilation options as well. As always, there are a number of variants we could do as well. E.g., we could use a vkd3d_shader_descriptor_binding structure above instead of a CBV id.
I... guess, but it seems like vkd3d_shader_parameter is made for this, and it lets users specify parameters as an immediate constant or spec constant or (eventually?) uniform constant as they wish. I also can't say I particularly like the pressure to get all of vkd3d_shader_d3dbc_source_info right at once...
What exactly is vkd3d_shader_parameter intended for if we don't want to use it here?
Also, is there a reason we couldn't handle shader parameters in vsir? I imagine it'd take some effort, but I don't think I see a fundamental blocker?
I guess we could add a new register type for spec constants. I'm not sure it's worthwhile, but it was shortsighted of me to say it's not possible.
I... guess, but it seems like vkd3d_shader_parameter is made for this, and it lets users specify parameters as an immediate constant or spec constant or (eventually?) uniform constant as they wish. I also can't say I particularly like the pressure to get all of vkd3d_shader_d3dbc_source_info right at once...
We may not need to. We've been using VKD3D_SHADER_UNSUPPORTED_* for things that we don't want to commit to yet, and we could conceivably use it here as well.
What exactly is vkd3d_shader_parameter intended for if we don't want to use it here?
I wouldn't necessarily say we don't want to use it here. And note that shader parameters aren't necessarily mutually exclusive with a struct vkd3d_shader_d3dbc_source_info either, although using them there would introduce some complications. However, I would say that being able to use a CBV to specify the reference value for the alpha test is desirable. I think we have a couple of other values like that, for example clip planes and point size; how do you intend to handle those?
I wouldn't necessarily say we don't want to use it here. And note that shader parameters aren't necessarily mutually exclusive with a struct vkd3d_shader_d3dbc_source_info either, although using them there would introduce some complications. However, I would say that being able to use a CBV to specify the reference value for the alpha test is desirable. I think we have a couple of other values like that, for example clip planes and point size; how do you intend to handle those?
I don't think there's any reason, at the moment, for the values not to all go through uniform buffers. I guess part of the idea behind vkd3d_shader_parameter, as I understand, is that it gives the user some freedom, in case we (or some hypothetical other user) find it more optimal to use immediate or spec constants for those. Currently only the latter are supported in this patch set, because we need some nontrivial changes to allow CBVs to even be specified from vkd3d_shader_parameter, but that's the intention.
I guess part of your question is "why even bother using parameters, since they don't support uniform buffers", i.e. parameters (which are only for immediate and spec constants) are orthogonal to anything that would want a uniform location. I guess the answer is, well, maybe hypothetically someone would want immediate or spec constants for alpha test (etc), and maybe hypothetically someone would want a uniform constant for rasterizer sample count. I don't know if either of those are actually realistic though.
Along similar lines my concern about API stability is not so much about upstreaming it as a monolithic thing, but that it's more likely than usual that we might want to provide more flexibility in how parameters are specified, or that we might need to add a new, previously unanticipated parameter (d3d1-9 is of course frozen at this moment in time, but we are still constantly discovering new things about it.) vkd3d_shader_parameter gives us a bit more flexibility inherently, by just separating individual parameters from the beginning, and already having the ability to specify values in different ways.
Of course, as an API consumer, I'm not afraid of rerolled structs either, and arguably a flat struct is nicer to consume anyway? But on the other hand it seems to make the API less consistent (and I already worry about consistency).
I wouldn't necessarily say we don't want to use it here. And note that shader parameters aren't necessarily mutually exclusive with a struct vkd3d_shader_d3dbc_source_info either, although using them there would introduce some complications. However, I would say that being able to use a CBV to specify the reference value for the alpha test is desirable. I think we have a couple of other values like that, for example clip planes and point size; how do you intend to handle those?
I don't think there's any reason, at the moment, for the values not to all go through uniform buffers. I guess part of the idea behind vkd3d_shader_parameter, as I understand, is that it gives the user some freedom, in case we (or some hypothetical other user) find it more optimal to use immediate or spec constants for those. Currently only the latter are supported in this patch set, because we need some nontrivial changes to allow CBVs to even be specified from vkd3d_shader_parameter, but that's the intention.
That helps. The apparently mistaken impression I got from the original MR description was something along the lines of "CBVs are hard; immediate/spec constants are good enough for alpha test/ref". If you intend to make CBV shader parameters work, that seems like a perfectly valid solution. VKD3D_SHADER_PARAMETER_NAME_ALPHA_TEST_FUNC is weird though; as currently implemented, it can't be used with VKD3D_SHADER_PARAMETER_TYPE_SPECIALIZATION_CONSTANT, and that setup wouldn't work with CBVs either.
Note also that clip planes and point size aren't single scalars or even single vectors. That doesn't have to be a problem, but probably implies having something like VKD3D_SHADER_PARAMETER_NAME_CLIP_PLANE_0..VKD3D_SHADER_PARAMETER_NAME_CLIP_PLANE_7.
Along similar lines my concern about API stability is not so much about upstreaming it as a monolithic thing, but that it's more likely than usual that we might want to provide more flexibility in how parameters are specified, or that we might need to add a new, previously unanticipated parameter (d3d1-9 is of course frozen at this moment in time, but we are still constantly discovering new things about it.) vkd3d_shader_parameter gives us a bit more flexibility inherently, by just separating individual parameters from the beginning, and already having the ability to specify values in different ways.
Of course, as an API consumer, I'm not afraid of rerolled structs either, and arguably a flat struct is nicer to consume anyway? But on the other hand it seems to make the API less consistent (and I already worry about consistency).
Sure. Note that in terms of consistency, there's probably an argument that shader parameters should be specified as an extension to struct vkd3d_shader_interface_info.
That somewhat ties into the other concern I have: Ideally this would work in a way that doesn't require spending significant effort to implement the same functionality for GLSL or other additional backends. Writing the shader code for these things as vsir and specifying the parameters on the frontend side would avoid that, and probably wouldn't even be that hard. CBV and immediate constant parameters could be resolved on the vsir level, even currently. Specialisation constants would probably require a new register type.
That helps. The apparently mistaken impression I got from the original MR description was something along the lines of "CBVs are hard; immediate/spec constants are good enough for alpha test/ref". If you intend to make CBV shader parameters work, that seems like a perfectly valid solution.
Ah, sorry. That certainly was my intention but I see how I communicated poorly.
Note that I'm not married to it, though. If you think it's better to use a separate struct for all the sm1 extra states, and just always using uniforms, I can write that instead.
VKD3D_SHADER_PARAMETER_NAME_ALPHA_TEST_FUNC is weird though; as currently implemented, it can't be used with VKD3D_SHADER_PARAMETER_TYPE_SPECIALIZATION_CONSTANT, and that setup wouldn't work with CBVs either.
Yeah. The only reason I did that is because it seemed weird to have alpha ref as a parameter but alpha test elsewhere. I was planning to write some documentation saying "you must use an immediate constant for this".
Note also that clip planes and point size aren't single scalars or even single vectors. That doesn't have to be a problem, but probably implies having something like VKD3D_SHADER_PARAMETER_NAME_CLIP_PLANE_0..VKD3D_SHADER_PARAMETER_NAME_CLIP_PLANE_7.
I was thinking of putting them in an array, and allowing them to be immediate constants via a pointer. Though we could also separate them like that.
Sure. Note that in terms of consistency, there's probably an argument that shader parameters should be specified as an extension to struct vkd3d_shader_interface_info.
That somewhat ties into the other concern I have: Ideally this would work in a way that doesn't require spending significant effort to implement the same functionality for GLSL or other additional backends. Writing the shader code for these things as vsir and specifying the parameters on the frontend side would avoid that, and probably wouldn't even be that hard. CBV and immediate constant parameters could be resolved on the vsir level, even currently. Specialisation constants would probably require a new register type.
I'll put together something vsir based.
That helps. The apparently mistaken impression I got from the original MR description was something along the lines of "CBVs are hard; immediate/spec constants are good enough for alpha test/ref". If you intend to make CBV shader parameters work, that seems like a perfectly valid solution.
Ah, sorry. That certainly was my intention but I see how I communicated poorly.
Note that I'm not married to it, though. If you think it's better to use a separate struct for all the sm1 extra states, and just always using uniforms, I can write that instead.
VKD3D_SHADER_PARAMETER_NAME_ALPHA_TEST_FUNC is weird though; as currently implemented, it can't be used with VKD3D_SHADER_PARAMETER_TYPE_SPECIALIZATION_CONSTANT, and that setup wouldn't work with CBVs either.
Yeah. The only reason I did that is because it seemed weird to have alpha ref as a parameter but alpha test elsewhere. I was planning to write some documentation saying "you must use an immediate constant for this".
I don't think it makes sense to use shader parameters if we're going to restrict them to VKD3D_SHADER_PARAMETER_TYPE_IMMEDIATE_CONSTANT.
That said, it's certainly possible to make VKD3D_SHADER_PARAMETER_NAME_ALPHA_TEST_FUNC work with specialisation constants and CBVs as well; we'd have to generate a switch and code for all the cases, but on modern GPUs that's probably fine. It's potentially useful too; I could certainly imagine an application creating generic versions of its shaders at startup, and then creating more specialised shaders later.
added 39 commits
-
dd6767a1...1fe7a658 - 30 commits from branch
wine:master
- 6bcb5308 - vkd3d-shader: Introduce struct vkd3d_shader_parameter_info and struct vkd3d_shader_parameter1.
- d7a3d77b - include: Document shader parameters.
- 90e2435d - vkd3d-shader/spirv: Support passing shader parameters through uniform buffers.
- 52854cfa - vkd3d-shader/hlsl: Implement the GetRenderTargetSampleCount() intrinsic.
- 54c6c537 - tests: Add a test for the vkd3d_shader_parameter APIs.
- 6643ec26 - vkd3d-shader: Allow controlling alpha test through vkd3d-shader parameters.
- 6b8a72e2 - tests: Offset the viewport by 0.5 when running d3dbc shaders.
- e305d5df - tests: Factor out a set_default_target() helper.
- eeeb7aa3 - tests: Test alpha test.
Toggle commit list-
dd6767a1...1fe7a658 - 30 commits from branch
I've pushed a new version, which is basically a complete rewrite:
-
The implementation is done as a vsir pass now.
-
Constant buffer support is included in this patch series rather than being deferred for a later one [largely because it led to a more natural patch order].
-
vkd3d_shader_parameter is rerolled and we now have a separate vkd3d_shader_parameter_info chained structure.
-
Documentation for vkd3d_shader_parameter and the new structures has also been written.
I was originally going to add a VKD3DSPR_SPECIALIZATION_CONSTANT as mentioned, but after looking at that and at what would be necessary to make buffer binding work from a neutral perspective, I instead opted for a different approach: add a VKD3DSPR_PARAMETER (whose "index" is the parameter name), and let the backend resolve that parameter as it sees fit. That ended up working pretty nicely.
-
added 7 commits
- 77b5784d - vkd3d-shader/hlsl: Implement the GetRenderTargetSampleCount() intrinsic.
- 180d9ae3 - tests: Handle multisampling in the Vulkan runner.
- bd36ae09 - tests: Add a test for the vkd3d_shader_parameter APIs.
- 902a5330 - vkd3d-shader: Allow controlling alpha test through vkd3d-shader parameters.
- 07346485 - tests: Offset the viewport by 0.5 when running d3dbc shaders.
- 9b36ba5c - tests: Factor out a set_default_target() helper.
- b401c598 - tests: Test alpha test.
Toggle commit listI don't think it makes sense to use shader parameters if we're going to restrict them to VKD3D_SHADER_PARAMETER_TYPE_IMMEDIATE_CONSTANT.
That said, it's certainly possible to make VKD3D_SHADER_PARAMETER_NAME_ALPHA_TEST_FUNC work with specialisation constants and CBVs as well; we'd have to generate a switch and code for all the cases, but on modern GPUs that's probably fine. It's potentially useful too; I could certainly imagine an application creating generic versions of its shaders at startup, and then creating more specialised shaders later.
This makes sense to me too, and for this reason I've elected to keep alpha test func a parameter. However, I haven't implemented the non-immediate behaviour here.
added 7 commits
- 72f16c73 - tests: Handle multisampling in the Vulkan runner.
- bd1a5c93 - vkd3d-shader/hlsl: Implement the GetRenderTargetSampleCount() intrinsic.
- 5e8633de - tests: Add a test for the vkd3d_shader_parameter APIs.
- ff55444a - vkd3d-shader: Allow controlling alpha test through vkd3d-shader parameters.
- 0eceef2b - tests: Offset the viewport by 0.5 when running d3dbc shaders.
- af07392c - tests: Factor out a set_default_target() helper.
- 1a8012a0 - tests: Test alpha test.
Toggle commit listNot a very thorough review, but this makes sense to me like this.
+/** + * The linkage of a parameter specified through a uniform buffer, used in + * struct vkd3d_shader_parameter and struct vkd3d_shader_parameter1. + */ +struct vkd3d_shader_parameter_buffer
It can't really be used by struct vkd3d_shader_parameter though, right?
+ * Alpha test comparison function. When this parameter is provided, if the + * alpha component of the pixel shader colour output at location 0 fails the + * test, as defined by this function and the reference value provided by + * VKD3D_SHADER_PARAMETER_NAME_ALPHA_TEST_REF, the fragment will be + * discarded.
Should we specify this as behaving like VKD3D_SHADER_COMPARISON_FUNC_ALWAYS if not provided?
+ if (runner->r.minimum_shader_model < SHADER_MODEL_4_0) + { + viewport.x = 0.5; + viewport.y = RENDER_TARGET_HEIGHT + 0.5; + } + else + { + viewport.x = 0; + viewport.y = RENDER_TARGET_HEIGHT; + }
Viewport values are floats. Also, how would you feel about the following?
if (runner->r.minimum_shader_model < SHADER_MODEL_4_0) { viewport.x += 0.5f; viewport.y += 0.5f; }
+ else if (match_string(line, "alpha test", &line)) + { + if (match_string(line, "greater", &line)) + runner->alpha_test_func = VKD3D_SHADER_COMPARISON_FUNC_GREATER; + else if (match_string(line, "always", &line)) + runner->alpha_test_func = VKD3D_SHADER_COMPARISON_FUNC_ALWAYS; + else if (match_string(line, "never", &line)) + runner->alpha_test_func = VKD3D_SHADER_COMPARISON_FUNC_NEVER; + else + fatal_error("Malformed alpha test arguments '%s'.\n", line); + + runner->alpha_test_ref = strtof(line, &rest); + line = rest; + }
parse_comparison_func(), IMO. That returns a D3D12_COMPARISON_FUNC, but that's easily converted to enum vkd3d_shader_comparison_func. I also think we may as well just test all of them.
It can't really be used by struct vkd3d_shader_parameter though, right?
Oops. The perils of copy-paste :-(
Should we specify this as behaving like VKD3D_SHADER_COMPARISON_FUNC_ALWAYS if not provided?
I think it follows, but it certainly wouldn't hurt to spell out.
Viewport values are floats. Also, how would you feel about the following?
if (runner->r.minimum_shader_model < SHADER_MODEL_4_0) { viewport.x += 0.5f; viewport.y += 0.5f; }
Indeed that's better.
parse_comparison_func(), IMO. That returns a D3D12_COMPARISON_FUNC, but that's easily converted to enum vkd3d_shader_comparison_func.
Ah, I didn't notice we already had one.
I also think we may as well just test all of them.
Fair enough; after writing this iteration I'm inclined to agree.
added 8 commits
- 5cdd56d0 - vkd3d-shader/spirv: Support passing shader parameters through uniform buffers.
- ca2bd52f - tests: Handle multisampling in the Vulkan runner.
- fae4b1df - vkd3d-shader/hlsl: Implement the GetRenderTargetSampleCount() intrinsic.
- 250f6762 - tests: Add a test for the vkd3d_shader_parameter APIs.
- ff82c30f - vkd3d-shader: Allow controlling alpha test through vkd3d-shader parameters.
- 8ab961ea - tests: Offset the viewport by 0.5 when running d3dbc shaders.
- 5ea9e4e9 - tests: Factor out a set_default_target() helper.
- a4ea822d - tests: Test alpha test.
Toggle commit list