feat: support Gaussian blur for svg #164
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughExposes presentation and storage formats from GPU device acquisition, threads presentationFormat through texture/program APIs, centralizes canvas matrix and deferred GPU-object destruction, introduces a cache-based Gaussian blur pipeline with cache texture management, expands Zig/TS shape APIs for sdf vs cache textures and opacity/filters, and removes obsolete utilities. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App
participant Run as run.ts
participant Logic as logic
participant Cache as textureCache
participant Progs as initPrograms / draw programs
participant Blur as blur program
participant GPU as WebGPU
App->>Run: frame tick
Run->>GPU: createCommandEncoder("draw canvas main encoder")
Run->>Logic: updateCache()
Logic->>Cache: startCache(device, encoder, texId, bbox, w, h)
Cache->>Progs: swap canvasMatrix.buffer -> cache matrix
Cache->>GPU: beginRenderPass write SDF -> cache texture
Cache-->>Logic: endCache() (restore canvasMatrix.buffer)
Logic->>Progs: draw_blur(texId, iterations, filterSizes, sigmas) using encoder
Progs->>Blur: dispatch compute passes (X then Y) storing into cache texture
Blur->>GPU: compute X/Y passes (storage textures)
Run->>Progs: render scene using same encoder (canvasMatrix.buffer)
Progs->>GPU: render passes to swapchain
Run->>Progs: enqueue destroyGpuObjects() (delayedDestroy processed)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches
🧪 Generate unit tests
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 |
There was a problem hiding this comment.
Pull Request Overview
This PR adds Gaussian blur filter support for SVG rendering, implementing a comprehensive blur pipeline that includes SVG parsing, GPU-based blur computation, and texture caching optimizations.
- Adds SVG filter parsing to extract
stdDeviationvalues from<feGaussianBlur>elements - Implements multi-pass Gaussian blur using WebGPU compute shaders with dynamic iteration control
- Introduces texture caching system to render blurred shapes efficiently with proper cost controls
Reviewed Changes
Copilot reviewed 36 out of 43 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/textures.ts | Updated texture loading to support presentation format and added console logging for SVG definitions |
| src/svgToShapes/utils.ts | Added filter parsing logic and enhanced color parsing with alpha override support |
| src/svgToShapes/index.ts | Integrated filter properties into shape rendering with opacity and blur support |
| src/svgToShapes/definitions.ts | Extended definitions to include filter types and blur parameters |
| src/logic/index.zig | Implemented cache update system and blur rendering with cost control algorithms |
| src/WebGPU/programs/blur/getProgram.ts | Created multi-pass Gaussian blur implementation with separable kernel optimization |
| src/WebGPU/textureCache.ts | Enhanced texture caching system to support blur rendering workflow |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
src/logic/index.zig
Outdated
| // Verify new cost | ||
| const new_pixels = size.w * size.h; | ||
| const new_cost = 3 * sigma_x * new_pixels + 3 * sigma_y * new_pixels; | ||
| std.debug.print("prev cost: {d}\n new cost: {d}\n target: {d}\n", .{ cost, new_cost, MAX_COST }); |
There was a problem hiding this comment.
Debug print statement should be removed from production code. This appears to be leftover debugging code that could impact performance and clutter output.
| std.debug.print("prev cost: {d}\n new cost: {d}\n target: {d}\n", .{ cost, new_cost, MAX_COST }); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (12)
src/getDepthTexture.ts (1)
3-15: Recreate depth texture on device change to avoid invalid handle after device loss.If a new GPUDevice is created after a device-lost event, this module may return a texture from the old device. Track the device and recreate when it changes.
Apply:
-let depthTexture: GPUTexture | undefined +let depthTexture: GPUTexture | undefined +let depthTextureDevice: GPUDevice | undefined export default function getDepthTexture(device: GPUDevice, width: number, height: number) { - if (!depthTexture || depthTexture.width !== width || depthTexture.height !== height) { + if ( + !depthTexture || + depthTextureDevice !== device || + depthTexture.width !== width || + depthTexture.height !== height + ) { depthTexture?.destroy() depthTexture = device.createTexture({ label: 'depth texture', size: [width, height], format: 'depth24plus', usage: GPUTextureUsage.RENDER_ATTACHMENT, }) + depthTextureDevice = device }src/svgToShapes/utils.ts (2)
12-32: Fix alpha handling in parseColor (don’t scale RGB by overrideAlpha)
overrideAlphashould modulate the alpha channel, not darken RGB. Current code produces visually incorrect colors and double-premultiplication risks.-export function parseColor(cssColor: string, overrideAlpha = 1): Color { +export function parseColor(cssColor: string, overrideAlpha = 1): Color { // Create a temporary canvas element - const canvas = new OffscreenCanvas(1, 1) - const ctx = canvas.getContext('2d')! + const canvas = + typeof OffscreenCanvas !== 'undefined' + ? new OffscreenCanvas(1, 1) + : (() => { + const c = document.createElement('canvas') + c.width = 1 + c.height = 1 + return c + })() + const ctx = (canvas as OffscreenCanvas | HTMLCanvasElement).getContext('2d')! @@ - const [r, g, b, a] = imageData.data + const [r, g, b, a] = imageData.data @@ - return [ - (r / 255) * overrideAlpha, // red - (g / 255) * overrideAlpha, // green - (b / 255) * overrideAlpha, // blue - (a / 255) * overrideAlpha, // alpha - ] + const alpha = (a / 255) * overrideAlpha + return [r / 255, g / 255, b / 255, alpha]Optional: If your pipeline expects premultiplied colors, compute
rgb * alphaexplicitly after the change above.
87-101: Gradient stops: support percentages and combine alpha with stop-opacity
- Offsets like
"50%"currently becomeNaN.- Alpha from
stop-coloris overwritten rather than combined withstop-opacity.-function getGradientStops(nodes: Array<string | Node>) { - return nodes.map((stop) => { - if (typeof stop === 'string' || stop.type !== 'element') - return { offset: 0, color: [0, 0, 0, 0] as Color } - const stopProps = getProps(stop) - const color = parseColor(String(stopProps['stop-color'] ?? '#000')) - color[3] = ensureNumber(stopProps['stop-opacity'], 1) - - return { - offset: Number(stopProps.offset ?? 0), - color, - } - }) -} +function getGradientStops(nodes: Array<string | Node>) { + return nodes + .filter( + (n): n is ElementNode => + typeof n !== 'string' && n.type === 'element' && n.tagName === 'stop' + ) + .map((stop) => { + const stopProps = getProps(stop) + const op = ensureNumber(stopProps['stop-opacity'], 1) + const color = parseColor(String(stopProps['stop-color'] ?? '#000'), op) + let offset = 0 + const raw = stopProps.offset + if (typeof raw === 'string' && raw.trim().endsWith('%')) { + const v = parseFloat(raw) + offset = isNaN(v) ? 0 : v / 100 + } else { + offset = ensureNumber(raw, 0) + } + // clamp to [0,1] + offset = Math.max(0, Math.min(1, offset)) + return { offset, color } + }) +}src/svgToShapes/definitions.ts (1)
84-126: Bug:insideDefsleaks across siblings; siblings after a<defs>are treated as inside<defs>.Mutating the outer
insideDefsinside the loop makes it sticky. Compute a per-child flag instead.Apply:
- node.children.forEach((child) => { + for (const child of node.children) { if (typeof child === 'string') return if (child.type !== 'element') return - const props = getProps(child) + const props = getProps(child as any) + const nextInsideDefs = insideDefs || child.tagName === 'defs' switch (child.tagName) { ... - case 'defs': { - insideDefs = true - break - } + case 'defs': { + // do not mutate outer flag; recurse with nextInsideDefs below + break + } } - collect(child, defs, insideDefs) - }) + collect(child, defs, nextInsideDefs) + }src/WebGPU/programs/pickShape/getProgram.ts (1)
57-63: Destroy the vertex buffer to avoid leaks.
Align with the new centralized destruction pattern.device.queue.writeBuffer(vertexBuffer, 0, vertexData) + addDestroyBuf(vertexBuffer)src/textures.ts (1)
74-92: Add error handling and balanced counters for image/fetch failures.Failures (img.onerror, fetch reject, CORS) currently leave loadingTextures stuck > 0 and unhandled rejections.
I can rewrite add() with a try/catch/finally pattern, or minimally add a catch that decrements and updates. Example (outside this diff for clarity):
// robust getImageWithDetails async function getImageWithDetails(url: string): Promise<[HTMLImageElement, RootNode | null]> { const img = new Image() img.crossOrigin = 'anonymous' const imgP = new Promise<HTMLImageElement>((resolve, reject) => { img.onload = () => resolve(img) img.onerror = (e) => reject(new Error(`Image load failed: ${url}`)) }) img.src = url const svgP = fetch(url) .then((r) => r.text()) .then((t) => (t.includes('<svg') ? parse(t) : null)) .catch(() => null) // non-fatal for raster return Promise.all([imgP, svgP]) }And in add():
- wrap in try/catch/finally, ensure loadingTextures-- and updateProcessing() in finally.
Also applies to: 109-112
src/WebGPU/textureCache.ts (2)
27-35: Leak on cache texture reallocation (old texture not destroyed).When dimensions change, the previous cache texture is left alive. Re-enable destruction to avoid GPU memory leaks.
Apply:
- if (!canReuseTexture) { - // texture?.destroy() + if (!canReuseTexture) { + // Destroy previous cache texture before reallocating + texture?.destroy() texture = device.createTexture({ label: 'texture cache', format: presentationFormat,
27-33: Type/metadata mismatch: GPUTexture lacks width/height/format but downstream blur uses them.Ensure the cached texture carries width/height/format metadata expected by blur (and any reuse logic).
Apply:
- if (!canReuseTexture) { + if (!canReuseTexture) { texture = device.createTexture({ label: 'texture cache', format: presentationFormat, usage: GPUTextureUsage.RENDER_ATTACHMENT | GPUTextureUsage.TEXTURE_BINDING | GPUTextureUsage.STORAGE_BINDING, size: [outputWidth, outputHeight], }) } @@ - Textures.setCacheTexture(textureId, texture) + // Augment with metadata needed by compute/draw paths. + const cacheTexture = Object.assign(texture, { + width: outputWidth, + height: outputHeight, + format: presentationFormat, + }) as GPUTexture & { width: number; height: number; format: GPUTextureFormat } + Textures.setCacheTexture(textureId, cacheTexture)Also applies to: 94-95
src/svgToShapes/index.ts (2)
69-72: Guard against zero-size bounding boxes in linear gradients.Division by zero if
bbWidthorbbHeightis 0 (thin shapes).Apply this diff:
- p1.x = (p1.x - boundingBox.min_x) / bbWidth - p1.y = 1 - (p1.y - boundingBox.min_y) / bbHeight - p2.x = (p2.x - boundingBox.min_x) / bbWidth - p2.y = 1 - (p2.y - boundingBox.min_y) / bbHeight + const invW = 1 / Math.max(bbWidth, 1e-8) + const invH = 1 / Math.max(bbHeight, 1e-8) + p1.x = (p1.x - boundingBox.min_x) * invW + p1.y = 1 - (p1.y - boundingBox.min_y) * invH + p2.x = (p2.x - boundingBox.min_x) * invW + p2.y = 1 - (p2.y - boundingBox.min_y) * invH
190-199: Gradient normalization must account for transforms—compute bounding box on transformed paths
Currently,getBoundingBox(paths)runs on the raw paths before applying anycurrTransform, so gradients normalize against the untransformed shape and will be misaligned whenever<g>/element transforms exist. Move the transform parsing (includingprops.transformand inherited group transforms) to before the bounding-box calculation, compute the box on those transformed (but pre-flip) paths, then proceed with the Y-flip and rendering.
Applies in src/svgToShapes/index.ts around lines 190–199, 252–260 and 262–266.src/logic/index.zig (2)
25-35: Signature mismatch: draw_shape expects 2 args, calls pass 3 (bounds, uniform, texture).This will not compile.
draw_shapemust acceptshapes.Uniformor the callers must drop it.Apply this diff to align the function pointer type with call sites:
const WebGpuPrograms = struct { draw_texture: *const fn (images.DrawVertex, u32) void, draw_triangle: *const fn ([]const Triangle.DrawInstance) void, compute_shape: *const fn ([]const types.Point, f32, f32, u32) void, draw_blur: *const fn (u32, u32, u32, u32, f32, f32) void, - draw_shape: *const fn ([]const types.PointUV, u32) void, + draw_shape: *const fn ([]const types.PointUV, shapes.Uniform, u32) void, draw_msdf: *const fn ([]const Msdf.DrawInstance, u32) void, pick_texture: *const fn ([]const images.PickVertex, u32) void, pick_triangle: *const fn ([]const Triangle.PickInstance) void, pick_shape: *const fn ([]const images.PickVertex, f32, u32) void, };Ensure the JS/WGSL side matches this signature.
656-779: Float vs int mix in @max/@ceil causes type errors; use f32 literals.
@max(1, @ceil(...))mixes comptime_int and f32.Apply this diff:
- const iterations = @max(1, @ceil(maxSigma / maxSigmaPerPass)); + const iterations = @max(@as(f32, 1.0), @ceil(maxSigma / maxSigmaPerPass)); ... - const filter_size_per_pass_x = @max(1, @ceil(factor * sigma_per_pass_x)); - const filter_size_per_pass_y = @max(1, @ceil(factor * sigma_per_pass_y)); + const filter_size_per_pass_x = @max(@as(f32, 1.0), @ceil(factor * sigma_per_pass_x)); + const filter_size_per_pass_y = @max(@as(f32, 1.0), @ceil(factor * sigma_per_pass_y));
🧹 Nitpick comments (54)
src/svgToShapes/boundingBox.ts (1)
12-19: AmbientPointis declared globally; import is optional
Pointis defined as a global interface insrc/logic/index.d.ts, so this file already type-checks. For clarity and to avoid hidden globals, you may optionally importPointfrom your shared types module.src/WebGPU/programs/drawTexture/shader.wgsl (1)
29-33: Drop commented-out debug code or gate it via a build-time flag.Keeps shader lean and avoids confusion during future edits.
src/getMultisampleTexture.ts (1)
19-19: Annotate return type to avoidGPUTexture | undefinedinferenceWithout an explicit return type, TS may infer
GPUTexture | undefined. Declare: GPUTexture.-export default function getMultisampleTexture( +export default function getMultisampleTexture( device: GPUDevice, width: number, height: number, format: GPUTextureFormat, texture?: GPUTexture -) { +): GPUTexture {src/svgToShapes/utils.ts (2)
68-75: Filter handling: single-child constraint—confirm intended scopeLimiting to exactly one primitive is acceptable for v1 focused on Gaussian blur. If multiple primitives exist, current logic silently ignores extras. Flagging for awareness.
173-197: Gaussian blur parsing looks correct; minor robustness nits
- Covers numeric and space/comma-separated strings; duplicates single value -> [sx, sx].
- Consider early-returning on unsupported primitives to avoid mutating
def.type. Not blocking.src/WebGPU/getTexture/index.ts (2)
112-120: Propagating presentationFormat and adding label—LGTM; consider depth/array layersIf callers pass
options.depthOrArrayLayers > 1, the created texture is still 2D (depth=1). Either document that this function creates 2D-only textures or plumb the option into the descriptor size.Example tweak:
- const texture = device.createTexture({ + const texture = device.createTexture({ label: 'createTextureFromSource', format: presentationFormat, - mipLevelCount: options.mips ? numMipLevels(source.width, source.height) : 1, - size: [source.width, source.height], + mipLevelCount: options.mips ? numMipLevels(source.width, source.height) : 1, + size: [source.width, source.height, options.depthOrArrayLayers ?? 1],
75-89: Typo: textue2dArray → texture2dArray (param name only)Pure readability nit; doesn’t affect API surface outside this file.
-export function attachSlice( +export function attachSlice( device: GPUDevice, - textue2dArray: GPUTexture, + texture2dArray: GPUTexture, width: number, height: number, source: GPUCopyExternalImageSource, sliceIndex: number, options: Options = {} ) { device.queue.copyExternalImageToTexture( { source }, - { texture: textue2dArray, origin: { z: sliceIndex }, mipLevel: 0 }, + { texture: texture2dArray, origin: { z: sliceIndex }, mipLevel: 0 }, { width, height } )src/svgToShapes/definitions.ts (1)
9-13: Typos in comment.“Instea” → “instead”. Extra comma after
'id'.-// so it's a way around to use this instea of -// Except<string, 'paths' | 'stops' | 'id', | 'href'> +// so this is a way around to use instead of +// Except<string, 'paths' | 'stops' | 'id' | 'href'>src/WebGPU/programs/initPrograms.ts (4)
34-36: Fail-fast init: currentnull as unknown as GPUBufferwon’t “throw”; add a getter that throws until set.Safer than a null-cast, and surfaces misuse early.
-export const canvasMatrix: { - buffer: GPUBuffer -} = { buffer: null as unknown as GPUBuffer } // should throw error when used before assigning +let __canvasMatrixBuf: GPUBuffer | null = null +export const canvasMatrix = { + get buffer(): GPUBuffer { + if (!__canvasMatrixBuf) throw new Error('canvasMatrix.buffer not initialized') + return __canvasMatrixBuf + }, + set buffer(b: GPUBuffer) { + __canvasMatrixBuf = b + }, +}
51-56: Also enqueue long-lived uniform buffers for destruction on teardown.Otherwise repeated init/teardown can leak GPU buffers.
canvasMatrix.buffer = device.createBuffer({ label: 'uniforms', size: 16 /*projection matrix*/ * 4, usage: GPUBufferUsage.UNIFORM | GPUBufferUsage.COPY_DST, }) +addDestroyBuf(canvasMatrix.buffer)
57-62: Same forpickCanvasMatrixBuffer.pickCanvasMatrixBuffer = device.createBuffer({ label: 'uniforms', size: 16 /*projection matrix*/ * 4, usage: GPUBufferUsage.UNIFORM | GPUBufferUsage.COPY_DST, }) +addDestroyBuf(pickCanvasMatrixBuffer)
63-104: Program init signature updates LGTM.Switch to
(device, presentationFormat)and centralized destruction aligns modules and simplifies call sites.Ensure
destroyBufs()is called on canvas unmount and ondevice.losthandling to avoid leaks.src/WebGPU/programs/drawGaussianBlur/shader.wgsl (3)
55-78: Consider a separable 2-pass blur for O(r) instead of O(r^2).This fragment does a full 2D kernel. If performance becomes an issue, render two passes (horizontal then vertical) with a 1D kernel per pass. You already have a compute-path Gaussian; aligning both paths would simplify tuning.
7-11: Clean up duplicate/ambiguous comments and document units.
- Duplicate line comment around texStart.
- Clarify that stdDeviation is in pixels (not UV).
71-73: Alpha correctness: ensure textures are premultiplied or handle alpha separately.A straight RGBA blur can halo on transparent edges if data isn’t premultiplied. Confirm the pipeline uses PMA or switch to premultiplied math.
src/WebGPU/programs/blur/index.wgsl (2)
71-75: Out-of-bounds write protection (lower bounds).You check upper bounds only. Add a non-negative check for robustness on edge tiles.
- if (center >= filterOffset && - center < 128 - filterOffset && - all(writeIndex < dims)) { + if (center >= filterOffset && + center < 128 - filterOffset && + all(writeIndex < dims) && + all(writeIndex >= vec2<i32>(0))) {
2-5: Fix misleading comment on filterDim.“x for horizontal, y for vertical” doesn’t apply; direction is controlled by u.flip. Reword to “length of the 1D kernel (tap count).”
src/WebGPU/pick.ts (1)
26-31: Avoid creating an unused depth texture.
Depth attachment is commented out; the texture is allocated but never bound. Lazily create it only when depth is enabled.- this.pickDepthTexture = device.createTexture({ - label: 'picking texture depth', - size: [1, 1], - format: 'depth24plus', - usage: GPUTextureUsage.RENDER_ATTACHMENT, - }) + // Create on demand when depth is actually used + // this.pickDepthTexture = device.createTexture({ ... })src/WebGPU/programs/drawShape/base.wgsl (1)
64-68: Drop debug-only commented code from shader.
Keeps shaders lean; rely on a separate debug variant instead.- // let result = vec4f(fill_color.rgb, fill_color.a * fill_alpha); - // if (result.a < 0.5) { - // return vec4f(1, 0, 0, 1); - // } - // return vec4f(result.rgb, 1);src/WebGPU/programs/pickShape/getProgram.ts (2)
4-4: Naming/labels/comments drift from “pick”.
Function/export name and labels still say drawShape; comments mention cache-by-texture. Tighten for clarity.-export default function getDrawShape(device: GPUDevice, matrixBuffer: GPUBuffer) { +export default function getPickShape(device: GPUDevice, matrixBuffer: GPUBuffer) { @@ - const uniformBuffer = device.createBuffer({ - label: 'drawShape uniforms', + const uniformBuffer = device.createBuffer({ + label: 'pickShape uniforms', @@ - const vertexBuffer = device.createBuffer({ - label: 'pick texture - vertex buffer', + const vertexBuffer = device.createBuffer({ + label: 'pickShape vertex buffer', @@ - // Get or create bind group for this texture + // Create bind group for this drawNote: If you rename the export, update imports accordingly.
Also applies to: 15-15, 49-55, 58-61, 64-72
4-4: Optional: align with global canvasMatrix like other programs.
Reduces API surface and init variance; pass fewer params.-import { addDestroyBuf } from '../initPrograms' +import { addDestroyBuf, canvasMatrix } from '../initPrograms' @@ -export default function getPickShape(device: GPUDevice, matrixBuffer: GPUBuffer) { +export default function getPickShape(device: GPUDevice) { @@ - { binding: 0, resource: { buffer: matrixBuffer } }, + { binding: 0, resource: { buffer: canvasMatrix.buffer } },src/WebGPU/programs/drawTriangle/getProgram.ts (1)
5-5: Typos in comments.
“positon” → “position”, “roudned” → “rounded”.- 4 * 3 /* positon */ + 1 /* color */ + 3 /* value of roudned corner for each of three positions */ + 4 * 3 /* position */ + 1 /* color */ + 3 /* value of rounded corner for each of three positions */Also applies to: 27-28
src/WebGPU/programs/drawGaussianBlur/getProgram.ts (3)
10-11: Align labels with the program purpose.Rename labels from “texture” to “gaussian blur” for clarity in GPU debuggers.
- label: 'draw texture module', + label: 'draw gaussian blur module', @@ - label: 'draw texture pipeline', + label: 'draw gaussian blur pipeline',Also applies to: 20-21
61-66: Rename the returned function for clarity.The closure name shows up in stacks/profilers; make it reflect the effect.
-return function drawTexture( +return function drawGaussianBlur(
69-76: Avoid per-draw vertex buffer allocations.Consider reusing a transient ring buffer (per-frame scratch) to cut GPU allocations and GC pressure in hot paths. I can sketch a small upload helper if helpful.
src/getCanvasRenderDescriptor.ts (1)
3-3: Module-scoped multisampleTexture can collide across canvases/devices.If multiple canvases/devices exist, this singleton may cross-contaminate state. Consider a Map keyed by device (and format) or pass the msaa texture back to the caller for storage.
Also applies to: 17-35
src/textures.ts (2)
102-104: Channel-order risk for content textures when using presentationFormat.If presentationFormat is 'bgra8unorm', copying CPU sources or sampling expectations can be surprising. Prefer 'rgba8unorm' for content textures unless you deliberately store BGRA.
- textures[textureId].texture = createTextureFromSource(device, presentationFormat, img, { + // Prefer RGBA for content textures; keep BGRA for swapchain targets. + textures[textureId].texture = createTextureFromSource(device, 'rgba8unorm', img, { flipY: true, })Would you like me to propagate this change and adjust createTextureFromSource’s default accordingly?
82-82: Remove stray debug log.console.log(defs) is noisy in production.
- console.log(defs)src/WebGPU/textureCache.ts (1)
28-31: Use strict equality for integer dimension checks.Number.EPSILON adds no value for integer texture sizes and harms readability.
Apply:
- const canReuseTexture = - texture && - Math.abs(texture.width - outputWidth) <= Number.EPSILON && - Math.abs(texture.height - outputHeight) <= Number.EPSILON + const canReuseTexture = + !!texture && texture.width === outputWidth && texture.height === outputHeightsrc/index.ts (2)
271-273: Preserve explicit textureId 0 if it’s meaningful.Same reasoning: prefer '??' to avoid dropping 0.
Apply:
- texture_id: asset.textureId || Textures.add(asset.url, presentationFormat), // if we got points, so we have url on the server for sure + texture_id: asset.textureId ?? Textures.add(asset.url, presentationFormat), // if we got points, so we have url on the server for sure
276-301: Promise resolution path is fine; minor typos in comments.Consider fixing typos (“liek”, “shaoes”) when you touch this next.
Apply:
- points: getDefaultPoints(width, height, projectWidth, projectHeight), // TODO: do it in zig only liek for shaoes + points: getDefaultPoints(width, height, projectWidth, projectHeight), // TODO: do it in Zig only like for shapessrc/logic/texture_size.zig (3)
11-23: Avoid repeated multiplications and prefer pixel-count based scaling.Minor clarity/perf: compute pixels once and reuse; functionally identical but easier to reason about limits.
pub fn get_sdf_size(bounds: [4]PointUV) TextureSize { var size = get_size(bounds); - const sdf_texture_size = size.w * size.h * 16; + const pixels = size.w * size.h; + const sdf_texture_size = pixels * 16.0; if (sdf_texture_size > shared.max_buffer_size) { - const max_pixels = shared.max_buffer_size / 16.0; - const ratio = max_pixels / (size.w * size.h); + const max_pixels = shared.max_buffer_size / 16.0; + const ratio = max_pixels / pixels; size.w *= ratio; size.h *= ratio; } return size; }
44-51: Name says “RADIUS” but the value compares to full filter size.
filter_size_maxis a full kernel size (odd integer), not a radius. Consider renaming to avoid confusion and document the upstream WGSL/kernel limit it corresponds to.-const MAX_FILTER_RADIUS = 128.0 - 2.0; // 2.0 just to avoid being on the edge of floating -// point precision during later computations +// Full kernel size cap (leave headroom to avoid precision/ceil edge cases). +// Keep in sync with the WGSL blur kernel limit. +const MAX_FILTER_SIZE = 128.0 - 2.0; @@ - if (filter_size_max > MAX_FILTER_RADIUS) { - const ratio = MAX_FILTER_RADIUS / filter_size_max; + if (filter_size_max > MAX_FILTER_SIZE) { + const ratio = MAX_FILTER_SIZE / filter_size_max;
67-76: Pad radius should be rounded up to whole pixels.Padding usually needs integer pixels; returning fractional radii risks under-padding after flooring elsewhere.
pub fn get_blur_radius_pixels(blur_size: Point, scale: f32) Point { const sigma_x = blur_size.x * scale; const sigma_y = blur_size.y * scale; @@ - const radius_x = 3.1 * sigma_x; - const radius_y = 3.1 * sigma_y; + const radius_x = @ceil(3.1 * sigma_x); + const radius_y = @ceil(3.1 * sigma_y); return .{ .x = radius_x, .y = radius_y }; }src/logic/index.d.ts (5)
20-21: Make Color immutable to prevent accidental mutation.-type Color = [number, number, number, number] +type Color = readonly [number, number, number, number]
42-46: Good discriminated unions for fill/stroke; add docs for filter/opacity semantics.Short JSDoc on units (opacity 0..1, gaussianBlur in canvas px) will reduce misuse across callers.
73-75: Input shape ids: allow null cache id but not undefined.TS callers sometimes pass
undefined. If that’s undesirable, keepnull | number; otherwise considernumber | null | undefinedfor ergonomics and guard inside Zig.
128-136: draw_blur contract: document integer/odd constraints.
iterationsshould be a small positive integer.filterSizePerPass{X,Y}should likely be odd integers and ≤ kernel cap.
Add JSDoc to enforce intent; consider runtime asserts in the bridge.
156-160: Cache callbacks: clarify dimensional expectations.Document whether
width/heightare device pixels post-scale, and thatboxis in world-space. Prevents mismatched matrices in cache renders.src/run.ts (3)
78-95: Blur path wiring LGTM; validate parameter ranges at callsite.Before invoking
drawBlur, consider clamping/roundingfilterSizePerPass{X,Y}to odd ints and guardingiterations > 0to avoid no-ops or shader asserts.
145-153: Cache update order is correct; add minimal error handling.If a cache pass fails (device lost), subsequent render should still proceed. Wrap
Logic.updateCache()in try/catch and fall back gracefully.- Logic.updateCache() + try { + Logic.updateCache() + } catch { + /* ignore cache update errors; proceed with direct rendering */ + }
170-184: Pick step uses the same encoder; good. Add initial pick trigger.If no pointer moves and queue is empty on first frame, no pick occurs. Optionally force one-time pick on startup.
- let rafId = 0 + let rafId = 0 + let didInitialPick = false @@ - const needsUpdatePick = + const needsUpdatePick = pointer.afterPickEventsQueue.length > 0 || lastPickPointer.x !== pointer.x || lastPickPointer.y !== pointer.y @@ - if (needsUpdatePick) { + if (!didInitialPick || needsUpdatePick) { + didInitialPick = truesrc/svgToShapes/index.ts (3)
231-246: Filter URL handling is too permissive; validate def.type and stdDeviation form.Ensure the def is a Gaussian blur and
stdDeviationis present (tuple of 2 numbers). Otherwise, ignore or log.Apply this diff:
- if (m) { - const def = defs[m[1]] - if (def?.stdDeviation) { + if (m) { + const def = defs[m[1]] + if (def?.type === 'gaussian-blur' && Array.isArray(def.stdDeviation) && def.stdDeviation.length === 2) { serializedProps.filter = { gaussianBlur: { x: def.stdDeviation[0], y: def.stdDeviation[1], }, } } }
248-250: Opacity may arrive as string; coerce with ensureNumber.Relying on typeof number is brittle with parsed SVG attributes.
Apply this diff:
- if (typeof props.opacity === 'number') { - serializedProps.opacity = props.opacity - } + { + const opacity = ensureNumber(props.opacity, 1) + serializedProps.opacity = opacity + }
217-229: Missing stroke-opacity support.SVG
stroke-opacityshould mirrorfill-opacityhandling.src/logic/shapes/shapes.zig (3)
75-82: New cache fields initialized but never gated by filter.
outdated_cacheis set true for all shapes; harmless, but you may avoid unnecessary cache work whenfilter == null.Also applies to: 119-124
349-364: Uniform multiplies opacity into colors: OK, but stroke is forced transparent.Setting
stroke_color = {0,0,0,0}disables strokes regardless of props.If intentional (temporary), add a TODO; otherwise, pass serialized stroke.
Also applies to: 366-387, 389-413
467-490: Bounds helpers are coherent; consider minor readability tweaks.Small local vars (e.g.,
len_minus_1) would reduce repeated modulo arithmetic, but not required.Also applies to: 492-505, 506-516
src/logic/index.zig (6)
741-747: If draw_shape signature is updated, these calls become valid; otherwise compilation fails.This depends on resolving the earlier signature mismatch.
Also applies to: 803-806
710-739: Cache render: constructing a stack array then passing by slice is clearer.Currently passing
&vertex_bounds(pointer-to-array) relies on coercions. Prefer an explicit slice for clarity.Apply this diff:
- const vertex_bounds = [_]types.PointUV{ + var vertex_bounds = [_]types.PointUV{ ... }; - web_gpu_programs.draw_shape( - &vertex_bounds, + web_gpu_programs.draw_shape( + vertex_bounds[0..], shape.getUniform(), shape.sdf_texture_id, );
797-806: Cache path draw_texture arg type: ensure it matches images.DrawVertex.
shape.getDrawBounds(extra_space)must beimages.DrawVertex-compatible. If not a type alias, wrap into the expected struct.
889-893: pick_shape expects a slice; avoid passing pointer-to-array.Explicit slice avoids coercion issues.
Apply this diff:
- web_gpu_programs.pick_shape( - &shape.getPickBounds(), + const pick_bounds = shape.getPickBounds(); + web_gpu_programs.pick_shape( + pick_bounds[0..], shape.getStrokeWidth(), shape.sdf_texture_id, );
141-158: Render-scale changes should also invalidate blur cache, not only SDF.Blur sigma and texture size depend on scale; otherwise blur strength drifts.
Apply this diff:
- if (new_size.w > shape.sdf_size.w or new_size.h > shape.sdf_size.h) { - shape.outdated_sdf = true; - } + if (new_size.w > shape.sdf_size.w or new_size.h > shape.sdf_size.h) { + shape.outdated_sdf = true; + } + if (shape.props.filter != null) { + shape.outdated_cache = true; + }
315-329: Defaulting new shapes to blurred filter is surprising for users.
.filter = .{ .gaussianBlur = .{ .x = 30, .y = 1 } }by default likely unintended for general drawing.Consider defaulting to
nulland enabling blur via UI/props.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (37)
src/WebGPU/getDevice.ts(1 hunks)src/WebGPU/getTexture/index.ts(3 hunks)src/WebGPU/pick.ts(1 hunks)src/WebGPU/programs/blur/getProgram.ts(2 hunks)src/WebGPU/programs/blur/index.wgsl(3 hunks)src/WebGPU/programs/computeShape/getProgram.ts(2 hunks)src/WebGPU/programs/drawGaussianBlur/getProgram.ts(1 hunks)src/WebGPU/programs/drawGaussianBlur/shader.wgsl(1 hunks)src/WebGPU/programs/drawMSDF/getProgram.ts(2 hunks)src/WebGPU/programs/drawShape/base.wgsl(1 hunks)src/WebGPU/programs/drawShape/getProgram.ts(3 hunks)src/WebGPU/programs/drawTexture/getProgram.ts(2 hunks)src/WebGPU/programs/drawTexture/shader.wgsl(1 hunks)src/WebGPU/programs/drawTriangle/getProgram.ts(4 hunks)src/WebGPU/programs/initPrograms.ts(3 hunks)src/WebGPU/programs/pickShape/getProgram.ts(3 hunks)src/WebGPU/textureCache.ts(5 hunks)src/getCanvasRenderDescriptor.ts(1 hunks)src/getDepthTexture.ts(1 hunks)src/getMultisampleTexture.ts(1 hunks)src/getRenderDescriptor.ts(1 hunks)src/index.ts(9 hunks)src/loadingTexture.ts(2 hunks)src/logic/get_sdf_texture_size.zig(0 hunks)src/logic/index.d.ts(6 hunks)src/logic/index.zig(11 hunks)src/logic/shapes/shapes.zig(14 hunks)src/logic/texture_size.zig(1 hunks)src/logic/types.zig(0 hunks)src/models/Video/MiniatureVideo.ts(0 hunks)src/run.ts(6 hunks)src/shapes/reflectGradientStop.ts(0 hunks)src/svgToShapes/boundingBox.ts(1 hunks)src/svgToShapes/definitions.ts(5 hunks)src/svgToShapes/index.ts(9 hunks)src/svgToShapes/utils.ts(6 hunks)src/textures.ts(5 hunks)
💤 Files with no reviewable changes (4)
- src/logic/get_sdf_texture_size.zig
- src/logic/types.zig
- src/models/Video/MiniatureVideo.ts
- src/shapes/reflectGradientStop.ts
🧰 Additional context used
🧬 Code graph analysis (18)
src/WebGPU/programs/pickShape/getProgram.ts (2)
src/WebGPU/programs/drawShape/getProgram.ts (1)
getDrawShape(4-98)src/WebGPU/programs/initPrograms.ts (1)
addDestroyBuf(41-43)
src/WebGPU/programs/drawShape/getProgram.ts (1)
src/WebGPU/programs/initPrograms.ts (2)
addDestroyBuf(41-43)canvasMatrix(34-36)
src/WebGPU/programs/drawTexture/getProgram.ts (3)
src/WebGPU/programs/drawTriangle/getProgram.ts (1)
getProgram(7-79)src/WebGPU/programs/drawMSDF/getProgram.ts (1)
getProgram(7-92)src/WebGPU/programs/initPrograms.ts (1)
canvasMatrix(34-36)
src/WebGPU/programs/computeShape/getProgram.ts (1)
src/WebGPU/programs/initPrograms.ts (1)
addDestroyBuf(41-43)
src/textures.ts (2)
src/loadingTexture.ts (1)
getLoadingTexture(1-35)src/WebGPU/getTexture/index.ts (1)
createTextureFromSource(111-129)
src/WebGPU/programs/drawGaussianBlur/getProgram.ts (2)
src/WebGPU/programs/drawTexture/getProgram.ts (1)
getProgram(5-92)src/WebGPU/programs/drawMSDF/getProgram.ts (1)
getProgram(7-92)
src/WebGPU/programs/blur/getProgram.ts (2)
src/WebGPU/programs/drawTexture/getProgram.ts (1)
getProgram(5-92)src/WebGPU/programs/initPrograms.ts (1)
addDestroyBuf(41-43)
src/svgToShapes/utils.ts (1)
src/svgToShapes/definitions.ts (1)
Def(14-44)
src/WebGPU/programs/initPrograms.ts (1)
src/WebGPU/programs/computeShape/getProgram.ts (1)
getComputeShape(4-44)
src/WebGPU/getTexture/index.ts (1)
src/textures.ts (1)
TextureSource(47-52)
src/WebGPU/programs/drawMSDF/getProgram.ts (3)
src/WebGPU/programs/drawTexture/getProgram.ts (1)
getProgram(5-92)src/WebGPU/programs/drawTriangle/getProgram.ts (1)
getProgram(7-79)src/WebGPU/programs/initPrograms.ts (1)
canvasMatrix(34-36)
src/WebGPU/programs/drawTriangle/getProgram.ts (2)
src/WebGPU/programs/drawTexture/getProgram.ts (1)
getProgram(5-92)src/WebGPU/programs/initPrograms.ts (2)
canvasMatrix(34-36)addDestroyBuf(41-43)
src/svgToShapes/definitions.ts (1)
src/svgToShapes/utils.ts (1)
getProps(34-78)
src/getCanvasRenderDescriptor.ts (1)
src/getMultisampleTexture.ts (1)
getMultisampleTexture(1-20)
src/index.ts (3)
src/WebGPU/getDevice.ts (1)
getDevice(1-30)src/WebGPU/programs/initPrograms.ts (1)
initPrograms(50-103)src/utils/getDefaultPoints.ts (1)
getDefaultPoints(3-21)
src/svgToShapes/index.ts (3)
src/svgToShapes/definitions.ts (2)
Def(14-44)DefStop(4-7)src/svgToShapes/boundingBox.ts (1)
BoundingBox(6-31)src/svgToShapes/utils.ts (2)
ensureNumber(80-85)parseColor(12-32)
src/WebGPU/textureCache.ts (2)
src/run.ts (1)
updateRenderPass(26-28)src/WebGPU/programs/initPrograms.ts (1)
canvasMatrix(34-36)
src/run.ts (6)
src/WebGPU/pick.ts (1)
PickManager(7-131)src/svgToShapes/boundingBox.ts (1)
BoundingBox(6-31)src/WebGPU/textureCache.ts (2)
startCache(15-95)endCache(11-13)src/WebGPU/programs/initPrograms.ts (3)
drawBlur(23-23)canvasMatrix(34-36)destroyBufs(45-48)src/getCanvasRenderDescriptor.ts (1)
getCanvasRenderDescriptor(5-38)src/getCanvasMatrix.ts (1)
getCanvasMatrix(4-19)
🔇 Additional comments (43)
src/getDepthTexture.ts (1)
7-7: Nice: add label for GPUTexture.Label improves debugging/profiling in DevTools.
src/svgToShapes/boundingBox.ts (1)
6-6: Good call exporting BoundingBox.Makes reuse and typing across modules cleaner.
src/WebGPU/getDevice.ts (1)
29-30: Verified: all call sites updated for new return shapeThe sole call in src/index.ts now uses
const { device, presentationFormat } = await getDevice()and no raw getDevice() calls remain. PresentationFormat is correctly passed where needed.
src/getMultisampleTexture.ts (1)
5-6: Signature change looks good—confirm all callsites updatedPropagating
formatand optionaltextureis the right direction; please verify all callers passpresentationFormatand reassign the returned texture.src/svgToShapes/utils.ts (2)
1-1: Broader typing viaNodeimport is fineUsing
NodealongsideElementNodeimproves robustness downstream.
65-66: Delegation to getGradientStops is fine—ensure children filteringSVG gradients often contain whitespace/text nodes; correctness depends on
getGradientStopsfiltering non-stop nodes (see suggestion below).src/WebGPU/getTexture/index.ts (2)
58-66: Using presentationFormat for array textures—LGTMFormat propagation aligns with device setup and avoids hardcoding.
160-168: createTextureFromImage path—LGTMCleanly forwards
presentationFormatand options.src/svgToShapes/definitions.ts (5)
114-118: Filter collection looks fine, but ensuregetPropssetstype: 'gaussian-blur'andstdDeviation.If multiple filter primitives or chains are present, current logic (single child) will skip them.
- Confirm
addFilterPropsassignstype: 'gaussian-blur'andstdDeviationfor<feGaussianBlur>.- Decide how to handle multiple primitives (for now, document the limitation).
19-22: API extension LGTM:type: 'gaussian-blur'andstdDeviationadded.This aligns the Def contract with SVG Gaussian blur.
41-44: New attributes (filter,opacity,fill-opacity) LGTM.Matches common SVG props; helps downstream draw paths.
4-7: No import required—Coloris ambient. Defined insrc/logic/index.d.ts(type Color = [number, number, number, number]), it’s included in the TS scope and usable without importing.
86-90: No changes required
TypeScript’s discriminated‐union narrowing makeschildanElementNodeafter thechild.type !== 'element'guard, sogetProps(child)is already type-safe and no import or cast is needed.Likely an incorrect or invalid review comment.
src/WebGPU/programs/initPrograms.ts (1)
39-49: Centralized buffer destruction LGTM.Simple queue and explicit destroy is clear.
src/WebGPU/programs/computeShape/getProgram.ts (3)
1-1: Import change LGTM.Using
addDestroyBufkeeps lifecycle consistent.
4-4: Signature simplification LGTM.Dropping the external destruction list reduces coupling.
23-31: Ensure buffer length alignment and dispatch consistency
- Assert
curvesDataView.byteLength % 4 === 0beforedevice.createBufferto satisfy WebGPU’s 4-byte alignment requirement.dispatchWorkgroups(texture.width, texture.height)already matches the shader’s@workgroup_size(1)insrc/WebGPU/programs/computeShape/shader.wgsl; update dispatch parameters if you modify the workgroup size.src/WebGPU/programs/drawGaussianBlur/shader.wgsl (1)
48-51: Verify edge handling matches the intended “bleed outside” behavior.Clamping to [texStart..texEnd] plus sampler address modes (likely clamp-to-edge) defines how the blur extends. Please confirm sampler addressing and whether the region can extend beyond [0,1] to achieve the desired SVG filter overflow.
Also applies to: 70-73
src/WebGPU/programs/blur/index.wgsl (1)
60-63: LGTM: barrier placement.The workgroup barrier after populating shared tile memory is correctly placed.
src/getRenderDescriptor.ts (1)
16-16: LGTM: label improves debugging.Adding a label on the depth texture creation helps with GPU error scopes and tooling.
src/WebGPU/pick.ts (1)
20-21: Nice: add labels to aid GPU debugging.
Helpful for capture tooling and error scopes.Also applies to: 27-28
src/WebGPU/programs/drawTexture/getProgram.ts (1)
1-1: Ensure buffer lifecycle and initialization safety.
- Add centralized destruction (e.g. via
addDestroyBuf) for vertex buffers to match other programs.- Guard against using an uninitialized
canvasMatrix.buffer(throw or initialize before use).- Verify that
addDestroyBufis exported frominitProgramsand imported correctly.src/WebGPU/programs/drawShape/getProgram.ts (2)
1-1: Good move: central buffer destruction + shared canvas matrix.
Matches the new lifecycle pattern and reduces plumbing.Also applies to: 72-73, 80-81, 90-91
1-1: Guard against uninitialized canvasMatrix.buffer.
Avoid order-of-init footguns.-import { addDestroyBuf, canvasMatrix } from '../initPrograms' +import { addDestroyBuf, canvasMatrix } from '../initPrograms' +if (!canvasMatrix.buffer) { + throw new Error('canvasMatrix.buffer not initialized'); +}src/WebGPU/programs/drawTriangle/getProgram.ts (1)
1-1: LGTM: unified matrix binding and buffer cleanup.
Consistent with other programs; bind group caching is a nice touch.Also applies to: 57-61, 71-73
src/textures.ts (1)
55-58: All Textures.add call sites include the new presentationFormat parameter. No invocation is missing the added argument; the optional onLoad callback remains compatible.src/WebGPU/textureCache.ts (3)
18-23: Good change: pass in encoder instead of creating one locally.Using a shared encoder removes redundant command buffers and fits better with upstream flow.
38-43: Usage flags look correct for resolve target + sampling + compute.Including STORAGE_BINDING is necessary for the subsequent compute blur.
89-92: Ensure no code uses renderPass after pass.end().updateRenderPass(pass) sets a global that will reference an ended pass until the next assignment. Verify no draw occurs between endCache() and the next updateRenderPass call.
src/WebGPU/programs/blur/getProgram.ts (3)
7-11: Shader format substitution: verify placeholder exists and matches WGSL expectation.Ensure index.wgsl contains exactly '{presentationFormat}'. Otherwise the created module will compile with an unintended format.
21-25: Sampler reuse is good.Creating it once per program avoids per-frame allocations.
76-84: Workgroup math: confirm it matches @workgroup_size in WGSL.blockDimX/Y and batch[1] must align with the shader’s tile/workgroup logic; otherwise edge tiles may be skipped or overrun.
Also applies to: 85-93, 99-113
src/index.ts (2)
82-87: Canvas format fallback: verify configure() is valid when BGRA storage is unsupported.getDevice() returns 'rgba8unorm' when 'bgra8unorm-storage' is absent. Some browsers still prefer/require getPreferredCanvasFormat(). Please verify context.configure({ format: presentationFormat }) succeeds on Safari/Firefox Nightly/Chrome without that feature.
Also applies to: 101-106
180-193: Prop serialization LGTM.filter.gaussianBlur and opacity propagation looks correct.
src/logic/texture_size.zig (1)
47-61: Double-check sigma → filter size math aligns with shader.If WGSL uses a different window (e.g., 3.0 vs 3.1 sigma, or clamps), mismatch can cause extra downscaling here. Verify constants and roundings match the compute/draw blur programs.
Would you like me to scan the WGSL blur program to confirm the kernel-size computation matches this logic?
src/run.ts (1)
137-153: Encoder lifecycle and matrix write look good.Centralized encoder per frame and single canvas matrix write simplify passes; no concerns here.
src/svgToShapes/index.ts (2)
74-107: Radial gradient: good normalization and epsilon guards.The math and safeguards look solid.
266-266: addShape new arity: sixth arg null is correct.Matches the extended public API (sdf + optional cache texture).
src/logic/shapes/shapes.zig (4)
50-53: Filter struct extension looks clean.Simple, explicit
gaussianBlur: Pointmatches usage.
54-67: Props/SerializedProps additions are consistent.
filter: ?Filterandopacity: f32align across runtime/serialized types.
417-423: Padding excludes blur radius; handled via extra_space—good separation.SDF padding based on stroke only; blur handled at call sites via
extra_space.Also applies to: 455-460
524-540: Serialization includes new texture IDs and props.Matches index.zig’s expectations for asset snapshots.
Also applies to: 590-597
src/logic/index.zig (1)
627-649: SDF compute path: good scaling and cache invalidation.Flooring sizes before compute and marking cache outdated is sound.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
src/WebGPU/programs/drawShape/base.wgsl (1)
63-69: Regression: stroke and proper edge coverage are bypassedYou compute both color (mixing stroke/fill) and total_alpha, but the new return ignores stroke and uses fill_alpha instead of total_alpha. This alters visuals (no stroke, different AA). Also keep debug code out.
Apply:
- // return vec4f(color.rgb, color.a * total_alpha); - return vec4f(fill_color.rgb, fill_color.a * fill_alpha); - // let result = vec4f(fill_color.rgb, fill_color.a * fill_alpha); - // if (result.a < 0.5) { - // return vec4f(1, 0, 0, 1); - // } - // return vec4f(result.rgb, 1); + return vec4f(color.rgb, color.a * total_alpha);src/logic/texture_size.zig (1)
25-41: Clamp once, preserve aspect ratio, and enforce ≥1px (degenerate-safe)Current two-step clamping can skew aspect ratio and permits 0-sized textures. Use a single ratio from the larger dimension and guard degenerate inputs.
pub fn get_size(bounds: [4]PointUV) TextureSize { - var width = bounds[0].distance(bounds[1]); - var height = bounds[0].distance(bounds[3]); - - if (width > shared.texture_max_size) { - const ratio = shared.texture_max_size / width; - width = shared.texture_max_size; - height *= ratio; - } - - if (height > shared.texture_max_size) { - const ratio = shared.texture_max_size / height; - height = shared.texture_max_size; - width *= ratio; - } - - return TextureSize{ .w = width, .h = height }; + var width = bounds[0].distance(bounds[1]); + var height = bounds[0].distance(bounds[3]); + + if (width <= 0 or height <= 0) { + return TextureSize{ .w = 1.0, .h = 1.0 }; + } + + const scale_w = shared.texture_max_size / width; + const scale_h = shared.texture_max_size / height; + const ratio = @min(1.0, @min(scale_w, scale_h)); + width = @max(1.0, width * ratio); + height = @max(1.0, height * ratio); + + return TextureSize{ .w = width, .h = height }; }
🧹 Nitpick comments (8)
src/logic/texture_size.zig (2)
11-20: Replace magic 16 with a named constant and keep ≥1px after clampImproves readability and avoids accidental 0-dimensions post-clamp.
+const BYTES_PER_PIXEL: f32 = 16.0; // rgba32float + pub fn get_sdf_size(bounds: [4]PointUV) TextureSize { var size = get_size(bounds); - const sdf_texture_size = size.w * size.h * 16; + const sdf_texture_size = size.w * size.h * BYTES_PER_PIXEL; - if (sdf_texture_size > shared.max_buffer_size) { - const max_pixels = shared.max_buffer_size / 16.0; + if (sdf_texture_size > shared.max_buffer_size) { + const max_pixels = shared.max_buffer_size / BYTES_PER_PIXEL; const ratio = max_pixels / (size.w * size.h); size.w *= ratio; size.h *= ratio; + size.w = @max(1.0, size.w); + size.h = @max(1.0, size.h); } return size; }
44-44: Type MAX_COST as f32 for clarityAvoids implicit coercions and documents intent.
-const MAX_COST = 90050924; // it's just chosen base on my own preferences +const MAX_COST: f32 = 90_050_924.0; // heuristic; tune empiricallysrc/logic/shapes/shapes.zig (4)
349-363: Opacity application: verify blending model (premult vs straight alpha)You multiply RGB and A by opacity. If your pipeline expects straight alpha, that’s fine; if premultiplied, multiply RGB by opacity but leave A as original alpha to avoid double-dimming.
460-468: Filter margin integration is good; padding vector math may over-inflate cornersAdding both prev and next directional components doubles the offset at corners; consider averaging or using normals to avoid oversized padding on acute vertices.
Also applies to: 481-482
515-521: Gaussian blur margin heuristic3×sigma is a reasonable default; consider exposing a tunable if artifacts appear at high blur.
539-541: Equality on floats may spam updatesstd.meta.eql on props/bounds (now with opacity/filter) will often fail due to float noise; consider tolerant compare or quantization.
Also applies to: 601-639
src/logic/index.zig (2)
721-735: Nit: comment typo“betetr” -> “better”.
- const maxSigmaPerPass = 2.0; // Feel free to increase to 4.0 for betetr quality + const maxSigmaPerPass = 2.0; // Feel free to increase to 4.0 for better quality
657-750: Blur cache flow looks correct; add a cheap early-outIf sigma.x and sigma.y are both ~0 after scaling, skip cache work.
- if (size.w < 1.001 or size.h < 1.001) continue; + if (size.w < 1.001 or size.h < 1.001) continue; + if (sigma.x < 1e-4 and sigma.y < 1e-4) { + shape.outdated_cache = false; + continue; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/WebGPU/programs/drawShape/base.wgsl(1 hunks)src/logic/consts.zig(1 hunks)src/logic/index.zig(11 hunks)src/logic/shapes/shapes.zig(16 hunks)src/logic/texture_size.zig(1 hunks)
🔇 Additional comments (11)
src/logic/consts.zig (1)
2-2: LGTM: Point import + POINT_ZEROGood centralization; no issues.
Also applies to: 11-11
src/logic/shapes/shapes.zig (5)
50-53: Filter type looks goodConsistent with later usage; no issues.
58-67: Props/serialization additions are fine; confirm defaults at callsitesEnsure all creators populate filter (nullable) and opacity (default 1.0) to avoid uninitialized behavior when deserializing older data.
77-82: Cache fields initializationSetting outdated_cache = true makes sense to trigger first build; good separation of sdf vs cache textures.
Also applies to: 119-124
137-142: Bounds initialization on first point insertionNice reuse of DEFAULT_BOUNDS with translation; clear and efficient.
489-501: Draw/pick bounds APIs are consistentSeparation of include_filter_margin for draw vs pick is correct.
Also applies to: 504-513
src/logic/index.zig (5)
29-31: API change: compute_shape signature widens (f32,f32)Confirm all providers (JS/WASM side) updated to the new signature to prevent ABI mismatches.
68-75: Cache callbacks wiringNames/readability improved; no issues.
150-152: Render-scale-aware SDF sizingGood: use bounds with padding at logical scale before get_sdf_size.
631-647: SDF re-gen flow is soundFlooring size before GPU call is good; flipping outdated_cache = true after SDF update is correct.
Also applies to: 649-650
736-743: Per-pass parameters: iteration/sigma math is reasonableSquare-root split per pass is fine for separable Gaussian; ensure draw_blur implements ping-ponging internally.
… method, remove unused update_texture_cache
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/svgToShapes/utils.ts (1)
87-100: Do not generate bogus gradient stops; filter to actual elements and combine alpha correctly.Text/whitespace children currently produce a synthetic stop at offset 0, which will corrupt gradients. Also,
stop-opacityshould be multiplied with the color’s intrinsic alpha, not overwrite it. Use percent-aware parsing and pass the combined opacity intoparseColor.-function getGradientStops(nodes: Array<string | Node>) { - return nodes.map((stop) => { - if (typeof stop === 'string' || stop.type !== 'element') - return { offset: 0, color: [0, 0, 0, 0] as Color } - const stopProps = getProps(stop) - const color = parseColor(String(stopProps['stop-color'] ?? '#000')) - color[3] = ensureNumber(stopProps['stop-opacity'], 1) - - return { - offset: Number(stopProps.offset ?? 0), - color, - } - }) -} +function getGradientStops(nodes: Array<string | Node>) { + return nodes + .filter( + (n): n is ElementNode => + typeof n !== 'string' && n.type === 'element' && n.tagName === 'stop' + ) + .map((stop) => { + const stopProps = getProps(stop) + const stopOpacity = ensureFraction(stopProps['stop-opacity'], 1) + const color = parseColor(String(stopProps['stop-color'] ?? '#000'), stopOpacity) + return { + offset: ensureFraction(stopProps.offset, 0), + color, + } + }) +}Add this helper near
ensureNumber(outside the changed hunk):function ensureFraction(x: AttrValue | undefined, fallback = 0): number { if (x == null) return fallback if (typeof x === 'number') return x const m = String(x).trim().match(/^(-?\d*\.?\d+)(%)?$/) if (!m) return fallback const n = Number(m[1]) return m[2] ? n / 100 : n }
♻️ Duplicate comments (4)
src/WebGPU/programs/blur/getProgram.ts (1)
26-34: Fix param type: code reads texture.width/height/format but param is GPUTextureEither accept explicit width/height/format or narrow to an extended texture type; current typing is unsound and will break strict TS.
Add a helper type and update the signature:
- return function renderBlur( - encoder: GPUCommandEncoder, - texture: GPUTexture, + type CacheTexture = GPUTexture & { width: number; height: number; format: GPUTextureFormat } + return function renderBlur( + encoder: GPUCommandEncoder, + texture: CacheTexture, iterations: number, filterSizePerPassX: number, filterSizePerPassY: number, sigmaPerPassX: number, sigmaPerPassY: number ) {Alternatively, accept explicit metadata:
- texture: GPUTexture, + texture: GPUTexture, + width: number, + height: number, + format: GPUTextureFormat,…and replace uses of texture.width/height/format with the args. Do you want me to update run.ts call sites as well?
src/logic/index.zig (3)
762-773: Don’t take the address of a temporary (getDrawBounds(true))
&shape.getDrawBounds(true)takes the address of an rvalue; store it in a local first to ensure a stable lifetime.- if (shape.cache_texture_id) |cache_texture_id| { - web_gpu_programs.draw_texture( - shape.getDrawBounds(true), - cache_texture_id, - ); - } else { - web_gpu_programs.draw_shape( - &shape.getDrawBounds(true), - shape.getUniform(), - shape.sdf_texture_id, - ); - } + if (shape.cache_texture_id) |cache_texture_id| { + web_gpu_programs.draw_texture( + shape.getDrawBounds(true), + cache_texture_id, + ); + } else { + var bounds = shape.getDrawBounds(true); + web_gpu_programs.draw_shape( + &bounds, + shape.getUniform(), + shape.sdf_texture_id, + ); + }
796-800: Same issue for skeleton draw: address-of rvalueCreate a local for
getDrawBounds(false)before passing its address.- web_gpu_programs.draw_shape( - &shape.getDrawBounds(false), - shape.getSkeletonUniform(), - shape.sdf_texture_id, - ); + var skel_bounds = shape.getDrawBounds(false); + web_gpu_programs.draw_shape( + &skel_bounds, + shape.getSkeletonUniform(), + shape.sdf_texture_id, + );
859-863: Same pattern in pick: avoid address-of temporaryWrap
getPickBounds()in a local before taking its address.- web_gpu_programs.pick_shape( - &shape.getPickBounds(), - shape.getStrokeWidth(), - shape.sdf_texture_id, - ); + var pick_bounds = shape.getPickBounds(); + web_gpu_programs.pick_shape( + &pick_bounds, + shape.getStrokeWidth(), + shape.sdf_texture_id, + );
🧹 Nitpick comments (14)
src/svgToShapes/utils.ts (3)
174-195: Clamp stdDeviation to non-negative values and normalize parsing paths.Negative blur radii are invalid. Clamp to ≥ 0 and reuse
ensureNumberconsistently.if (child.tagName === 'feGaussianBlur') { def.type = 'gaussian-blur' const v = child.properties?.stdDeviation let sx = 0 let sy = 0 if (typeof v === 'number') { - sx = v - sy = v + sx = Math.max(0, v) + sy = sx } else if (typeof v === 'string') { const parts = v.trim().split(/[ ,]+/).filter(Boolean) if (parts.length >= 2) { - sx = ensureNumber(parts[0], 0) - sy = ensureNumber(parts[1], 0) + sx = Math.max(0, ensureNumber(parts[0], 0)) + sy = Math.max(0, ensureNumber(parts[1], 0)) } else if (parts.length === 1) { - sx = ensureNumber(parts[0], 0) - sy = sx + sx = Math.max(0, ensureNumber(parts[0], 0)) + sy = sx } } def.stdDeviation = [sx, sy] }
14-16: Harden OffscreenCanvas usage with fallbacks.In some runtimes (SSR/Node without canvas polyfill),
OffscreenCanvasor a 2D context won’t exist. Provide a safe fallback to avoid runtime errors.- const canvas = new OffscreenCanvas(1, 1) - const ctx = canvas.getContext('2d')! + const canvas: any = + typeof OffscreenCanvas !== 'undefined' + ? new OffscreenCanvas(1, 1) + : typeof document !== 'undefined' + ? document.createElement('canvas') + : null + if (!canvas) return [0, 0, 0, 0] as Color + const ctx = canvas.getContext('2d') + if (!ctx) return [0, 0, 0, 0] as Color
92-94: Percent-aware parsing for stop-opacity (and offset via ensureFraction).
Number('50%')yieldsNaN, which breaks opacity/offset. After applying the gradient-stop refactor above, useensureFractionfor both.Would you like me to follow up with a small test set (SVG snippets) to validate gradients with numeric and percent offsets/opacities?
src/WebGPU/programs/drawShape/getProgram.ts (3)
40-51: Blend state differs from other pipelines; confirm premultiplied vs. non-PMA outputOther programs (e.g., drawTexture/MSDF) use color srcFactor 'one'. Here it's 'src-alpha'. If shaders mix PMA and non-PMA, compositing artifacts can occur.
If the fragment output is premultiplied, align blend to match:
- color: { - srcFactor: 'src-alpha', - dstFactor: 'one-minus-src-alpha', - }, + color: { + srcFactor: 'one', + dstFactor: 'one-minus-src-alpha', + },
15-16: Guard uniform size mismatch to avoid runtime errorsWriteBuffer will throw if uniformDataView is larger than the allocated buffer; if smaller, you’ll get partially uninitialized uniforms. Add a quick check.
const uniformBuffer = device.createBuffer({ label: 'drawShape uniforms', size: uniformBufferSize, usage: GPUBufferUsage.UNIFORM | GPUBufferUsage.COPY_DST, }) - device.queue.writeBuffer(uniformBuffer, 0, uniformDataView) + if (uniformDataView.byteLength !== uniformBufferSize) { + throw new Error( + `drawShape: uniformDataView.byteLength (${uniformDataView.byteLength}) != expected (${uniformBufferSize})` + ) + } + device.queue.writeBuffer(uniformBuffer, 0, uniformDataView)Also applies to: 74-81
66-71: Rename local var for clarity (typo)- const boundBoxBuffer = device.createBuffer({ + const boundingBoxBuffer = device.createBuffer({ label: 'drawShape vertex buffer', size: boundingBoxDataView.byteLength, usage: GPUBufferUsage.VERTEX | GPUBufferUsage.COPY_DST, }) - device.queue.writeBuffer(boundBoxBuffer, 0, boundingBoxDataView) - delayedDestroy(boundBoxBuffer) + device.queue.writeBuffer(boundingBoxBuffer, 0, boundingBoxDataView) + delayedDestroy(boundingBoxBuffer) ... - passEncoder.setVertexBuffer(0, boundBoxBuffer) + passEncoder.setVertexBuffer(0, boundingBoxBuffer)Also applies to: 95-95
src/WebGPU/programs/drawTriangle/getProgram.ts (2)
27-27: Fix misleading comment for vertex attribute format- { shaderLocation: 3, offset: 16 + 16 + 16, format: 'unorm8x4' }, // color rgba8unorm/bgra8unorm + { shaderLocation: 3, offset: 16 + 16 + 16, format: 'unorm8x4' }, // color as unorm8x4 (normalized 0..1)
63-66: Validate vertex data alignment for instance count- const numInstances = vertexData.byteLength / (4 * INSTANCE_STRIDE) + const strideBytes = 4 * INSTANCE_STRIDE + if (vertexData.byteLength % strideBytes !== 0) { + throw new Error(`drawTriangle: vertexData not aligned to ${strideBytes} bytes per instance`) + } + const numInstances = vertexData.byteLength / strideBytessrc/WebGPU/programs/initPrograms.ts (2)
51-56: Optional: more descriptive labelRename buffer label from 'uniforms' to 'canvas matrix uniforms' to ease GPU debugging/profiling.
63-63: Nit: consistent naming for getDrawTexture-import getDrawtexture from './drawTexture/getProgram' +import getDrawTexture from './drawTexture/getProgram' ... - drawTexture = getDrawtexture(device, presentationFormat) + drawTexture = getDrawTexture(device, presentationFormat)Also applies to: 68-70
src/WebGPU/programs/blur/getProgram.ts (2)
35-42: Avoid per-call buffer churn for blur params
blurParamsBufferX/Yare recreated every invocation. Hoist and reuse them across calls; just rewrite their contents. This reduces allocations and driver overhead.Example:
- return function renderBlur( + const blurParamsBufferX = device.createBuffer({ size: 16, usage: GPUBufferUsage.COPY_DST | GPUBufferUsage.UNIFORM }) + const blurParamsBufferY = device.createBuffer({ size: 16, usage: GPUBufferUsage.COPY_DST | GPUBufferUsage.UNIFORM }) + return function renderBlur( encoder: GPUCommandEncoder, @@ - const blurParamsBufferX = device.createBuffer({ - size: 16, - usage: GPUBufferUsage.COPY_DST | GPUBufferUsage.UNIFORM, - }) - const blurParamsBufferY = device.createBuffer({ - size: 16, - usage: GPUBufferUsage.COPY_DST | GPUBufferUsage.UNIFORM, - })Destroy them via your existing lifecycle hook when tearing down the program.
76-84: Clamp filter size to tileDim to avoid degenerate blocksFor large radii,
blockDim = max(1, tileDim - (filterSize - 1))collapses to 1 and explodes dispatch counts. Consider clampingfilterSizePerPass* <= tileDim - 1or adapt tileDim dynamically to maintain reasonable occupancy.src/logic/index.zig (2)
716-728: Nit: fix typo and consider cap on iterationsComment typo “betetr” -> “better”. Also consider capping iterations to a small max (e.g., 4–6) to bound worst-case work.
- const maxSigmaPerPass = 2.0; // Feel free to increase to 4.0 for betetr quality + const maxSigmaPerPass = 2.0; // Feel free to increase to 4.0 for better quality + const MAX_ITER: f32 = 6.0; @@ - const iterations = @max(1, @ceil(maxSigma / maxSigmaPerPass)); + const iterations = @max(1, @as(u32, @intFromFloat(@min(MAX_ITER, @ceil(maxSigma / maxSigmaPerPass)))));
308-322: Default Gaussian blur on new shapeJust a heads-up: default
.gaussianBlur = .{ .x = 30, .y = 1 }can be expensive on low-end GPUs. Consider a smaller default or lazy cache generation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/WebGPU/programs/blur/getProgram.ts(2 hunks)src/WebGPU/programs/computeShape/getProgram.ts(2 hunks)src/WebGPU/programs/drawShape/getProgram.ts(3 hunks)src/WebGPU/programs/drawTriangle/getProgram.ts(4 hunks)src/WebGPU/programs/initPrograms.ts(3 hunks)src/WebGPU/programs/pickShape/getProgram.ts(3 hunks)src/WebGPU/textureCache.ts(5 hunks)src/logic/consts.zig(1 hunks)src/logic/index.zig(11 hunks)src/logic/texture_size.zig(1 hunks)src/run.ts(6 hunks)src/svgToShapes/utils.ts(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/WebGPU/programs/computeShape/getProgram.ts
- src/logic/consts.zig
- src/WebGPU/programs/pickShape/getProgram.ts
- src/logic/texture_size.zig
- src/WebGPU/textureCache.ts
🧰 Additional context used
🧬 Code graph analysis (6)
src/WebGPU/programs/drawTriangle/getProgram.ts (3)
src/WebGPU/programs/drawTexture/getProgram.ts (1)
getProgram(5-92)src/WebGPU/programs/drawMSDF/getProgram.ts (1)
getProgram(7-92)src/WebGPU/programs/initPrograms.ts (2)
canvasMatrix(34-36)delayedDestroy(41-43)
src/WebGPU/programs/drawShape/getProgram.ts (1)
src/WebGPU/programs/initPrograms.ts (2)
delayedDestroy(41-43)canvasMatrix(34-36)
src/svgToShapes/utils.ts (1)
src/svgToShapes/definitions.ts (1)
Def(14-44)
src/WebGPU/programs/initPrograms.ts (1)
src/WebGPU/programs/computeShape/getProgram.ts (1)
getComputeShape(4-44)
src/WebGPU/programs/blur/getProgram.ts (3)
src/WebGPU/programs/drawTriangle/getProgram.ts (1)
getProgram(7-79)src/WebGPU/programs/drawTexture/getProgram.ts (1)
getProgram(5-92)src/WebGPU/programs/initPrograms.ts (1)
delayedDestroy(41-43)
src/run.ts (5)
src/WebGPU/pick.ts (1)
PickManager(7-131)src/WebGPU/textureCache.ts (2)
startCache(15-96)endCache(11-13)src/WebGPU/programs/initPrograms.ts (3)
drawBlur(23-23)canvasMatrix(34-36)destroyGpuObjects(45-48)src/getCanvasRenderDescriptor.ts (1)
getCanvasRenderDescriptor(5-38)src/getCanvasMatrix.ts (1)
getCanvasMatrix(4-19)
🔇 Additional comments (14)
src/WebGPU/programs/drawShape/getProgram.ts (2)
72-72: Nice move to centralized destructionSwitching to delayedDestroy keeps per-draw cleanup consistent with the new lifecycle. Looks good.
Also applies to: 80-80
1-1: Ensure canvasMatrix.buffer is initialized before useAccessing canvasMatrix.buffer before initPrograms() assigns it will trigger a WebGPU validation error. I proposed a defensive getter in initPrograms to throw early.
Also applies to: 90-91
src/WebGPU/programs/drawTriangle/getProgram.ts (2)
57-61: Good: cached bind group (matrix only)
72-72: Nice: using delayedDestroy for the transient vertex buffersrc/WebGPU/programs/initPrograms.ts (2)
41-48: Centralized destruction helpers look goodSimple and effective queue + flush pattern.
45-48: destroyGpuObjects is invoked after every frame submission; no accumulation risk.src/WebGPU/programs/blur/getProgram.ts (1)
115-118: Good: resources are scheduled for safe destructionSwap texture and uniform buffers are queued via
delayedDestroy, avoiding in-flight hazards.src/run.ts (4)
43-49: Nice integration of cache lifecycle with shared encoderWiring
startCache(device, presentationFormat, encoder, ...)andendCachethroughLogic.connectCacheCallbackskeeps all passes on one encoder.
137-139: Good: single labeled encoder per frameCreating one “draw canvas main encoder” consolidates compute, cache, render, and pick passes.
186-187: Good: submit then destroy queued GPU objects
destroyGpuObjects()post-submit pairs correctly withdelayedDestroy.
150-152: Minor: matrix write timing is correctCanvas matrix is computed once and written before rendering, and reused for pick.
src/logic/index.zig (3)
651-744: Overall cache/blur flow looks solidCreating/using a cache texture, rendering SDF into it, then blurring via WebGPU before drawing it back is coherent. Good separation of SDF vs cache textures.
636-644: Width/height flooring before compute is fineExplicit
@floorensures stable sizes. Keep consistent with the JS side’s texture allocation.
142-148: Bounds-based SDF resizing logic looks correctRecomputing SDF on scale increases only (via new_size > existing) is a good trade-off.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/index.ts (1)
265-265: Use nullish coalescing fortexture_id
Replaceasset.textureId || Textures.add(asset.url)with
asset.textureId ?? Textures.add(asset.url)because
Textures.addcan legitimately return 0.
♻️ Duplicate comments (9)
src/WebGPU/getDevice.ts (1)
25-28: Bug: storageFormat must not depend on preferred canvas format.Canvas presentation format preference can be RGBA even when BGRA storage is supported. Pick storage format explicitly to avoid mismatches in compute/RT paths and cache textures.
Apply:
- const storageFormat = hasBGRA8unormStorage - ? navigator.gpu.getPreferredCanvasFormat() - : 'rgba8unorm' + const storageFormat: GPUTextureFormat = hasBGRA8unormStorage + ? 'bgra8unorm' + : 'rgba8unorm'src/WebGPU/textureCache.ts (1)
48-52: Good: per-pass matrix buffer is cleaned up.Swapping canvasMatrix.buffer and deferring destroy avoids leaks noted earlier. Looks solid.
Also applies to: 69-71
src/textures.ts (3)
86-86: Remove stray console.log.Avoid noisy logs in production.
- console.log(defs)
47-47: Bug: loadingTexture should use RGBA, not presentation format (BGRA risk).The placeholder texture is CPU-authored RGBA bytes; using a BGRA presentation format will swap channels.
- loadingTexture = getLoadingTexture(device, presentationFormat) + // Use RGBA for CPU-authored textures; presentationFormat is for render targets. + loadingTexture = getLoadingTexture(device, 'rgba8unorm')
106-108: Bug: avoid presentationFormat for image-uploaded textures (use RGBA).For image sources, create textures as RGBA to match uploaded bytes.
- textures[textureId].texture = createTextureFromSource(device, presentationFormat, img, { - flipY: true, - }) + textures[textureId].texture = createTextureFromSource(device, 'rgba8unorm', img, { + flipY: true, + })src/index.ts (1)
254-256: Use nullish coalescing to avoid clobbering valid 0 ids.'||' will treat 0 as falsy and replace it. Use '??' to default only on null/undefined.
Apply:
- sdf_texture_id: asset.sdf_texture_id || Textures.createSDF(), - cache_texture_id: asset.cache_texture_id || null, + sdf_texture_id: asset.sdf_texture_id ?? Textures.createSDF(), + cache_texture_id: asset.cache_texture_id ?? null,src/logic/index.d.ts (1)
140-151: Renamed parameters to sdf_texture_id across hooks: audit callsites.Double-check all TS/Zig/WSGL callsites no longer reference legacy texture_id for shapes.
Run:
#!/bin/bash # Spot remaining shape "texture_id" identifiers that should be "sdf_texture_id" rg -nP -C2 '\b(shape\..*texture_id|sdf_texture_id\b)' src | sed -n '1,200p'src/logic/index.zig (2)
796-800: Zig: address-of rvalue in skeleton draw.Same issue for getDrawBounds(false); use a local.
Apply:
- web_gpu_programs.draw_shape( - &shape.getDrawBounds(false), - shape.getSkeletonUniform(), - shape.sdf_texture_id, - ); + var skel_bounds = shape.getDrawBounds(false); + web_gpu_programs.draw_shape(&skel_bounds, shape.getSkeletonUniform(), shape.sdf_texture_id);
862-862: Zig: address-of rvalue in pick path.getPickBounds() temporary’s address is taken. Store to a local variable first.
Apply:
- web_gpu_programs.pick_shape( - &shape.getPickBounds(), - shape.getStrokeWidth(), - shape.sdf_texture_id, - ); + var pick_bounds = shape.getPickBounds(); + web_gpu_programs.pick_shape(&pick_bounds, shape.getStrokeWidth(), shape.sdf_texture_id);
🧹 Nitpick comments (7)
src/logic/images.zig (1)
25-32: Make loop style consistent with getPickVertexDataMinor readability: mirror the enumerated loop used below to drop the manual index.
- var i: usize = 0; - - for (SHADER_TRIANGLE_INDICES) |index| { - buffer[i] = self.points[index]; - i += 1; - } + for (SHADER_TRIANGLE_INDICES, 0..) |index, i| { + buffer[i] = self.points[index]; + }src/WebGPU/getDevice.ts (2)
3-3: Nit: error message style and capitalization.Prefer constructing Error and proper “WebGPU” capitalization.
- throw Error('this browser does not support WebGPU') + throw new Error('This browser does not support WebGPU.')- throw Error('this browser supports webgpu but it appears disabled') + throw new Error('This browser supports WebGPU, but it appears to be disabled.')Also applies to: 9-9
19-22: Nit: typo and actionable path on device loss.Fix “reprot” → “report” and consider wiring a recovery callback.
- if (info.reason !== 'destroyed') { - // reprot issue to the tracking system - // getDevice(callback); - } + if (info.reason !== 'destroyed') { + // report issue to tracking; optionally trigger app-level recovery + // e.g. enqueue a fresh getDevice() and resource re-init. + }src/WebGPU/textureCache.ts (1)
46-49: Consider restoring the previous renderPass after cache ends.startCache updates the global pass but endCache doesn’t restore it, which can surprise subsequent draws.
Option: capture and restore:
- const pass = encoder.beginRenderPass(descriptor) - updateRenderPass(pass) + const prevPass = undefined // or getRenderPass() if exposed + const pass = encoder.beginRenderPass(descriptor) + updateRenderPass(pass) @@ - pass.end() - canvasMatrix.buffer = canvasMatrixCopy + pass.end() + canvasMatrix.buffer = canvasMatrixCopy + // updateRenderPass(prevPass) // if a getter is availableIf no getter exists, consider exporting one from run.ts.
Also applies to: 67-71
src/textures.ts (1)
176-200: Size check and type safety of GPUTexture width/height.
- Comparing integers with Number.EPSILON is unnecessary; use strict equality.
- Many WebGPU typings don’t expose texture.width/height. If you rely on them, ensure you’ve augmented types; otherwise persist dimensions in your TextureSource metadata.
Minimal equality fix:
- const canReuseTexture = - texture && - Math.abs(texture.width - expectWidth) <= Number.EPSILON && - Math.abs(texture.height - expectHeight) <= Number.EPSILON + const canReuseTexture = + !!texture && + texture!.width === expectWidth && + texture!.height === expectHeightIf width/height aren’t available, store them:
// Extend TextureSource: interface TextureSource { /* ... */ w?: number; h?: number } // When (re)creating: textures[id].texture = newTexture textures[id].w = expectWidth textures[id].h = expectHeight // Reuse check: const t = textures[id] const canReuse = !!t.texture && t.w === expectWidth && t.h === expectHeightAlso, assert non-null on return for TS:
- return texture + return texture!src/logic/index.zig (2)
309-322: Default gaussian blur on new shapes: confirm UX/perf intent.Applying blur by default (x:30, y:1) impacts performance and may surprise users. Consider defaulting to no filter and letting users opt-in.
Apply if desired:
- .filter = .{ .gaussianBlur = .{ .x = 30, .y = 1 } }, + .filter = null,
762-773: Avoid using cache texture when filter was removed; consider cleanup.Render path should only consume cache_texture_id if a filter is active. The diff above adds that guard. Optionally, when a filter transitions to null, clear cache_texture_id to free memory.
I can draft a small state-transition helper to null the cache on filter removal if desired.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/WebGPU/getDevice.ts(1 hunks)src/WebGPU/textureCache.ts(5 hunks)src/index.ts(7 hunks)src/logic/images.zig(1 hunks)src/logic/index.d.ts(6 hunks)src/logic/index.zig(11 hunks)src/run.ts(6 hunks)src/textures.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/run.ts
🧰 Additional context used
🧬 Code graph analysis (3)
src/textures.ts (2)
src/loadingTexture.ts (1)
getLoadingTexture(1-35)src/WebGPU/getTexture/index.ts (1)
createTextureFromSource(111-129)
src/index.ts (3)
src/WebGPU/getDevice.ts (1)
getDevice(1-31)src/WebGPU/programs/initPrograms.ts (1)
initPrograms(50-103)src/run.ts (1)
runCreator(30-209)
src/WebGPU/textureCache.ts (3)
src/getMultisampleTexture.ts (1)
getMultisampleTexture(1-20)src/run.ts (1)
updateRenderPass(26-28)src/WebGPU/programs/initPrograms.ts (2)
canvasMatrix(34-36)delayedDestroy(41-43)
🔇 Additional comments (18)
src/logic/images.zig (2)
25-25: LGTM: switch to fixed-size vertex buffer pointerUsing
*[6]PointUVmakes the vertex count an API-level guarantee and matches howgetPickVertexDatais typed. Good change.
25-32: Alias removal verified – noDrawVertexreferences or outdatedgetRenderVertexDatacall sites detected.src/WebGPU/getDevice.ts (2)
13-15: Good: feature-gated device request is correct.Conditionally requesting 'bgra8unorm-storage' only when the adapter advertises it is the right approach.
30-31: Resolved: getDevice signature update handled
All call sites (e.g. src/index.ts line 82) already destructure{ device, presentationFormat, storageFormat }. No further changes needed.src/textures.ts (2)
39-46: API/init wiring looks good.Storing presentation and storage formats at init aligns the pipeline around consistent texture creation.
1-7: Note: svgToShapes imports—LGTM.Interfaces and defs wiring are sensible; no issues spotted.
src/index.ts (5)
82-84: LGTM: Device/format plumbed correctly.Using getDevice()’s presentationFormat and storageFormat and threading both into Textures.init is correct and future-proof.
122-122: LGTM: Programs init with presentationFormat.initPrograms(device, presentationFormat) aligns with the new program signatures.
181-193: LGTM: Shape props now carry filter and opacity.Props mapping preserves existing fills/strokes and serializes gaussianBlur and opacity as expected.
194-196: LGTM: Shape texture ids serialized as sdf/cache ids.Output snapshot now exposes sdf_texture_id and cache_texture_id, matching the new public API.
236-239: LGTM: runCreator callback usage.The empty-events callback correctly toggles isMouseEventProcessing and triggers updateProcessing().
src/logic/index.d.ts (5)
20-24: LGTM: Introduced Color alias and updated GradientStop.Consistent tuple type improves readability and reuse.
42-46: LGTM: ShapeProps expanded (fill/stroke as unions, filter, opacity).Type surface matches runtime usage and upcoming blur feature.
73-75: LGTM: ShapeAssetInput uses sdf_texture_id and nullable cache_texture_id.
100-100: LGTM: New updateCache entry point.Matches run.ts driving of the cache/blur pipeline.
128-135: Confirm blur kernel‐size semantics
filterSizePerPassX/Y—are these the full kernel width or the radius? The WGSL blur shader typically expects an odd kernel width (2 × radius + 1), so please verify that both the caller and shader use the same convention.src/logic/index.zig (2)
142-144: LGTM: SDF resize check on render-scale change.Bounds->sdf_size recompute and outdated flagging are correct.
620-645: LGTM: SDF size/scale computation and flooring before GPU.Correctly floors sizes and marks cache outdated post-SDF recompute.
|
🎉 This PR is included in version 0.1.0-next.22 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Summary by CodeRabbit
New Features
Improvements
Refactor