Skip to content
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

Upgrade to wpgu 0.10 #66

Merged
merged 1 commit into from
Sep 3, 2021
Merged

Upgrade to wpgu 0.10 #66

merged 1 commit into from
Sep 3, 2021

Conversation

kylc
Copy link
Contributor

@kylc kylc commented Aug 19, 2021

This PR updates the library to be compatible with wgpu 0.10.

Everything compiles and all the demos run, except for the cube.rs demo. It seems to stall out in the Example::render() function when submitting the command buffer to the queue. I will need to investigate if this is due to a change in wgpu or just a bug in updating to the latest version.

Because of this unsolved issue this PR is marked as WIP.

Fixes #65

@willemv
Copy link
Contributor

willemv commented Aug 23, 2021

For me, every example fails except the cube sample:

C:\Users\user\Development\imgui-wgpu-rs(master)> cargo run --example hello_world
   Compiling imgui-wgpu v0.16.0 (C:\Users\user\Development\imgui-wgpu-rs)
    Finished dev [unoptimized + debuginfo] target(s) in 8.75s
     Running `target\debug\examples\hello_world.exe`
[2021-08-23T20:27:33Z ERROR wgpu_core::validation] Unexpected varying type: Array { base: [1], size: Constant([5]), stride: 4 }
[2021-08-23T20:27:33Z ERROR wgpu_core::validation] Unexpected varying type: Array { base: [1], size: Constant([5]), stride: 4 }
[2021-08-23T20:27:33Z ERROR wgpu::backend::direct] wgpu error: Validation Error

    Caused by:
        In Device::create_render_pipeline
          note: label = `imgui-wgpu pipeline`
        Internal error in VERTEX shader: D3DCompile error (0x80004005): C:\Users\user\Development\imgui-wgpu-rs\imgui.vert.spv(38,12-17): error X3003: redefinition of 'VertexInput_main::member'



thread 'main' panicked at 'Handling wgpu errors as fatal by default', C:\Users\user\.cargo\registry\src\github.com-1ecc6299db9ec823\wgpu-0.10.1\src\backend\direct.rs:2159:5
stack backtrace:
   0: std::panicking::begin_panic<str>
             at /rustc/a178d0322ce20e33eac124758e837cbd80a6f633\library\std\src\panicking.rs:541
   1: wgpu::backend::direct::default_error_handler
             at C:\Users\user\.cargo\registry\src\github.com-1ecc6299db9ec823\wgpu-0.10.1\src\backend\direct.rs:2159
   2: core::ops::function::Fn::call<fn(enum$<wgpu::Error>),tuple<enum$<wgpu::Error>>>
             at /rustc/a178d0322ce20e33eac124758e837cbd80a6f633\library\core\src\ops\function.rs:70
   3: alloc::boxed::{{impl}}::call<tuple<enum$<wgpu::Error>>,UncapturedErrorHandler,alloc::alloc::Global>
             at /rustc/a178d0322ce20e33eac124758e837cbd80a6f633\library\alloc\src\boxed.rs:1589
   4: wgpu::backend::direct::ErrorSinkRaw::handle_error
             at C:\Users\user\.cargo\registry\src\github.com-1ecc6299db9ec823\wgpu-0.10.1\src\backend\direct.rs:2146
   5: wgpu::backend::direct::Context::handle_error<enum$<wgpu_core::pipeline::CreateRenderPipelineError>>
             at C:\Users\user\.cargo\registry\src\github.com-1ecc6299db9ec823\wgpu-0.10.1\src\backend\direct.rs:166
   6: wgpu::backend::direct::{{impl}}::device_create_render_pipeline
             at C:\Users\user\.cargo\registry\src\github.com-1ecc6299db9ec823\wgpu-0.10.1\src\backend\direct.rs:1240
   7: wgpu::Device::create_render_pipeline
             at C:\Users\user\.cargo\registry\src\github.com-1ecc6299db9ec823\wgpu-0.10.1\src\lib.rs:1709
   8: imgui_wgpu::Renderer::new
             at .\src\lib.rs:387
   9: hello_world::main
             at .\examples\hello_world.rs:98
  10: core::ops::function::FnOnce::call_once<fn(),tuple<>>
             at /rustc/a178d0322ce20e33eac124758e837cbd80a6f633\library\core\src\ops\function.rs:227
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
error: process didn't exit successfully: `target\debug\examples\hello_world.exe` (exit code: 101)

@willemv
Copy link
Contributor

willemv commented Aug 23, 2021

All examples work when forcing the backend to VULKAN

@willemv willemv mentioned this pull request Aug 23, 2021
6 tasks
@willemv
Copy link
Contributor

willemv commented Aug 23, 2021

The failing build steps are caused by new clippy warnings, #68 fixes those in a separate PR.

@cwfitzgerald
Copy link
Collaborator

Thanks for this! #68 is in, so you can rebase.

@kylc
Copy link
Contributor Author

kylc commented Aug 25, 2021

@willemv Interesting that it seems to fail in the shader validation stage. In this PR those compiled shaders are unchanged. I wonder if this is an issue introduced in the wgpu D3D backend in version 0.10.

You might try this PR in conjunction with #67. Naga will likely compile shaders to more wgpu-compliant SPIR-V.

@willemv
Copy link
Contributor

willemv commented Aug 26, 2021

@kylc Thanks for the suggestion. I tried in conjunction with your #67 PR, but that didn't fix it

