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 library to latest webgpu-native headers #427

Open
wants to merge 19 commits into
base: trunk
Choose a base branch
from

Conversation

PJB3005
Copy link
Contributor

@PJB3005 PJB3005 commented Sep 19, 2024

This goes up to webgpu-native/webgpu-headers@6f549cc
Things I didn't do:

  • I didn't update the library to make sure "instance dropped" callback error codes are guaranteed to happen, like they seem to be in Dawn.

List of changes (roughly in order of header commits):

  • Various enum and struct renames
  • Updated callbacks to use the new *CallbackInfo structs and 2-userdata system. Also updated functions to return WGPUFuture, though the WGPUFuture thing is just stubbed out at the moment as I don't think wgpu-core has the necessary functionality for it. wgpuInstanceWaitAny is unimplemented!()
  • DepthClipControl merged into PrimitiveState, related code simplified.
  • Updated depthWriteEnabled to use an optional bool, mostly matters due to added validation.
  • Add TODOs for missing features (sliced 3D compressed textures)
  • *Reference() became *AddRef()
  • Added unorm10-10-10-2 vertex format
  • Usage field in TextureViewDescriptor, just used for validation as wgpu-core doesn't allow specifying it anyways.
  • Removed maxInterStageShaderComponents
  • Added clang_macro_fallback() to bindgen config, since the headers switched to using UINT32_MAX etc. UINT64_MAX still doesn't work so I had to manually define those.
  • Renamed flags enums. Added a conversion helper function to convert them from u64 -> u32 for mapping. (means added direct dependency on bitflags crate)
  • Removed device argument from (unimplemented) wgpuGetProcAddress
  • Suboptimal surface texture acquisition moved to enum return value, was easy since wgpu-core already returns it like that.
  • "Undefined" present mode added, it just selects FIFO.
  • *EnumerateFeatures() has been replaced with *GetFeatures().
  • All strings are now passed as WGPUStringView.

This goes up to webgpu-native/webgpu-headers@2b59747

Things I *didn't* do:

* I didn't update the library to make sure "instance dropped" callback
  error codes are guaranteed to happen, like they seem to be in Dawn.

List of changes (roughly in order of header commits):

Various enum and struct renames

Updated callbacks to use the new *CallbackInfo structs and 2-userdata system. Also updated functions to return WGPUFuture, though the WGPUFuture thing is just stubbed out at the moment as I don't think wgpu-core has the necessary functionality for it. wgpuInstanceWaitAny is unimplemented!()

DepthClipControl merged into PrimitiveState, related code simplified.

Updated depthWriteEnabled to use an optional bool, mostly matters due to added validation.

Add TODOs for missing features (sliced 3D compressed textures)

*Reference() became *AddRef()

Added unorm10-10-10-2 vertex format

Usage field in TextureViewDescriptor, just used for validation as wgpu-core doesn't allow specifying it anyways.

Removed maxInterStageShaderComponents

Added clang_macro_fallback to bindgen config, since the headers switched to using UINT32_MAX etc. UINT64_MAX still doesn't work so I had to manually define those.

Renamed flags enums. Added a conversion helper function to convert them from u64 -> u32 for mapping. (means added direct dependency on bitflags crate)

Removed device argument from (unimplemented) wgpuGetProcAddress

Suboptimal surface texture acquisition moved to enum return value, was easy since wgpu-core already returns it like that.

"Undefined" present mode added, it just selects FIFO.
This enum was replaced with WGPUMapAsyncStatus, but I didn't quite notice. The error codes map different.
@PJB3005
Copy link
Contributor Author

PJB3005 commented Sep 19, 2024

Remaining Windows CI failure is caused by webgpu-native/webgpu-headers#339

Replaces *EnumerateFeatures with *GetFeatures.

Also fixes CI due to fix in headers.
Copy link
Collaborator

@almarklein almarklein left a comment

Choose a reason for hiding this comment

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

LGTM but maybe @rajveermalviya can have a look at the Rust code.

