Update WebGPU backend to current wgpu/winit APIs and add validation example#113
Update WebGPU backend to current wgpu/winit APIs and add validation example#113mors119 wants to merge 3 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughModernizes bracket-terminal WebGPU HAL and winit integration: Arc-based window ownership, rewritten adapter/init, winit 0.30 event-loop/input mapping, updated pipeline/RenderPass descriptor shapes across console backings, TexelCopy texture/buffer APIs, surface present/reconfigure logic, and a new minimal WebGPU example. ChangesWebGPU HAL Modernization
Sequence Diagram(s)sequenceDiagram
participant App
participant Init as init_adapter
participant Instance as wgpu::Instance
participant Surface as wgpu::Surface
App->>Init: Arc<Window>
Init->>Instance: InstanceDescriptor::new_without_display_handle()
Init->>Surface: create_surface(window)?
Init->>Surface: get_default_config(adapter)
Init-->>App: Surface<'static> + config
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bracket-terminal/src/hal/webgpu/mainloop.rs (1)
739-810:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winScreenshot capture is now a silent no-op.
The texture→buffer copy still runs (allocating a buffer and submitting a copy each request), but the buffer is never mapped/encoded to a file — the PNG-writing path is fully commented out and
_filenameis ignored. Callers settingrequest_screenshotget no file and no error, just wasted GPU work.I can implement the
map_asyncread-back + PNG encode against the current wgpu 29 API (usingpadded_bytes_per_rowand trimming to the unpadded row width). Want me to draft it or open a tracking issue?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bracket-terminal/src/hal/webgpu/mainloop.rs` around lines 739 - 810, In take_screenshot the output_buffer is created and a copy is submitted but the mapping + PNG write path is commented out so screenshots are a silent no-op; restore a proper readback by mapping output_buffer (use output_buffer.slice(..).map_async(wgpu::MapMode::Read) and await the result via device.poll or a blocking executor), then call slice.get_mapped_range(), trim each padded row using buffer_dimensions.padded_bytes_per_row and buffer_dimensions.unpadded_bytes_per_row, feed the bytes into a png::Encoder (RGBA, 8-bit) writing rows of unpadded bytes, finish the writer, drop the mapped range and call output_buffer.unmap(); update take_screenshot to use _filename and handle/map map_async errors (Propagate/log errors) so callers get a real PNG file instead of a no-op.
🧹 Nitpick comments (3)
bracket-terminal/src/hal/webgpu/mainloop.rs (2)
36-145: ⚡ Quick winAdd a unit test for
map_keycode.
map_keycodeis new, pure, and easily testable (sampling a few representative mappings plus theNonefallback) — but no tests were added.Want me to generate a colocated
testsmodule covering a representative subset of mappings and the unmapped fallback?As per coding guidelines: "Add tests for new functionality in the relevant module; for split domains, prefer colocated
tests.rs".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bracket-terminal/src/hal/webgpu/mainloop.rs` around lines 36 - 145, The PR is missing unit tests for the new pure function map_keycode; add a colocated tests module (e.g., a #[cfg(test)] mod tests or tests.rs alongside the module) that asserts several representative mappings (e.g., W::Digit1 -> V::Key1, W::KeyA -> V::A, W::Enter -> V::Return, W::ShiftLeft -> V::LShift) and that an unmapped value returns None; use the same crate::hal::VirtualKeyCode and KeyCode imports as the function to construct inputs and compare outputs so the tests exercise map_keycode directly.
480-500: 💤 Low valueConsider reconfiguring on
Suboptimal.Rendering/presenting a
Suboptimalframe is valid, but wgpu's guidance recommends callingSurface::configureagain for optimal performance; otherwise a suboptimal surface persists until the nextOutdated/resize. FoldingSuboptimalinto the reconfigure path (or reconfiguring after present) aligns with the wgpu 29 migration example.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bracket-terminal/src/hal/webgpu/mainloop.rs` around lines 480 - 500, Split the CurrentSurfaceTexture match arm so Suboptimal is handled separately from Success: keep the existing rendering/screenshot/present logic in the Success arm, and duplicate that logic in a new Suboptimal arm but call wgpu.surface.configure(&wgpu.device, &wgpu.config) after current_tex.present() (or immediately after rendering if you prefer) so the surface is reconfigured when suboptimal frames occur; update references to backing_flip.render, take_screenshot, be.request_screenshot and current_tex.present in the new Suboptimal arm to match the Success behavior.bracket-terminal/src/hal/webgpu/backing/fancy_console_backing.rs (1)
124-136: 💤 Low value
_scale_centeris now effectively dead.The rename silences the unused-parameter warning, but the scale-center logic remains commented out (Lines 155-158), so the parameter no longer influences rendering. This is consistent across all four console backings. Consider tracking removal of the parameter (and its call sites) or reinstating the functionality in a follow-up, so the public signature doesn't advertise an input that's ignored.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bracket-terminal/src/hal/webgpu/backing/fancy_console_backing.rs` around lines 124 - 136, In rebuild_vertices, the parameter _scale_center is unused (renamed to silence warning) while the scale-center logic remains commented out; either remove the parameter from the public signature and update all call sites and the other three console backing implementations to match, or restore the original behavior by re-enabling the commented scale-center math: use the provided scale_center to compute a translation delta that recenters geometry around that point before applying scale (adjust offset_x/offset_y by (scale_center - position) * (1.0 - scale) or the existing commented formula), update rebuild_vertices in fancy_console_backing.rs and the three sibling backing files consistently, and run tests to ensure rendering/method signatures stay in sync.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bracket-terminal/src/hal/webgpu/mainloop.rs`:
- Around line 491-493: The call to QuadRender::render is passing an extra borrow
(&&WgpuLink) which triggers clippy::needless_borrow; change the call site where
backing_flip.render(&wgpu, &target) is used to pass wgpu directly
(backing_flip.render(wgpu, &target)) so the first argument is &WgpuLink as
expected by QuadRender::render, leaving the rest of the screenshot logic
(take_screenshot(..., &wgpu.backing_buffer.texture)) unchanged.
---
Outside diff comments:
In `@bracket-terminal/src/hal/webgpu/mainloop.rs`:
- Around line 739-810: In take_screenshot the output_buffer is created and a
copy is submitted but the mapping + PNG write path is commented out so
screenshots are a silent no-op; restore a proper readback by mapping
output_buffer (use output_buffer.slice(..).map_async(wgpu::MapMode::Read) and
await the result via device.poll or a blocking executor), then call
slice.get_mapped_range(), trim each padded row using
buffer_dimensions.padded_bytes_per_row and
buffer_dimensions.unpadded_bytes_per_row, feed the bytes into a png::Encoder
(RGBA, 8-bit) writing rows of unpadded bytes, finish the writer, drop the mapped
range and call output_buffer.unmap(); update take_screenshot to use _filename
and handle/map map_async errors (Propagate/log errors) so callers get a real PNG
file instead of a no-op.
---
Nitpick comments:
In `@bracket-terminal/src/hal/webgpu/backing/fancy_console_backing.rs`:
- Around line 124-136: In rebuild_vertices, the parameter _scale_center is
unused (renamed to silence warning) while the scale-center logic remains
commented out; either remove the parameter from the public signature and update
all call sites and the other three console backing implementations to match, or
restore the original behavior by re-enabling the commented scale-center math:
use the provided scale_center to compute a translation delta that recenters
geometry around that point before applying scale (adjust offset_x/offset_y by
(scale_center - position) * (1.0 - scale) or the existing commented formula),
update rebuild_vertices in fancy_console_backing.rs and the three sibling
backing files consistently, and run tests to ensure rendering/method signatures
stay in sync.
In `@bracket-terminal/src/hal/webgpu/mainloop.rs`:
- Around line 36-145: The PR is missing unit tests for the new pure function
map_keycode; add a colocated tests module (e.g., a #[cfg(test)] mod tests or
tests.rs alongside the module) that asserts several representative mappings
(e.g., W::Digit1 -> V::Key1, W::KeyA -> V::A, W::Enter -> V::Return,
W::ShiftLeft -> V::LShift) and that an unmapped value returns None; use the same
crate::hal::VirtualKeyCode and KeyCode imports as the function to construct
inputs and compare outputs so the tests exercise map_keycode directly.
- Around line 480-500: Split the CurrentSurfaceTexture match arm so Suboptimal
is handled separately from Success: keep the existing
rendering/screenshot/present logic in the Success arm, and duplicate that logic
in a new Suboptimal arm but call wgpu.surface.configure(&wgpu.device,
&wgpu.config) after current_tex.present() (or immediately after rendering if you
prefer) so the surface is reconfigured when suboptimal frames occur; update
references to backing_flip.render, take_screenshot, be.request_screenshot and
current_tex.present in the new Suboptimal arm to match the Success behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bb8a38ad-52a8-4e79-9333-9a78b8648189
📒 Files selected for processing (17)
bracket-terminal/examples/webgpu_minimal.rsbracket-terminal/src/hal/mod.rsbracket-terminal/src/hal/webgpu/backend.rsbracket-terminal/src/hal/webgpu/backing/fancy_console_backing.rsbracket-terminal/src/hal/webgpu/backing/index_array_helper.rsbracket-terminal/src/hal/webgpu/backing/simple_console_backing.rsbracket-terminal/src/hal/webgpu/backing/sparse_console_backing.rsbracket-terminal/src/hal/webgpu/backing/sprite_console_backing.rsbracket-terminal/src/hal/webgpu/backing/vertex_array_helper.rsbracket-terminal/src/hal/webgpu/font.rsbracket-terminal/src/hal/webgpu/framebuffer.rsbracket-terminal/src/hal/webgpu/init.rsbracket-terminal/src/hal/webgpu/mainloop.rsbracket-terminal/src/hal/webgpu/mod.rsbracket-terminal/src/hal/webgpu/platform.rsbracket-terminal/src/hal/webgpu/quadrender.rsbracket-terminal/src/lib.rs
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Format all Rust code usingcargo fmt --all
Fix allcargo clippywarnings — the CI enforces-D warnings
Add tests for new functionality in the relevant module; for split domains, prefer colocatedtests.rs
Files:
bracket-terminal/src/lib.rsbracket-terminal/src/hal/webgpu/backing/index_array_helper.rsbracket-terminal/src/hal/mod.rsbracket-terminal/src/hal/webgpu/backend.rsbracket-terminal/src/hal/webgpu/platform.rsbracket-terminal/src/hal/webgpu/backing/vertex_array_helper.rsbracket-terminal/src/hal/webgpu/backing/sprite_console_backing.rsbracket-terminal/examples/webgpu_minimal.rsbracket-terminal/src/hal/webgpu/init.rsbracket-terminal/src/hal/webgpu/quadrender.rsbracket-terminal/src/hal/webgpu/framebuffer.rsbracket-terminal/src/hal/webgpu/backing/fancy_console_backing.rsbracket-terminal/src/hal/webgpu/backing/sparse_console_backing.rsbracket-terminal/src/hal/webgpu/backing/simple_console_backing.rsbracket-terminal/src/hal/webgpu/font.rsbracket-terminal/src/hal/webgpu/mod.rsbracket-terminal/src/hal/webgpu/mainloop.rs
🔇 Additional comments (28)
bracket-terminal/src/hal/mod.rs (1)
59-67: LGTM!bracket-terminal/src/hal/webgpu/backend.rs (1)
8-16: LGTM!bracket-terminal/src/hal/webgpu/platform.rs (1)
5-5: LGTM!Also applies to: 43-43
bracket-terminal/src/hal/webgpu/init.rs (1)
109-151: LGTM!
init_adaptercorrectly adopts the wgpu 29 falliblerequest_adapter/request_device(nowResult-returning),new_without_display_handle(), andget_default_config(...)with the zero-size guard via.max(1). ReturningSurface<'static>is sound because ownership is moved in throughArc<Window>.bracket-terminal/src/hal/webgpu/mainloop.rs (1)
698-737: LGTM!
clear_screen_passnow returnsBResult<()>with a proper error on uninitialized WebGPU, and the render-pass descriptor adds the required wgpu 29 fields (depth_slice,StoreOp::Store,multiview_mask).bracket-terminal/src/hal/webgpu/mod.rs (1)
3-5: LGTM!Sourcing
VirtualKeyCodefrom the shareddummy/keycodes.rs(rather than aliasing winit'sKeyCode) is consistent with the newmap_keycodetranslation layer and keeps the engine keycode type backend-independent.bracket-terminal/src/lib.rs (1)
15-16: LGTM!bracket-terminal/examples/webgpu_minimal.rs (1)
1-103: LGTM!Return type
BErroris compatible withmain_loop'sBResult<()>, the clamp bounds keep the@inside the drawn border, and the input/quit handling matches the updated winit 0.30 event flow.bracket-terminal/src/hal/webgpu/backing/simple_console_backing.rs (3)
156-156: Same_scale_centerobservation as infancy_console_backing.rsapplies here.
47-93: LGTM!
278-299: LGTM!bracket-terminal/src/hal/webgpu/backing/sparse_console_backing.rs (3)
122-122: Same_scale_centerobservation as infancy_console_backing.rsapplies here.
32-78: LGTM!
230-245: LGTM!bracket-terminal/src/hal/webgpu/backing/fancy_console_backing.rs (2)
37-83: LGTM!
254-269: LGTM!bracket-terminal/src/hal/webgpu/backing/sprite_console_backing.rs (2)
29-75: LGTM!
224-245: LGTM!bracket-terminal/src/hal/webgpu/font.rs (2)
121-145: LGTM!
149-159: LGTM!bracket-terminal/src/hal/webgpu/framebuffer.rs (1)
18-44: LGTM!bracket-terminal/src/hal/webgpu/backing/index_array_helper.rs (1)
30-50: LGTM!bracket-terminal/src/hal/webgpu/backing/vertex_array_helper.rs (1)
64-89: LGTM!bracket-terminal/src/hal/webgpu/quadrender.rs (5)
116-117: RenderPipelineDescriptor: confirmmultiview_mask/cacheset toNoneis valid in wgpu 29.0.3.In wgpu 29.0.3, both
multiview_maskandcacheonRenderPipelineDescriptorareOptionfields (multiview_mask: Option<NonZeroU32>,cache: Option<&PipelineCache>). The documented behavior is to setmultiview_mask = Nonewhen multiview isn’t used, andcache = Nonewhen noPipelineCacheis provided—so the current values are appropriate for those cases.
88-88: wgpu 29.0.3:compilation_optionsis required
wgpu::VertexStateandwgpu::FragmentStateboth have a non-optionalcompilation_options: PipelineCompilationOptionsfield, so providingcompilation_options: wgpu::PipelineCompilationOptions::default()in both states (lines ~88 and ~98) is required and appropriate.
86-86: Confirm WGSL entry points matchQuadRender’s hardcoded"vs_main"/"fs_main"
QuadRendersetsvertex.entry_point: Some("vs_main")andfragment.entry_point: Some("fs_main")(quadrender.rs ~86 and ~92). Since it’s instantiated asQuadRender::new(wgpu, &bit.shaders[2])(mainloop.rs), ensure the WGSL module stored inbit.shaders[2]defines shader entry functions with the exact namesvs_mainandfs_main; otherwise render-pipeline creation will fail at runtime.
204-204: Confirm RenderPassDescriptor / RenderPassColorAttachment fields match wgpu 29.x API
- Line 204:
view: targetis correct—render(..., target: &TextureView)already provides&TextureView, so it’s not&&TextureView.- Lines 206, 218-220:
depth_slice,occlusion_query_set,timestamp_writes, andmultiview_maskare valid fields and are correctly set toNone.- Line 214:
ops.storeis explicitly set towgpu::StoreOp::Storeinwgpu::Operations<Color>.
76-77: ConfirmPipelineLayoutDescriptorbind_group_layouts+immediate_sizeusage (wgpu 29.x)
bind_group_layoutsis expected to be&[Option<&BindGroupLayout>]in wgpu 29.x, so usingSome(&texture_bind_group_layout)/Some(&uniform_layout)is correct.immediate_size: 0is appropriate: the WGSL inhal/webgpu/shader_sourceusesvar<uniform>(novar<override>/ immediates), and the repo doesn’t enablewgpu::Features::IMMEDIATES.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
bracket-terminal/src/hal/webgpu/mainloop.rs (2)
508-552: ⚡ Quick winDe-duplicate the
SuccessandSuboptimalrender paths.These two arms are identical except that
Suboptimalreconfigures the surface afterwards. Collapsing them avoids the two render paths drifting apart over time. The variant migration itself looks correct for the wgpu 29 API.♻️ Proposed dedup of the present paths
if let Some(wgpu) = be.wgpu.as_ref() { - match wgpu.surface.get_current_texture() { - wgpu::CurrentSurfaceTexture::Success(current_tex) => { - backing_flip.update_uniform( - wgpu, - bterm.post_scanlines, - bterm.post_screenburn, - bterm.screen_burn_color, - ); - let target = current_tex - .texture - .create_view(&TextureViewDescriptor::default()); - if backing_flip.render(wgpu, &target).is_ok() { - if let Some(filename) = &screenshot_request { - take_screenshot(filename, wgpu, bterm, &wgpu.backing_buffer.texture); - } - clear_screenshot_request = true; - current_tex.present(); - } - } - wgpu::CurrentSurfaceTexture::Suboptimal(current_tex) => { - backing_flip.update_uniform( - wgpu, - bterm.post_scanlines, - bterm.post_screenburn, - bterm.screen_burn_color, - ); - let target = current_tex - .texture - .create_view(&TextureViewDescriptor::default()); - if backing_flip.render(wgpu, &target).is_ok() { - if let Some(filename) = &screenshot_request { - take_screenshot(filename, wgpu, bterm, &wgpu.backing_buffer.texture); - } - clear_screenshot_request = true; - current_tex.present(); - } - wgpu.surface.configure(&wgpu.device, &wgpu.config); - } - wgpu::CurrentSurfaceTexture::Outdated | wgpu::CurrentSurfaceTexture::Lost => { - wgpu.surface.configure(&wgpu.device, &wgpu.config); - } - wgpu::CurrentSurfaceTexture::Timeout - | wgpu::CurrentSurfaceTexture::Occluded - | wgpu::CurrentSurfaceTexture::Validation => {} - } + let (current_tex, reconfigure) = match wgpu.surface.get_current_texture() { + wgpu::CurrentSurfaceTexture::Success(t) => (Some(t), false), + wgpu::CurrentSurfaceTexture::Suboptimal(t) => (Some(t), true), + wgpu::CurrentSurfaceTexture::Outdated | wgpu::CurrentSurfaceTexture::Lost => { + wgpu.surface.configure(&wgpu.device, &wgpu.config); + (None, false) + } + wgpu::CurrentSurfaceTexture::Timeout + | wgpu::CurrentSurfaceTexture::Occluded + | wgpu::CurrentSurfaceTexture::Validation => (None, false), + }; + if let Some(current_tex) = current_tex { + backing_flip.update_uniform( + wgpu, + bterm.post_scanlines, + bterm.post_screenburn, + bterm.screen_burn_color, + ); + let target = current_tex + .texture + .create_view(&TextureViewDescriptor::default()); + if backing_flip.render(wgpu, &target).is_ok() { + if let Some(filename) = &screenshot_request { + take_screenshot(filename, wgpu, bterm, &wgpu.backing_buffer.texture); + } + clear_screenshot_request = true; + current_tex.present(); + } + if reconfigure { + wgpu.surface.configure(&wgpu.device, &wgpu.config); + } + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bracket-terminal/src/hal/webgpu/mainloop.rs` around lines 508 - 552, The Success and Suboptimal arms are duplicated; collapse them into a single match arm that handles both variants, perform backing_flip.update_uniform, create the view, call backing_flip.render, run take_screenshot and current_tex.present as before, then conditionally call wgpu.surface.configure(&wgpu.device, &wgpu.config) only when the matched variant is Suboptimal; reference the variants wgpu::CurrentSurfaceTexture::Success and ::Suboptimal and the functions backing_flip.update_uniform, backing_flip.render, take_screenshot, current_tex.present and wgpu.surface.configure to locate where to implement the change.
519-524: Heads-up: screenshot requests are silently consumed without writing a file.
take_screenshotstill performs the texture→buffer copy, but its save path remains commented out (lines 828-859) and_filenameis ignored, so arequest_screenshothere is cleared (clear_screenshot_request = true) while producing no output — just a wasted GPU copy. Not a blocker, but worth tracking since this PR re-enables the request flow.Want me to implement the buffer map-back + PNG write to restore screenshots under the wgpu 29 API, or open an issue to track it?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bracket-terminal/src/hal/webgpu/mainloop.rs` around lines 519 - 524, The code clears screenshot_request (clear_screenshot_request = true) after calling take_screenshot, but take_screenshot currently ignores the filename and its file-write block is commented out, so screenshots are consumed without being saved; either (A) reimplement the save path inside take_screenshot: perform the buffer->host map using wgpu 0.29 async mapping (buffer.slice(..).map_async), await the mapping, read mapped_range, construct an image buffer and encode to PNG using the image crate, unmap the buffer, and use the passed filename string to write the file (referencing take_screenshot and wgpu.backing_buffer.texture), or (B) if you don't want to implement saving now, stop clearing the request in mainloop (don’t set clear_screenshot_request = true when screenshot_request.is_some()) so the request is not silently consumed (referencing screenshot_request, clear_screenshot_request, backing_flip.render, and current_tex.present).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@bracket-terminal/src/hal/webgpu/mainloop.rs`:
- Around line 508-552: The Success and Suboptimal arms are duplicated; collapse
them into a single match arm that handles both variants, perform
backing_flip.update_uniform, create the view, call backing_flip.render, run
take_screenshot and current_tex.present as before, then conditionally call
wgpu.surface.configure(&wgpu.device, &wgpu.config) only when the matched variant
is Suboptimal; reference the variants wgpu::CurrentSurfaceTexture::Success and
::Suboptimal and the functions backing_flip.update_uniform, backing_flip.render,
take_screenshot, current_tex.present and wgpu.surface.configure to locate where
to implement the change.
- Around line 519-524: The code clears screenshot_request
(clear_screenshot_request = true) after calling take_screenshot, but
take_screenshot currently ignores the filename and its file-write block is
commented out, so screenshots are consumed without being saved; either (A)
reimplement the save path inside take_screenshot: perform the buffer->host map
using wgpu 0.29 async mapping (buffer.slice(..).map_async), await the mapping,
read mapped_range, construct an image buffer and encode to PNG using the image
crate, unmap the buffer, and use the passed filename string to write the file
(referencing take_screenshot and wgpu.backing_buffer.texture), or (B) if you
don't want to implement saving now, stop clearing the request in mainloop (don’t
set clear_screenshot_request = true when screenshot_request.is_some()) so the
request is not silently consumed (referencing screenshot_request,
clear_screenshot_request, backing_flip.render, and current_tex.present).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8d107d67-c975-4dd4-9881-2e7533102532
📒 Files selected for processing (1)
bracket-terminal/src/hal/webgpu/mainloop.rs
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Format all Rust code usingcargo fmt --all
Fix allcargo clippywarnings — the CI enforces-D warnings
Add tests for new functionality in the relevant module; for split domains, prefer colocatedtests.rs
Files:
bracket-terminal/src/hal/webgpu/mainloop.rs
🔇 Additional comments (1)
bracket-terminal/src/hal/webgpu/mainloop.rs (1)
147-172: LGTM!
|
@coderabbitai review accept coderabbit suggestion: handle suboptimal surface reconfiguration |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Find and display the surface texture present handling in mainloop.rs
rg -n "CurrentSurfaceTexture\|Suboptimal\|Outdated\|SurfaceError\|surface_texture\|get_current_texture\|present" bracket-terminal/src/hal/webgpu/mainloop.rs -A 3 -B 3Length of output: 492 🏁 Script executed: #!/bin/bash
# Find the mainloop file
fd "mainloop.rs" --type f
echo "---"
# Also search across all webgpu files for surface texture handling
rg -n "get_current_texture\|Suboptimal\|surface_texture\|CurrentSurface" --type rust -A 3 -B 3Length of output: 612 🏁 Script executed: #!/bin/bash
cat bracket-terminal/src/hal/webgpu/mainloop.rsLength of output: 31446
let (current_tex, reconfigure) = match wgpu.surface.get_current_texture() {
wgpu::CurrentSurfaceTexture::Success(t) => (Some(t), false),
wgpu::CurrentSurfaceTexture::Suboptimal(t) => (Some(t), true), // ✅ reconfigure flagged
wgpu::CurrentSurfaceTexture::Outdated | wgpu::CurrentSurfaceTexture::Lost => {
wgpu.surface.configure(&wgpu.device, &wgpu.config);
(None, false)
}
...
};
if let Some(current_tex) = current_tex {
...
if backing_flip.render(wgpu, &target).is_ok() {
...
current_tex.present();
}
if reconfigure { // ✅ applied post-present
wgpu.surface.configure(&wgpu.device, &wgpu.config);
}
}This follows the standard wgpu recommendation: render and present the suboptimal frame (to avoid a missed frame), then reconfigure for subsequent frames. The logic looks correctly applied. Triggering a full review now. ✅ Actions performedReview triggered.
|
1 participant
What
Update the 'Bracket Terminal' WebGPU backend to the current 'wgpu 29.0.3' and 'winit 0.30' APIs, restore the working '--features webgpu' build, and add a small validation example to 'bracket terminal/example/webgpu_minimal.rs'.
Key changes:
Why
The WebGPU backend was already behind the declared dependency version in the workspace, and no longer matches the active 'wgpu'/'winit' API.
This resulted in some immediate problems:
Related issues
Closes #111
Closes #112
Checklist
Required
cargo check --allpassescargo fmt --all -- --checkpassescargo clippy --workspace --all-targets -- -D warnings -A clippy::multiple-crate-versionspassescargo test --allpassesCloses #123)Functional Validation
Configuration & Docs
README.md,ARCHITECTURE.md, or relevant manual pages, if applicable)If Applicable
cargo auditlocally if needed)Summary by CodeRabbit
New Features
Chores
Tests