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

Update to wgpu-rs 0.8 #54

Closed
klashenriksson opened this issue May 2, 2021 · 11 comments · Fixed by #55
Closed

Update to wgpu-rs 0.8 #54

klashenriksson opened this issue May 2, 2021 · 11 comments · Fixed by #55

Comments

@klashenriksson
Copy link

Hello!

wgpu-rs 0.8 was just released and had some minor changes to structure names and parameters. I have a fork ready which is updated to use wgpu 0.8, should I do a PR of that to bring this up to 0.8? :)

@klashenriksson
Copy link
Author

klashenriksson commented May 2, 2021

update: I spoke too soon. There's a runtime crash with error message that index buffers must be multiple of 4, which seems a bit weird... Crash occurs when uploading index buffers:

        for draw_list in draw_data.draw_lists() {
            self.vertex_buffers
                .push(self.upload_vertex_buffer(device, draw_list.vtx_buffer()));
            self.index_buffers
                .push(self.upload_index_buffer(device, draw_list.idx_buffer()));
        }

starting at line 498 in lib.rs

parasyte added a commit to parasyte/pixels that referenced this issue May 5, 2021
- Also update other dependencies
- TODO: imgui-wgpu is blocking this. See: Yatekii/imgui-wgpu-rs#54
@Uriopass
Copy link
Contributor

Uriopass commented May 6, 2021

Any update? Did you manage to get it working? I might take a look if you didn't :-)

@benmkw
Copy link
Contributor

benmkw commented May 6, 2021

@Uriopass maybe you can look at #52 (comment) which still at least has the 4 byte issue

@klashenriksson
Copy link
Author

klashenriksson commented May 6, 2021

Hi @Uriopass ! I did get it working, however the fix is certainly not the most effective one, but works fine for my cases atm. Upon index upload I just convert the indices to u32's instead of u16's.

Have a look at https://github.com/klashenriksson/imgui-wgpu-rs/commits/wgpu-v0.8 (and klashenriksson@3c01713 for the alignment fix). I dont really have the bandwidth to do a proper fix for this atm but maybe my code can help get you started :)

@Uriopass
Copy link
Contributor

Uriopass commented May 7, 2021

Found the issue upstream, buffer initialization was not done properly when providing unaligned data :-)

bors bot added a commit to gfx-rs/wgpu-rs that referenced this issue May 7, 2021
900: Fix initializing buffer with unaligned data r=grovesNL a=Uriopass

In wgpu 0.7, this was a valid operation, the zero padding was done by hand. 
In #872 this logic was removed as an optimization since buffers are already zero initialized, however the slice end was taken from buffer.slice which has to be aligned when mapped.

This is causing issues when trying to update imgui-wgpu-rs (Yatekii/imgui-wgpu-rs#54) to  wgpu 0.8 since they  use U16 index buffers with odd lengths.

Co-authored-by: Pâris DOUADY <[email protected]>
@klashenriksson
Copy link
Author

Nice! Good find :)

@benmkw
Copy link
Contributor

benmkw commented May 7, 2021

@Uriopass nice, it still seems to be an issue that TriangleList can not be used with u16 indices though?

I get [0.094421 ERROR]()(no module): Unexpected varying type: Array { base: [1], size: Constant([5]), stride: 4 } when using u32 as the index format (which is wrong as far as I can see) although rendering works, I'm not sure why though 😄 when I change it to u16 I get strip index format was not set to None but to Some(Uint16) while using the non-strip topology TriangleList

@Uriopass
Copy link
Contributor

Uriopass commented May 7, 2021

when I change it to u16 I get strip index format was not set to None but to Some(Uint16) while using the non-strip topology TriangleList

I'm pretty sure imgui uses a TriangleList topology :) you should keep the
PrimitiveTopology::TriangleList
and keep strip_index_format to None.

I do get the error "0.094421 ERROR(no module): Unexpected varying type: Array { base: [1], size: Constant([5]), stride: 4 }" however I think that might just be a naga problem and it works for me it's not critical.

@benmkw
Copy link
Contributor

benmkw commented May 7, 2021

you should keep the PrimitiveTopology::TriangleList

yes thats what I figured as well but when you look at upload_index_buffer and DrawIdx you can see that its u16 so it may only work by luck ...

keep strip_index_format to None

this makes the indices be u32

@Uriopass
Copy link
Contributor

Uriopass commented May 7, 2021

this makes the indices be u32

No it doesn't, the indices format is determined at the set_index_buffer time in the renderpass.
https://docs.rs/wgpu/0.8.1/wgpu/struct.RenderPass.html#method.set_index_buffer

the strip_index_format should only be specified when in TriangleStrip mode, thus why you get the error.

@benmkw
Copy link
Contributor

benmkw commented May 7, 2021

ah right somehow mixed None and Default in my mind and did not know enough about the API, thanks for the explanation 👍

kvark pushed a commit to kvark/wgpu that referenced this issue Jun 3, 2021
900: Fix initializing buffer with unaligned data r=grovesNL a=Uriopass

In wgpu 0.7, this was a valid operation, the zero padding was done by hand. 
In gfx-rs#872 this logic was removed as an optimization since buffers are already zero initialized, however the slice end was taken from buffer.slice which has to be aligned when mapped.

This is causing issues when trying to update imgui-wgpu-rs (Yatekii/imgui-wgpu-rs#54) to  wgpu 0.8 since they  use U16 index buffers with odd lengths.

Co-authored-by: Pâris DOUADY <[email protected]>
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 a pull request may close this issue.

3 participants