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 0.6.0 #52

Merged
merged 1 commit into from
Oct 21, 2020
Merged

Update to wgpu 0.6.0 #52

merged 1 commit into from
Oct 21, 2020

Conversation

ncharlton02
Copy link
Collaborator

Updates to wgpu-core 0.6.0. This is very much a WIP and many things still need to be implemented (textures, push constants), but I thought I would throw this up for comments. Also for error handling, anytime a result is an error, it just panics with a description of the function that panicked.

Testing:
Triangle/Compute Examples

After I finish updating everything I will test it against my own stuff

Copy link
Contributor

@monocodus monocodus bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an autogenerated code review.

Checker summary (by rust_clippy):
The tool has found 0 warnings, 1 errors.

The .monocodus config not found in your repo. Default config is used.
Check config documentation here

@kvark
Copy link
Member

kvark commented Oct 12, 2020

Should this be marked as a "draft" until you finish?

@ncharlton02 ncharlton02 marked this pull request as draft October 12, 2020 03:00
@ncharlton02
Copy link
Collaborator Author

oops! meant to do that 😄

@ncharlton02 ncharlton02 force-pushed the update-to-0.6.0 branch 3 times, most recently from 5d0c0f3 to 38b3b83 Compare October 18, 2020 02:40
@ncharlton02
Copy link
Collaborator Author

This is hopefully finished now!

Notes:

  • Updates wgpu-core to master branch (rev 8ce2530b)
  • Adds repr(C) versions for BufferCopyView, TextureCopyView, TextureViewDescriptor, CompareFunction, PipelineLayoutDescriptor, VertexBufferDescriptor, VertexStateDescriptor, RenderPipelineDescriptor, ComputePipelineDescriptor
  • Adds error messages when wgpu-core returns and Err(_).

Testing
wgpu-native and kgpu examples work

cbingen bug note:

The following pattern causes cbindgen to crash:

//In wgt
mod wgt {
    pub struct Foo<T> {
        foo: T,
    }
}

//In wgc
pub type Foo = wgt::Foo<u32>;

//In wgpu-native
mod native {
    #[repr(C)]
    pub struct Foo{
        foo: u32
    }
}

Basically, cbindgen will panic because it ignores the module name and therefore will have multiple types to export with the same name. This pattern is used by BufferCopyView and TextureCopyView. To work around it, the native versions of the struct are called BufferCopyViewC and TextureCopyViewC, and they are renamed when CBindgen generates the header. A solution would be to move TextureCopyView and BufferCopyView into wgpu-core and remove the type alias.

See mozilla/cbindgen#573 for more details

@kvark
Copy link
Member

kvark commented Oct 18, 2020

To work around it, the native versions of the struct are called BufferCopyViewC and TextureCopyViewC

I think we used the Raw suffix for those previously. I don't mind either way, let's just try to be consistent here.

Copy link
Member

@kvark kvark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing work here, thank you! A few notes before we proceed

cbindgen.toml Outdated
@@ -27,7 +28,8 @@ style = "both"
[export]
prefix = "WGPU"
include = [
"AnisotropicSamplerDescriptorExt"
"AnisotropicSamplerDescriptorExt",
"SamplerBorderColor"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tab detected :)

src/command.rs Outdated
pub buffer: id::BufferId,
}

impl Into<wgc::command::BufferCopyView> for BufferCopyViewC {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it would be simpler to just have this as pub fn to_wgpu(&self) -> wgc::command::BufferCopyView. since it would turn xxx.cloned().into() into xxx.to_wgpu(), which is much cleaner


#[repr(C)]
#[derive(Clone)]
pub struct BufferCopyViewC {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it help to slap repr(C) on the relevant wgt structs?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can do that for now but we will eventually need to make a separate version here to match the WebGPU headers

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, so maybe we should try too hard to re-use it, I agree!

src/lib.rs Outdated
fn ref_cow(&self) -> Option<Cow<str>> {
self.0.as_ref().map(|s| Cow::Borrowed(s.as_str()))
}
fn owned_cow<'a>(self) -> Option<Cow<'a, str>> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be named into_cow

src/lib.rs Outdated
@@ -23,8 +26,11 @@ impl OwnedLabel {
)
})
}
fn as_ref(&self) -> Option<&str> {
self.0.as_ref().map(|s| s.as_str())
fn ref_cow(&self) -> Option<Cow<str>> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can call it as_cow

@ncharlton02 ncharlton02 marked this pull request as ready for review October 20, 2020 19:18
@ncharlton02
Copy link
Collaborator Author

Ok all of those recommendations should be fixed!

Copy link
Member

@kvark kvark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bors r+

@bors
Copy link
Contributor

bors bot commented Oct 21, 2020

@bors bors bot merged commit f8b8615 into gfx-rs:master Oct 21, 2020
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.

2 participants