Skip to content

Update API according to the upstream spec #285

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Aug 17, 2019
Merged

Update API according to the upstream spec #285

merged 2 commits into from
Aug 17, 2019

Conversation

kvark
Copy link
Member

@kvark kvark commented Aug 15, 2019

Includes changes in texture view creation, enumeration mapping,
binding types, and more.
This would be a nice target for our 0.3 release.

@kvark kvark requested a review from grovesNL August 15, 2019 03:11
This was referenced Aug 15, 2019
Merged
Closed
grovesNL
grovesNL approved these changes Aug 16, 2019
View reviewed changes
Copy link
Collaborator

@grovesNL grovesNL left a comment

Choose a reason for hiding this comment

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

Some other notes skimming through the recent spec history:

wgpu-native/src/device.rs

#[cfg(feature = "local")]
#[no_mangle]
pub extern "C" fn wgpu_texture_create_default_view(texture_id: TextureId) -> TextureViewId {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: slightly prefer to keep both to avoid intentionally passing null in libraries using the header, and the current proposal has both

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm basing this off gpuweb/gpuweb#389, which is a tiny bit ahead

wgpu-native/src/instance.rs Outdated
@@ -256,7 +256,7 @@ pub fn instance_get_adapter(

#[cfg(feature = "local")]
#[no_mangle]
pub extern "C" fn wgpu_instance_get_adapter(
pub extern "C" fn wgpu_instance_request_adapter(
instance_id: InstanceId,
desc: &AdapterDescriptor,
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: should probably be Option<&RequestAdapterOptions>

wgpu-native/src/pipeline.rs Outdated
@@ -284,6 +326,8 @@ pub struct RenderPipelineDescriptor {
pub depth_stencil_state: *const DepthStencilStateDescriptor,
pub vertex_input: VertexInputDescriptor,
pub sample_count: u32,
pub sample_mask: u32,
pub alpha_coverage_enabled: bool,
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: alpha_to_coverage_enabled

@kvark
Update API according to the upstream spec
37afa0d 
Includes changes in texture view creation, enumeration mapping,
binding types, and more.
@kvark
Copy link
Member Author

kvark commented Aug 16, 2019

@grovesNL wow, what a great review. Thank you!

  • copyImageBitmapToTexture - I'm hesitating to add this, since it's JS-specific. We can add it later, not a breaking change.
  • LoadOp - I did change, but it's hard to express in C API, so I figured what we have now is the closest thing to C we can get.

Everything else is going to be addressed shortly.

@kvark
Copy link
Member Author

kvark commented Aug 16, 2019 •
edited
Loading

I addressed the comments but now failing to run cbindgen (again):

rustup run nightly cbindgen -o ffi/wgpu.h wgpu-native
ERROR: Parsing crate wgpu-native: couldn't run cargo rustc --pretty=expanded: Compile("error: An unknown error occurred\n\nTo learn more, run the command again with --verbose.\n")
ERROR: Couldn't generate bindings for wgpu-native.

Eh, gotta make it spew out the errors properly some day

Edit: magically resolved with cargo clean... :/

@kvark kvark added this to the Version 0.3 milestone Aug 16, 2019
@kvark
Copy link
Member Author

kvark commented Aug 17, 2019

Also found this to be relevant - servo/media#280 (comment)
Will modify cbindgen to spew better errors...
In the meantime, this should be ready 🎉
bors r=grovesNL

bors bot added a commit that referenced this pull request Aug 17, 2019
@bors @kvark
Merge #285
38743c6 
285: Update API according to the upstream spec r=grovesNL a=kvark

Includes changes in texture view creation, enumeration mapping,
binding types, and more.
This would be a nice target for our 0.3 release.

Co-authored-by: Dzmitry Malyshau <kvarkus@gmail.com>
@bors
Copy link
Contributor

bors bot commented Aug 17, 2019

Build succeeded

@bors bors bot merged commit f82ceba into gfx-rs:master Aug 17, 2019
@kvark kvark deleted the spec-update branch August 17, 2019 04:16
@rukai rukai mentioned this pull request Aug 17, 2019
Merged
bors bot added a commit that referenced this pull request Aug 18, 2019
@bors @rukai
Merge #291
a47ff09 
291: Fix sample_mask field r=kvark a=rukai

Looks like it was just missed in #285

Co-authored-by: Rukai <rubickent@gmail.com>
kvark added a commit to kvark/wgpu that referenced this pull request Jun 3, 2021
@bors @kvark
[rs] Merge gfx-rs#291
3586f12 
291: Update for wgpu-core r=grovesNL a=kvark

Depends on gfx-rs#619
Closes gfx-rs#285

Co-authored-by: Dzmitry Malyshau <kvarkus@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@grovesNL grovesNL grovesNL approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Version 0.3
Development

Successfully merging this pull request may close these issues.
2 participants
@kvark @grovesNL