Skip to content

feat(lambda-rs): buffers to default to DEVICE_LOCAL as opposed to CPU_VISIBLE #180

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.

Sign up for GitHub

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

Merged
vmarcella merged 4 commits into from
Feb 9, 2026
Merged

feat(lambda-rs): buffers to default to DEVICE_LOCAL as opposed to CPU_VISIBLE #180

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
51a70ea
[update] buffers to default to DEVICE_LOCAL as opposed to CPU_VISIBLE
vmarcella Feb 9, 2026
f9c7357
[update] writes to return results.
vmarcella Feb 9, 2026
bfc65a4
Merge branch 'main' into vmrcella/default-buffers-to-device-local
vmarcella Feb 9, 2026
6071c93
[fix] test assertion.
vmarcella Feb 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 27 additions & 6 deletions crates/lambda-rs/src/render/buffer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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).
Comment on lines +84 to +86
Copy link

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With Properties now defaulting to DEVICE_LOCAL (i.e., no COPY_DST), it becomes easy for callers to create a buffer and then call Buffer::write_*, which will hit a wgpu validation error/panic because Queue::write_buffer requires COPY_DST. Consider making write_bytes/write_value/write_slice return a Result (or adding an explicit runtime check that produces a clear error) when the underlying buffer wasn’t created with CPU_VISIBLE/COPY_DST.

Copilot uses AI. Check for mistakes.
///
/// 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.
Comment on lines +84 to +90
Copy link

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docs here imply CPU_VISIBLE impacts actual memory residency (system RAM vs VRAM). In this codebase, cpu_visible ultimately only adds wgpu::BufferUsages::COPY_DST (see lambda-rs-platform buffer builder) and does not request mapping (e.g., MAP_WRITE), so it likely doesn’t control where the buffer is placed. Suggest rewording to focus on “uploadable via queue writes (COPY_DST)” vs “not uploadable”, and avoid claims about RAM/VRAM placement unless the platform layer truly enforces that.

Copilot uses AI. Check for mistakes.
pub struct Properties {
cpu_visible: bool,
}
Expand All @@ -101,7 +106,7 @@ impl Properties {

impl Default for Properties {
fn default() -> Self {
Properties::CPU_VISIBLE
Properties::DEVICE_LOCAL
}
}

Expand Down Expand Up @@ -287,7 +292,7 @@ impl<T: PlainOldData> UniformBuffer<T> {
/// let vertices: Vec<Vertex> = 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)
Comment on lines 354 to 359
Copy link

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doc example still imports Properties but no longer uses it after removing the explicit .with_properties(...) call. To keep the example self-consistent, either drop Properties from the use list or keep an explicit .with_properties(Properties::DEVICE_LOCAL) call (with a comment that it matches the default).

Copilot uses AI. Check for mistakes.
/// .build(render_context, vertices)
/// .unwrap();
Expand All @@ -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,
}
Expand Down Expand Up @@ -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());
}
Expand Down Expand Up @@ -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();
Expand Down
Loading