-
Notifications
You must be signed in to change notification settings - Fork 186
Add in the shader authoring guide chapter. #333
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
(cherry picked from commit 3eb823c)
|
I know @SaschaWillems already wrote a whole chapter around this "area" so want him approval Also @swoods-nv would also be great to review |
swoods-nv
left a comment
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.
My main overarching comment is that I'd recommend caution when phrasing migration from HLSL->Slang as "upgrading" or similar. Vulkan consciously chooses not to have an official shading language, and both Slang and HLSL continue to evolve and gain new features.
|
There is def. some duplication here. The chapters I did are:
The latter was written with additional shading languages in mind. So maybe it's better to
A separate chapter that kinda combines what we have + slang might be confusing. |
|
Okay, sounds good, I'll try to move things around. |
|
Btw. the official slang docs do have migration guides, so maybe something we could link to: |
|
Good call on using those links, I'll add it in. The next version will have a lot of copy pasting to place the details in the right place. Should have a new version once I finish reviewing all changes. |
… slang.adoc to match hlsl.adoc
|
Alright, significant copy/pasting and reorganizing into the existing structure. I don't think I lost any of the details I wrote in the original version. However, hopefully that's closer to where things should go. |
# Conflicts: # README.adoc
spencer-lunarg
left a comment
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.
- Adding Slang to the
high_level_shader_language_comparison.adocpage is great (I often know a term in in GLSL and need to look up the Slang equivalent) - Will wait for @SaschaWillems approval on the HLSL additions added
- Will wait for @swoods-nv for a re-read and approval on the new Slang chapter
| * Be mindful of matrix layout differences | ||
| * Consider using a shader generation system | ||
|
|
||
| === Performance Optimization |
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.
Everything after (and including) this chapter (up to the conclusion) isn't specific to HLSL. Might be better to move this to a separate chapter.
|
That's a lot of text to review ^^ Added a few comments. One of the things that IMO needs more attention is Skang's ability to automatically generate binding slots. For me, that was one of the best things about Slang, and while you can somehow find that information in this PR I'd like to see that more explicitly stated. |
…clude GLSL compatibility example.
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.
Hmm... I think this requires quite a bit of work still. There are lots of references to deprecated and sometimes even non-existent features. The text is also very repetitive at places. Many sections are simply lists with little substance. Lots of repetition of modules-interfaces-generics.
I hope other reviewers can fill in more details, especially about the overall structure. I focused more on correctness in code examples, where I likely also didn't find every problem, so I hope others will check those carefully as well.
| ---- | ||
| // Generic function | ||
| generic<T> | ||
| T min(T a, T b) { |
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 and a couple other cases here appear to be using an old version of the generic syntax. I think this should work:
T min<T>(T a, T b) {
return a < b ? a : b;
}| // Generic struct | ||
| generic<T, let N : int> | ||
| struct Array { |
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 too, struct Array<T, let N : int> { should work.
| ---- | ||
| // Generic interpolation function | ||
| generic<T> | ||
| T interpolate(T a, T b, float t) |
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.
Could be T interpolate<T>(T a, T b, float t) here as well.
| // In lighting.slang | ||
| module Lighting; | ||
| export interface ILight { |
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.
export -> public
|
|
||
| Slang's module system enables powerful composition patterns: | ||
|
|
||
| * *Interface-based composition*: Define interfaces for pluggable components |
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.
It's odd to lead with
Slang's module system enables powerful composition patterns:
and then start talking about interfaces and generics, when those can be used without modules as well. They're independent features.
|
|
||
| * *Compile-time reflection*: Inspect types and fields at compile time | ||
| * *Runtime reflection*: Generate reflection data for runtime use | ||
| * *Automatic binding generation*: Use reflection to automate resource binding |
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.
Do we actually have this? Or is it just a thing that users can implement themselves with the reflection data?
| Slang provides powerful reflection capabilities: | ||
|
|
||
| * *Compile-time reflection*: Inspect types and fields at compile time | ||
| * *Runtime reflection*: Generate reflection data for runtime use |
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.
Odd distinction between "compile-time reflection" and "runtime reflection". I don't understand what this split is, when all the reflection data is "compile-time" in the sense that we can get all of it after compiling the program but before running it.
| __generic<T> | ||
| void bindResources(ParameterBlock<T> block, T data) { | ||
| __for(field in getFields(T)) { |
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 this __for, where is this coming from? (and also, void bindResources<T>(... 😉 )
|
Tbh. I think we should remove the migrating from glsl/hlsl paragraphs. They're kind hard to read, some of the best practices feel out-of-place or don't apply and the official slang docs already have good migration guides: |
| [source,slang] | ||
| ---- | ||
| // Define parameter blocks with explicit bindings | ||
| [[vk::binding(0, 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.
I don't think this shows a best practice. Slang does not require the explicit bindings, IMO best practices is to don't specify them and let Slang do the binding slots implicitly.
|
|
||
| There are several advantages to using Slang for Vulkan development: | ||
|
|
||
| * *Cross-API compatibility*: Write shaders that can be used with both Vulkan and DirectX with minimal changes |
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.
Slang also supports other targets like OpenGL, Metal and WebGPU.
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.
AFAIK OpenGL is not officially supported, it's just that it might work with the GLSL target sometimes (but the GLSL target also targets Vulkan primarily instead of OpenGL). In addition to Metal and WebGPU, CUDA and CPU execution are supported.
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 Slang compiler can generate code for a wide variety of targets: D3D12, Vulkan, Metal, D3D11, OpenGL, CUDA, and even generate code to run on a CPU."
From the Slang repo.
Although OpenGL might not be fully supported yet. But nonetheless, above bullet point should add at least some of the other targets.
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 is correct-- our generated GLSL and SPIRV are Vulkan-flavored. TBF the Slang user guide doesn't make super-clear the state of OpenGL support, it just says "Note: Slang has only limited support for compiling code for OpenGL."
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 Slang compiler can generate code for a wide variety of targets: D3D12, Vulkan, Metal, D3D11, OpenGL, CUDA, and even generate code to run on a CPU."
From the Slang repo.
Ah, this should be updated on our side; I've submitted a PR to the Slang repo to remove OpenGL from the targets list.
| * *Cross-compilation*: Generate code for different shader stages from a single source | ||
| * *Built-in reflection*: Generate reflection data during compilation | ||
| * *Shader linking*: Link multiple shader modules together | ||
| * *Diagnostic quality*: More detailed error messages and warnings |
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 sounds very generic. What does that actually mean?
|
|
||
| === Runtime Behavior Differences | ||
|
|
||
| Slang introduces some runtime behavior differences: |
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.
"differences" compared to what? It's kinda hard to put this into perspective.
|
|
||
| === Slang-specific features for Vulkan | ||
|
|
||
| When using Slang with Vulkan, you can take advantage of several Slang-specific features: |
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.
None of the bullet points listed here are specific to Vulkan.
| === Vulkan-Specific Best Practices | ||
|
|
||
| * *Use parameter blocks*: Organize resources into parameter blocks for better organization | ||
| * *Explicit bindings*: Always specify explicit descriptor set and binding indices |
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 wouldn't consider that best practices. Not having to specify these is actually one of the best things about using Slang with Vulkan.
No description provided.