Bumping wgpu from 0.14.2 to 0.15.1

[jpteb]

Hello,
You guys don't have a policy for contributing, or I didn't find it, so I hope a pull request out of the blue is no problem. If it is, you can reject it, no problem.

Updated all code necessary to run on wgpu 0.15.

• For now, the new view_formats aren't used and just contain the same format as the texture itself.
• Expecting on the now fallible Instance::create_surface() method. I found other expect()s in the code base, so I assume it should be fine for now? There is no error handling in Gpu::with_config(), might be a todo.
• The new instance descriptor uses the FXC compiler for Windows (previous default AFAIK) and all others. For all others, it shouldn't matter as it is DX12 only. For Windows, the possibility of using the DXC compiler should be explored as it seems to be a better alternative to FXC. But the DXC compiler requires two DLLs to be shipped with the executable. Not sure if that is something that is wanted.

Maybe I misunderstood the new view_formats parameter, so might be worth to double check that.

Closes #27.

[SK83RJOSH]

I'll leave this for Fredrik to review in more detail, but LGTM from a cursory glance! If you haven't already, I would test a few of the guest/rust/examples (using cargo install --path=app + ambient run --debug; and via the .vscode debug task) to see if there's any regressions or validation errors.

Nice work, and thank you for the contribution. It's very much appreciated. 😄

[jpteb]

Oh, I forgot something I thought of earlier.. Updating the shader code, as a global let is not valid anymore in wgsl. I will add those changes now.

[SK83RJOSH]

@Gosuo there's a fair bit of wgsl being generated in gpu_ecs if I recall correctly. If you grep for get_entity_# you should be able to find it. Not sure if it's using any global let – but best to be aware of it 🙂

[jpteb]

Okay, like you suggested I built ambient with cargo install and tried to run the project created by ambient new. I found some top level lets I changed to consts. I got a new problem now though. There seems to be a redefinition in one of the shaders which seems to be an error with wgpu v0.15. I built ambient with wgpu v0.14 and that error disappeared. I get this error with wgpu v0.15:

[2023-02-24T01:46:49Z ERROR wgpu::backend::direct] Handling wgpu errors as fatal by default
thread 'main' panicked at 'wgpu error: Validation Error

Caused by:
    In Device::create_shader_module
      note: label = `collect`

Shader 'collect' parsing error: redefinition of `MeshMetadata`
    ┌─ wgsl:241:8
    │
241 │ struct MeshMetadata {
    │        ^^^^^^^^^^^^ previous definition of `MeshMetadata`
    ·
258 │ struct MeshMetadata {
    │        ^^^^^^^^^^^^ redefinition of `MeshMetadata`


    redefinition of `MeshMetadata`

', /home/jan/.cargo/registry/src/github.com-1ecc6299db9ec823/wgpu-0.15.1/src/backend/direct.rs:3024:5
stack backtrace:
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

I dug a little further and I'm pretty sure the error originates from these lines:
collect.rs, renderer/lib.rs, mesh_buffer.rs, and it might even be in here. Unfortunately those shader creations are a little over my head for now, but I'd guess, removing either MESH_BUFFER_TYPES_WGSL or get_mesh_buffer_types() in crates/gpu/src/mesh_buffer.rs might be enough.

There might be more incompatible WGSL things that I cannot check right now, but will test for, if there is a solution for the above mentioned redefinition.

[FredrikNoren]

@Gosuo This is awesome! Yeah I think your analysis that removing the MESH_BUFFER_TYPES_WGSL in one of those locations should fix it.

[jpteb]

I think the problem runs more deeply. I'm finding lots of shadowed variables in wgsl code. As an example:

let ang = dot(normalize((mid - anchor)), normalize(corner));
let ang = dot(normalize(mid - anchor), normalize(corner - anchor));

There are a lot more cases like this. I think shadowing is against the wgsl spec, note:

Two declarations in the same WGSL source program must not simultaneously:

• introduce the same identifier name, and
• have the same end-of-scope.

and this comment by kvark.

Maybe I misunderstood the spec, but I'm going to start rewriting all the shader code to reflect those rules. This might be something you'd need to talk about in your team, as I expect to change a lot of code there.

[jpteb]

I vastly overestimated the amount of code that needed to be fixed for this. I think this should be it @FredrikNoren. Building ambient on this branch works for me and I get no more shader errors. It might be a good idea to double check if it works on other machines as well, as I am away from home and cannot check on multiple machines for now.

[philpax]

Awesome, thanks! We'll test it across the platforms next week and merge it if all goes well. This might end up in our 0.2 release depending on scheduling, but I'm looking forward to seeing it landing. Thanks for the contribution, we really appreciate it!

[FredrikNoren]

@Gosuo This is awesome! I tested it locally, and the only thing that didn't work was the sun example.

Also please note we changed the license of the repo today to dual MIT / Apache2: #160

[ten3roberts]

When porting to the web, we are likely to encounter this issue:

gfx-rs/wgpu#3430

I have a minimal wgpu web app demo which encounters the previously mentioned issue when upgrading wgpu

[philpax]

When porting to the web, we are likely to encounter this issue:

gfx-rs/wgpu#3430

I have a minimal wgpu web app demo which encounters the previously mentioned issue when upgrading wgpu

Looks like this has been fixed gfx-rs/wgpu#3657 🚀

Now we just need to wait for a new release of wgpu, and to update this PR + fix any lingering issues 🙂

[philpax]

I'm going to close this PR in favour of #308, as this PR is quite out of date now (our renderer has changed significantly to accommodate the web)

Thanks for the work, everyone! Sorry about it not being able to land :(