Skip to content

Commit

Permalink
Make PersistentGpuBufferable a safe trait (#12744)
Browse files Browse the repository at this point in the history
# Objective
Fixes #12727. All parts that `PersistentGpuBuffer` interact with should
be 100% safe both on the CPU and the GPU: `Queue::write_buffer_with`
zeroes out the slice being written to and when uploading to the GPU, and
all slice writes are bounds checked on the CPU side.

## Solution
Make `PersistentGpuBufferable` a safe trait. Enforce it's correct
implementation via assertions. Re-enable `forbid(unsafe_code)` on
`bevy_pbr`.
  • Loading branch information
james7132 authored Mar 29, 2024
1 parent afff818 commit e62a01f
Show file tree
Hide file tree
Showing 4 changed files with 15 additions and 17 deletions.
1 change: 1 addition & 0 deletions crates/bevy_pbr/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// FIXME(3492): remove once docs are ready
#![allow(missing_docs)]
#![cfg_attr(docsrs, feature(doc_auto_cfg))]
#![forbid(unsafe_code)]
#![doc(
html_logo_url = "https://bevyengine.org/assets/icon.png",
html_favicon_url = "https://bevyengine.org/assets/icon.png"
Expand Down
18 changes: 9 additions & 9 deletions crates/bevy_pbr/src/meshlet/persistent_buffer.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,7 @@
#![allow(unsafe_code)]

use bevy_render::{
render_resource::{
BindingResource, Buffer, BufferAddress, BufferDescriptor, BufferUsages,
CommandEncoderDescriptor,
CommandEncoderDescriptor, COPY_BUFFER_ALIGNMENT,
},
renderer::{RenderDevice, RenderQueue},
};
Expand Down Expand Up @@ -41,6 +39,7 @@ impl<T: PersistentGpuBufferable> PersistentGpuBuffer<T> {
/// Queue an item of type T to be added to the buffer, returning the byte range within the buffer that it will be located at.
pub fn queue_write(&mut self, data: T, metadata: T::Metadata) -> Range<BufferAddress> {
let data_size = data.size_in_bytes() as u64;
debug_assert!(data_size % COPY_BUFFER_ALIGNMENT == 0);
if let Ok(buffer_slice) = self.allocation_planner.allocate_range(data_size) {
self.write_queue
.push((data, metadata, buffer_slice.clone()));
Expand Down Expand Up @@ -110,17 +109,18 @@ impl<T: PersistentGpuBufferable> PersistentGpuBuffer<T> {
}

/// A trait representing data that can be written to a [`PersistentGpuBuffer`].
///
/// # Safety
/// * All data must be a multiple of `wgpu::COPY_BUFFER_ALIGNMENT` bytes.
/// * The amount of bytes written to `buffer` in `write_bytes_le()` must match `size_in_bytes()`.
pub unsafe trait PersistentGpuBufferable {
pub trait PersistentGpuBufferable {
/// Additional metadata associated with each item, made available during `write_bytes_le`.
type Metadata;

/// The size in bytes of `self`.
/// The size in bytes of `self`. This will determine the size of the buffer passed into
/// `write_bytes_le`.
///
/// All data written must be in a multiple of `wgpu::COPY_BUFFER_ALIGNMENT` bytes. Failure to do so will
/// result in a panic when using [`PersistentGpuBuffer`].
fn size_in_bytes(&self) -> usize;

/// Convert `self` + `metadata` into bytes (little-endian), and write to the provided buffer slice.
/// Any bytes not written to in the slice will be zeroed out when uploaded to the GPU.
fn write_bytes_le(&self, metadata: Self::Metadata, buffer_slice: &mut [u8]);
}
11 changes: 4 additions & 7 deletions crates/bevy_pbr/src/meshlet/persistent_buffer_impls.rs
Original file line number Diff line number Diff line change
@@ -1,12 +1,9 @@
#![allow(unsafe_code)]
#![allow(clippy::undocumented_unsafe_blocks)]

use super::{persistent_buffer::PersistentGpuBufferable, Meshlet, MeshletBoundingSphere};
use std::{mem::size_of, sync::Arc};

const MESHLET_VERTEX_SIZE_IN_BYTES: u32 = 48;

unsafe impl PersistentGpuBufferable for Arc<[u8]> {
impl PersistentGpuBufferable for Arc<[u8]> {
type Metadata = ();

fn size_in_bytes(&self) -> usize {
Expand All @@ -18,7 +15,7 @@ unsafe impl PersistentGpuBufferable for Arc<[u8]> {
}
}

unsafe impl PersistentGpuBufferable for Arc<[u32]> {
impl PersistentGpuBufferable for Arc<[u32]> {
type Metadata = u64;

fn size_in_bytes(&self) -> usize {
Expand All @@ -37,7 +34,7 @@ unsafe impl PersistentGpuBufferable for Arc<[u32]> {
}
}

unsafe impl PersistentGpuBufferable for Arc<[Meshlet]> {
impl PersistentGpuBufferable for Arc<[Meshlet]> {
type Metadata = (u64, u64);

fn size_in_bytes(&self) -> usize {
Expand Down Expand Up @@ -65,7 +62,7 @@ unsafe impl PersistentGpuBufferable for Arc<[Meshlet]> {
}
}

unsafe impl PersistentGpuBufferable for Arc<[MeshletBoundingSphere]> {
impl PersistentGpuBufferable for Arc<[MeshletBoundingSphere]> {
type Metadata = ();

fn size_in_bytes(&self) -> usize {
Expand Down
2 changes: 1 addition & 1 deletion crates/bevy_render/src/render_resource/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ pub use wgpu::{
TextureDescriptor, TextureDimension, TextureFormat, TextureSampleType, TextureUsages,
TextureViewDescriptor, TextureViewDimension, VertexAttribute,
VertexBufferLayout as RawVertexBufferLayout, VertexFormat, VertexState as RawVertexState,
VertexStepMode,
VertexStepMode, COPY_BUFFER_ALIGNMENT,
};

pub mod encase {
Expand Down

0 comments on commit e62a01f

Please sign in to comment.