Looking into this some more, it looks like a bug in naga. It translates the compiled spirv file to a hlsl file containing the following struct :

struct VertexInput_vs_main {
    float2 member : LOC0;
    float2 member : LOC1;
    float4 member : LOC2;
};

which is obviously wrong (three members with the same name).

I'll log an issue with naga to follow up on this.

In the meantime, we could convert the shaders to straight wgsl, which, I think, will avoid the bug by losing less naming information.

@cwfitzgerald
Copy link
Collaborator

I think converting the shaders to wgsl is likely a good idea anyway. I can help with this process if it's needed.

@willemv
Copy link
Contributor

willemv commented Aug 26, 2021

I'm not familiar with wgsl, so @cwfitzgerald if you could help with that that would be nice

@kylc
Copy link
Contributor Author

kylc commented Aug 26, 2021

I have a WGSL conversion over on this branch: https://github.com/kylc/imgui-wgpu-rs/tree/to-wgsl

The differences are 1) the individual vertex and fragment shaders have been combined into a single shader in the imgui code, 2) there is no WGSL preprocessor so we have to use simple string substitution to manage the SRGB/linear color definitions.

I had originally abandoned the effort because I read WGSL was not yet supported by some wgpu backends (web?), but I can no longer find that info so I may be mistaken.

@cwfitzgerald
Copy link
Collaborator

@kylc the code looks good, but we should be able to do without the string replacement by having multiple entry points:

[[stage(fragment)]]
fn fs_main_linear(in: VertexOutput) -> FragmentOutput {
    let color = srgb_to_linear(in.v_Color);

    return FragmentOutput(color * textureSample(u_Texture, u_Sampler, in.v_UV));
}

[[stage(fragment)]]
fn fs_main_srgb(in: VertexOutput) -> FragmentOutput {
    let color = in.v_Color;

    return FragmentOutput(color * textureSample(u_Texture, u_Sampler, in.v_UV));
}

Then when we make the pipeline, just also pass in the fragment shader entry point name.

@kylc
Copy link
Contributor Author

kylc commented Aug 26, 2021

Great idea, I hadn't considered that. I will update the branch and open a PR when I have a chance.

@cwfitzgerald
Copy link
Collaborator

Thank you so much!

@kylc
Copy link
Contributor Author

kylc commented Aug 27, 2021

See #69 for shader translation to WGSL.

parasyte added a commit to parasyte/pixels that referenced this pull request Aug 31, 2021
- It would be nice to return an error from the render function
- imgui-winit is still a WIP (open PR: Yatekii/imgui-wgpu-rs#66)
- minimal-fltk flashes wildly! :(
@willemv
Copy link
Contributor

willemv commented Sep 1, 2021

I'll log an issue with naga to follow up on this.

I finally got around to logging that issue: gfx-rs/naga#1316

@cwfitzgerald
Copy link
Collaborator

cwfitzgerald commented Sep 1, 2021

@kylc what's the status here, naga patch will be out in the next couple hours :)

parasyte added a commit to parasyte/pixels that referenced this pull request Sep 1, 2021
- It would be nice to return an error from the render function
- imgui-winit is still a WIP (open PR: Yatekii/imgui-wgpu-rs#66)
- Update README

Co-authored-by: Mohammed Alyousef <mohammed.alyousef@neurosrg.com>
@kylc
Copy link
Contributor Author

kylc commented Sep 1, 2021

I haven't yet been able to resolve the deadlock issue in the cube example that I mentioned in the PR. That said, it only happens on one of my machines (Intel UHD 620/Vulkan backend) and the cube demo seems like a somewhat non-standard usage of wgpu, creating multiple CommandEncoders per render frame.

I'd be curious to hear if anyone else has trouble with the cube example. If not, maybe this is just down to the configuration of that one machine and shouldn't hold up this merge.

@cwfitzgerald
Copy link
Collaborator

@kylc this seems like gfx-rs/wgpu#1672 which, while unfortunate, we shouldn't block on.

@kylc kylc force-pushed the wgpu-0.10 branch 2 times, most recently from fd1913c to a234b59 Compare September 2, 2021 05:09
@kylc kylc marked this pull request as ready for review September 2, 2021 05:09
@kylc
Copy link
Contributor Author

kylc commented Sep 2, 2021

Thanks for tracking that down. Ready to merge on my end. I just rebased and updated the changelog.

@kylc kylc changed the title WIP: Upgrade to wpgu 0.10 Upgrade to wpgu 0.10 Sep 2, 2021
@kylc
Copy link
Contributor Author

kylc commented Sep 2, 2021

Fixed formatting issues.

@cwfitzgerald
Copy link
Collaborator

Thank you so much! I want to land this and #69 and I'll cut a release.

@cwfitzgerald cwfitzgerald merged commit d5e82a9 into Yatekii:master Sep 3, 2021
@ChristianIvicevic
Copy link

Is there any plan when the release will be rolled out? I'd like to upgrade to wgpu 0.10 myself but have to wait for the changes in this PR to be released.

@cwfitzgerald
Copy link
Collaborator

@ChristianIvicevic cutting a release as we speak

@cwfitzgerald
Copy link
Collaborator

Published!

@kylc
Copy link
Contributor Author

kylc commented Sep 4, 2021

Thank you @cwfitzgerald!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

WGPU 0.10
4 participants