Upstream removed the "Flags" suffix from flags types and moved them to no longer be C enums. This matches that change. WGPUInstanceFlag still has "Flag" in the name because, well, there'd be nothing left to distinguish it from WGPUInstance, and it makes sense for it.
Copy link
Contributor

@eliemichel eliemichel left a comment

Choose a reason for hiding this comment

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

Nice job! I started looking at how to implement futures in wgpu a couple of days ago, nothing obvious but I'll share if I figure sth out!

src/conv.rs Outdated Show resolved Hide resolved
@@ -1701,3 +1694,11 @@ pub fn map_adapter_type(device_type: wgt::DeviceType) -> native::WGPUAdapterType
wgt::DeviceType::Cpu => native::WGPUAdapterType_CPU,
}
}

pub fn from_u64_bits<T: bitflags::Flags<Bits = u32>>(value: u64) -> Option<T> {
Copy link
Contributor

Choose a reason for hiding this comment

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

(Note: Ideally bitfields would switch to u64 in upstream wgpu instead of needing a conversion here)

Cargo.toml Outdated
@@ -157,6 +157,7 @@ log = "0.4"
thiserror = "1"
parking_lot = "0.12"
smallvec = "1"
bitflags = "2.6.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's to be able to refer to bitflags types for the from_u64_bits helper.

@eliemichel
Copy link
Contributor

eliemichel commented Oct 3, 2024

For WGPUFuture, we need to have access to submission index (and return that in the future's id). Here is the beginning of an attempt at adding this upstream: gfx-rs/wgpu#6360

_ => make_slice(string_view.data as *const u8, string_view.length),
};

Some(std::str::from_utf8_unchecked(bytes))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Important to note: this doesn't check whether strings passed into the library are valid UTF-8. I previously many locations did, but I figured this wasn't necessary.

@eliemichel
Copy link
Contributor

FYI I started trying to build upon this PR and upgrade wgpu to my PR that adds submission indices for futures, WIP is here: https://github.com/eliemichel/wgpu-native/tree/eliemichel/2024-10-10-upgrade-wgpu-futures

Ongoing work: I need to figure out what replaced the DynComputePass and DynRenderPass types in newest wgpu.

@ygdrasil-io
Copy link
Contributor

There is a missing fonction wgpuGetInstanceFeatures on the rust code but describe on yaml as :

  - name: get_instance_features
    doc: Query the supported instance features
    args:
      - name: features
        doc: The supported instance features
        type: struct.instance_features
        pointer: mutable

@eliemichel
Copy link
Contributor

FYI I started trying to build upon this PR and upgrade wgpu

This is now ready, PR: #441

All stubs, since we don't have WaitAny at the moment.
@PJB3005
Copy link
Contributor Author

PJB3005 commented Oct 19, 2024

@ygdrasil-io done, thanks for pointing that out. Though note it's not very useful since WaitAny isn't implemented yet.

Update to webgpu-native/webgpu-headers@f1cdc3f

Also went through a bunch of existing enum conversions and fixed up some seemingly spec-incorrect cases of undefined enums not being handled properly.

As part of this, I made a new helper map_enum_with_undefined!() which distinguishes undefined and unknown enum values. Previously, much code relied on undefined being caught in the same net as an unknown value. This is no longer the case.
This updates the headers to webgpu-native/webgpu-headers@af63d34

These changes are exclusively in the header enum values, so no Rust code needs changing. Making this a separate commit for easier review.
Update headers to webgpu-native/webgpu-headers@b7656d0

Adds dual source blending. Other two features (float32 blendable and clip distances feature in wgsl) are not supported by wgpu.

Also made map_blend_factor use the shorter macro form, as all the enum names match (they didn't 3 years ago when this code was originally written, according to Git history).
Oops that's not how these chained structs work.
Updates headers to webgpu-native/webgpu-headers@6f549cc

Also made matching changes to wgpu.h
@eliemichel
Copy link
Contributor

@rajveermalviya Any specific reason why this PR is on hold? (You not having had time to review it yet is a perfectly fine reason, just trying to figure out if there is a technical blocker I could help with!)

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.

4 participants