From 51a70ea219a896a5c1c49ac5f45d037e490997a0 Mon Sep 17 00:00:00 2001 From: vmarcella Date: Mon, 9 Feb 2026 12:06:51 -0800 Subject: [PATCH 1/3] [update] buffers to default to DEVICE_LOCAL as opposed to CPU_VISIBLE --- crates/lambda-rs/src/render/buffer.rs | 33 ++++++++++++++++++++++----- 1 file changed, 27 insertions(+), 6 deletions(-) diff --git a/crates/lambda-rs/src/render/buffer.rs b/crates/lambda-rs/src/render/buffer.rs index f4e80094..5f1d1164 100644 --- a/crates/lambda-rs/src/render/buffer.rs +++ b/crates/lambda-rs/src/render/buffer.rs @@ -81,8 +81,13 @@ impl Default for Usage { /// Buffer allocation properties that control residency and CPU visibility. #[derive(Clone, Copy, Debug)] /// -/// Use `CPU_VISIBLE` for frequently updated data (e.g., uniform uploads). -/// Prefer `DEVICE_LOCAL` for static geometry uploaded once. +/// Use `CPU_VISIBLE` for buffers you plan to update from the CPU using +/// `Buffer::write_*` (this enables `wgpu::Queue::write_buffer` by adding the +/// required `COPY_DST` usage). +/// +/// Prefer `DEVICE_LOCAL` for static geometry uploaded once and never modified. +/// This is typically the best default for vertex and index buffers on discrete +/// GPUs, where CPU-visible memory may live in system RAM rather than VRAM. pub struct Properties { cpu_visible: bool, } @@ -101,7 +106,7 @@ impl Properties { impl Default for Properties { fn default() -> Self { - Properties::CPU_VISIBLE + Properties::DEVICE_LOCAL } } @@ -287,7 +292,7 @@ impl UniformBuffer { /// let vertices: Vec = build_vertices(); /// let vb = BufferBuilder::new() /// .with_usage(Usage::VERTEX) -/// .with_properties(Properties::DEVICE_LOCAL) +/// // Defaults to `Properties::DEVICE_LOCAL` (recommended for static geometry). /// .with_buffer_type(BufferType::Vertex) /// .build(render_context, vertices) /// .unwrap(); @@ -308,11 +313,16 @@ impl Default for BufferBuilder { impl BufferBuilder { /// Creates a new buffer builder of type vertex. + /// + /// Defaults: + /// - `usage`: `Usage::VERTEX` + /// - `properties`: `Properties::DEVICE_LOCAL` + /// - `buffer_type`: `BufferType::Vertex` pub fn new() -> Self { Self { buffer_length: 0, usage: Usage::VERTEX, - properties: Properties::CPU_VISIBLE, + properties: Properties::default(), buffer_type: BufferType::Vertex, label: None, } @@ -396,7 +406,7 @@ impl BufferBuilder { return builder .with_length(std::mem::size_of_val(mesh.vertices())) .with_usage(Usage::VERTEX) - .with_properties(Properties::CPU_VISIBLE) + .with_properties(Properties::DEVICE_LOCAL) .with_buffer_type(BufferType::Vertex) .build(gpu, mesh.vertices().to_vec()); } @@ -426,6 +436,17 @@ impl BufferBuilder { mod tests { use super::*; + #[test] + fn properties_default_is_device_local() { + assert!(!Properties::default().cpu_visible()); + } + + #[test] + fn buffer_builder_defaults_to_device_local_properties() { + let builder = BufferBuilder::new(); + assert!(!builder.properties.cpu_visible()); + } + #[test] fn resolve_length_rejects_zero() { let builder = BufferBuilder::new(); From f9c735722a58dfb9090cb16509fe5ca5d78b2f38 Mon Sep 17 00:00:00 2001 From: vmarcella Date: Mon, 9 Feb 2026 12:39:07 -0800 Subject: [PATCH 2/3] [update] writes to return results. --- crates/lambda-rs/src/render/buffer.rs | 100 +++++++++++++++++++++++++- 1 file changed, 98 insertions(+), 2 deletions(-) diff --git a/crates/lambda-rs/src/render/buffer.rs b/crates/lambda-rs/src/render/buffer.rs index 5f1d1164..db5aa20a 100644 --- a/crates/lambda-rs/src/render/buffer.rs +++ b/crates/lambda-rs/src/render/buffer.rs @@ -119,11 +119,15 @@ impl Default for Properties { /// - Writing is performed via the device queue using `write_value` or by /// creating CPU‑visible buffers and re‑building with new contents when /// appropriate. +/// - `write_*` operations require the buffer to be created with +/// `Properties::CPU_VISIBLE`. Use `try_write_*` variants if you want to +/// handle this as an error rather than panicking. #[derive(Debug)] pub struct Buffer { buffer: Rc, stride: u64, buffer_type: BufferType, + cpu_visible: bool, } impl Buffer { @@ -144,12 +148,41 @@ impl Buffer { return self.buffer_type; } + /// Whether this buffer supports CPU-side queue writes (`write_*`). + pub fn cpu_visible(&self) -> bool { + return self.cpu_visible; + } + + fn validate_cpu_write(&self) -> Result<(), &'static str> { + return validate_cpu_write_supported(self.cpu_visible); + } + /// Write a single plain-old-data value into this buffer at the specified /// byte offset. This is intended for updating uniform buffer contents from /// the CPU. The `data` type must implement `PlainOldData`. + /// + /// # Panics + /// Panics if the buffer was not created with `Properties::CPU_VISIBLE`. pub fn write_value(&self, gpu: &Gpu, offset: u64, data: &T) { + self + .try_write_value(gpu, offset, data) + .expect("Buffer::write_value requires a CPU-visible buffer. Create the buffer with `.with_properties(Properties::CPU_VISIBLE)` or use `try_write_value` to handle the error."); + } + + /// Fallible variant of [`Buffer::write_value`]. + /// + /// Returns an error if the buffer was not created with + /// `Properties::CPU_VISIBLE`. + pub fn try_write_value( + &self, + gpu: &Gpu, + offset: u64, + data: &T, + ) -> Result<(), &'static str> { + self.validate_cpu_write()?; let bytes = value_as_bytes(data); - self.write_bytes(gpu, offset, bytes); + self.buffer.write_bytes(gpu.platform(), offset, bytes); + return Ok(()); } /// Write raw bytes into this buffer at the specified byte offset. @@ -162,8 +195,28 @@ impl Buffer { /// let raw_data: &[u8] = load_binary_data(); /// buffer.write_bytes(render_context.gpu(), 0, raw_data); /// ``` + /// + /// # Panics + /// Panics if the buffer was not created with `Properties::CPU_VISIBLE`. pub fn write_bytes(&self, gpu: &Gpu, offset: u64, data: &[u8]) { + self + .try_write_bytes(gpu, offset, data) + .expect("Buffer::write_bytes requires a CPU-visible buffer. Create the buffer with `.with_properties(Properties::CPU_VISIBLE)` or use `try_write_bytes` to handle the error."); + } + + /// Fallible variant of [`Buffer::write_bytes`]. + /// + /// Returns an error if the buffer was not created with + /// `Properties::CPU_VISIBLE`. + pub fn try_write_bytes( + &self, + gpu: &Gpu, + offset: u64, + data: &[u8], + ) -> Result<(), &'static str> { + self.validate_cpu_write()?; self.buffer.write_bytes(gpu.platform(), offset, data); + return Ok(()); } /// Write a slice of plain-old-data values into this buffer at the @@ -187,12 +240,22 @@ impl Buffer { offset: u64, data: &[T], ) -> Result<(), &'static str> { + self.validate_cpu_write()?; let bytes = slice_as_bytes(data)?; - self.write_bytes(gpu, offset, bytes); + self.buffer.write_bytes(gpu.platform(), offset, bytes); return Ok(()); } } +fn validate_cpu_write_supported(cpu_visible: bool) -> Result<(), &'static str> { + if !cpu_visible { + return Err( + "Buffer was not created with Properties::CPU_VISIBLE, so CPU writes are not supported. Recreate the buffer with `.with_properties(Properties::CPU_VISIBLE)`.", + ); + } + return Ok(()); +} + fn value_as_bytes(data: &T) -> &[u8] { let bytes = unsafe { std::slice::from_raw_parts( @@ -394,6 +457,7 @@ impl BufferBuilder { buffer: Rc::new(buffer), stride: element_size as u64, buffer_type: self.buffer_type, + cpu_visible: self.properties.cpu_visible(), }); } @@ -437,17 +501,38 @@ mod tests { use super::*; #[test] + // Ensures callers get a clear engine-level error instead of a wgpu + // validation panic when attempting CPU writes to a device-local buffer. + fn validate_cpu_write_supported_rejects_non_cpu_visible() { + let result = validate_cpu_write_supported(false); + assert!(result.is_err()); + } + + #[test] + // Verifies CPU-visible buffers are accepted for `write_*` operations. + fn validate_cpu_write_supported_accepts_cpu_visible() { + let result = validate_cpu_write_supported(true); + assert!(result.is_ok()); + } + + #[test] + // Confirms `Properties::default()` is now device-local to avoid placing + // static buffers in CPU-visible memory by accident. fn properties_default_is_device_local() { assert!(!Properties::default().cpu_visible()); } #[test] + // Confirms `BufferBuilder::new()` inherits the default properties so buffer + // residency matches `Properties::default()`. fn buffer_builder_defaults_to_device_local_properties() { let builder = BufferBuilder::new(); assert!(!builder.properties.cpu_visible()); } #[test] + // Validates that zero-length buffers are rejected even when size is inferred + // from the provided data. fn resolve_length_rejects_zero() { let builder = BufferBuilder::new(); let result = builder.resolve_length(std::mem::size_of::(), 0); @@ -455,6 +540,8 @@ mod tests { } #[test] + // Verifies `with_label` stores the label on the builder so it can be applied + // to the underlying platform buffer for debugging/profiling. fn label_is_recorded_on_builder() { let builder = BufferBuilder::new().with_label("buffer-test"); // Indirect check: validate the internal label is stored on the builder. @@ -463,6 +550,8 @@ mod tests { } #[test] + // Ensures buffer size math guards against integer overflow when resolving + // byte lengths from element size and element count. fn resolve_length_rejects_overflow() { let builder = BufferBuilder::new(); let result = builder.resolve_length(usize::MAX, 2); @@ -470,6 +559,8 @@ mod tests { } #[test] + // Confirms `value_as_bytes` produces the same byte representation as the + // native `to_ne_bytes` conversion for POD values. fn value_as_bytes_matches_native_bytes() { let value: u32 = 0x1122_3344; let expected = value.to_ne_bytes(); @@ -477,6 +568,8 @@ mod tests { } #[test] + // Confirms `slice_as_bytes` produces the same byte layout as concatenating + // each element's native-endian bytes in order. fn slice_as_bytes_matches_native_bytes() { let values: [u16; 3] = [0x1122, 0x3344, 0x5566]; let mut expected: Vec = Vec::new(); @@ -487,12 +580,15 @@ mod tests { } #[test] + // Ensures the empty slice case works and does not error or return junk data. fn slice_as_bytes_empty_is_empty() { let values: [u32; 0] = []; assert_eq!(slice_as_bytes(&values).unwrap(), &[]); } #[test] + // Ensures the shared byte-length helper rejects overflows rather than + // silently wrapping and producing undersized buffers/slices. fn checked_byte_len_rejects_overflow() { let result = checked_byte_len(usize::MAX, 2); assert!(result.is_err()); From 6071c9355975b2ac618e96d9accde87d75b20c45 Mon Sep 17 00:00:00 2001 From: vmarcella Date: Mon, 9 Feb 2026 13:47:12 -0800 Subject: [PATCH 3/3] [fix] test assertion. --- Cargo.lock | 4 +-- crates/lambda-rs/src/render/buffer.rs | 38 +++++++++------------------ 2 files changed, 14 insertions(+), 28 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 140bb568..2693f935 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2366,7 +2366,7 @@ checksum = "e7b05ce9599a62eec9f6b761cf32647ab1dee43477b5c00ff9221a4b7cb80c4b" dependencies = [ "approx", "arrayvec", - "bitflags 2.9.0", + "bitflags 2.10.0", "downcast-rs 2.0.2", "either", "ena", @@ -2651,7 +2651,7 @@ dependencies = [ "approx", "arrayvec", "bit-vec", - "bitflags 2.9.0", + "bitflags 2.10.0", "downcast-rs 2.0.2", "glamx", "log", diff --git a/crates/lambda-rs/src/render/buffer.rs b/crates/lambda-rs/src/render/buffer.rs index 6cdbd34c..b7dae9af 100644 --- a/crates/lambda-rs/src/render/buffer.rs +++ b/crates/lambda-rs/src/render/buffer.rs @@ -500,50 +500,44 @@ impl BufferBuilder { mod tests { use super::*; - /// Rejects constructing a buffer with a logical length of zero elements. + /// Rejects CPU-side writes for buffers not created with + /// `Properties::CPU_VISIBLE` (prevents wgpu validation panics). #[test] - // Ensures callers get a clear engine-level error instead of a wgpu - // validation panic when attempting CPU writes to a device-local buffer. fn validate_cpu_write_supported_rejects_non_cpu_visible() { let result = validate_cpu_write_supported(false); assert!(result.is_err()); } + /// Accepts CPU-side writes for buffers created with `Properties::CPU_VISIBLE`. #[test] - // Verifies CPU-visible buffers are accepted for `write_*` operations. fn validate_cpu_write_supported_accepts_cpu_visible() { let result = validate_cpu_write_supported(true); assert!(result.is_ok()); } + /// Confirms `Properties::default()` is `DEVICE_LOCAL` (not CPU-visible). #[test] - // Confirms `Properties::default()` is now device-local to avoid placing - // static buffers in CPU-visible memory by accident. fn properties_default_is_device_local() { assert!(!Properties::default().cpu_visible()); } + /// Confirms `BufferBuilder::new()` inherits the default properties. #[test] - // Confirms `BufferBuilder::new()` inherits the default properties so buffer - // residency matches `Properties::default()`. fn buffer_builder_defaults_to_device_local_properties() { let builder = BufferBuilder::new(); assert!(!builder.properties.cpu_visible()); } + /// Validates that size resolution rejects creating a zero-length buffer. #[test] - // Validates that zero-length buffers are rejected even when size is inferred - // from the provided data. fn resolve_length_rejects_zero() { let builder = BufferBuilder::new(); let result = builder.resolve_length(std::mem::size_of::(), 0); assert!(result.is_err()); } - /// Ensures builder labels are stored for later propagation/debugging. + /// Ensures `with_label` stores the label on the builder. #[test] - // Verifies `with_label` stores the label on the builder so it can be applied - // to the underlying platform buffer for debugging/profiling. fn label_is_recorded_on_builder() { let builder = BufferBuilder::new().with_label("buffer-test"); // Indirect check: validate the internal label is stored on the builder. @@ -551,30 +545,25 @@ mod tests { assert_eq!(builder.label.as_deref(), Some("buffer-test")); } - /// Rejects length computations that would overflow `usize`. + /// Rejects length computations that would overflow `usize` when converting + /// element counts/sizes to byte sizes. #[test] - // Ensures buffer size math guards against integer overflow when resolving - // byte lengths from element size and element count. fn resolve_length_rejects_overflow() { let builder = BufferBuilder::new(); let result = builder.resolve_length(usize::MAX, 2); assert!(result.is_err()); } - /// Confirms `value_as_bytes` uses native-endian byte order and size. + /// Confirms `value_as_bytes` matches the native byte representation. #[test] - // Confirms `value_as_bytes` produces the same byte representation as the - // native `to_ne_bytes` conversion for POD values. fn value_as_bytes_matches_native_bytes() { let value: u32 = 0x1122_3344; let expected = value.to_ne_bytes(); assert_eq!(value_as_bytes(&value), expected.as_slice()); } - /// Confirms `slice_as_bytes` flattens a typed slice to the native bytes. + /// Confirms `slice_as_bytes` matches the expected concatenated native bytes. #[test] - // Confirms `slice_as_bytes` produces the same byte layout as concatenating - // each element's native-endian bytes in order. fn slice_as_bytes_matches_native_bytes() { let values: [u16; 3] = [0x1122, 0x3344, 0x5566]; let mut expected: Vec = Vec::new(); @@ -586,7 +575,6 @@ mod tests { /// Ensures converting an empty slice to bytes yields an empty output slice. #[test] - // Ensures the empty slice case works and does not error or return junk data. fn slice_as_bytes_empty_is_empty() { let values: [u32; 0] = []; assert_eq!(slice_as_bytes(&values).unwrap(), &[]); @@ -594,8 +582,6 @@ mod tests { /// Rejects byte length computations that would overflow `usize`. #[test] - // Ensures the shared byte-length helper rejects overflows rather than - // silently wrapping and producing undersized buffers/slices. fn checked_byte_len_rejects_overflow() { let result = checked_byte_len(usize::MAX, 2); assert!(result.is_err()); @@ -611,7 +597,7 @@ mod tests { let combined = Usage::VERTEX | Usage::INDEX; let _ = combined.to_platform(); - assert!(Properties::default().cpu_visible()); + assert!(!Properties::default().cpu_visible()); assert!(!Properties::DEVICE_LOCAL.cpu_visible()); }