-
Notifications
You must be signed in to change notification settings - Fork 755
Add Slang shaders to additional samples #1406
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
base: main
Are you sure you want to change the base?
Conversation
This is needed to get proper support for SPIR-V compiled from Slang shaders
- instancing - conditional rendering - conservative rasterization
- dynamic uniform buffers - descriptor buffer basic - fragment shading rate
- hdr (+hpp) - graphics pipeline library - mesh shading (+hpp)
- texture loading (+hpp) - push descriptors (+hpp - open cl interop
- separte image sampler (+hpp) - texture mipmap generation (+hpp) - shader debug printf - texture compression basisu
- compute nbody (+hpp)
- dynamic primitive clipping - logic op dynamic state - sparse image
- color write enable - debug utils - dynamic line rasterization
- ray queries - ray tracing basics - ray tracing extended - ray tracing position fetch
- terrain tessellation - patch control points - synchronization 2 - vertex dynamic state
It looks like two issues are occurring here with Apple CI:
/Users/runner/work/Vulkan-Samples/Vulkan-Samples/shaders/sparse_image/slang/sparse.frag.slang(45): error 36107: entrypoint 'main' does not support compilation target 'spirv' with stage 'fragment' Also note that macOS CI does not access the VulkanSDK and will not try to compile any slang shaders, but would likely fail on the same sparse image shader if it did. |
It's a draft for a reason. |
Understood - just trying to help out re issue #1405 since this has nothing to do with your PR, only the Apple CI. |
You're welcome. We discussed that on one of our calls, and having each CI step rebuild the SPIR-V files that are already part of the repo doesn't make sense anyway. Esp. as the SDK tends to include broken/outdated compilers. |
I agree. However, the scope of the problem may be limited to iOS. At this point iOS seems to be the only CI target that requires and accesses the Vulkan SDK for static linkage to the MoltenVK framework. I believe all other CI targets depend on Vulkan-Headers only without need for the SDK (or compilers), assuming dynamic linkage to the Vulkan runtime library. |
Should put that in a separate issue. I'd like the comments section of this PR related to the actual PR if possible. |
- descriptor indexing
- dyamic blending
- time stamp queries (+hpp) - host image copy
- fragment shader barycentric - memory budget
- calibrated timestamps - dynamic multisample rasterization
- open gl interop
- portability
I get it. However, if you merge this PR all Apple interactive builds will be broken. Could you at least fix this by disabling slangc all the time on Apple? |
This won't be merged until we (inside the samples group) agree. This includes the apple platform discussion. |
Created #1409 to make sure your remarks won't get lost. |
Thanks. I too am in favour of a general solution that works across platforms and shading languages. |
I don't have a local slang compiler, so I should be running against the pre-built versions. I am seeing some validation errors (and some other problems which I'll share afterwards). These may, of course, not be anything to do with the slang, but could be compilers problems or spirv-val problems. I thought I'd list them anyway. compute_nbody
hello_triangle hpp_compute_nbody
hpp_texture_mipmap_generation color_write_enable synchronization_2
|
Thanks for testing 👍🏻 Most of the validation errors were caused by the Slang version that I used generating invalid SPIR-V. I have recompiled the shaders with a newer versions and most samples should now be free of validation errors. For some like hello triangle and color write enable, the samples need special handling when using Slang shaders due to a slightly different shading environment. Will try to fix these next. |
hello_triangle and color_write_enable should also be fixed now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks much better now, but I did still see conditional rendering looking different. This is why.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
float2 CenterPos; | ||
float PSize : SV_PointSize; | ||
// remove | ||
float PointSize; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would, instead of PSize
and PointSize
something like
float PointSize : SV_PointSize;
be enough?
{ | ||
float4 Pos : SV_POSITION; | ||
float GradientPos; | ||
float2 CenterPos; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is CenterPos
some kind of emulation of the gl_PointCoord
?
I could not find a lot about the semantic name SV_PointCoord
, but would that be the right approach, somehow?
float4 main(VSOutput input) | ||
{ | ||
float3 color = samplerGradientRamp.Sample(float2(input.GradientPos, 0.0)).rgb; | ||
float2 PointCoord = (input.Pos.xy - input.CenterPos.xy) / input.PointSize + 0.5; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't follow the math behind PointCoord
.
Maybe some comment describing this?
{ | ||
float3 Pos; | ||
float3 Normal; | ||
float3 Color; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This Color
is not used. Can it be removed?
[shader("fragment")] | ||
float4 main(VSOutput input) | ||
{ | ||
return float4(input.Color, 1.0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The corresponding glsl-shader also returns a vec4, but just sets rgb.
Shouldn't we fix that right here?
[shader("fragment")] | ||
float4 main() | ||
{ | ||
return float4(1.0, 1.0, 1.0, 1.0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here again, the corresponding glsl-shader returns a vec4, but just sets rgb.
Should be fixed there.
0.0118349786570722, | ||
0.0073599963704157, | ||
0.0043538453346397, | ||
0.0024499299678342}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you change the tab width, you see that those numbers are not aligned any more.
|
||
|
||
const float blurScale = 0.003; | ||
const float blurStrength = 1.0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These constants are different in the corresponding glsl-shader.
ar = ts.y / ts.x; | ||
} | ||
|
||
float2 P = inUV.yx - float2(0, (25 >> 1) * ar * blurScale); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The corresponding glsl-shader uses weights.length()
, instead of the magic 25
.
Is something like that length()
not available in slang (and hlsl as well)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mixing tabs and spaces in this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also mixing tabs and spaces here.
|
||
switch(object_type) { | ||
case 0: // Skybox | ||
worldPos = mul((float4x3) ubo.skyboxModelview, input.Pos).xyz; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't you cast ubo.skyboxModelView
to a float3x3
?
Or do not cast at all, but extend input.Pos
to a float4
?
I don't know, what's more efficient.
output.Pos = mul(ubo.projection, mul(ubo.modelview, float4(localPos, 1.0))); | ||
break; | ||
} | ||
output.Normal = mul((float4x3)ubo.modelview, input.Normal).xyz; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, a float4x3
cast.
output.Normal = mul((float4x3)ubo.modelview, input.Normal).xyz; | ||
output.UV = input.UV; | ||
|
||
float3 lightPos = float3(0.0f, -5.0f, 5.0f); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The corresponding glsl-shader calculates a different lightPos
.
struct VSOutput | ||
{ | ||
float4 Pos : SV_POSITION; | ||
float3 Normal; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Normal
isn't used here.
{ | ||
float4 Pos : SV_POSITION; | ||
float2 UV; | ||
uint ColorOffset; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the ColorOffset
need the nointerpolation
interpolation modifier?
float4 color = input.BaseColorFactor; | ||
|
||
if (input.BaseTextureIndex != -1) | ||
color = Textures[uint(round(input.BaseTextureIndex))].Sample(input.UV); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know, you did not change that part of the code, but why is it needed to round a not interpolated uint
and cast it to an uint
again?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some tab-and-space mixing in this file.
float RoughnessFactor; | ||
uint BaseTextureIndex; | ||
uint NormalTextureIndex; | ||
uint MetallicRoughnessTextureIndex; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to mark the last five members as nointerpolation
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, some tab-and-space mixing.
Description
Important note: If you compile shaders locally, you have to use a very recent version of the slang compiler. The one shipped with the SDK is outdated and broken. Make sure to grab the latest one from https://github.com/shader-slang/slang/releases
This adds Slang shaders to additional samples:
Also enables SPIR-V 1.4 at framework level when using Slang.
Additional notes:
VKB_SKIP_SLANG_SHADER_COMPILATION
to skip compilation of Slang shaders (you can still use Slang shaders via the precompiled SPIR-V files)Refs #1385
General Checklist:
Please ensure the following points are checked:
Note: The Samples CI runs a number of checks including:
If this PR contains framework changes:
batch
command line argument to make sure all samples still work properly