Sketch simple shaders #14

Jasper-Bekkers · 2020-08-19T17:16:24Z

These are GLSL shaders we're using in Ark and versions converted into pseudo-rust.

I didn't touch on bindings yet, because I think that'll be something we can debate endlessly about.

This uses a glam-rs inspired math library since it seems most likely that we'll use something like that.

None of this compiles / builds, but it should give a bit of an overview of how these shaders will end up looking. I think many things can be improved so I'll go through and comment on a bunch of things.

Part of #4

…xt to it

Jasper-Bekkers · 2020-08-19T17:17:11Z

examples/ark-lighting/rust/lighting.rs

+fn degamma(v: Vec4) -> Vec4 { 
+    decode_srgb(v.xyz).extend(v.w)
+}
+
+fn hsv_to_rgb(c: Vec3) -> Vec3 {
+    let K = Vec4::new(1.0, 2.0 / 3.0, 1.0 / 3.0, 3.0);
+    let p = ((c.xxx + K.xyz).fract() * 6.0 - K.www).abs();
+    c.z * lerp(K.xxx, (p - K.xxx).clamp(0.0, 1.0), c.y)
+}


Looks like I copy 'n pasted swizzles inhere. Can remove.

Looks like glam-rs will need Add<f32> and Sub<f32> implemented for it's vector types.

Jasper-Bekkers · 2020-08-19T17:17:51Z

examples/ark-lighting/rust/mesh.rs

