chore: split renderer into separate files#37
Merged
Conversation
📝 WalkthroughWalkthroughIntroduces a full-featured GPU renderer: new Renderer constructors, draw-queue and caching APIs, multi-pass effect system with backdrop support, stencil-based rendering passes, GPU buffer preparation and readback, surface/MSAA management, traversal utilities, and configurable render-pass load operations. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Renderer
participant DrawTree as Draw Tree
participant GPU as wgpu Device/Queue
App->>Renderer: add_shape(shape, clip, cache_key)
Renderer->>DrawTree: insert DrawCommand (Shape/CachedShape)
DrawTree-->>Renderer: node_id
App->>Renderer: set_shape_transform/node_props(node_id,...)
App->>Renderer: render()
Renderer->>Renderer: prepare_render() / tessellate & aggregate
Renderer->>GPU: upsert_gpu_buffer(vertex/index/instance)
GPU-->>Renderer: buffers ready
Renderer->>Renderer: plan_traversal()
Renderer->>Renderer: render_to_texture_view(texture_view)
Renderer->>GPU: render passes (stencil inc/dec, composite)
GPU-->>Renderer: offscreen textures / bind groups
Renderer->>Renderer: apply_effect_passes() -> composite
Renderer->>GPU: final render + present
GPU-->>App: frame presented
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (9)
src/renderer/draw_queue.rs (2)
67-71: Silent no-op when node not found.
mutate_draw_commandsilently ignores invalidnode_idvalues. This is likely intentional for robustness, but consider whether a debug-level log or returning a bool would help callers detect programming errors during development.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/draw_queue.rs` around lines 67 - 71, mutate_draw_command currently silently no-ops when node_id is missing from draw_tree; update the method (mutate_draw_command) to surface this condition by either returning a bool indicating success (true when self.draw_tree.get_mut(node_id) existed and mutator was called, false otherwise) or emitting a debug-level log via the existing logger if your project has one; locate the function in draw_queue.rs and modify its signature and callers if you choose the bool-return option, or add a debug log inside the None branch referencing node_id and DrawCommand to help detect invalid node lookups during development.
97-110: Silent early return for out-of-bounds layer.When
layer > 1, the function returns without any indication to the caller. Consider logging at debug level to help diagnose misuse:🔧 Optional: Add debug logging
pub fn set_shape_texture_layer( &mut self, node_id: usize, layer: usize, texture_id: Option<u64>, ) { if layer > 1 { + log::debug!("set_shape_texture_layer: layer {} out of range (max 1)", layer); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/draw_queue.rs` around lines 97 - 110, The function set_shape_texture_layer silently returns when an out-of-bounds layer (>1) is passed, making misuse hard to diagnose; update set_shape_texture_layer to log a debug-level message when layer > 1 (including node_id and invalid layer value) before returning, using the existing logging facility in the module, and keep the existing mutate_draw_command/set_texture_id behavior otherwise.src/renderer/construction.rs (2)
15-42: Consider propagating initialization errors instead of panicking.The constructor uses multiple
unwrap()andexpect()calls for surface creation, adapter request, and device request. For a library, returning aResultwould give callers flexibility in error handling.This is a common pattern in wgpu examples, but for production use, consider:
pub async fn new(...) -> Result<Self, RendererError>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/construction.rs` around lines 15 - 42, Change the constructor (pub async fn new(...)) to return Result<Self, RendererError> and stop panicking: replace the expect/unwrap calls around InstanceDescriptor::default() -> instance.create_surface(window), instance.request_adapter(...).await, and adapter.request_device(...).await with proper error propagation using ? or converting Option/Result into a RendererError (e.g., map_err or ok_or) so callers can handle failures; ensure you define or use an existing RendererError enum/struct to represent surface creation, adapter selection, and device creation errors and return it from new.
343-410: Redundant layout return value.
create_default_shape_texture_bind_grouptakes a&BindGroupLayoutparameter and returns it cloned as part of the tuple (line 409). The callers at lines 125-128 already have the layout, so this return value appears unused.🔧 Suggested simplification
fn create_default_shape_texture_bind_group( device: &Arc<wgpu::Device>, queue: &Arc<wgpu::Queue>, shape_texture_bind_group_layout: &wgpu::BindGroupLayout, - ) -> (wgpu::BindGroup, wgpu::BindGroupLayout) { + ) -> wgpu::BindGroup { // ... existing code ... - (bind_group, shape_texture_bind_group_layout.clone()) + bind_group }Then update callers to not destructure the layout.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/construction.rs` around lines 343 - 410, The function create_default_shape_texture_bind_group currently returns a (wgpu::BindGroup, wgpu::BindGroupLayout) tuple but the passed-in shape_texture_bind_group_layout is already available to callers and the returned clone is redundant; change the function signature to return only wgpu::BindGroup (remove the cloned layout return and its clone call inside), update all callers that destructure the tuple (e.g., sites that do let (bind_group, layout) = create_default_shape_texture_bind_group(...)) to only assign the bind_group (let bind_group = create_default_shape_texture_bind_group(...)), and adjust any type expectations accordingly so the layout parameter is kept as &BindGroupLayout and not returned.src/renderer/surface.rs (1)
118-124: Consider returning Result instead of panicking.
set_surfaceusesexpect()which will panic if surface creation fails. Since this is a public API that could be called with invalid window handles, consider returning aResultto let callers handle the error gracefully.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/surface.rs` around lines 118 - 124, The method set_surface currently panics on surface creation; change its signature from pub fn set_surface(&mut self, window: impl Into<SurfaceTarget<'static>>) to return a Result (e.g., Result<(), wgpu::CreateSurfaceError> or a crate-specific error) so callers can handle failures, call self.instance.create_surface(window).map_err(...) to propagate the error instead of expect, only call self.surface.configure(&self.device, &self.config) after successful creation, and update any call sites to handle the Result; reference symbols: set_surface, SurfaceTarget<'static>, instance.create_surface, self.surface.configure, device, config.src/renderer/rendering.rs (2)
242-336: Significant code duplication between traversal paths.The
traverse_subtree_mut(lines 251-336) andtraverse_mut(lines 452-534) closures contain nearly identical logic for handlingDrawCommand::ShapeandDrawCommand::CachedShapewithhandle_increment_passandhandle_decrement_pass. Consider extracting the common traversal callback logic into shared helper functions.Also applies to: 443-534
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/rendering.rs` around lines 242 - 336, Extract the duplicated logic in the traversal enter/exit closures into shared helper functions (e.g., fn traversal_enter_callback(...) and fn traversal_exit_callback(...)) that accept the same mutable context tuple (render_pass, stencil_stack, current_pipeline, skipped_stack) plus references used: effect_results (effect_results_ref), composite_pipeline (composite_pipeline_ref), pipelines, buffers, and the node_id; move the common handling of DrawCommand::Shape and DrawCommand::CachedShape to call handle_increment_pass/handle_decrement_pass inside those helpers and centralize the "skip via effect_results" logic (including setting composite_pipeline, bind_group, stencil ref, draw, and updating skipped_stack/current_pipeline). Then replace the inline closures passed to traverse_subtree_mut and traverse_mut with simple calls to these helpers so both traversal calls reuse the same enter and exit behavior.
49-66: Multiple unwrap() calls assume prepare_render was called.The
Buffersstruct construction uses multipleunwrap()calls on optional buffers. While this should be safe ifprepare_render()is called first (as done inrender()),render_to_texture_viewispub(super)and could potentially be called without preparation.Consider either:
- Making the prepare/render coupling more explicit through documentation
- Adding a debug assertion to verify buffers are initialized
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/rendering.rs` around lines 49 - 66, The constructor for crate::renderer::types::Buffers in render_to_texture_view uses several unwrap() calls (e.g., aggregated_vertex_buffer, aggregated_index_buffer, identity_instance_transform_buffer, identity_instance_color_buffer, identity_instance_metadata_buffer) which assumes prepare_render() was run; add defensive debug assertions at the start of pub(super) fn render_to_texture_view to verify those Option fields are initialized (e.g., debug_assert!(self.aggregated_vertex_buffer.is_some()), debug_assert!(self.identity_instance_transform_buffer.is_some()), etc.) before constructing Buffers, or alternatively document in the function comment that prepare_render() must be called first—prefer adding debug_asserts referencing the prepare_render() coupling and the listed field names to fail fast in debug builds.src/renderer/passes.rs (1)
509-516: Consider defensive handling for missing loaded effect.Line 516's
unwrap()assumes the effect is loaded, but ifunload_effectis called between checkingbackdrop_effectsand this lookup, it could panic. While unlikely in single-threaded code, consider usinggetwith error handling for robustness.Proposed defensive handling
let effect_instance = backdrop_ctx .backdrop_effects .get(&backdrop_node_id) .unwrap(); - let loaded_effect = backdrop_ctx + let Some(loaded_effect) = backdrop_ctx .loaded_effects .get(&effect_instance.effect_id) - .unwrap(); + else { + warn!("Loaded effect {} not found for backdrop node {}", effect_instance.effect_id, backdrop_node_id); + event_idx += 1; + continue; + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/passes.rs` around lines 509 - 516, The code currently unwraps the loaded effect after retrieving effect_instance (backdrop_ctx.backdrop_effects.get(&backdrop_node_id).unwrap() and backdrop_ctx.loaded_effects.get(&effect_instance.effect_id).unwrap()), which can panic if the effect was unloaded; replace the second unwrap with a safe lookup (e.g., backdrop_ctx.loaded_effects.get(&effect_instance.effect_id)) and handle the None case by logging/returning/continuing (choose the existing error-handling convention in this function) so the function no longer panics when the loaded effect is missing.src/renderer/types.rs (1)
92-111: Consider grouping related fields inBackdropContext.The
BackdropContextstruct has 18 fields which makes it harder to maintain. Consider grouping related fields (e.g., texture-related fields, pipeline-related fields) into smaller sub-structs for better organization. This is optional and can be deferred.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/types.rs` around lines 92 - 111, BackdropContext currently exposes many related fields making it unwieldy; refactor by introducing small grouped structs (e.g., TextureResources { backdrop_snapshot_texture, backdrop_snapshot_view, physical_size, config_format, texture_manager }, PipelineResources { composite_pipeline, stencil_only_pipeline, backdrop_color_pipeline, composite_bgl, effect_sampler }, BindGroups { and_bind_group, default_shape_texture_bind_groups, shape_texture_bind_group_layout_background, shape_texture_bind_group_layout_foreground, shape_texture_layout_epoch }) and replace the flat fields in BackdropContext with these grouped members while preserving lifetimes and visibility (pub(super)) and keeping references to wgpu::Device and existing types; update any code that constructs or accesses BackdropContext to use the new nested fields (look for uses of BackdropContext, backdrop_snapshot_texture, composite_pipeline, and_bind_group, default_shape_texture_bind_groups, etc.) so behavior is unchanged but structure is clearer.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/renderer/preparation.rs`:
- Around line 37-50: The code silently corrupts indices when vertex_start >
u16::MAX because vertex_start is cast to u16 (vertex_offset) and index +
vertex_offset can overflow; update the logic in preparation.rs (around variables
vertex_start, vertex_offset, temp_vertices, temp_indices, indices) to detect
this case and fail early: if vertex_start > u16::MAX as usize return None or Err
immediately (instead of only warn), and avoid the truncated cast; alternatively
migrate index storage/operations to u32 by changing vertex_offset to u32 and
temp_indices/indices arithmetic to u32 (or use checked_add when combining index
+ vertex_offset and return Err on overflow) so no silent wrapping occurs. Ensure
the calling code handles the new None/Err or u32 index type accordingly.
In `@src/renderer/readback.rs`:
- Around line 100-115: The code uses map_readback_buffer_to_vec and then
performs copy_from_slice into buffer (in both the full-copy branch and the
per-row branch) without verifying that data.len() is large enough, which can
panic if the readback is truncated; add defensive length checks after let data =
self.map_readback_buffer_to_vec(output_buffer) to validate data.len() >=
expected_size (and for the per-row loop ensure each slice range
padded_offset..padded_offset+unpadded_bytes_per_row as usize is within data
bounds) and return an Err or early-safe-truncate if the check fails, so
copy_from_slice is only called when the source and destination lengths are
guaranteed to match (references: map_readback_buffer_to_vec, buffer.resize,
copy_from_slice).
In `@src/renderer/rendering.rs`:
- Around line 414-415: The code currently calls
output_texture.expect("output_texture required for backdrop effects") in
render_to_texture_view, which will panic if output_texture is None when backdrop
effects are configured; change this to return a clear Err from
render_to_texture_view (or convert the surrounding function to return a Result)
instead of panicking: check output_texture before using it, return a descriptive
error (e.g., Err(RenderError::MissingOutputTextureForBackdrop)) referencing
output_texture and src_texture usage, and update callers to handle the Result;
alternatively document the function contract clearly if you prefer a hard
requirement, but do not use expect() here.
In `@src/renderer/surface.rs`:
- Around line 126-133: The VSync toggle in Surface::set_vsync is inconsistent
with Renderer::new: set_vsync currently maps true/false to
wgpu::PresentMode::Fifo/Immediate while Renderer::new uses
wgpu::PresentMode::AutoVsync/AutoNoVsync; update set_vsync to use the same
PresentMode variants as the constructor (use wgpu::PresentMode::AutoVsync when
vsync is true and wgpu::PresentMode::AutoNoVsync when false), assign that to
self.config.present_mode and keep the existing
self.surface.configure(&self.device, &self.config) call so runtime toggles match
construction behavior and avoid behavioral divergence between set_vsync and
Renderer::new.
---
Nitpick comments:
In `@src/renderer/construction.rs`:
- Around line 15-42: Change the constructor (pub async fn new(...)) to return
Result<Self, RendererError> and stop panicking: replace the expect/unwrap calls
around InstanceDescriptor::default() -> instance.create_surface(window),
instance.request_adapter(...).await, and adapter.request_device(...).await with
proper error propagation using ? or converting Option/Result into a
RendererError (e.g., map_err or ok_or) so callers can handle failures; ensure
you define or use an existing RendererError enum/struct to represent surface
creation, adapter selection, and device creation errors and return it from new.
- Around line 343-410: The function create_default_shape_texture_bind_group
currently returns a (wgpu::BindGroup, wgpu::BindGroupLayout) tuple but the
passed-in shape_texture_bind_group_layout is already available to callers and
the returned clone is redundant; change the function signature to return only
wgpu::BindGroup (remove the cloned layout return and its clone call inside),
update all callers that destructure the tuple (e.g., sites that do let
(bind_group, layout) = create_default_shape_texture_bind_group(...)) to only
assign the bind_group (let bind_group =
create_default_shape_texture_bind_group(...)), and adjust any type expectations
accordingly so the layout parameter is kept as &BindGroupLayout and not
returned.
In `@src/renderer/draw_queue.rs`:
- Around line 67-71: mutate_draw_command currently silently no-ops when node_id
is missing from draw_tree; update the method (mutate_draw_command) to surface
this condition by either returning a bool indicating success (true when
self.draw_tree.get_mut(node_id) existed and mutator was called, false otherwise)
or emitting a debug-level log via the existing logger if your project has one;
locate the function in draw_queue.rs and modify its signature and callers if you
choose the bool-return option, or add a debug log inside the None branch
referencing node_id and DrawCommand to help detect invalid node lookups during
development.
- Around line 97-110: The function set_shape_texture_layer silently returns when
an out-of-bounds layer (>1) is passed, making misuse hard to diagnose; update
set_shape_texture_layer to log a debug-level message when layer > 1 (including
node_id and invalid layer value) before returning, using the existing logging
facility in the module, and keep the existing mutate_draw_command/set_texture_id
behavior otherwise.
In `@src/renderer/passes.rs`:
- Around line 509-516: The code currently unwraps the loaded effect after
retrieving effect_instance
(backdrop_ctx.backdrop_effects.get(&backdrop_node_id).unwrap() and
backdrop_ctx.loaded_effects.get(&effect_instance.effect_id).unwrap()), which can
panic if the effect was unloaded; replace the second unwrap with a safe lookup
(e.g., backdrop_ctx.loaded_effects.get(&effect_instance.effect_id)) and handle
the None case by logging/returning/continuing (choose the existing
error-handling convention in this function) so the function no longer panics
when the loaded effect is missing.
In `@src/renderer/rendering.rs`:
- Around line 242-336: Extract the duplicated logic in the traversal enter/exit
closures into shared helper functions (e.g., fn traversal_enter_callback(...)
and fn traversal_exit_callback(...)) that accept the same mutable context tuple
(render_pass, stencil_stack, current_pipeline, skipped_stack) plus references
used: effect_results (effect_results_ref), composite_pipeline
(composite_pipeline_ref), pipelines, buffers, and the node_id; move the common
handling of DrawCommand::Shape and DrawCommand::CachedShape to call
handle_increment_pass/handle_decrement_pass inside those helpers and centralize
the "skip via effect_results" logic (including setting composite_pipeline,
bind_group, stencil ref, draw, and updating skipped_stack/current_pipeline).
Then replace the inline closures passed to traverse_subtree_mut and traverse_mut
with simple calls to these helpers so both traversal calls reuse the same enter
and exit behavior.
- Around line 49-66: The constructor for crate::renderer::types::Buffers in
render_to_texture_view uses several unwrap() calls (e.g.,
aggregated_vertex_buffer, aggregated_index_buffer,
identity_instance_transform_buffer, identity_instance_color_buffer,
identity_instance_metadata_buffer) which assumes prepare_render() was run; add
defensive debug assertions at the start of pub(super) fn render_to_texture_view
to verify those Option fields are initialized (e.g.,
debug_assert!(self.aggregated_vertex_buffer.is_some()),
debug_assert!(self.identity_instance_transform_buffer.is_some()), etc.) before
constructing Buffers, or alternatively document in the function comment that
prepare_render() must be called first—prefer adding debug_asserts referencing
the prepare_render() coupling and the listed field names to fail fast in debug
builds.
In `@src/renderer/surface.rs`:
- Around line 118-124: The method set_surface currently panics on surface
creation; change its signature from pub fn set_surface(&mut self, window: impl
Into<SurfaceTarget<'static>>) to return a Result (e.g., Result<(),
wgpu::CreateSurfaceError> or a crate-specific error) so callers can handle
failures, call self.instance.create_surface(window).map_err(...) to propagate
the error instead of expect, only call self.surface.configure(&self.device,
&self.config) after successful creation, and update any call sites to handle the
Result; reference symbols: set_surface, SurfaceTarget<'static>,
instance.create_surface, self.surface.configure, device, config.
In `@src/renderer/types.rs`:
- Around line 92-111: BackdropContext currently exposes many related fields
making it unwieldy; refactor by introducing small grouped structs (e.g.,
TextureResources { backdrop_snapshot_texture, backdrop_snapshot_view,
physical_size, config_format, texture_manager }, PipelineResources {
composite_pipeline, stencil_only_pipeline, backdrop_color_pipeline,
composite_bgl, effect_sampler }, BindGroups { and_bind_group,
default_shape_texture_bind_groups, shape_texture_bind_group_layout_background,
shape_texture_bind_group_layout_foreground, shape_texture_layout_epoch }) and
replace the flat fields in BackdropContext with these grouped members while
preserving lifetimes and visibility (pub(super)) and keeping references to
wgpu::Device and existing types; update any code that constructs or accesses
BackdropContext to use the new nested fields (look for uses of BackdropContext,
backdrop_snapshot_texture, composite_pipeline, and_bind_group,
default_shape_texture_bind_groups, etc.) so behavior is unchanged but structure
is clearer.
Comment on lines
+37
to
+50
| let vertex_start = temp_vertices.len(); | ||
| if vertex_start > u16::MAX as usize { | ||
| warn!( | ||
| "Aggregated vertex count ({}) exceeds u16 limit. Rendering artifacts may occur.", | ||
| vertex_start | ||
| ); | ||
| } | ||
|
|
||
| let index_start = temp_indices.len(); | ||
| let vertex_offset = vertex_start as u16; | ||
| temp_vertices.extend_from_slice(vertices); | ||
|
|
||
| for &index in indices { | ||
| temp_indices.push(index + vertex_offset); |
There was a problem hiding this comment.
Potential u16 overflow causes silent corruption.
When vertex_start exceeds u16::MAX, the code logs a warning but continues with a truncated cast at line 46. Additionally, line 50 can overflow when adding index + vertex_offset for indices near the limit. This silently produces incorrect index values and rendering artifacts.
Consider returning None (or an error) when the vertex count exceeds the u16 limit, or switch to u32 indices if large scenes are expected.
Proposed fix to fail early on overflow
let vertex_start = temp_vertices.len();
if vertex_start > u16::MAX as usize {
- warn!(
- "Aggregated vertex count ({}) exceeds u16 limit. Rendering artifacts may occur.",
- vertex_start
- );
+ warn!(
+ "Aggregated vertex count ({}) exceeds u16 limit. Skipping shape.",
+ vertex_start
+ );
+ return None;
}
let index_start = temp_indices.len();
let vertex_offset = vertex_start as u16;
temp_vertices.extend_from_slice(vertices);
for &index in indices {
- temp_indices.push(index + vertex_offset);
+ temp_indices.push(index.checked_add(vertex_offset).unwrap_or_else(|| {
+ warn!("Index overflow detected");
+ 0
+ }));
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/preparation.rs` around lines 37 - 50, The code silently corrupts
indices when vertex_start > u16::MAX because vertex_start is cast to u16
(vertex_offset) and index + vertex_offset can overflow; update the logic in
preparation.rs (around variables vertex_start, vertex_offset, temp_vertices,
temp_indices, indices) to detect this case and fail early: if vertex_start >
u16::MAX as usize return None or Err immediately (instead of only warn), and
avoid the truncated cast; alternatively migrate index storage/operations to u32
by changing vertex_offset to u32 and temp_indices/indices arithmetic to u32 (or
use checked_add when combining index + vertex_offset and return Err on overflow)
so no silent wrapping occurs. Ensure the calling code handles the new None/Err
or u32 index type accordingly.
Comment on lines
+100
to
+115
| let data = self.map_readback_buffer_to_vec(output_buffer); | ||
| let output_size = (unpadded_bytes_per_row * height) as usize; | ||
| buffer.resize(output_size, 0); | ||
|
|
||
| if padded_bytes_per_row == unpadded_bytes_per_row { | ||
| buffer.copy_from_slice(&data); | ||
| } else { | ||
| for row in 0..height { | ||
| let padded_offset = (row * padded_bytes_per_row) as usize; | ||
| let unpadded_offset = (row * unpadded_bytes_per_row) as usize; | ||
| let row_data = | ||
| &data[padded_offset..padded_offset + unpadded_bytes_per_row as usize]; | ||
| buffer[unpadded_offset..unpadded_offset + unpadded_bytes_per_row as usize] | ||
| .copy_from_slice(row_data); | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential panic if readback data is truncated.
If map_readback_buffer_to_vec returns fewer bytes than expected (e.g., due to a mapping failure that returns partial data), the copy_from_slice calls could panic. Consider adding a length check before copying.
🛡️ Suggested defensive check
let data = self.map_readback_buffer_to_vec(output_buffer);
+ if data.len() < (padded_bytes_per_row * height) as usize {
+ log::warn!("Readback buffer smaller than expected");
+ return;
+ }
let output_size = (unpadded_bytes_per_row * height) as usize;
buffer.resize(output_size, 0);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let data = self.map_readback_buffer_to_vec(output_buffer); | |
| let output_size = (unpadded_bytes_per_row * height) as usize; | |
| buffer.resize(output_size, 0); | |
| if padded_bytes_per_row == unpadded_bytes_per_row { | |
| buffer.copy_from_slice(&data); | |
| } else { | |
| for row in 0..height { | |
| let padded_offset = (row * padded_bytes_per_row) as usize; | |
| let unpadded_offset = (row * unpadded_bytes_per_row) as usize; | |
| let row_data = | |
| &data[padded_offset..padded_offset + unpadded_bytes_per_row as usize]; | |
| buffer[unpadded_offset..unpadded_offset + unpadded_bytes_per_row as usize] | |
| .copy_from_slice(row_data); | |
| } | |
| } | |
| let data = self.map_readback_buffer_to_vec(output_buffer); | |
| if data.len() < (padded_bytes_per_row * height) as usize { | |
| log::warn!("Readback buffer smaller than expected"); | |
| return; | |
| } | |
| let output_size = (unpadded_bytes_per_row * height) as usize; | |
| buffer.resize(output_size, 0); | |
| if padded_bytes_per_row == unpadded_bytes_per_row { | |
| buffer.copy_from_slice(&data); | |
| } else { | |
| for row in 0..height { | |
| let padded_offset = (row * padded_bytes_per_row) as usize; | |
| let unpadded_offset = (row * unpadded_bytes_per_row) as usize; | |
| let row_data = | |
| &data[padded_offset..padded_offset + unpadded_bytes_per_row as usize]; | |
| buffer[unpadded_offset..unpadded_offset + unpadded_bytes_per_row as usize] | |
| .copy_from_slice(row_data); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/readback.rs` around lines 100 - 115, The code uses
map_readback_buffer_to_vec and then performs copy_from_slice into buffer (in
both the full-copy branch and the per-row branch) without verifying that
data.len() is large enough, which can panic if the readback is truncated; add
defensive length checks after let data =
self.map_readback_buffer_to_vec(output_buffer) to validate data.len() >=
expected_size (and for the per-row loop ensure each slice range
padded_offset..padded_offset+unpadded_bytes_per_row as usize is within data
bounds) and return an Err or early-safe-truncate if the check fails, so
copy_from_slice is only called when the source and destination lengths are
guaranteed to match (references: map_readback_buffer_to_vec, buffer.resize,
copy_from_slice).
Comment on lines
+414
to
+415
| let src_texture = | ||
| output_texture.expect("output_texture required for backdrop effects"); |
There was a problem hiding this comment.
Panic on missing output_texture with backdrop effects.
The expect() will panic if output_texture is None when backdrop effects are present. This could occur if a caller uses render_to_texture_view directly without providing the output texture while having backdrop effects configured.
Consider returning an error or providing a clearer contract in documentation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/rendering.rs` around lines 414 - 415, The code currently calls
output_texture.expect("output_texture required for backdrop effects") in
render_to_texture_view, which will panic if output_texture is None when backdrop
effects are configured; change this to return a clear Err from
render_to_texture_view (or convert the surrounding function to return a Result)
instead of panicking: check output_texture before using it, return a descriptive
error (e.g., Err(RenderError::MissingOutputTextureForBackdrop)) referencing
output_texture and src_texture usage, and update callers to handle the Result;
alternatively document the function contract clearly if you prefer a hard
requirement, but do not use expect() here.
Owner
Author
There was a problem hiding this comment.
This is an internal implementation detail, so I don't think it's important
There was a problem hiding this comment.
@antouhou, understood! Since this is an internal function with a clear contract between callers within the renderer module, the expect() is reasonable. Thanks for clarifying!
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/renderer/surface.rs`:
- Around line 118-124: The current set_surface method blindly calls
create_surface and configure with the existing self.config, but new surfaces can
have different supported formats/alpha/present modes; update set_surface to
accept the previously-available Adapter (or store adapter on Renderer) so you
can re-query surface capabilities (via adapter and instance) for the created
surface, then choose a supported format/alpha_mode/present_mode and update
self.config accordingly before calling self.surface.configure; if the chosen
format differs from the prior config, call self.recreate_pipelines() and
self.recreate_msaa_texture() after reconfiguration. Ensure you reference
set_surface, self.surface, self.instance, self.device, self.config, adapter (or
a stored adapter field), recreate_pipelines, and recreate_msaa_texture when
implementing the change.
- Around line 8-11: The change_scale_factor method must guard against
non-positive or non-finite new_scale_factor to avoid invalid uniforms used by
to_logical and the WGSL shader; update change_scale_factor to first validate
new_scale_factor with new_scale_factor.is_finite() && new_scale_factor > 0.0 and
only assign self.scale_factor and call self.resize(self.physical_size) when
valid, otherwise reject (or clamp to a sane default like 1.0) and optionally log
a warning so bad values (0, NaN, negative, infinite) never propagate into
rendering.
- Around line 26-59: The resize method must skip surface.configure() and MSAA
texture creation when width or height is zero to avoid validation errors: in
resize (fn resize) after updating self.physical_size, self.config, uniforms and
writing buffers, add a guard like if new_physical_size.0 == 0 ||
new_physical_size.1 == 0 { return; } so that calls to
self.surface.configure(&self.device, &self.config) and
self.recreate_msaa_texture() (and optionally offscreen_texture_pool.trim(...,
self.msaa_sample_count)) are not executed for zero-sized surfaces.
Comment on lines
+8
to
+11
| pub fn change_scale_factor(&mut self, new_scale_factor: f64) { | ||
| self.scale_factor = new_scale_factor; | ||
| self.resize(self.physical_size) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and see the full context around lines 8-11
head -20 src/renderer/surface.rsRepository: antouhou/grafo
Length of output: 551
🏁 Script executed:
# Find all references to change_scale_factor to understand call sites
rg -n "change_scale_factor" --type=rs -B2 -A2Repository: antouhou/grafo
Length of output: 84
🏁 Script executed:
# Find the to_logical function to verify it divides by scale_factor
rg -n "to_logical" --type=rs -B5 -A5Repository: antouhou/grafo
Length of output: 84
🏁 Script executed:
# Check how scale_factor is used throughout the codebase
rg -n "scale_factor" --type=rs -B1 -A1Repository: antouhou/grafo
Length of output: 84
🏁 Script executed:
# Find all references to change_scale_factor
rg -n "change_scale_factor" -B2 -A2Repository: antouhou/grafo
Length of output: 2168
🏁 Script executed:
# Find the to_logical function
rg -n "to_logical" -B5 -A5Repository: antouhou/grafo
Length of output: 3546
🏁 Script executed:
# Search for scale_factor usage and initialization
rg -n "scale_factor" -B1 -A1 | head -100Repository: antouhou/grafo
Length of output: 5277
🏁 Script executed:
# Find the resize function in the codebase
rg -n "fn resize" -B2 -A10Repository: antouhou/grafo
Length of output: 987
🏁 Script executed:
# Look for any validation patterns or assertions related to scale
rg -n "scale|is_finite|is_normal" | head -50Repository: antouhou/grafo
Length of output: 3789
Guard against non-positive or non-finite scale factors.
to_logical divides by scale_factor (src/util.rs:62-63), and the shader also divides by it (src/shaders/shader.wgsl:101). Passing 0, NaN, or negative values produces invalid uniforms and breaks rendering. Add validation in change_scale_factor before calling resize.
🛠️ Proposed fix
pub fn change_scale_factor(&mut self, new_scale_factor: f64) {
+ if !new_scale_factor.is_finite() || new_scale_factor <= 0.0 {
+ log::warn!("Invalid scale factor: {}", new_scale_factor);
+ return;
+ }
self.scale_factor = new_scale_factor;
self.resize(self.physical_size)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| pub fn change_scale_factor(&mut self, new_scale_factor: f64) { | |
| self.scale_factor = new_scale_factor; | |
| self.resize(self.physical_size) | |
| } | |
| pub fn change_scale_factor(&mut self, new_scale_factor: f64) { | |
| if !new_scale_factor.is_finite() || new_scale_factor <= 0.0 { | |
| log::warn!("Invalid scale factor: {}", new_scale_factor); | |
| return; | |
| } | |
| self.scale_factor = new_scale_factor; | |
| self.resize(self.physical_size) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/surface.rs` around lines 8 - 11, The change_scale_factor method
must guard against non-positive or non-finite new_scale_factor to avoid invalid
uniforms used by to_logical and the WGSL shader; update change_scale_factor to
first validate new_scale_factor with new_scale_factor.is_finite() &&
new_scale_factor > 0.0 and only assign self.scale_factor and call
self.resize(self.physical_size) when valid, otherwise reject (or clamp to a sane
default like 1.0) and optionally log a warning so bad values (0, NaN, negative,
infinite) never propagate into rendering.
Comment on lines
+26
to
+59
| pub fn resize(&mut self, new_physical_size: (u32, u32)) { | ||
| self.physical_size = new_physical_size; | ||
| self.config.width = new_physical_size.0; | ||
| self.config.height = new_physical_size.1; | ||
|
|
||
| let logical_size = to_logical(new_physical_size, self.scale_factor); | ||
| self.and_uniforms.canvas_size = [logical_size.0, logical_size.1]; | ||
| self.and_uniforms.scale_factor = self.scale_factor as f32; | ||
| self.and_uniforms.fringe_width = self.fringe_width; | ||
|
|
||
| self.decrementing_uniforms.canvas_size = [logical_size.0, logical_size.1]; | ||
| self.decrementing_uniforms.scale_factor = self.scale_factor as f32; | ||
| self.decrementing_uniforms.fringe_width = self.fringe_width; | ||
|
|
||
| self.queue.write_buffer( | ||
| &self.and_uniform_buffer, | ||
| 0, | ||
| bytemuck::cast_slice(&[self.and_uniforms]), | ||
| ); | ||
| self.queue.write_buffer( | ||
| &self.decrementing_uniform_buffer, | ||
| 0, | ||
| bytemuck::cast_slice(&[self.decrementing_uniforms]), | ||
| ); | ||
|
|
||
| self.surface.configure(&self.device, &self.config); | ||
| self.recreate_msaa_texture(); | ||
|
|
||
| self.offscreen_texture_pool.trim( | ||
| new_physical_size.0, | ||
| new_physical_size.1, | ||
| self.msaa_sample_count, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the actual file structure and find the resize method
fd "surface.rs" --type fRepository: antouhou/grafo
Length of output: 81
🏁 Script executed:
# Read the surface.rs file to see the current implementation
wc -l src/renderer/surface.rsRepository: antouhou/grafo
Length of output: 85
🏁 Script executed:
# Get the resize method and surrounding code
sed -n '1,150p' src/renderer/surface.rs | cat -nRepository: antouhou/grafo
Length of output: 5172
🏁 Script executed:
# Search for all resize-related methods and zero-size checks
rg -n "fn resize|physical_size.*==\s*0|width.*==\s*0|height.*==\s*0" src/renderer/surface.rs -A2 -B2Repository: antouhou/grafo
Length of output: 238
🏁 Script executed:
# Search for other references to physical_size in the file
rg -n "physical_size" src/renderer/surface.rs -C2Repository: antouhou/grafo
Length of output: 1548
🏁 Script executed:
# Look for other files in the renderer module that might have resize paths
fd . src/renderer --type f -name "*.rs" | head -20Repository: antouhou/grafo
Length of output: 288
🏁 Script executed:
# Find where create_msaa_color_texture is defined
rg -n "create_msaa_color_texture" src/renderer --type=rs -A5Repository: antouhou/grafo
Length of output: 84
🏁 Script executed:
# Check if there are any error handling or panic patterns around texture creation
rg -n "panic|unwrap|expect" src/renderer/surface.rsRepository: antouhou/grafo
Length of output: 110
🏁 Script executed:
# Look for any comments or documentation about zero-sized surfaces
rg -n "zero|0\s*size|minimize|width.*0|height.*0" src/renderer --type=rsRepository: antouhou/grafo
Length of output: 84
🏁 Script executed:
# Check the full context around offscreen_texture_pool.trim to see how it handles zero sizes
rg -n "offscreen_texture_pool\|trim" src/renderer --type=rs -B2 -A2Repository: antouhou/grafo
Length of output: 84
🏁 Script executed:
# Find where create_msaa_color_texture is defined
rg -n "create_msaa_color_texture" src/renderer -A5Repository: antouhou/grafo
Length of output: 429
🏁 Script executed:
# Look for any comments or documentation about zero-sized surfaces
rg -n "zero|minimize" src/rendererRepository: antouhou/grafo
Length of output: 139
🏁 Script executed:
# Check the full context around offscreen_texture_pool.trim
rg -n "offscreen_texture_pool" src/renderer -B2 -A2Repository: antouhou/grafo
Length of output: 2008
🏁 Script executed:
# Look at imports and other renderer files
ls -la src/renderer/Repository: antouhou/grafo
Length of output: 796
🏁 Script executed:
# Find where create_msaa_color_texture is defined
rg -n "fn create_msaa_color_texture" src/rendererRepository: antouhou/grafo
Length of output: 40
🏁 Script executed:
# Check construction.rs more thoroughly
head -100 src/renderer/construction.rs | cat -nRepository: antouhou/grafo
Length of output: 4303
🏁 Script executed:
# Search for create_msaa_color_texture in construction.rs
grep -n "create_msaa_color_texture" src/renderer/construction.rs -A10Repository: antouhou/grafo
Length of output: 40
🏁 Script executed:
# Look at the imports in surface.rs to understand dependencies
head -5 src/renderer/surface.rsRepository: antouhou/grafo
Length of output: 162
🏁 Script executed:
# Search all files for create_msaa_color_texture
rg "create_msaa_color_texture" src/ --type rust -B2 -A8Repository: antouhou/grafo
Length of output: 2275
🏁 Script executed:
# Check the super::* import - look at module structure
cat src/renderer/mod.rsRepository: antouhou/grafo
Length of output: 109
🏁 Script executed:
# Look at types.rs which might have the function
cat src/renderer/types.rsRepository: antouhou/grafo
Length of output: 5422
🏁 Script executed:
# Get the full create_msaa_color_texture implementation
grep -n "pub fn create_msaa_color_texture" src/pipeline.rs -A20Repository: antouhou/grafo
Length of output: 724
🏁 Script executed:
# Look for create_offscreen_color_texture to see if it has similar issues
grep -n "pub fn create_offscreen_color_texture" src/pipeline.rs -A20Repository: antouhou/grafo
Length of output: 753
🏁 Script executed:
# Let's check wgpu requirements - search for any validation or error handling around zero sizes
rg "width.*0|height.*0" src/ -B2 -A2Repository: antouhou/grafo
Length of output: 6729
🏁 Script executed:
# Check if there's any documentation in comments about this
rg "resize|minimize|zero" src/ -B1 -A1Repository: antouhou/grafo
Length of output: 3604
🏁 Script executed:
# Look at the offscreen_texture_pool to see if it handles zero sizes
rg -n "OffscreenTexturePool\|impl.*OffscreenTexturePool" src/ -A50 | head -100Repository: antouhou/grafo
Length of output: 40
🏁 Script executed:
# Search for OffscreenTexturePool definition
rg "struct OffscreenTexturePool|impl.*OffscreenTexturePool" src/Repository: antouhou/grafo
Length of output: 154
🏁 Script executed:
# Find where OffscreenTexturePool is defined
rg "OffscreenTexturePool" src/ --type rust | head -20Repository: antouhou/grafo
Length of output: 399
🏁 Script executed:
# Get the OffscreenTexturePool implementation
rg "impl OffscreenTexturePool" src/effect.rs -A100 | head -120Repository: antouhou/grafo
Length of output: 3941
🏁 Script executed:
# Also check the trim method specifically
rg "pub fn trim" src/effect.rs -B2 -A15Repository: antouhou/grafo
Length of output: 891
Guard surface.configure() and MSAA texture creation against zero-sized surfaces.
When the window is minimized or during resize, width/height can become 0. Calling surface.configure() and creating MSAA textures with zero dimensions will fail with validation errors. Add a zero-size check before these operations to prevent runtime failures.
🛠️ Proposed fix
pub fn resize(&mut self, new_physical_size: (u32, u32)) {
self.physical_size = new_physical_size;
self.config.width = new_physical_size.0;
self.config.height = new_physical_size.1;
@@
self.queue.write_buffer(
&self.decrementing_uniform_buffer,
0,
bytemuck::cast_slice(&[self.decrementing_uniforms]),
);
+ if new_physical_size.0 == 0 || new_physical_size.1 == 0 {
+ self.recreate_msaa_texture();
+ return;
+ }
self.surface.configure(&self.device, &self.config);
self.recreate_msaa_texture();
@@
pub(super) fn recreate_msaa_texture(&mut self) {
- if self.msaa_sample_count > 1 {
+ let size_zero = self.physical_size.0 == 0 || self.physical_size.1 == 0;
+ if self.msaa_sample_count > 1 && !size_zero {
let texture = create_msaa_color_texture(
&self.device,
self.physical_size,
self.config.format,
self.msaa_sample_count,
);
@@
} else {
self.msaa_color_texture = None;
self.msaa_color_texture_view = None;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/surface.rs` around lines 26 - 59, The resize method must skip
surface.configure() and MSAA texture creation when width or height is zero to
avoid validation errors: in resize (fn resize) after updating
self.physical_size, self.config, uniforms and writing buffers, add a guard like
if new_physical_size.0 == 0 || new_physical_size.1 == 0 { return; } so that
calls to self.surface.configure(&self.device, &self.config) and
self.recreate_msaa_texture() (and optionally offscreen_texture_pool.trim(...,
self.msaa_sample_count)) are not executed for zero-sized surfaces.
Comment on lines
+118
to
+124
| pub fn set_surface(&mut self, window: impl Into<SurfaceTarget<'static>>) { | ||
| self.surface = self | ||
| .instance | ||
| .create_surface(window) | ||
| .expect("Failed to create surface"); | ||
| self.surface.configure(&self.device, &self.config); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Run the verification checks suggested in the review
rg -n "SurfaceConfiguration" --type=rs -C2
rg -n "get_capabilities" --type=rs -C2
rg -n "struct Renderer" --type=rs -C2
rg -n "adapter" --type=rs -C2Repository: antouhou/grafo
Length of output: 165
🏁 Script executed:
#!/bin/bash
# Find Rust files and search for key terms
fd -e rs | head -20
echo "---"
rg "SurfaceConfiguration" -C2
echo "---"
rg "get_capabilities" -C2
echo "---"
rg "struct Renderer" -C2
echo "---"
rg "recreate_pipelines\|recreate_msaa_texture" -C2Repository: antouhou/grafo
Length of output: 1938
🏁 Script executed:
#!/bin/bash
# Look at the surface.rs file directly
find . -name "surface.rs" -type f
echo "---"
# Check if surface.rs exists in renderer subdirectory
ls -la src/renderer/ 2>/dev/null | head -20
echo "---"
# Look for all files in src/renderer/
fd -e rs src/renderer/Repository: antouhou/grafo
Length of output: 1205
🏁 Script executed:
#!/bin/bash
# Read the surface.rs file to see the actual implementation
cat -n src/renderer/surface.rs
echo "=== CHECKING RENDERER STRUCT ==="
# Check for adapter field in Renderer
rg "pub struct Renderer" -A 30
echo "=== CHECKING RECREATE METHODS ==="
# Look for any methods that might recreate pipelines or textures
rg "fn recreate" -i
echo "=== CHECKING CONSTRUCTION PATTERN ==="
# See how surface is initially configured
cat -n src/renderer/construction.rs | head -100Repository: antouhou/grafo
Length of output: 11268
Re‑derive SurfaceConfiguration for new surfaces.
When swapping surfaces, the old config.format/alpha_mode/present_mode may be unsupported by the new surface. Re‑query capabilities and update config before configure; if the format changes, also recreate pipelines/MSAA textures.
However, the proposed fix references self.adapter which is not stored in the Renderer struct (only available during construction). To implement this fix, you would need to either: (1) store the adapter as a field in Renderer, (2) pass adapter as a parameter to set_surface, or (3) use an alternative approach. The calls to recreate_pipelines() and recreate_msaa_texture() are valid—both methods exist and should be invoked when surface configuration changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/surface.rs` around lines 118 - 124, The current set_surface
method blindly calls create_surface and configure with the existing self.config,
but new surfaces can have different supported formats/alpha/present modes;
update set_surface to accept the previously-available Adapter (or store adapter
on Renderer) so you can re-query surface capabilities (via adapter and instance)
for the created surface, then choose a supported format/alpha_mode/present_mode
and update self.config accordingly before calling self.surface.configure; if the
chosen format differs from the prior config, call self.recreate_pipelines() and
self.recreate_msaa_texture() after reconfiguration. Ensure you reference
set_surface, self.surface, self.instance, self.device, self.config, adapter (or
a stored adapter field), recreate_pipelines, and recreate_msaa_texture when
implementing the change.
This was referenced Feb 22, 2026
This was referenced Mar 1, 2026
Merged
This was referenced Apr 18, 2026
This was referenced Apr 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Tests
Documentation