Cleanup pass on example shader and vk example-runner #109

fu5ha · 2020-10-23T00:56:53Z

Removes vertex and index buffers; instead, generates position data based on vertex index in the shader
Removes depth buffer as it's not being used anyway
Removes the unused vertex_color input from vertex shader
Renames the wrongly-names out_color output in the vertex shader to out_pos with corresponding in_pos in the frag shader, which is what it's being used for (that's the data that gets wired to the fragment shader input at location 0, because the builtin "position" output is separate). We could also just scrap this output and use #[spirv(builtin = "frag_coord")] in the fragment shader but I think it's helpful to have there in the example.
Renames a couple of variables in the fragment shader for better clarity
Uses negative viewport height (flips viewport) which brings clip space to parity with wgpu's (and DirectX's) clip space, useful if we want to directly use the same shader in WGPU example #92 because otherwise, it will be flipped
Uses sRGB-encoded surface rather than doing odd sorta-gamma correction in shader
Replaces the weird tone mapping step and uncharted 2 tone mapping operator with a new (better ;) ) one https://www.desmos.com/calculator/igytv8ck36

hrydgard · 2020-10-23T06:47:33Z

Nice! But I think you'll end up needing the depth buffer later, for future more complex examples. Also, vertex input I'd keep around for the same reason - that still needs to work with Rust shaders, so why not keep it so it gets tested?

I do of course agree that removing this stuff makes the example code a lot nicer and shorter so if you intend to have a second bigger example-runner later, keeping this smaller one like this might be worth it.

Jasper-Bekkers · 2020-10-23T11:41:27Z

examples/example-shader/src/lib.rs

+    #[spirv(builtin = "position")] mut gl_pos: Output<Vec4>,
+    mut out_pos: Output<Vec4>,


Could we call these out_pos and out_pos_interpolator or something? I would love us to not have the gl_ prefix anywhere if we can avoid it.

Jasper-Bekkers · 2020-10-23T11:55:56Z

examples/example-shader/src/lib.rs

-    tex_color *= Vec3::splat(0.04);
-    tex_color += Vec3::new(0.0, 0.001, 0.0025) * 0.3;
+    let tex_color = lin + l0;

    // Tonemapping
-    let white_scale = 1.0 / uncharted2_tonemap(TONEMAP_WEIGHTING);
-    let curr = uncharted2_tonemap(((2.0 / LUMINANCE.pow(4.0)).log2()) * tex_color);
-    let color = curr * white_scale;
-
-    color.pow(1.0 / (1.2 + (1.2 * sunfade)))
+    let color = tex_color.min(Vec3::splat(1024.0));


I'm not sure about most of what's going on here, for example, I'm not even sure why the sky model chose to embed the tone-mapping operator in here to begin with (or even why it's doing srgb conversions tbh). It might be nicer to hoist all of that out of sky all together?

One thing I do notice is that your change seems to remove the application of sunfade. I suspect this was done to make the sun seem brighter.

I'm not even sure why the sky model chose to embed the tone-mapping operator in here to begin with
...
It might be nicer to hoist all of that out of sky all together?

Yeah it would... the tonemapping is really just because the sky is being displayed directly... so it makes sense to put it in the main frag shader and not in sky().

and while it does remove sunfade from here, I suspect that the application of sunfade here was really just a hacked in "auto brightness slider" (which really shouldnt be done as a gamma anyway) based on a vaguely related value... it's still being used in the calculation of the rayleigh coefficient right after it gets defined.

fu5ha · 2020-10-23T16:58:37Z

Nice! But I think you'll end up needing the depth buffer later, for future more complex examples. Also, vertex input I'd keep around for the same reason - that still needs to work with Rust shaders, so why not keep it so it gets tested?

I do of course agree that removing this stuff makes the example code a lot nicer and shorter so if you intend to have a second bigger example-runner later, keeping this smaller one like this might be worth it.

while I agree that it's probable we'll want these things at some point, I'm a bit skeptical of having a unified example-runner that's capable of doing lots of things at this point... and in any case, it's quite easy to add those back in the same state they are now, since it's just copied from the ash examples

khyperia · 2020-10-23T17:03:41Z