@@ -0,0 +1,131 @@
+use lighting;
+
+bitflags! {


Would be quite nice to use this.

can also try https://crates.io/crates/enumflags2, I find it even more ergonomic but it is a proc-macro instead so slower to build

Jasper-Bekkers · 2020-08-19T17:18:12Z

examples/ark-lighting/rust/mesh.rs

+    world_pos: Vec3,
+    opacity: Vec3,
+
+    #[flat]


Not sure what kind of shape these attributes will end up taking, but this is an example.

Alternative syntax: #[spriv_attr(flat)] or something like this.

as this is fragment shader input/layout it would likely have some attribute on the struct as as well including that name, so then this flat keyword ideally share namespace / part of the naming here also to make it explicit.

not too common to use though, but need to be supported

Jasper-Bekkers · 2020-08-19T17:18:28Z

examples/ark-lighting/rust/mesh.rs

+    constants: Constants,
+    inputs: Inputs,


Probably should talk about bindings in a new issue / PR.

Jasper-Bekkers · 2020-08-19T17:18:54Z

examples/ark-lighting/rust/mesh.rs

+    env_params: lighting::EnvParams,
+    view_params: lighting::ViewParams,


Would be nice to just use our own types here instead of what the GLSL thing does (raw vec4's).

yup for sure, will be interesting what repr we will end up on the structs with, and if this top level struct has a repr here, can it also enforce (I hope so) that all the structs and types inside it also has it

Jasper-Bekkers · 2020-08-19T17:19:24Z

examples/ark-lighting/rust/mesh.rs

+        lighting::ark_light(surface_params, constants.view_params, constants.env_params).extend(input.opacity)
+    } else {
+        (input.diffuse_color.truncate() * input.diffuse_color.w()).extend(input.opacity)


These .extend and .truncate calls are kind of fine, but they do get tiresome after a while.

it is rather cryptic of what does it even mean to truncate a diffuse color? may be other options than having an rgba color stored in a Vec4 with the glam naming here. the GLSL/HLSL swizzles and alias of rgba/xyzw is nice and more familar (just as the original code was diffuse_color.rgb which is very clear).

Could simply have diffuse_color.xyz() function in the math library instead of truncate here

don't look the wording of .extend() either, also cryptic

Jasper-Bekkers · 2020-08-19T17:20:19Z

examples/ark-lighting/rust/mesh.rs

+        let scale = extract_conservative_scale_from_transform(object_to_world);
+        object_to_world = Mat4::from_mat3(
+            if spirv::view_index() == 0 {
+                global_uniforms.view_to_world.to_mat3()


I invented glam-rs functions to_mat3 and from_mat3 here.

Jasper-Bekkers · 2020-08-19T17:21:00Z

examples/ark-lighting/rust/mesh.rs

+    if instance.flags.contains(ALPHA_BLENDING_ENABLE | PREMULTIPLIED_ALPHA_ENABLE) {
+        if instance.flags.contains(PREMULTIPLIED_ALPHA_ENABLE)  {


Somehow complex match patterns on bitflags! isn't ideal.

think this is pretty fine though, somewhat cleaner and less error prone to read than the GLSL switch statement with bitwise logic in it me thinks.
and for other cases Rust's match will be really good

Jasper-Bekkers · 2020-08-19T17:22:01Z

examples/ark-lighting/rust/mesh.rs

+        diffuse_color.a = 1.0;
+    }
+
+    let position = if spirv::view_index() == 0 {


I figured we could have a bunch of these builtins in core::arch::spirv similar to how _mm_set_p1 for example lives in core::arch::x86_64

Jasper-Bekkers · 2020-08-19T17:23:02Z

examples/ark-lighting/rust/mesh.rs

+
+    position.set_y(-position.y())
+
+    position


The GLSL function sets more interpolators then just position; we should find a way of setting this reliably between shader stages so that we can safely match then up. Potentially without all regular shader language magic around these.

Jasper-Bekkers · 2020-08-19T19:01:19Z

From @h3r2tic

lighting.rs looks rather good I think -- except the swizzles which you noticed yourself
would structs need a #repr(C) or similar? I'm not sure at which stage any reordering might happen -- if the frontend messes with field order, it could be annoying

h3r2tic · 2020-08-19T19:08:14Z

examples/ark-lighting/rust/mesh.rs

+
+#[vertex_shader]
+fn main() -> Vec4 {
+    let instance = instance_data[spirv::instance_index()]; // gl_InstanceIndex


spirv::instance_index looks a bit weird -- as in, the namespace sounds weird. I would expect the instance_index to be in some draw context, not in a namespace related to the instruction set. It's almost as if having x86_64::current_thread

agreed. probably should be a decorated parameter to the main function that you declare that you want to have the instance index. GLSL is not a good base here with loose globals, prefer the explicit inputs & outputs of HLSL and think that would fit better in explicit Rust land also

h3r2tic · 2020-08-19T19:09:49Z

examples/ark-lighting/rust/mesh.rs

+        let hsv = rgb_to_hsv(rgb);
+
+        let hsv = Vec3::new(
+            (hsv.x() + instance.hsv_transform_data.x()).fract(),


The .x() etc could probably become just .x. I believe glam uses methods since internally it uses SIMD types that we don't need on the GPU. With different vector types we can have just .x

good point, that would be nice

Not needing SIMD types isn't entirely true, unfortunately - NVIDIA still has massive performance losses when they non-SIMD types are used for loads and stores instead of the SIMD types.

Now, if we want that to impact our std-lib design is another question, because a load-store-vectorizer optimization pass can take care of that just as well.

Not needing SIMD types isn't entirely true, unfortunately - NVIDIA still has massive performance losses when they non-SIMD types are used for loads and stores instead of the SIMD types

It is also possible to just map types to spir-v types. I have been doing that in rlsl, although reusing rusts simd types probably makes more sense.

It looks like this

#[spirv(Vec4)] #[repr(C)] #[derive(Debug, Clone, Copy, PartialEq)] pub struct Vec4<T> { pub x: T, pub y: T, pub z: T, pub w: T, }

although reusing rusts simd types probably makes more sense.

Looked up the tracking issue for #[repr(simd)], looks like it's quite intense. Anyway, I believe the current WIP would automatically compile it to a spir-v SIMD type, I'm guessing it shows up as an Abi::Vector which just straight dumps to a spir-v vector (probably want to deopt in a couple cases that aren't supported eventually, but anyway, first draft)

repi

Nice work!

repi · 2020-08-19T19:25:52Z

examples/ark-lighting/rust/mesh.rs

@@ -0,0 +1,131 @@
+use lighting;
+
+bitflags! {


can also try https://crates.io/crates/enumflags2, I find it even more ergonomic but it is a proc-macro instead so slower to build

repi · 2020-08-19T19:27:10Z

examples/ark-lighting/rust/mesh.rs

+    env_params: lighting::EnvParams,
+    view_params: lighting::ViewParams,


yup for sure, will be interesting what repr we will end up on the structs with, and if this top level struct has a repr here, can it also enforce (I hope so) that all the structs and types inside it also has it

repi · 2020-08-19T19:29:41Z

examples/ark-lighting/rust/mesh.rs

+    world_pos: Vec3,
+    opacity: Vec3,
+
+    #[flat]


as this is fragment shader input/layout it would likely have some attribute on the struct as as well including that name, so then this flat keyword ideally share namespace / part of the naming here also to make it explicit.

not too common to use though, but need to be supported

repi · 2020-08-19T19:32:56Z

examples/ark-lighting/rust/mesh.rs

+        lighting::ark_light(surface_params, constants.view_params, constants.env_params).extend(input.opacity)
+    } else {
+        (input.diffuse_color.truncate() * input.diffuse_color.w()).extend(input.opacity)


it is rather cryptic of what does it even mean to truncate a diffuse color? may be other options than having an rgba color stored in a Vec4 with the glam naming here. the GLSL/HLSL swizzles and alias of rgba/xyzw is nice and more familar (just as the original code was diffuse_color.rgb which is very clear).

Could simply have diffuse_color.xyz() function in the math library instead of truncate here

repi · 2020-08-19T19:33:12Z

examples/ark-lighting/rust/mesh.rs

+        lighting::ark_light(surface_params, constants.view_params, constants.env_params).extend(input.opacity)
+    } else {
+        (input.diffuse_color.truncate() * input.diffuse_color.w()).extend(input.opacity)


don't look the wording of .extend() either, also cryptic

repi · 2020-08-19T19:34:35Z

examples/ark-lighting/rust/mesh.rs

+
+#[vertex_shader]
+fn main() -> Vec4 {
+    let instance = instance_data[spirv::instance_index()]; // gl_InstanceIndex


agreed. probably should be a decorated parameter to the main function that you declare that you want to have the instance index. GLSL is not a good base here with loose globals, prefer the explicit inputs & outputs of HLSL and think that would fit better in explicit Rust land also

repi · 2020-08-19T19:36:08Z

examples/ark-lighting/rust/mesh.rs

+    if instance.flags.contains(BILLBOARD_ENABLE) {
+        let scale = extract_conservative_scale_from_transform(object_to_world);
+        object_to_world = Mat4::from_mat3(
+            if spirv::view_index() == 0 {


same here with spirv::view_index (gl_ViewIndex), think let's try doing it as an explicit function parameter/input instead

repi · 2020-08-19T19:37:32Z

examples/ark-lighting/rust/mesh.rs

+    if instance.flags.contains(ALPHA_BLENDING_ENABLE | PREMULTIPLIED_ALPHA_ENABLE) {
+        if instance.flags.contains(PREMULTIPLIED_ALPHA_ENABLE)  {


think this is pretty fine though, somewhat cleaner and less error prone to read than the GLSL switch statement with bitwise logic in it me thinks.
and for other cases Rust's match will be really good

Jasper-Bekkers · 2020-08-20T09:49:45Z

@repi your comments appear duplicated, any way to collapse them?

repi · 2020-08-20T11:24:41Z

Oh that is very odd, I think it happened somehow because I left comments in existing comments while I was doing my review, and GitHub somehow duplicates that, odd. Doesn't seem to be a way to remove it.

I probably shouldn't have choosen "Request changes", but instead just "Add comments". Think it lists them as the changes I requested

repi · 2020-08-26T12:40:36Z

@Jasper-Bekkers think we could merge this in and file some follow up issues around the specific larger syntax discussions here? Then one can iterate further on it in future PRs

Jasper-Bekkers · 2020-11-01T10:07:08Z

Now that we have specific shaders to test with, we should probably close this issue and address ergonomics on a case-by-case basis.

Sketch out some fragment & lighting shaders, with comparitive glsl ne…

227694b

…xt to it

Jasper-Bekkers commented Aug 19, 2020

View reviewed changes

Jasper-Bekkers requested review from khyperia and repi August 19, 2020 17:23

Jasper-Bekkers mentioned this pull request Aug 19, 2020

Pseudo port of a simple lighting shader #13

Closed

Sketch out some vertex shader stuff, not final

4132683

h3r2tic reviewed Aug 19, 2020

View reviewed changes

repi suggested changes Aug 19, 2020

View reviewed changes

repi approved these changes Aug 20, 2020

View reviewed changes

repi mentioned this pull request Aug 20, 2020

Meeting 2020-08-20 #15

Closed

repi mentioned this pull request Sep 18, 2020

Meeting 2020-09-17 #30

Closed

Jasper-Bekkers closed this Nov 1, 2020

khyperia removed their request for review January 11, 2021 09:59

XAMPPRocky deleted the sketch-shaders branch April 30, 2021 07:36

		env_params: lighting::EnvParams,
		view_params: lighting::ViewParams,

		if instance.flags.contains(ALPHA_BLENDING_ENABLE \| PREMULTIPLIED_ALPHA_ENABLE) {
		if instance.flags.contains(PREMULTIPLIED_ALPHA_ENABLE) {

Sketch simple shaders #14

Sketch simple shaders #14

Conversation

Jasper-Bekkers commented Aug 19, 2020 • edited by repi Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jasper-Bekkers commented Aug 19, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

repi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jasper-Bekkers commented Aug 20, 2020

repi commented Aug 20, 2020 • edited Loading

repi commented Aug 26, 2020

Jasper-Bekkers commented Nov 1, 2020

Jasper-Bekkers commented Aug 19, 2020 •

edited by repi

Loading

repi commented Aug 20, 2020 •

edited

Loading