Degenerification part 1/3: wgpu-hal DynDevice & friends#6098
Degenerification part 1/3: wgpu-hal DynDevice & friends#6098Wumpf wants to merge 43 commits intogfx-rs:trunkfrom
wgpu-hal DynDevice & friends#6098Conversation
7bfbdc4 to
eba1072
Compare
|
I'll commit to this first part in the next couple of days. 😀 |
cwfitzgerald
left a comment
There was a problem hiding this comment.
I don't see anything that should block this
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
To be clear: I also see nothing blocking so far. 👏🏻 |
eba1072 to
1389902
Compare
…s buffer operations
…PassTimestampWrites usable with DynQuerySet
collateral: ComputePass uses DynCommandEncoder during recording
bindgroup fixup
1389902 to
75aaa13
Compare
|
merged in part 3 |
ErichDonGubler
left a comment
There was a problem hiding this comment.
LGTM; really the only issue I noticed is that we don't have any docs for most of this. Buuut 🤷🏻♂️ I think it's better to defer to an issue, rather than block here and keep merge conflicts between this and probably most other PRs in total limbo.
wgpu-hal/src/dynamic/command.rs
Outdated
| pub trait DynCommandEncoder { | ||
| unsafe fn set_index_buffer<'a>( | ||
| &mut self, | ||
| binding: BufferBinding<'a, dyn DynBuffer>, | ||
| format: wgt::IndexFormat, | ||
| ); | ||
|
|
||
| unsafe fn set_vertex_buffer<'a>( | ||
| &mut self, | ||
| index: u32, | ||
| binding: BufferBinding<'a, dyn DynBuffer>, | ||
| ); | ||
| } | ||
|
|
||
| impl<C: CommandEncoder> DynCommandEncoder for C { | ||
| unsafe fn set_index_buffer<'a>( | ||
| &mut self, | ||
| binding: BufferBinding<'a, dyn DynBuffer>, | ||
| format: wgt::IndexFormat, | ||
| ) { | ||
| let binding = binding.expect_downcast(); | ||
| unsafe { self.set_index_buffer(binding, format) }; | ||
| } | ||
|
|
||
| unsafe fn set_vertex_buffer<'a>( | ||
| &mut self, | ||
| index: u32, | ||
| binding: BufferBinding<'a, dyn DynBuffer>, | ||
| ) { | ||
| let binding = binding.expect_downcast(); | ||
| unsafe { self.set_vertex_buffer(index, binding) }; | ||
| } | ||
| } |
There was a problem hiding this comment.
issue: We don't document a safety contract or adherence thereto. We should do this.
| } | ||
| } | ||
|
|
||
| unsafe fn set_index_buffer<'a>( |
There was a problem hiding this comment.
issue: We don't document a safety contract or adherence thereto for these methods. We should do this.
wgpu-hal/src/dynamic/command.rs
Outdated
| use super::DynBuffer; | ||
| use super::{DynBuffer, DynResourceExt as _}; | ||
|
|
||
| pub trait DynCommandEncoder: std::fmt::Debug { |
There was a problem hiding this comment.
issue: We don't document a safety contract or adherence thereto for these methods. We should do this.
wgpu-hal/src/dynamic/device.rs
Outdated
| pub trait DynDevice { | ||
| unsafe fn destroy_buffer(&self, buffer: Box<dyn DynBuffer>); | ||
| } | ||
|
|
||
| impl<D: Device> DynDevice for D { | ||
| unsafe fn destroy_buffer(&self, buffer: Box<dyn DynBuffer>) { | ||
| unsafe { D::destroy_buffer(self, buffer.unbox()) }; | ||
| } | ||
| } |
There was a problem hiding this comment.
issue: We don't document a safety contract or adherence thereto for these methods. We should do this.
| pub suboptimal: bool, | ||
| } | ||
|
|
||
| pub trait DynSurface: DynResource { |
There was a problem hiding this comment.
issue: We don't document a safety contract or adherence thereto for these methods. We should do this.
|
|
||
| use super::DynResourceExt as _; | ||
|
|
||
| pub trait DynQueue: DynResource { |
There was a problem hiding this comment.
issue: We don't document a safety contract or adherence thereto for these methods. We should do this.
| // Box casts are needed, alternative would be a temporaries which are more verbose and not more expressive. | ||
| #![allow(trivial_casts)] | ||
|
|
There was a problem hiding this comment.
suggestion: This is actually not necessary; the offending lines of code can be fixed thusly:
- .map(|b| Box::new(b) as Box<dyn DynCommandEncoder>)
+ .map(|b| -> Box<dyn DynCommandEncoder> { Box::new(b) })Will push a fix myself anon.
| } | ||
|
|
||
| pub trait DynInstance: DynResource { | ||
| unsafe fn create_surface( |
There was a problem hiding this comment.
issue: We don't document a safety contract or adherence thereto for these methods. We should do this.
Connections
wgpu-halDynDevice& friends #6098wgpu-core#6099gfx_select, remove compute/render pass indirections #6100Description
Introduces a new
Dyn"backend" which uses dynamic dispatch &std::any::Anybased casting while keeping changes towgpu-coreas minimal as possible.Note that casts through
Anycould be avoided altogether, but right now part2 of the series relies on standard any casts, so I'd advocate to keep this as a future improvement.Internal unboxing mechanism as proposed by @cwfitzgerald however sidesteps
Anyfor the most part (except for debug assertion) which allows us to keepwgpu-halbackend unchanged for the most part which profits their isolation & direct consumers ofwgpu-hal. Concrete, this means thatdestroymethods continue to consume objects via move (which is both semantically & syntactically convenient).Also note that this is intentionally not exposed as part of the public interface.
Speaking of future improvements:
Something that has been discussed in the maintainer group previously while this was already under way is to use
enumbasedDynResourcewhich would allow to trivially know the size of each resource and instead dispatch viamatch.This PR series sticks with
BoxandAnyin favor of expedience but is my belief that such refactors will be easier once this lands!Commits have been cleaned up as good as possible to make them digestable for review, but there might be still some slight meandering in there hinting at earlier experimentations.
Testing
Checklist
cargo fmt.cargo clippy. If applicable, add:--target wasm32-unknown-unknown--target wasm32-unknown-emscriptencargo xtask testto run tests.[ ] Add change tochangelog in Part 3CHANGELOG.md. See simple instructions inside file.