Throwing in my two cents, I'm strongly in favor of nuking the ash example and just going with wgpu. I think the only reason we're still on ash is because we were waiting for you to join and do the work, @termhn :P - sorry, probably should have made that more clear, so you didn't put too much work into cleaning the ash example up.

Although, Viktor already started on the wgpu example, could use that as a base

(So I'm in favor of doing whatever to the ash example right now, because I imagine it'll be gone soon)

fu5ha · 2020-10-23T17:06:16Z

Throwing in my two cents, I'm strongly in favor of nuking the ash example and just going with wgpu. I think the only reason we're still on ash is because we were waiting for you to join and do the work, @termhn :P - sorry, probably should have made that more clear, so you didn't put too much work into cleaning the ash example up.

Although, Viktor already started on the wgpu example, could use that as a base

(So I'm in favor of doing whatever to the ash example right now, because I imagine it'll be gone soon)

I'm sorta of the same mind haha. And don't worry, didn't put too much work into the ash one, the main purpose originally was actually just to change it so that the sky shader example works with no friction with the wgpu example Viktor started :D

repi · 2020-10-24T09:29:02Z

So I'm in favor of doing whatever to the ash example right now, because I imagine it'll be gone soon

I would like the wgpu example to be the primary one as it is nicer and faster to use and built on more of a Rust stack.

But do think we'll have to keep a ash + MoltenVK example and test suite as we'll need a lower level mechanism where we are not blocked by the gfx-rs stack, the WebGPU standard nor gfx-portability for Vulkan and SPIRV translation to Metal and MetalSL respectively. MoltenVK is problematic to build but it is a lot more mature than gfx-portability and I do think we will need to be able to test on both.

fu5ha · 2020-10-28T02:38:22Z

Rebased on main and applied changes in order to normalize output between all three example runners. Also renamed example-shader to sky-shader and wgpu-example-shader to simplest-shader and addressed previous review comments from @Jasper-Bekkers

khyperia · 2020-10-28T08:00:24Z

Looks like there's still a fair number of references to example-shader in docs and comments and whatnot: README.md, examples/wgpu-example-runner/build.rs, examples/example-runner/build.rs, and docs/src/introduction.md

Also there are two Cargo.lock files not at the root that were accidentally added in #92, can you remove those? I know you're just moving the files and not touching them, but it'd be nice to clean those up too. edit: oh whoops apparently one of them was added by me a while back

repi · 2020-10-28T09:28:09Z

The Cargo.lock files are gone on main in e2be058

khyperia · 2020-10-28T10:25:41Z

Also, optional thing, winit is out of date (0.22.1 -> 0.23.0), feel free to update it here, or we can do it afterwards

Jasper-Bekkers · 2020-10-28T13:51:43Z

What do we think about @hrydgard's comment?

Nice! But I think you'll end up needing the depth buffer later, for future more complex examples. Also, vertex input I'd keep around for the same reason - that still needs to work with Rust shaders, so why not keep it so it gets tested?

I tend to agree with him - keeping it is probably best (which is why I +1'd it), but there may be other opinions here?

fu5ha · 2020-10-28T21:37:15Z

I think my opinion is #109 (comment) and @khyperia 's is #109 (comment)

fu5ha · 2020-10-29T14:22:07Z

@XAMPPRocky made a couple docs changes in 38d2735 that might be relevant for your review

Jasper-Bekkers · 2020-10-29T19:43:13Z

Oh @termhn this PR should probably update the screenshot in the README.md as well to the new one, would be less confusing for new people.

- get rid of unused attributes in example shader - use negative viewport height to match wgpu

remove 'gamma correction' in favor of hardware srgb support

fix ci example name on maOS

fu5ha · 2020-10-30T18:38:03Z

Pushing the big green button manually 'cause mergify can't merge this one :) 🎉

Jasper-Bekkers suggested changes Oct 23, 2020

View reviewed changes

Jasper-Bekkers reviewed Oct 23, 2020

View reviewed changes

fu5ha requested a review from Jasper-Bekkers October 28, 2020 02:14

khyperia mentioned this pull request Oct 28, 2020

fingerprint.rs uses different target roots when serializing/deserializing rust-lang/cargo#8817

Closed

