-
Notifications
You must be signed in to change notification settings - Fork 965
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
Rework wgpu-rs Context #6619
Rework wgpu-rs Context #6619
Conversation
wgpu/src/dispatch.rs
Outdated
#[cfg(wgpu_core)] | ||
#[inline] | ||
#[allow(unused)] | ||
pub fn as_core(&self) -> &$core { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Result
would be nicer imho
depends a bit on the usecase tbf
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll get there later, no idea what the current behavior is if you mix context types, at least now it'll panic in a nice well defined place.
wgpu/src/dispatch.rs
Outdated
pub enum $name { | ||
#[cfg(wgpu_core)] | ||
Core($core), | ||
#[cfg(webgpu)] | ||
WebGPU($webgpu), | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So for #6330 we would need to add Custom(Arc<dyn $trait>)
variant, but we would probably want to put it behind a cfg.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's the idea.
5216d7d
to
0e53174
Compare
[[test]] | ||
name = "wgpu-compile-test" | ||
path = "compile_tests/root.rs" | ||
harness = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lookie: Our test suite has grown a new feature. We can now ensure that a given bit of wgpu code does not compile. This is very useful to make sure that #6145 doesn't come back.
crate::cmp::impl_eq_ord_hash_arc_address!(ContextWgpuCore => .0); | ||
crate::cmp::impl_eq_ord_hash_proxy!(CoreAdapter => .id); | ||
crate::cmp::impl_eq_ord_hash_proxy!(CoreDevice => .id); | ||
crate::cmp::impl_eq_ord_hash_proxy!(CoreQueue => .id); | ||
crate::cmp::impl_eq_ord_hash_proxy!(CoreShaderModule => .id); | ||
crate::cmp::impl_eq_ord_hash_proxy!(CoreBindGroupLayout => .id); | ||
crate::cmp::impl_eq_ord_hash_proxy!(CoreBindGroup => .id); | ||
crate::cmp::impl_eq_ord_hash_proxy!(CoreTextureView => .id); | ||
crate::cmp::impl_eq_ord_hash_proxy!(CoreSampler => .id); | ||
crate::cmp::impl_eq_ord_hash_proxy!(CoreBuffer => .id); | ||
crate::cmp::impl_eq_ord_hash_proxy!(CoreTexture => .id); | ||
crate::cmp::impl_eq_ord_hash_proxy!(CoreBlas => .id); | ||
crate::cmp::impl_eq_ord_hash_proxy!(CoreTlas => .id); | ||
crate::cmp::impl_eq_ord_hash_proxy!(CoreQuerySet => .id); | ||
crate::cmp::impl_eq_ord_hash_proxy!(CorePipelineLayout => .id); | ||
crate::cmp::impl_eq_ord_hash_proxy!(CoreRenderPipeline => .id); | ||
crate::cmp::impl_eq_ord_hash_proxy!(CoreComputePipeline => .id); | ||
crate::cmp::impl_eq_ord_hash_proxy!(CorePipelineCache => .id); | ||
crate::cmp::impl_eq_ord_hash_proxy!(CoreCommandEncoder => .id); | ||
crate::cmp::impl_eq_ord_hash_proxy!(CoreComputePass => .id); | ||
crate::cmp::impl_eq_ord_hash_proxy!(CoreRenderPass => .id); | ||
crate::cmp::impl_eq_ord_hash_proxy!(CoreCommandBuffer => .id); | ||
crate::cmp::impl_eq_ord_hash_proxy!(CoreRenderBundleEncoder => .id); | ||
crate::cmp::impl_eq_ord_hash_proxy!(CoreRenderBundle => .id); | ||
crate::cmp::impl_eq_ord_hash_proxy!(CoreSurface => .id); | ||
crate::cmp::impl_eq_ord_hash_proxy!(CoreSurfaceOutputDetail => .surface_id); | ||
crate::cmp::impl_eq_ord_hash_proxy!(CoreQueueWriteBuffer => .mapping.ptr); | ||
crate::cmp::impl_eq_ord_hash_proxy!(CoreBufferMappedRange => .ptr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lookie: Now the backend types are responsible for implementing PartialEq etc how they see fit. Most of them use this proxy type, which just uses a single member for the partial eq behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once we get rid of the registries in wgpu-core this will be easy to adapt, just use impl_eq_ord_hash_arc_address
. Nice design!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lookie! This file is very important
c787967
to
55c839b
Compare
impl Identifier { | ||
pub fn create() -> Self { | ||
let id = NEXT_ID.fetch_add(1, Ordering::Relaxed); | ||
// Safety: Will take 7000+ years of constant incrementing to overflow. It's fine. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall a really nice design! Much more straightforward than what we had before.
I tried to review it as best as I could, I didn't review code movement though since that would have been a nightmare.
There is something I was wondering about: Since the public pub struct X {
pub(crate) inner: dispatch::DispatchX,
} Wouldn't it be possible to simplify things further and use the |
Connections
Closes #6324
Closes #6145
Description
Here it is. This reworks how the
wgpu
context works so that there is:Drop
code into the backends as opposed to being in the context.Testing
Tested manually on WebGPU and native tests. Covered through CI. Added compile tests to make sure #6145 doesn't come back.
Checklist
cargo fmt
.taplo format
.cargo clippy
. If applicable, add:--target wasm32-unknown-unknown
--target wasm32-unknown-emscripten
cargo xtask test
to run tests.CHANGELOG.md
. See simple instructions inside file.