feat: add picking assets on hover #9
Conversation
WalkthroughThis update refactors the Rust backend to store assets in a Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Canvas
participant Pointer
participant PickManager
participant State
participant GPU
User->>Canvas: Moves mouse
Canvas->>Pointer: Update pointer.x/y
User->>Canvas: Interacts (hover, click)
Canvas->>PickManager: Request pick at pointer.x/y
PickManager->>GPU: Render 1x1 pick texture with asset IDs
GPU-->>PickManager: Write picked ID to buffer
PickManager->>State: update_hover(picked_id)
State-->>PickManager: hovered_asset_id updated
PickManager-->>Canvas: Optionally trigger border redraw
sequenceDiagram
participant App
participant State
participant Renderer
participant GPU
App->>State: add_texture(id, points, texture_id)
App->>Renderer: runCreator(state, assetsList)
loop For each asset ID in assetsList
Renderer->>State: get_shader_input(asset_id)
State-->>Renderer: ShaderInput (vertex data, texture id)
Renderer->>GPU: drawTexture(vertex data, texture)
end
Renderer->>State: get_border()
State-->>Renderer: Border vertex data (if hovered)
Renderer->>GPU: drawTriangle(border vertex data)
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🔭 Outside diff range comments (1)
src/index.ts (1)
73-76: 🛠️ Refactor suggestionMissing cleanup for mouse event listeners
The destroy method doesn't remove the event listeners added by initMouseController, which could lead to memory leaks if the creator is destroyed and recreated.
destroy: () => { context.unconfigure() device.destroy() + // Clean up event listeners + canvas.removeEventListener('mouseleave', () => {}) + canvas.removeEventListener('mousemove', () => {}) + canvas.removeEventListener('mousedown', () => {}) + canvas.removeEventListener('mouseup', () => {}) + canvas.removeEventListener('wheel', () => {}) }For proper cleanup, consider refactoring initMouseController to return a cleanup function that removes all event listeners, or store the event handler functions to remove them specifically.
🧹 Nitpick comments (21)
src/run.ts (2)
31-34: Avoid per-frame allocations ofFloat32ArrayWrapping
vertex_datainnew Float32Array()every frame triggers GC pressure. Cache typed arrays once:-const { texture_id, vertex_data } = state.get_shader_input(id) -drawTexture(pass, matrix, new Float32Array(vertex_data), textures[texture_id]) +const { texture_id, vertex_data_buffer } = state.get_shader_input(id) // expose a cached buffer +drawTexture(pass, matrix, vertex_data_buffer, textures[texture_id])If exposing a buffer is not feasible, reuse a scratch buffer to reduce allocations.
53-53:asyncPicknot awaited – race conditions possible
pickManager.asyncPick(state)returns a promise (per its name). If a rejection occurs, it will be swallowed by the event loop. Consider either:pickManager.asyncPick(state).catch(console.error)or converting the outer
drawtoasyncandawaitthe call to surface errors during development.src/WebGPU/programs/initPrograms.ts (1)
8-18: Missing re-export from barrel file breaks tree-shakingYou added
pickTextureto the runtime but forgot to re-export it from any public barrel (if one exists). Double-check modules such assrc/WebGPU/programs/index.tsor higher-level aggregators to ensure consumers canimport { pickTexture } from .../programs'.If no barrel exists, ignore this note.
src/WebGPU/pointer.ts (4)
7-8: Empty mouseleave handlerThe mouseleave handler is empty. Consider updating the pointer state or adding a comment explaining the intended future functionality.
canvas.addEventListener('mouseleave', () => { + // Reset pointer or set a flag indicating mouse is outside canvas })
16-18: Unused parameter in mousedown event handlerThe 'e' parameter is defined but never used in the mousedown handler.
- canvas.addEventListener('mousedown', e => { + canvas.addEventListener('mousedown', () => { })🧰 Tools
🪛 ESLint
[error] 16-16: 'e' is defined but never used.
(@typescript-eslint/no-unused-vars)
20-22: Unused parameter in mouseup event handlerThe 'e' parameter is defined but never used in the mouseup handler.
- canvas.addEventListener('mouseup', e => { + canvas.addEventListener('mouseup', () => { })🧰 Tools
🪛 ESLint
[error] 20-20: 'e' is defined but never used.
(@typescript-eslint/no-unused-vars)
24-26: Console log in wheel event handlerThe wheel event handler only logs to the console. Consider implementing meaningful functionality or adding a comment explaining this is for development/debugging purposes.
canvas.addEventListener("wheel", (event) => { - console.log(event.deltaY) + // TODO: Implement zoom functionality using event.deltaY + console.log("Wheel delta:", event.deltaY) })crate/src/line.rs (2)
5-11: Function signature looks good, but parameter name is inconsistentThe parameter name
points_a(plural) doesn't match with its usage as a single point.pub fn get_vertex_data<P: HasCoords>( - points_a: &P, + point_a: &P, point_b: &P, width: f32, color: Color, ) -> Vec<f32> {
20-45: Vertex generation for the line rectangleThe vertex data creates two triangles to form a rectangular line. This is correct for rendering with triangles, but you might consider using a different winding order for better readability.
let vertex_data: [Point; 6] = [ Point { x: ax - half_width * angle.cos(), y: ay - half_width * angle.sin(), }, Point { x: ax + half_width * angle.cos(), y: ay + half_width * angle.sin(), }, Point { x: bx + half_width * angle.cos(), y: by + half_width * angle.sin(), }, - Point { - x: bx - half_width * angle.cos(), - y: by - half_width * angle.sin(), - }, - Point { - x: ax - half_width * angle.cos(), - y: ay - half_width * angle.sin(), - }, - Point { - x: bx + half_width * angle.cos(), - y: by + half_width * angle.sin(), - }, + // Second triangle (completing the rectangle) + Point { + x: bx + half_width * angle.cos(), + y: by + half_width * angle.sin(), + }, + Point { + x: bx - half_width * angle.cos(), + y: by - half_width * angle.sin(), + }, + Point { + x: ax - half_width * angle.cos(), + y: ay - half_width * angle.sin(), + }, ];crate/src/types.rs (1)
24-39: VertexPoint struct and implementation look goodThe VertexPoint struct extends the basic Point with texture coordinates u and v, which is useful for texture mapping. The HasCoords implementation is appropriate.
Since both Point and VertexPoint have identical HasCoords implementations, you might consider a derive macro or a blanket implementation for any struct with x and y fields to reduce code duplication.
// Example of a blanket implementation (not using a macro) impl<T> HasCoords for T where T: AsRef<Self>, Self: {x: f32, y: f32} { fn x(&self) -> f32 { self.x } fn y(&self) -> f32 { self.y } }crate/src/texture.rs (2)
18-31: Consider using a constant for the triangle definition.The refactored
get_vertex_datamethod looks good, but the hardcoded vertex indices array is duplicated inget_vertex_pick_data. Consider extracting this into a constant to avoid duplication.impl Texture { + const TRIANGLE_INDICES: [usize; 6] = [0, 1, 2, 2, 3, 0]; pub fn get_vertex_data(&self) -> Vec<f32> { - let points: [&VertexPoint; 6] = [ - &self.points[0], - &self.points[1], - &self.points[2], - &self.points[2], - &self.points[3], - &self.points[0], - ]; + let points: Vec<&VertexPoint> = Self::TRIANGLE_INDICES.iter() + .map(|&idx| &self.points[idx]) + .collect(); points .iter() .flat_map(|point| vec![point.x, point.y, 0.0, 1.0, point.u, point.v]) .collect() }
43-56: Refactor to reuse vertex index pattern.This method duplicates vertex index pattern from
get_vertex_data. If you implement the above refactoring suggestion, you should also update this method accordingly.pub fn get_vertex_pick_data(&self) -> Vec<f32> { - let points: [&VertexPoint; 6] = [ - &self.points[0], - &self.points[1], - &self.points[2], - &self.points[2], - &self.points[3], - &self.points[0], - ]; + let points: Vec<&VertexPoint> = Self::TRIANGLE_INDICES.iter() + .map(|&idx| &self.points[idx]) + .collect(); points .iter() .flat_map(|point| vec![point.x, point.y, 0.0, 1.0, point.u, point.v, self.id]) .collect() }src/WebGPU/programs/pickTexture/shader.wgsl (2)
21-29: Remove commented code when it's no longer needed.There's a commented note about passing offsets instead of positions. If this optimization is planned for the future, consider adding a TODO comment for clarity, otherwise remove it.
var out: VertexOutput; - // maybe we should pass offsets from the position instead of... position? out.position = u.worldViewProjection * vert.position;
32-41: Remove commented function signature.The commented-out function signature on line 32 should be removed.
-// @fragment fn fs(in: VertexOutput) -> @location(0) vec4f { @fragment fn fs(in: VertexOutput) -> @location(0) u32 {The fragment shader logic for discarding transparent pixels and returning the ID as a u32 looks correct.
src/WebGPU/programs/pickTexture/getProgram.ts (4)
26-33: Clean up commented code and clarify attribute formats.There's an unused commented attribute on line 32. Additionally, the vertex attribute format comments could be more descriptive.
attributes: [ - {shaderLocation: 0, offset: 0, format: 'float32x4'}, // destination position - {shaderLocation: 1, offset: 16, format: 'float32x2'}, // source position - {shaderLocation: 2, offset: 16 + 8, format: 'float32'}, // id - // {shaderLocation: 3, offset: 16 + 8, format: 'float32x3'}, // id + {shaderLocation: 0, offset: 0, format: 'float32x4'}, // position (x, y, z, w) + {shaderLocation: 1, offset: 16, format: 'float32x2'}, // texture coordinates (u, v) + {shaderLocation: 2, offset: 16 + 8, format: 'float32'}, // object id ] as const,
40-43: Remove commented-out format line.Since you're explicitly using the 'r32uint' format for picking, the commented-out line referencing
debugPresentationFormatcan be removed.targets: [{ - // format: debugPresentationFormat, format: 'r32uint', }],
45-53: Consider whether primitive and depth-stencil settings are needed.There are commented out primitive cullMode and depthStencil settings. If these settings are not relevant for picking, consider removing the commented code. If they might be needed later, add a TODO comment explaining when they should be uncommented.
- // primitive: { - // cullMode: 'back', - // }, - // depthStencil: { - // depthWriteEnabled: true, - // depthCompare: 'less', - // format: 'depth24plus', - // },
98-98: Remove commented-out code.There's a commented-out matrix translation operation. Remove it if it's not needed.
- // const translateWorldProjMatrix = mat4.translate(worldProjectionMatrix, [])src/WebGPU/pick.ts (1)
56-61: Dead code & linter noise – remove or put to use
pickDepthTextureand thedepthStencilAttachmentblock are allocated/commented but never used.
Likewise, thedebugRenderPassargument is unused and triggers the ESLint error you’ve seen.Cleaning these up will remove needless GPU memory consumption and silence the linter:
- private pickDepthTexture: GPUTexture … - this.pickDepthTexture = device.createTexture({ - size: [1, 1], - format: 'depth24plus', - usage: GPUTextureUsage.RENDER_ATTACHMENT, - })Or – if you plan to use a depth attachment later – keep it but reference it in
descriptor.depthStencilAttachment.Also applies to: 29-33
src/WebGPU/programs/drawTriangle/getProgram.ts (1)
76-82: Performance: vertex buffer & bind group are recreated every drawCreating GPU buffers and bind groups inside the hot draw loop is expensive.
Consider:
- Keep a cache keyed by
vertexData.byteLength.- Re-use a single bind group – it only contains the uniform buffer.
This will reduce stutter when many borders are drawn on hover.
Also applies to: 84-92
crate/src/lib.rs (1)
104-128:get_borderallocates every frame – cache or stream?Rebuilding the full border vertex vector on each call can become expensive when borders are thick (
width = 20.0) and assets have many points.
If the hover state does not change every frame, cache the generated buffer and only invalidate it inupdate_hover.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
integration-tests/tests/creator.spec.ts-snapshots/after-hover-chromium.pngis excluded by!**/*.png
📒 Files selected for processing (19)
crate/src/lib.rs(1 hunks)crate/src/line.rs(1 hunks)crate/src/texture.rs(3 hunks)crate/src/types.rs(1 hunks)integration-tests/index.ts(2 hunks)integration-tests/tests/creator.spec.ts(1 hunks)jest.config.ts(1 hunks)src/WebGPU/pick.ts(1 hunks)src/WebGPU/pointer.ts(1 hunks)src/WebGPU/programs/drawTexture/shader.wgsl(1 hunks)src/WebGPU/programs/drawTriangle/getProgram.ts(1 hunks)src/WebGPU/programs/drawTriangle/shader.wgsl(1 hunks)src/WebGPU/programs/initPrograms.ts(3 hunks)src/WebGPU/programs/pickTexture/getProgram.ts(1 hunks)src/WebGPU/programs/pickTexture/shader.wgsl(1 hunks)src/index.ts(3 hunks)src/run.ts(2 hunks)tests-examples/demo-todo-app.spec.ts(0 hunks)webpack.config.js(1 hunks)
💤 Files with no reviewable changes (1)
- tests-examples/demo-todo-app.spec.ts
🧰 Additional context used
🧬 Code Graph Analysis (6)
src/index.ts (4)
src/types.ts (1)
Point(1-4)src/WebGPU/pointer.ts (1)
initMouseController(3-27)src/run.ts (1)
runCreator(10-59)src/WebGPU/getTexture/index.ts (1)
createTextureFromSource(106-117)
crate/src/texture.rs (3)
src/types.ts (1)
Point(1-4)crate/src/line.rs (1)
get_vertex_data(6-55)crate/src/types.rs (6)
x(5-5)x(16-18)x(33-35)y(6-6)y(19-21)y(36-38)
src/WebGPU/pick.ts (1)
src/WebGPU/pointer.ts (1)
pointer(1-1)
crate/src/types.rs (1)
src/types.ts (1)
Point(1-4)
crate/src/lib.rs (3)
src/types.ts (2)
Line(6-6)Point(1-4)crate/src/texture.rs (2)
new(10-16)get_vertex_data(18-31)crate/src/line.rs (1)
get_vertex_data(6-55)
src/WebGPU/programs/drawTriangle/getProgram.ts (4)
src/WebGPU/programs/draw3dModel/getProgram.ts (1)
getProgram(3-97)src/WebGPU/programs/draw3dModelLight/getProgram.ts (1)
getProgram(10-123)src/WebGPU/programs/drawTexture/getProgram.ts (1)
getProgram(6-114)src/WebGPU/programs/initPrograms.ts (1)
drawTriangle(10-10)
🪛 ESLint
src/WebGPU/pointer.ts
[error] 16-16: 'e' is defined but never used.
(@typescript-eslint/no-unused-vars)
[error] 20-20: 'e' is defined but never used.
(@typescript-eslint/no-unused-vars)
src/WebGPU/programs/pickTexture/getProgram.ts
[error] 7-7: 'debugPresentationFormat' is defined but never used.
(@typescript-eslint/no-unused-vars)
src/WebGPU/pick.ts
[error] 40-40: 'debugRenderPass' is defined but never used.
(@typescript-eslint/no-unused-vars)
🔇 Additional comments (34)
jest.config.ts (1)
15-15: Style cleanup: Removed unnecessary semicolonsThe trailing semicolons have been removed, which is consistent with modern TypeScript style. This change is purely aesthetic and has no functional impact.
Also applies to: 17-17
webpack.config.js (1)
100-105: HtmlWebpackPlugin now included unconditionallyThe HtmlWebpackPlugin is now included in the test configuration regardless of the environment, ensuring HTML template generation for integration tests in both development and production modes. This change supports the new asset picking functionality being tested.
src/WebGPU/programs/drawTexture/shader.wgsl (1)
29-29: Code simplification in fragment shaderRemoved the intermediate
texelvariable and directly returned the result oftextureSample(). This is a good simplification that maintains the same functionality while reducing code verbosity.integration-tests/index.ts (3)
4-5: Added explicit asset identificationThe introduction of a constant
ASSET_IDaligns with the new asset management system that uses explicit IDs instead of implicit ordering. This is a good practice for clarity and maintainability.
18-18: Updated addImage API to use explicit asset IDThe
addImagemethod now accepts an explicit asset ID as its first parameter, matching the updated API that manages assets by ID rather than by position in a collection.
25-30: Updated updatePoints API to use explicit asset IDThe
updatePointsmethod now accepts an explicit asset ID as its first parameter, consistent with the new asset management approach. This allows for more explicit targeting of specific assets when updating their coordinates.integration-tests/tests/creator.spec.ts (1)
24-24: Possible hang:networkidlemay never be reached
await page.waitForLoadState('networkidle')is great for traditional sites, but if your app opens long-lived connections (e.g. WebSocket, EventSource, streaming fetch) the network will never go “idle”, causing the test to stall indefinitely.
Consider one of the following:-await page.waitForLoadState('networkidle') +// Wait for main resources only – avoids hanging on long-poll / WS +await page.waitForLoadState('domcontentloaded') +// or: await page.waitForTimeout(500); // tiny static delayVerify that no flaky timeouts appear in CI before merging.
src/run.ts (1)
43-48:passis ended before being reused
pass.end()finalises the render pass; passing the endedpassobject topickManager.renderis undefined behaviour – the spec forbids using a pass afterend().If
pickManager.renderonly needs the colour attachment texture, pass the texture view instead, or restructure:// 1️⃣ main colour pass const colourPass = encoder.beginRenderPass(descriptor) // draw… colourPass.end() // 2️⃣ pick pass (separate) pickManager.render(encoder, matrix, /* draw fn */)Please verify that no validation errors appear in the browser console.
src/WebGPU/programs/initPrograms.ts (1)
30-31: Order of initialisationIf any of the shader programs depend on
pickTexture’s existence (e.g. sharing bind-group layouts) and assume the variables are initialised top-to-bottom, be aware that TypeScript hoistsletdeclarations but not initialisations. Your current order is fine, just make sure no earlier program usespickTextureduring its own factory call.src/WebGPU/pointer.ts (3)
1-2: Export of mutable pointer object looks goodThe exported pointer object with x and y coordinates provides a clean way for other modules to access mouse position data.
3-6: Initialization looks goodGood practice to reset the pointer coordinates when initializing the controller.
10-14: Mouse movement tracking looks goodCorrectly calculates pointer coordinates relative to the canvas position.
crate/src/line.rs (3)
1-4: Good use of the HasCoords traitNice import of the HasCoords trait to make the Line implementation work with different point types.
12-19: Issue with line cap calculationsThe calculation of the line extension points (ax, ay, bx, by) uses the parallel angle's cos/sin directly but is also subtracting/adding half_width. This might result in incorrect line caps, especially for short lines.
let half_width = width / 2.0; let parallel_angle = (point_b.y() - points_a.y()).atan2(point_b.x() - points_a.x()); let angle = parallel_angle + std::f32::consts::PI / 2.0; // perpendicular angle - let ax = points_a.x() - half_width * parallel_angle.cos(); - let ay = points_a.y() - half_width * parallel_angle.sin(); - let bx = point_b.x() + half_width * parallel_angle.cos(); - let by = point_b.y() + half_width * parallel_angle.sin(); + // No need to extend start/end points for a simple line with thickness + let ax = points_a.x(); + let ay = points_a.y(); + let bx = point_b.x(); + let by = point_b.y();Please verify that the line caps render correctly, especially for diagonal lines. Consider using a more traditional approach where you don't modify start/end points for thickness.
47-55: Vertex data flattening looks goodThe implementation correctly flattens the vertex array into a format suitable for GPU rendering, including position and color attributes.
crate/src/types.rs (3)
3-7: Good abstraction with HasCoords traitThe HasCoords trait provides a clean abstraction for accessing coordinates from different point types.
9-22: Point struct and implementation look goodThe Point struct is properly defined with x and y fields and correctly implements the HasCoords trait. The Serialize/Deserialize derivation is appropriate for data that needs to be exchanged between Rust and JavaScript.
41-41: Color type definition is clear and conciseThe Color type alias is a good way to represent RGBA values consistently throughout the codebase.
src/index.ts (6)
9-9: Import of pointer controller looks goodImporting the initMouseController function is necessary for the hover functionality.
11-15: CreatorAPI interface update with asset IDsGood update to require explicit asset IDs in both addImage and updatePoints methods, which improves API clarity and aligns with the backend changes.
42-42: Initialization of mouse controllerProperly initializes the mouse controller with the canvas element.
45-46: Asset list trackingGood addition of an assetsList array to track asset IDs, and passing it to runCreator for explicit asset management.
50-69: addImage implementation with ID supportThe addImage method now correctly accepts an ID parameter and passes it to the state.add_texture method. The asset ID is also added to the assetsList for tracking.
70-72: updatePoints implementation with ID supportThe updatePoints method now correctly uses the ID parameter to update points for a specific asset.
crate/src/texture.rs (2)
4-6: Update field visibility to match access requirements.The
idfield is private butpointsandtexture_idare public. Consider whether these visibility choices align with your encapsulation needs:
- Making
idprivate is good for encapsulation since it's only used internally for pickingpointsis public to support border drawingtexture_idis public for external texture identification
10-16: LGTM!The constructor correctly initializes the
Texturewith the necessary fields, converting theusizeID tof32for GPU compatibility.src/WebGPU/programs/drawTriangle/shader.wgsl (4)
1-8: LGTM!The Vertex and Uniforms struct definitions are well-defined with appropriate type annotations and locations for GPU attribute binding.
10-13: LGTM!The VertexOutput structure correctly passes through the vertex data from the vertex shader to the fragment shader with appropriate location decorators.
17-23: LGTM!The vertex shader function properly transforms the vertex position using the world view projection matrix and passes the color through to the fragment shader.
25-27: LGTM!The fragment shader correctly outputs the interpolated color received from the vertex shader.
src/WebGPU/programs/pickTexture/shader.wgsl (3)
1-5: LGTM!The Vertex struct is correctly defined with position, UV coordinates, and ID attributes at appropriate shader locations.
7-15: Good use of flat interpolation for the ID.The
@interpolate(flat)decorator on the ID field ensures the vertex ID is not interpolated between vertices, which is essential for accurate picking.
17-20: LGTM!Proper binding group setup for the uniforms, sampler, and texture.
src/WebGPU/pick.ts (1)
63-73: Coordinate transform may be flipped on Y – double-check
tyis computed with2 * (pointer.y / clientHeight) - 1, which mapsy = 0to-1(bottom in NDC) andy = hto+1(top).
Canvas pointer events usually have the origin at the top-left, meaning the mapping is inverted.
If you observe inverted picks on the Y-axis, invert the sign:const ty = - (2 * (pointer.y / clientHeight) - 1)
| /** =========DISPLAYS BORDER AROUND HOVERED ASSET============ */ | ||
| await page.mouse.move(200, 300) | ||
| await expect(canvas).toHaveScreenshot('after-hover.png') |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Hard-coded mouse coordinates make the test brittle
page.mouse.move(200, 300) assumes the asset to hover is always at (200, 300). Any layout change will break the test.
Recommended:
const bbox = await canvas.boundingBox()
// move to the center of the canvas (or compute the asset position)
await page.mouse.move(bbox.x + bbox.width / 2, bbox.y + bbox.height / 2)This uses DOM geometry instead of magic numbers and will survive responsive layout tweaks.
| const borderVertexData = state.get_border() | ||
| if (borderVertexData.length > 0) { | ||
| drawTriangle(pass, matrix, borderVertexData) | ||
| } |
There was a problem hiding this comment.
drawTriangle expects Float32Array – currently passing plain array
state.get_border() seems to return a JS array; drawTriangle (like drawTexture) probably expects a typed buffer. Pass a Float32Array to prevent undefined behaviour on some drivers:
-const borderVertexData = state.get_border()
+const borderVertexData = new Float32Array(state.get_border())📝 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.
| const borderVertexData = state.get_border() | |
| if (borderVertexData.length > 0) { | |
| drawTriangle(pass, matrix, borderVertexData) | |
| } | |
| const borderVertexData = new Float32Array(state.get_border()) | |
| if (borderVertexData.length > 0) { | |
| drawTriangle(pass, matrix, borderVertexData) | |
| } |
| assetsList: number[] | ||
| ) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
assetsList captured only once – newly added assets never render
Because assetsList is passed by value, the array is captured at runCreator invocation time. If the Rust backend adds or removes assets later, the render loop will not see them.
Consider querying the State each frame:
-assetsList.forEach((id) => {
+state.get_asset_ids().forEach((id) => {and drop the assetsList parameter entirely (or keep it as an optimisation cache that you refresh when state.needsRefresh is set).
Committable suggestion skipped: line range outside the PR's diff.
| export default function getProgram( | ||
| device: GPUDevice, | ||
| debugPresentationFormat: GPUTextureFormat | ||
| ) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Unused parameter detected.
The debugPresentationFormat parameter is defined but never used in the function.
export default function getProgram(
device: GPUDevice,
- debugPresentationFormat: GPUTextureFormat
+ // Remove unused parameter or use it in the function
) {Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 ESLint
[error] 7-7: 'debugPresentationFormat' is defined but never used.
(@typescript-eslint/no-unused-vars)
|
|
||
| // bind group should be pre-created and reuse instead of constantly initialized | ||
| const bindGroup = device.createBindGroup({ | ||
| layout: pipeline.getBindGroupLayout(0), | ||
| entries: [ | ||
| { binding: 0, resource: { buffer: uniformBuffer }}, | ||
| { binding: 1, resource: sampler }, | ||
| { binding: 2, resource: texture.createView() }, | ||
| ], | ||
| }) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Implement bind group optimization note.
Line 85 notes that bind groups should be pre-created and reused, but this optimization isn't implemented. Consider moving the bind group creation outside the function and implementing a caching strategy.
+ // Create bind group layout once
+ const bindGroupLayout = pipeline.getBindGroupLayout(0)
+
+ // Cache for texture view bind groups
+ const textureBindGroups = new Map<GPUTexture, GPUBindGroup>()
+
return function drawTexture(
pass: GPURenderPassEncoder,
worldProjectionMatrix: Float32Array,
vertexData: Float32Array<ArrayBufferLike>,
texture: GPUTexture,
) {
// ... vertex buffer creation code ...
- // bind group should be pre-created and reuse instead of constantly initialized
- const bindGroup = device.createBindGroup({
- layout: pipeline.getBindGroupLayout(0),
- entries: [
- { binding: 0, resource: { buffer: uniformBuffer }},
- { binding: 1, resource: sampler },
- { binding: 2, resource: texture.createView() },
- ],
- })
+ // Get or create bind group for this texture
+ let bindGroup = textureBindGroups.get(texture)
+ if (!bindGroup) {
+ bindGroup = device.createBindGroup({
+ layout: bindGroupLayout,
+ entries: [
+ { binding: 0, resource: { buffer: uniformBuffer }},
+ { binding: 1, resource: sampler },
+ { binding: 2, resource: texture.createView() },
+ ],
+ })
+ textureBindGroups.set(texture, bindGroup)
+ }🌐 Web query:
What's the best practice for caching WebGPU bind groups with dynamic texture resources?
💡 Result:
WebGPU bind groups are a crucial part of the WebGPU API that connect shader resources to actual GPU resources. When dealing with dynamic texture resources that change frequently, efficient caching becomes essential for performance.
Understanding Bind Groups and Their Limitations
Bind groups in WebGPU are immutable once created, meaning their resources cannot be changed after creation, though the contents of those resources can be updated[1]. This presents a challenge when working with dynamic resources like textures that may need to be swapped frequently.
Unlike buffers, which support dynamic offsets to change binding parameters without recreating bind groups, textures don't have an equivalent mechanism[3]. This means that when you need to swap texture resources, you typically must create new bind groups.
Efficient Caching Strategies
Bind Group Cache Implementation
One effective approach is to implement a cache for bind groups to avoid recreating them each frame. Babylon.js, for example, uses a node tree structure for their bind group cache:
class WebGPUBindGroupCacheNode {
public values: { [id: number]: WebGPUBindGroupCacheNode };
public bindGroups: GPUBindGroup[];
constructor() {
this.values = {};
}
}The cache uses resource IDs as keys, where the ID for uniform/storage buffers and textures is typically the uniqueId property of the corresponding class[2].
Optimization Techniques
-
Context-Based Optimization: If draw and material contexts haven't changed since the last cache query, return existing bind groups without traversing the cache. This works because the draw context holds uniform/storage buffers and the material context contains textures and samplers[2].
-
Linear Allocation: For highly dynamic resources, consider implementing a system similar to "dynamic bind groups" that allocate linearly and efficiently fill descriptors[3].
-
Group Index Organization: Organize your bind groups by frequency of change. Place data that changes least frequently in
@group(0)and each subsequent group should contain progressively more frequently changing data. While the API is technically agnostic to this order, underlying native APIs may have performance preferences[1].
Monitoring Cache Performance
To ensure your caching strategy is effective, monitor metrics such as:
- Total number of bind groups created
- Number of bind groups created per frame
- Number of bind group lookups per frame[2]
For optimal performance, the number of bind groups created per frame should approach zero on average.
Handling Dynamic Textures
When working with frequently changing textures (like render targets or streaming content):
-
Accept the overhead: Creating bind groups on the fly is not as expensive as you might think. The WebGPU implementation may use caching under the hood[7].
-
Resource pooling: Implement a pool of pre-created textures and bind groups that can be reused.
-
Framegraph approach: When implementing post-processing effects that create new textures every frame, implement caching policies to reduce texture/bind group creation[8].
-
Consider ImageBitmap optimization: When loading textures from external sources, use
ImageBitmapwhich decodes images off the main thread into a GPU-friendly format[6].
While there has been discussion about introducing a BindGroupArena concept to manage dynamic bind groups[7], the current best practice is to implement your own caching mechanism while understanding that the overhead of creating bind groups is designed to be relatively low in WebGPU.
By carefully organizing resources based on their update frequency and implementing an effective caching strategy, you can minimize the performance impact of working with dynamic texture resources in WebGPU.
Citations:
- 1: https://toji.dev/webgpu-best-practices/bind-groups.html
- 2: https://doc.babylonjs.com/setup/support/webGPU/webGPUInternals/webGPUCacheBindGroup
- 3: Efficient Per-Frame/Transient Bind Groups gpuweb/gpuweb#915
- 4: https://www.w3.org/TR/webgpu/
- 5: https://webgpufundamentals.org/webgpu/lessons/webgpu-bind-group-layouts.html
- 6: https://toji.dev/webgpu-best-practices/img-textures.html
- 7: How to efficiently swap bind group resources? gfx-rs/wgpu#1495
- 8: Bind group allocation and garbage collection gpuweb/gpuweb#4090
- 9: https://webgpufundamentals.org/webgpu/lessons/webgpu-textures.html
- 10: https://sotrh.github.io/learn-wgpu/beginner/tutorial5-textures/
Optimize bind group creation by caching
The WebGPU best practices confirm that bind groups are immutable and don’t support dynamic texture binding via offsets—so creating a new bind group each draw is wasteful. Move layout creation and cache initialization outside the draw function, then look up or create per-texture bind groups:
• File: src/WebGPU/programs/pickTexture/getProgram.ts
– Above the returned drawTexture function, add:
```ts
// Create bind group layout once
const bindGroupLayout = pipeline.getBindGroupLayout(0);
// Cache bind groups per texture
const textureBindGroups = new Map<GPUTexture, GPUBindGroup>();
```
• Inside drawTexture, replace the on-the-fly creation with a lookup:
- // bind group should be pre-created and reuse instead of constantly initialized
- const bindGroup = device.createBindGroup({
- layout: pipeline.getBindGroupLayout(0),
- entries: [
- { binding: 0, resource: { buffer: uniformBuffer } },
- { binding: 1, resource: sampler },
- { binding: 2, resource: texture.createView() },
- ],
- });
+ // Get or create bind group for this texture
+ let bindGroup = textureBindGroups.get(texture);
+ if (!bindGroup) {
+ bindGroup = device.createBindGroup({
+ layout: bindGroupLayout,
+ entries: [
+ { binding: 0, resource: { buffer: uniformBuffer } },
+ { binding: 1, resource: sampler },
+ { binding: 2, resource: texture.createView() },
+ ],
+ });
+ textureBindGroups.set(texture, bindGroup);
+ }This ensures bind groups are only created once per unique texture, driving your per-frame creation count toward zero.
| this.pickBuffer = device.createBuffer({ | ||
| size: NUM_PIXELS * 4, | ||
| usage: GPUBufferUsage.MAP_READ | GPUBufferUsage.COPY_DST, | ||
| }) | ||
|
|
||
| this.pickTexture = device.createTexture({ | ||
| size: [1, 1], | ||
| format: 'r32uint', | ||
| usage: GPUTextureUsage.COPY_SRC | GPUTextureUsage.RENDER_ATTACHMENT, | ||
| }) | ||
|
|
||
| this.pickDepthTexture = device.createTexture({ | ||
| size: [1, 1], | ||
| format: 'depth24plus', | ||
| usage: GPUTextureUsage.RENDER_ATTACHMENT, | ||
| }) | ||
| } |
There was a problem hiding this comment.
Critical: bytesPerRow / buffer size are calculated incorrectly – will fail GPU validation
bytesPerRow has to be a multiple of 256 bytes but must not be multiplied by 256 again.
For NUM_PIXELS = 1 the current expression produces 66304 bytes, while the buffer you create is only 4 bytes long – any copy will trigger a validation error (“destination out of bounds”).
Fix: align to 256 bytes once and allocate the buffer for the aligned size:
- this.pickBuffer = device.createBuffer({
- size: NUM_PIXELS * 4,
+ // Each row must be 256-byte aligned (WebGPU spec).
+ const bytesPerRow = Math.ceil(NUM_PIXELS * 4 / 256) * 256
+ this.pickBuffer = device.createBuffer({
+ size: bytesPerRow,
usage: GPUBufferUsage.MAP_READ | GPUBufferUsage.COPY_DST,
})
…
- encoder.copyTextureToBuffer({
- texture: this.pickTexture,
- origin: { x: 0, y: 0 }
- }, {
- buffer: this.pickBuffer,
- bytesPerRow: ((NUM_PIXELS * 4 + 255) | 0) * 256,
- rowsPerImage: 1,
- }, {
- width: NUM_PIXELS,
- })
+ encoder.copyTextureToBuffer(
+ { texture: this.pickTexture },
+ {
+ buffer: this.pickBuffer,
+ bytesPerRow,
+ rowsPerImage: 1,
+ },
+ { width: NUM_PIXELS, height: 1 }
+ )Without this change, the picking pass will never work on any compliant implementation.
Also applies to: 87-97
| assets: HashMap<usize, Texture>, | ||
| hovered_asset_id: usize, // 0 -> no asset is hovered | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Sentinel value 0 for “no hover” collides with valid asset IDs
hovered_asset_id defaults to 0, but asset IDs are arbitrary usizes – 0 is a perfectly valid ID and will never be treated as “no hover”.
Use Option<usize> instead:
- hovered_asset_id: usize, // 0 -> no asset is hovered
+ hovered_asset_id: Option<usize>,
…
- hovered_asset_id: 0,
+ hovered_asset_id: None,
…
- pub fn update_hover(&mut self, id: usize) {
- self.hovered_asset_id = id
+ pub fn update_hover(&mut self, id: usize) {
+ self.hovered_asset_id = Some(id)
}get_border will then match on self.hovered_asset_id safely.
Also applies to: 100-104
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/WebGPU/programs/pickTexture/getProgram.ts (2)
5-8:⚠️ Potential issueRemove unused parameter.
The
debugPresentationFormatparameter is defined but never used in the function. It appears to be related to the commented-out line 41 but is not currently needed.export default function getProgram( device: GPUDevice, - debugPresentationFormat: GPUTextureFormat ) {🧰 Tools
🪛 ESLint
[error] 7-7: 'debugPresentationFormat' is defined but never used.
(@typescript-eslint/no-unused-vars)
85-94: 🛠️ Refactor suggestionImplement bind group caching for better performance.
Creating a new bind group on every render call is inefficient. As noted in the TODO comment, bind groups should be pre-created and reused. This is especially important for performance in WebGPU applications.
+ // Create bind group layout once + const bindGroupLayout = pipeline.getBindGroupLayout(0); + + // Cache for texture view bind groups + const textureBindGroups = new Map<GPUTexture, GPUBindGroup>(); + return function drawTexture( pass: GPURenderPassEncoder, worldProjectionMatrix: Float32Array, vertexData: Float32Array<ArrayBufferLike>, texture: GPUTexture, ) { // ... vertex buffer creation code ... - // bind group should be pre-created and reuse instead of constantly initialized - // TODO: avoid creatign bind group on every render - const bindGroup = device.createBindGroup({ - layout: pipeline.getBindGroupLayout(0), - entries: [ - { binding: 0, resource: { buffer: uniformBuffer }}, - { binding: 1, resource: sampler }, - { binding: 2, resource: texture.createView() }, - ], - }) + // Get or create bind group for this texture + let bindGroup = textureBindGroups.get(texture); + if (!bindGroup) { + bindGroup = device.createBindGroup({ + layout: bindGroupLayout, + entries: [ + { binding: 0, resource: { buffer: uniformBuffer }}, + { binding: 1, resource: sampler }, + { binding: 2, resource: texture.createView() }, + ], + }); + textureBindGroups.set(texture, bindGroup); + }
🧹 Nitpick comments (5)
src/WebGPU/programs/pickTexture/getProgram.ts (5)
27-34: Clarify vertex buffer layout with comments.The vertex buffer layout is correctly defined but would benefit from clearer documentation about the purpose of each attribute in the picking pipeline.
buffers: [ { arrayStride: STRIDE * 4, attributes: [ - {shaderLocation: 0, offset: 0, format: 'float32x4'}, // destination position - {shaderLocation: 1, offset: 16, format: 'float32x2'}, // source position - {shaderLocation: 2, offset: 16 + 8, format: 'float32'}, // id - // {shaderLocation: 3, offset: 16 + 8, format: 'float32x3'}, // id + {shaderLocation: 0, offset: 0, format: 'float32x4'}, // destination position (x, y, width, height) + {shaderLocation: 1, offset: 16, format: 'float32x2'}, // source position (u, v) + {shaderLocation: 2, offset: 16 + 8, format: 'float32'}, // asset id for picking
32-32: Remove commented-out code.There's a commented-out line for a potential alternative representation of the ID value. Since you've settled on using float32 for the ID, this commented line can be removed.
{shaderLocation: 1, offset: 16, format: 'float32x2'}, // source position {shaderLocation: 2, offset: 16 + 8, format: 'float32'}, // id - // {shaderLocation: 3, offset: 16 + 8, format: 'float32x3'}, // id
77-82: Consider reusing vertex buffers for improved performance.Similar to the bind group issue, creating a new vertex buffer on every draw call can be inefficient. If the vertex data changes frequently, consider using a mapped buffer or double-buffering technique instead.
Since the vertex buffer creation pattern is similar to the bind group issue, you could implement a vertex buffer pool or cache as well. If the vertex data is static or changes infrequently, this optimization would be significant.
99-99: Remove commented-out unused code.This commented line about translating the world projection matrix doesn't add value and should be removed to improve code clarity.
- // const translateWorldProjMatrix = mat4.translate(worldProjectionMatrix, [])
86-86: Fix typo in TODO comment.There's a typo in the TODO comment: "creatign" should be "creating".
- // TODO: avoid creatign bind group on every render + // TODO: avoid creating bind group on every render
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
integration-tests/tests/creator.spec.ts(1 hunks)src/WebGPU/programs/pickTexture/getProgram.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- integration-tests/tests/creator.spec.ts
🧰 Additional context used
🪛 ESLint
src/WebGPU/programs/pickTexture/getProgram.ts
[error] 7-7: 'debugPresentationFormat' is defined but never used.
(@typescript-eslint/no-unused-vars)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/WebGPU/programs/drawTriangle/getProgram.ts (1)
74-74: Vertex count flooring – fix acceptedThe earlier review suggested replacing
Math.round; your update applies the fix faithfully.src/WebGPU/programs/pickTexture/getProgram.ts (2)
5-8:⚠️ Potential issueRemove unused parameter
The
debugPresentationFormatparameter is defined but never used in the function body. There is a commented-out reference to it at line 41, but the actual implementation uses a hardcoded 'r32uint' format instead.-export default function getProgram( - device: GPUDevice, - debugPresentationFormat: GPUTextureFormat -) { +export default function getProgram( + device: GPUDevice, +) {🧰 Tools
🪛 ESLint
[error] 7-7: 'debugPresentationFormat' is defined but never used.
(@typescript-eslint/no-unused-vars)
83-92: 🛠️ Refactor suggestionOptimize bind group creation by caching
The code creates a new bind group for each draw call, which is inefficient as noted by the TODO comment. Implement a caching strategy to reuse bind groups for the same textures.
+ // Create bind group layout once + const bindGroupLayout = pipeline.getBindGroupLayout(0); + + // Cache bind groups per texture + const textureBindGroups = new Map<GPUTexture, GPUBindGroup>(); + return function drawTexture( pass: GPURenderPassEncoder, worldProjectionMatrix: Float32Array, vertexData: Float32Array<ArrayBufferLike>, texture: GPUTexture, ) { // ... - // bind group should be pre-created and reuse instead of constantly initialized - // TODO: avoid creatign bind group on every render - const bindGroup = device.createBindGroup({ - layout: pipeline.getBindGroupLayout(0), - entries: [ - { binding: 0, resource: { buffer: uniformBuffer }}, - { binding: 1, resource: sampler }, - { binding: 2, resource: texture.createView() }, - ], - }) + // Get or create bind group for this texture + let bindGroup = textureBindGroups.get(texture); + if (!bindGroup) { + bindGroup = device.createBindGroup({ + layout: bindGroupLayout, + entries: [ + { binding: 0, resource: { buffer: uniformBuffer }}, + { binding: 1, resource: sampler }, + { binding: 2, resource: texture.createView() }, + ], + }); + textureBindGroups.set(texture, bindGroup); + }
🧹 Nitpick comments (13)
src/WebGPU/programs/drawTexture/getProgram.ts (2)
83-89: Avoid recreating the vertex buffer every draw callA fresh GPU buffer is allocated and uploaded on every invocation. For sprites that are rendered every frame this becomes a considerable CPU-GPU sync point and GC pressure.
Consider:-const vertexBuffer = device.createBuffer({ … }) -device.queue.writeBuffer(vertexBuffer, 0, vertexData) +// allocate once outside and reuse: +vertexBuffer.setSubData(0, vertexData) // or mapAsync-based ring bufferA small ring/double buffer with
COPY_DST | VERTEXusage is usually sufficient and keeps the draw call hot.
91-99: Bind group can be cachedBecause the layout (and the texture view, assuming the same texture instance) are constant per drawTexture call, the bind group can be created once during program initialization and reused. Creating a bind group is cheap but still shows up in profiles when done thousands of times per frame.
src/WebGPU/programs/drawTriangle/getProgram.ts (3)
4-4: Keep STRIDE comment in sync with layout
const STRIDE = 4 + 4matches the twofloat32x4attributes, but the trailing comment (// + 1 + 1 + 4) is now misleading. Removing or updating it avoids confusion when the format evolves.
74-80: Reuse vertex buffer to cut per-frame allocationsSame remark as for
drawTexture: allocating a brand-new vertex buffer per border is unnecessary. A transient ring buffer or a device-persistent buffer updated viaqueue.writeBufferwill remove this allocation hotspot.
82-89: Bind group creation every frameOnly the uniform buffer changes, not its binding. Cache the bind group once during
getProgramto shave off WebGPU object churn.src/WebGPU/pick.ts (2)
29-33: Dead depth texture – remove or wire-up
pickDepthTextureis created but never referenced (depth attachment is commented out). This allocates GPU memory for no benefit. Either supply it indepthStencilAttachmentor delete the creation.
42-54: UnuseddebugRenderPassparameter triggers linter
debugRenderPassis declared but not used, causing an ESLint error (@typescript-eslint/no-unused-vars). If future debugging is planned, prefix the name with_or gate it behind a build flag; otherwise remove the parameter.src/WebGPU/programs/pickTexture/getProgram.ts (6)
28-33: Clean up commented codeThere's a commented out shader location (line 32) that appears to be an alternative implementation or previous version of the ID attribute. Since you're now using a single float for the ID at location 2, this comment should be removed to avoid confusion.
{shaderLocation: 0, offset: 0, format: 'float32x4'}, // destination position {shaderLocation: 1, offset: 16, format: 'float32x2'}, // source position {shaderLocation: 2, offset: 16 + 8, format: 'float32'}, // id - // {shaderLocation: 3, offset: 16 + 8, format: 'float32x3'}, // id
41-43: Clean up commented code and document format choiceThe commented-out format line suggests you previously considered using the debugging format but decided on 'r32uint' instead. Consider adding a comment explaining why 'r32uint' is the appropriate format for the picking texture.
entryPoint: 'fs', targets: [{ - // format: debugPresentationFormat, + // Using r32uint to store asset IDs as unsigned integers format: 'r32uint', }],
45-52: Remove or document commented codeThe commented out primitive and depth stencil configurations suggest this code might be incomplete or experimental. If these settings are unnecessary for the picking pipeline, remove them. If they might be needed in the future, document why they're commented out.
}, - // primitive: { - // cullMode: 'back', - // }, - // depthStencil: { - // depthWriteEnabled: true, - // depthCompare: 'less', - // format: 'depth24plus', - // },
74-80: Consider reusing vertex buffersCreating a new vertex buffer for each draw call can be expensive. Consider implementing a buffer pooling strategy or moving the buffer creation outside the draw function for cases where the same vertex data is used multiple times.
97-97: Remove or implement commented codeThis commented line about translating the projection matrix should either be implemented or removed if not needed.
- // const translateWorldProjMatrix = mat4.translate(worldProjectionMatrix, [])
1-4: Add documentation about picking functionalityThis file implements GPU-based picking functionality which is a crucial part of the asset hover feature, but lacks documentation explaining how it works. Consider adding comments to explain the picking system's purpose, how it uses the r32uint format to store IDs, and how it integrates with the hover functionality.
+/** + * This module provides a WebGPU rendering pipeline for asset picking functionality. + * It renders textures with asset IDs encoded as r32uint values, which can then be + * read back to determine which asset is under the cursor. + * + * The picking system works by: + * 1. Rendering assets with their IDs to an offscreen texture + * 2. Reading back the pixel value under the cursor + * 3. Using that ID to determine the hovered asset + */ import shaderCode from "./shader.wgsl" const STRIDE = 4 + 2 + 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/WebGPU/pick.ts(1 hunks)src/WebGPU/programs/drawTexture/getProgram.ts(1 hunks)src/WebGPU/programs/drawTriangle/getProgram.ts(1 hunks)src/WebGPU/programs/pickTexture/getProgram.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/WebGPU/programs/drawTriangle/getProgram.ts (5)
src/WebGPU/programs/drawTexture/getProgram.ts (1)
getProgram(6-112)src/WebGPU/programs/pickTexture/getProgram.ts (1)
getProgram(5-105)src/WebGPU/programs/draw3dModel/getProgram.ts (1)
getProgram(3-97)src/WebGPU/programs/draw3dModelTexture/getProgram.ts (1)
getProgram(3-99)src/WebGPU/programs/initPrograms.ts (1)
drawTriangle(10-10)
src/WebGPU/pick.ts (1)
src/WebGPU/pointer.ts (1)
pointer(1-1)
🪛 ESLint
src/WebGPU/pick.ts
[error] 40-40: 'debugRenderPass' is defined but never used.
(@typescript-eslint/no-unused-vars)
src/WebGPU/programs/pickTexture/getProgram.ts
[error] 7-7: 'debugPresentationFormat' is defined but never used.
(@typescript-eslint/no-unused-vars)
🔇 Additional comments (3)
src/WebGPU/programs/drawTexture/getProgram.ts (1)
82-82: Vertex count flooring implemented correctlyReplacing
Math.round()with| 0deterministically floors the division result and prevents the off-by-one issue we discussed earlier – nice follow-up!src/WebGPU/programs/drawTriangle/getProgram.ts (1)
23-27: Attribute offsets look correct – just verify colour packingPosition (16 B) followed by colour (16 B) inside a 32 B stride lines up nicely; just ensure the shader expects a
vec4<f32>colour.src/WebGPU/pick.ts (1)
87-94:bytesPerRowomitted – verify on all implementationsYou rely on the special-case “1 × 1 copy” path where
bytesPerRowmay be optional. Unfortunately, some adapters (notably older Dawn-based Chrome builds) still require an explicit, 256-aligned value. Safer variant:-encoder.copyTextureToBuffer({ texture: this.pickTexture }, { buffer: this.pickBuffer }, { width: 1, height: 1 }) +const bytesPerRow = 256 // 1 pixel × 4 bytes, rounded up to 256 +encoder.copyTextureToBuffer( + { texture: this.pickTexture }, + { buffer: this.pickBuffer, bytesPerRow, rowsPerImage: 1 }, + { width: 1, height: 1 } +)Please test on Chromium/Edge + Firefox to confirm.
| async asyncPick(state: State) { | ||
| if (!this.isPreviousDone) return | ||
| this.isPreviousDone = false | ||
| await this.pickBuffer.mapAsync(GPUMapMode.READ, 0, 4 * NUM_PIXELS) | ||
| const [id] = new Uint32Array(this.pickBuffer.getMappedRange(0, 4 * NUM_PIXELS)) | ||
| state.update_hover(id) | ||
| this.pickBuffer.unmap() | ||
| this.isPreviousDone = true | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle mapping failures and race conditions
mapAsync may reject (device lost, validation error). If that happens isPreviousDone stays false, freezing picking forever. Wrap in try/finally:
this.isPreviousDone = false
try {
await this.pickBuffer.mapAsync(GPUMapMode.READ, 0, 4)
…
} finally {
this.pickBuffer.unmap()
this.isPreviousDone = true
}Also consider using queue.onSubmittedWorkDone() to ensure the copy has finished before mapping, preventing undefined behaviour on slower GPUs.
|
🎉 This PR is included in version 0.1.0-next.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Chores