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

Remove panics in Deref impl of QueueWriteBufferView and BufferViewMut #3336

Merged
merged 13 commits into from
Jan 12, 2023
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,7 @@ Additionally `Surface::get_default_config` now returns an Option and returns Non
- Add missing `DEPTH_BIAS_CLAMP` and `FULL_DRAW_INDEX_UINT32` downlevel flags. By @teoxoy in [#3316](https://github.com/gfx-rs/wgpu/pull/3316)
- Make `ObjectId` structure and invariants idiomatic. By @teoxoy in [#3347](https://github.com/gfx-rs/wgpu/pull/3347)
- Add validation in accordance with WebGPU `GPUSamplerDescriptor` valid usage for `lodMinClamp` and `lodMaxClamp`. By @James2022-rgb in [#3353](https://github.com/gfx-rs/wgpu/pull/3353)
- Remove panics in `Deref` implementations for `QueueWriteBufferView` and `BufferViewMut`. Instead, warnings are logged, since reading from these types is not recommended. By @botahamec in [#3336]

#### WebGPU

Expand Down
63 changes: 35 additions & 28 deletions wgpu/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ use std::{
future::Future,
marker::PhantomData,
num::{NonZeroU32, NonZeroU8},
ops::{Bound, Range, RangeBounds},
ops::{Bound, Deref, DerefMut, Range, RangeBounds},
sync::Arc,
thread,
};
Expand Down Expand Up @@ -2297,6 +2297,9 @@ pub struct BufferView<'a> {
}

/// Write only view into mapped buffer.
///
/// It is possible to read the buffer using this view, but doing so is not
/// recommended, as it is likely to be slow.
#[derive(Debug)]
pub struct BufferViewMut<'a> {
slice: BufferSlice<'a>,
Expand All @@ -2313,37 +2316,34 @@ impl std::ops::Deref for BufferView<'_> {
}
}

impl std::ops::Deref for BufferViewMut<'_> {
type Target = [u8];

impl AsRef<[u8]> for BufferView<'_> {
#[inline]
fn deref(&self) -> &[u8] {
assert!(
self.readable,
"Attempting to read a write-only mapping for buffer {:?}",
self.slice.buffer.id
);
fn as_ref(&self) -> &[u8] {
self.data.slice()
}
}

impl std::ops::DerefMut for BufferViewMut<'_> {
impl AsMut<[u8]> for BufferViewMut<'_> {
#[inline]
fn deref_mut(&mut self) -> &mut Self::Target {
fn as_mut(&mut self) -> &mut [u8] {
self.data.slice_mut()
}
}

impl AsRef<[u8]> for BufferView<'_> {
#[inline]
fn as_ref(&self) -> &[u8] {
impl Deref for BufferViewMut<'_> {
type Target = [u8];

fn deref(&self) -> &Self::Target {
if !self.readable {
log::warn!("Reading from a BufferViewMut is slow and not recommended.");
}

self.data.slice()
}
}

impl AsMut<[u8]> for BufferViewMut<'_> {
#[inline]
fn as_mut(&mut self) -> &mut [u8] {
impl DerefMut for BufferViewMut<'_> {
fn deref_mut(&mut self) -> &mut Self::Target {
self.data.slice_mut()
}
}
Expand Down Expand Up @@ -3768,7 +3768,11 @@ impl<'a> RenderBundleEncoder<'a> {
}
}

/// A write-only view into a staging buffer
/// A read-only view into a staging buffer.
///
/// Reading into this buffer won't yield the contents of the buffer from the
/// GPU and is likely to be slow. Because of this, although [`AsMut`] is
/// implemented for this type, [`AsRef`] is not.
pub struct QueueWriteBufferView<'a> {
queue: &'a Queue,
buffer: &'a Buffer,
Expand All @@ -3777,21 +3781,27 @@ pub struct QueueWriteBufferView<'a> {
}
static_assertions::assert_impl_all!(QueueWriteBufferView: Send, Sync);

impl<'a> std::ops::Deref for QueueWriteBufferView<'a> {
impl Deref for QueueWriteBufferView<'_> {
type Target = [u8];

fn deref(&self) -> &Self::Target {
panic!("QueueWriteBufferView is write-only!");
log::warn!("Reading from a QueueWriteBufferView won't yield the contents of the buffer and may be slow.");
self.inner.slice()
}
}

impl<'a> std::ops::DerefMut for QueueWriteBufferView<'a> {
#[inline]
impl DerefMut for QueueWriteBufferView<'_> {
fn deref_mut(&mut self) -> &mut Self::Target {
self.inner.slice_mut()
}
}

impl<'a> AsMut<[u8]> for QueueWriteBufferView<'a> {
fn as_mut(&mut self) -> &mut [u8] {
self.inner.slice_mut()
}
}

impl<'a> Drop for QueueWriteBufferView<'a> {
fn drop(&mut self) {
DynContext::queue_write_staging_buffer(
Expand Down Expand Up @@ -3827,12 +3837,9 @@ impl Queue {
}

/// Schedule a data write into `buffer` starting at `offset` via the returned
/// [QueueWriteBufferView].
/// [`QueueWriteBufferView`].
///
/// The returned value can be dereferenced to a `&mut [u8]`; dereferencing it to a
/// `&[u8]` panics!
/// (It is not unsound to read through the `&mut [u8]` anyway, but doing so will not
/// yield the existing contents of `buffer` from the GPU, and it is likely to be slow.)
/// Reading from this buffer is slow and will not yield the actual contents of the buffer.
///
/// This method is intended to have low performance costs.
/// As such, the write is not immediately submitted, and instead enqueued
Expand Down