fu5ha mentioned this pull request Oct 28, 2020

Crate, Example, Documentation and Test Organisation #170

Closed

fu5ha requested a review from XAMPPRocky October 29, 2020 14:21

Jasper-Bekkers approved these changes Oct 29, 2020

View reviewed changes

XAMPPRocky approved these changes Oct 30, 2020

View reviewed changes

khyperia mentioned this pull request Oct 30, 2020

feat: implemented swap chain recreation #103 #130

Closed

fu5ha added 8 commits October 30, 2020 10:28

- get rid of vertex buffers in example

904d0a2

- get rid of unused attributes in example shader - use negative viewport height to match wgpu

remove depth buffer

cb4da77

use SRGB surface

4ebfa94

improve tonemapping

c57c642

remove 'gamma correction' in favor of hardware srgb support

make clippy happy

d665e90

move tonemapping out of sky(), rename gl_pos to builtin_pos

413c53f

rename shaders and use sky shader in wgpu example runner

4336822

apply srgb OETF and invert clip space for cpu example runner

1920c6f

fu5ha added 8 commits October 30, 2020 10:43

restructure example directory structure according to #170

439f711

update winit in wgpu example runner

e00a77d

fix deny.toml example crate refs

84f53e1

fix ci example name on maOS

example-shader -> sky-shader in docs

01a2b76

update sky shader image

ddd0397

fix ci clippy script

22a8b4d

actually fix clippy due to #186 hopefully

102764b

re-enable clippy and make it happy

fffeafd

fu5ha merged commit b12a2f3 into EmbarkStudios:main Oct 30, 2020

fu5ha deleted the example-cleanup branch October 30, 2020 18:39

XAMPPRocky mentioned this pull request Nov 2, 2020

Move top-level crates into crates/ folder #173

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup pass on example shader and vk example-runner #109

Cleanup pass on example shader and vk example-runner #109

fu5ha commented Oct 23, 2020 •

edited

Loading

hrydgard commented Oct 23, 2020 •

edited

Loading

Jasper-Bekkers Oct 23, 2020

Jasper-Bekkers Oct 23, 2020

fu5ha Oct 23, 2020

fu5ha commented Oct 23, 2020

khyperia commented Oct 23, 2020

fu5ha commented Oct 23, 2020

repi commented Oct 24, 2020

fu5ha commented Oct 28, 2020

khyperia commented Oct 28, 2020 •

edited

Loading

repi commented Oct 28, 2020

khyperia commented Oct 28, 2020

Jasper-Bekkers commented Oct 28, 2020

fu5ha commented Oct 28, 2020

fu5ha commented Oct 29, 2020 •

edited

Loading

Jasper-Bekkers commented Oct 29, 2020

fu5ha commented Oct 30, 2020

		#[spirv(builtin = "position")] mut gl_pos: Output<Vec4>,
		mut out_pos: Output<Vec4>,

Cleanup pass on example shader and vk example-runner #109

Cleanup pass on example shader and vk example-runner #109

Conversation

fu5ha commented Oct 23, 2020 • edited Loading

hrydgard commented Oct 23, 2020 • edited Loading

Jasper-Bekkers Oct 23, 2020

Choose a reason for hiding this comment

Jasper-Bekkers Oct 23, 2020

Choose a reason for hiding this comment

fu5ha Oct 23, 2020

Choose a reason for hiding this comment

fu5ha commented Oct 23, 2020

khyperia commented Oct 23, 2020

fu5ha commented Oct 23, 2020

repi commented Oct 24, 2020

fu5ha commented Oct 28, 2020

khyperia commented Oct 28, 2020 • edited Loading

repi commented Oct 28, 2020

khyperia commented Oct 28, 2020

Jasper-Bekkers commented Oct 28, 2020

fu5ha commented Oct 28, 2020

fu5ha commented Oct 29, 2020 • edited Loading

Jasper-Bekkers commented Oct 29, 2020

fu5ha commented Oct 30, 2020

fu5ha commented Oct 23, 2020 •

edited

Loading

hrydgard commented Oct 23, 2020 •

edited

Loading

khyperia commented Oct 28, 2020 •

edited

Loading

fu5ha commented Oct 29, 2020 •

edited

Loading