GPU accelerated maskeditor rendering #6767
Conversation
…equidistant points using equidistant point sampling on splines, including arc length sampling for equidistance in curves
…rush cross artefacts, fixed brush size
…dedicated file, fixed errors
dosubot
bot
added
the
size:XXL
trsommer
changed the title
🎨 Storybook Build Status
⏰ Started at: 11/21/2025, 11:50:36 PM UTC 🚀 Building Storybook
⏱️ Please wait while the Storybook build is in progress... |
🎭 Playwright Test Results⏰ Completed at: 11/22/2025, 12:01:40 AM UTC 📈 Summary
📊 Test Reports by Browser
🎉 Click on the links above to view detailed test results for each browser configuration. |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Bundle Size ReportSummary
Category Glance Per-category breakdownApp Entry Points — 3.18 MB (baseline 3.13 MB) • 🔴 +48.8 kBMain entry bundles and manifests
Status: 3 added / 3 removed Graph Workspace — 940 kB (baseline 940 kB) • ⚪ 0 BGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 7.97 kB (baseline 7.97 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Status: 1 added / 1 removed Panels & Settings — 306 kB (baseline 306 kB) • ⚪ 0 BConfiguration panels, inspectors, and settings screens
Status: 6 added / 6 removed UI Components — 141 kB (baseline 141 kB) • ⚪ 0 BReusable component library chunks
Status: 6 added / 6 removed Data & Services — 12.5 kB (baseline 12.5 kB) • ⚪ 0 BStores, services, APIs, and repositories
Status: 3 added / 3 removed Utilities & Hooks — 2.94 kB (baseline 2.94 kB) • ⚪ 0 BHelpers, composables, and utility bundles
Status: 1 added / 1 removed Vendor & Third-Party — 5.7 MB (baseline 5.32 MB) • 🔴 +374 kBExternal libraries and shared vendor chunks
Status: 3 added / 3 removed Other — 3.87 MB (baseline 3.87 MB) • ⚪ 0 BBundles that do not match a named category
Status: 19 added / 19 removed |
jtydhr88
added
the
claude-review
|
@trsommer i just asked @christian-byrne adding Claude review but don’t worry, you don’t need to fix everything from Claude review, we can discuss which ones make sense and need to fix, and which ones you think we could ignore |
|
Also please fix the failed tests |
christian-byrne
added
claude-review
…ht mouse button gesture not expanding brush to max size
|
@jtydhr88 test is fixed, I added some more comments as well and fixed a bug. Claude review is still missing |
|
@trsommer can you give me write access to your repo? |
jtydhr88
removed
the
claude-review
|
@jtydhr88 I invited you, you should have access now |
jtydhr88
added
the
claude-review
|
likely claude review still does not work, I will run it on my local |
… editor canvas layering, gpu lifecycle optimizations, shader preview fixes
📝 WalkthroughWalkthroughAdds TypeGPU/WebGPU tooling and a GPU-backed brush renderer, refactors brush settings (smoothingPrecision → stepSize, max size 500), implements spline-based stroke resampling and StrokeProcessor, extends canvas history to handle ImageBitmap, and wires GPU preview and lifecycle into the mask editor. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as Brush UI
participant Drawing as useBrushDrawing
participant Processor as StrokeProcessor
participant GPU as GPUBrushRenderer
participant Preview as GPU Canvas
participant History as useCanvasHistory
User->>UI: pointer down / move
UI->>Drawing: startDrawing / addPoint
Drawing->>Processor: addPoint(point)
Processor-->>Drawing: resampled points
alt GPU available
Drawing->>GPU: gpuDrawPoint / renderStroke
GPU->>GPU: accumulate & composite
GPU->>Preview: blit preview
Preview-->>User: visual feedback
GPU->>Drawing: readback (copyGpuToCanvas)
else CPU fallback
Drawing->>Drawing: CPU draw path (canvas 2D)
Drawing-->>User: visual feedback
end
User->>UI: pointer up / end
UI->>Drawing: endStroke
Drawing->>History: saveState(mask, rgb)
History-->>Drawing: state stored
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/composables/maskeditor/useCanvasHistory.ts (1)
58-90: Add dimension validation for provided data and clarify ImageBitmap ownership.Two concerns with the optional parameters:
Dimension mismatch risk: The function doesn't validate that provided
ImageDataorImageBitmapdimensions match the canvas dimensions. Mismatched dimensions could cause rendering issues during restoration.Memory ownership concern: When
ImageBitmapinstances are provided, they may later be closed automatically (lines 96-101 when exceedingmaxStates, or line 150-154 inclearStates). If the caller retains a reference to the providedImageBitmap, closing it will invalidate their reference, potentially causing crashes when they attempt to use it.Recommendation: Consider either:
- Cloning provided
ImageBitmapinstances usingcreateImageBitmap()so this history manager owns its copies- Documenting that the history manager takes ownership and callers must not use provided bitmaps after passing them
- Adding dimension validation to ensure provided data matches canvas size
const saveState = ( providedMaskData?: ImageData | ImageBitmap, providedRgbData?: ImageData | ImageBitmap ) => { const maskCtx = store.maskCtx const rgbCtx = store.rgbCtx const maskCanvas = store.maskCanvas const rgbCanvas = store.rgbCanvas if (!maskCtx || !rgbCtx || !maskCanvas || !rgbCanvas) return if (!initialized.value || currentStateIndex.value === -1) { saveInitialState() return } states.value = states.value.slice(0, currentStateIndex.value + 1) let maskState: ImageData | ImageBitmap let rgbState: ImageData | ImageBitmap if (providedMaskData && providedRgbData) { + // Validate dimensions + if (providedMaskData.width !== maskCanvas.width || providedMaskData.height !== maskCanvas.height) { + console.error('Provided mask data dimensions do not match canvas dimensions') + return + } + if (providedRgbData.width !== rgbCanvas.width || providedRgbData.height !== rgbCanvas.height) { + console.error('Provided RGB data dimensions do not match canvas dimensions') + return + } maskState = providedMaskData rgbState = providedRgbData } else { maskState = maskCtx.getImageData( 0, 0, maskCanvas.width, maskCanvas.height ) rgbState = rgbCtx.getImageData(0, 0, rgbCanvas.width, rgbCanvas.height) }src/components/maskeditor/MaskEditorContent.vue (1)
194-207: GuardbrushDrawingon unmount and ensure GPU resources are releasedOn unmount you now call:
toolManager.brushDrawing.saveBrushSettings()without a null/undefined check.
- If
toolManager.brushDrawingcan ever be absent for this component (e.g., due to initialization failure, different tool configuration, or future changes), this will throw during unmount. The previous optional‑chaining call suggests this was a realistic scenario.Additionally, given that this component is the one calling
initGPUResourcesandinitPreviewCanvas, it’s a natural place to ensure GPU resources are torn down:
- If
brushDrawingexposes adestroy()method (as indicated in the PR summary), it should be invoked during unmount to release GPU textures/buffers and avoid leaking GPU memory across multiple editor sessions.Suggested change:
onBeforeUnmount(() => { if (toolManager.brushDrawing) { toolManager.brushDrawing.saveBrushSettings() if (typeof toolManager.brushDrawing.destroy === 'function') { toolManager.brushDrawing.destroy() } } keyboard?.removeListeners() // …rest unchanged })
🧹 Nitpick comments (20)
src/composables/maskeditor/useCanvasHistory.ts (1)
163-163: Document that currentStateIndex is read-only for consumers.Exposing
currentStateIndexcan be useful for UI components (e.g., showing "Step 3 of 10" in history). However, external code should not mutate this value directly, as it would desynchronize the history state.Consider adding a JSDoc comment to clarify the intended usage:
return { canUndo, canRedo, + /** Current position in the history stack (read-only). Mutation by external code is not supported. */ currentStateIndex, saveInitialState, saveState, undo, redo, clearStates }src/composables/maskeditor/useMaskEditorLoader.ts (1)
104-145: Use normalizednodeImageRef.filenameinstead of rawwidgetFilenamefor/files/mask-layersqueryThe concern is valid. The code exhibits an inconsistency:
mkFileUrl()(used for other API calls) separatesfilename,subfolder, andtypeinto distinct query parameters (lines 51–66)imageLayerFilenamesIfApplicable()expects a bare filename, not a composite string (lines 26–41)- Downstream code calls
imageLayerFilenamesIfApplicable(nodeImageRef.filename)with the normalized filename (line 161)- Yet
/files/mask-layersreceiveswidgetFilenameas-is, which may contain"subfolder/filename [type]"format (line 150)The normalized
nodeImageRefobject is already computed with subfolder and type properly separated (lines 104–143). Passing the raw widget string to the endpoint breaks consistency with how other API calls are structured and how downstream logic expects the filename.- const fileToQuery = widgetFilename || nodeImageRef.filename + const fileToQuery = nodeImageRef.filenameThis ensures the backend receives the bare filename consistently with the
mkFileUrlpattern and downstreamimageLayerFilenamesIfApplicablelogic.vite.config.mts (1)
13-13: TypeGPU Vite plugin is wired in at a sensible point in the pipelineImporting
typegpuPluginfrom'unplugin-typegpu/vite'and insertingtypegpuPlugin({})aftertailwindcss()fits well into the existing plugin chain and should allow shader/codegen transforms to run without interfering with Vue or your custom plugins. If builds remain fast, the default configuration is fine; if you notice slowdowns, you can later narrowinclude/excludeglobs in the plugin options.Please confirm a full
pnpm build(cloud + desktop targets if applicable) runs successfully with this plugin enabled and that the GPU mask editor paths behave correctly in both dev and prod.Also applies to: 230-236
src/components/maskeditor/dialog/TopBarHeader.vue (1)
99-106: Clear now updates both canvas and store, keeping GPU/CPU state in syncHooking
store.triggerClear()aftercanvasTools.clearMask()makes the clear action observable to the mask editor store, which should help history/GPU layers react correctly to a full clear.Longer‑term, consider centralizing clear logic behind a single store or composable method (e.g.,
store.clearMask()calling intocanvasTools) so callers don’t have to remember to invoke both effects.src/composables/maskeditor/gpu/gpuSchema.ts (1)
1-17: TypeGPU brush/point schemas look correct; keep them tightly aligned with WGSLThe
BrushUniformsandStrokePointstructs are well structured for GPU use: scalar/vec fields are in a sensible order, and vertex locations forStrokePoint(0–2) are clearly documented. This should map cleanly onto your brush shaders and GPUBrushRenderer logic.
- Ensure the corresponding WGSL structs (and bind group layouts) use the exact same field order and types.
- Consider defining a single shared mapping (e.g.,
GPU_BRUSH_SHAPE = { Arc: 0, Rect: 1 }) next to this schema so thebrushShape: d.u32convention (0: Circle, 1: Square) cannot drift from theBrushShapeenum.src/components/maskeditor/BrushSettingsPanel.vue (1)
82-89: Consider localising the “Stepsize” controlThe slider is correctly wired to store.brushSettings.stepSize / setBrushStepSize, but the label is now a hard-coded "Stepsize" string while the rest of the panel uses t(...). Defining a new key like t('maskEditor.stepSize') would keep this control localisable and consistent with surrounding UI text.
Also applies to: 122-124
src/composables/maskeditor/brushUtils.ts (1)
1-34: Clamp hardness values to match the documented [0,1] rangeBoth helpers assume hardness is in [0,1], and getEffectiveHardness’ comment promises a 0..1 return, but neither enforces this. With slightly out-of-range inputs or effectiveSize < size * hardness, you can get hardness < 0 or > 1. Clamping inputs/outputs makes these utilities safer and consistent with the docs.
You could do something like:
-export function getEffectiveBrushSize(size: number, hardness: number): number { - // Scale factor for maximum softness - const MAX_SCALE = 1.5 - const scale = 1.0 + (1.0 - hardness) * (MAX_SCALE - 1.0) - return size * scale -} +export function getEffectiveBrushSize(size: number, hardness: number): number { + const clampedHardness = Math.min(1, Math.max(0, hardness)) + const MAX_SCALE = 1.5 + const scale = 1.0 + (1.0 - clampedHardness) * (MAX_SCALE - 1.0) + return Math.max(0, size) * scale +} @@ -export function getEffectiveHardness( +export function getEffectiveHardness( size: number, hardness: number, effectiveSize: number ): number { if (effectiveSize <= 0) return 0 - // Adjust hardness to maintain the physical radius of the hard core - return (size * hardness) / effectiveSize + const base = (size * hardness) / effectiveSize + // Adjust hardness to maintain the physical radius of the hard core, + // but keep it in the [0, 1] range. + return Math.min(1, Math.max(0, base)) }src/composables/maskeditor/StrokeProcessor.ts (2)
4-85: Clarify or enforce per-stroke usage of StrokeProcessorcontrolPoints, remainder, isFirstPoint, and hasProcessedSegment are all preserved after
endStroke(). If a caller ever reuses the same StrokeProcessor instance for multiple strokes, leftover control points and remainder will bleed into the next stroke and distort geometry. If the intended contract is “one instance per stroke”, it would help to document that explicitly; otherwise, consider adding areset()or clearing this internal state inendStroke()so reuse is safe.
87-114: Defensively validate spacing and consider capping sample density
processSegmentassumesthis.spacing > 0and passes it intoresampleSegment, whose inner while-loops would misbehave if a future caller accidentally constructedStrokeProcessor(0)or with a negative spacing. Adding a simple guard in the constructor (e.g. throw or clamp to a small epsilon) would harden this. You might also want to capsamples = Math.max(5, Math.ceil(dist))to avoid excessive work if two points are extremely far apart.src/composables/maskeditor/gpu/brushShaders.ts (1)
5-38: Brush vertex/fragment shaders are coherent; clarifysize(radius vs diameter) semanticsThe vertex/fragment pair are wired correctly:
- Bindings and locations line up with
StrokePoint(vec2 + size + pressure) andBrushUniforms.- Using
quadPosin [-1,1] as both local UVs and expansion basis, withdistdiscard andfwidth‑based AA, is a good approach for both square and circular brushes.- Premultiplied‑alpha output in the fragment shader matches how the composite and blit passes treat textures.
One thing to clarify to avoid future confusion:
- The WGSL comment says
// Convert diameter to radius(let radius = size * pressure;), while theStrokePointschema comment refers tosizeas “Brush radius”. If CPU code is already treatingsizeas a radius, this comment is misleading; if it’s actually a diameter, the schema comment is misleading.I’d suggest aligning the comments (and, if needed, the naming) so it’s unambiguous whether
sizeis a radius or diameter throughout the GPU path.Also applies to: 49-75
src/components/maskeditor/MaskEditorContent.vue (1)
13-37: GPU preview canvas wiring and init look good; consider keeping GPU canvas size in sync on resize
- The additional
gpuCanvasReflayer with dynamicz-20/z-40classes andpointer-events-noneis a clean way to overlay the GPU preview without disturbing input handling.initUIcorrectly:
- Waits for the main canvases to be ready.
- Initializes GPU resources only if
toolManager.brushDrawingexists.- Matches
gpuCanvasRefwidth/height to the mask canvas before callinginitPreviewCanvas.One thing to consider:
- If the mask canvas is resized later (e.g., due to DPI/zoom or container size changes),
gpuCanvasRef’s width/height won’t be updated here. Depending on howuseBrushDrawingandpanZoomhandle resizes, you may want to:
- Either re‑size
gpuCanvasRefinside whatever path resizesmaskCanvas, or- Watch
maskCanvasRef.value.width/heightand keep the GPU preview canvas in lockstep.This would avoid subtle resolution mismatches between the mask and GPU preview on window resizes.
Also applies to: 94-101, 120-171
src/stores/maskEditorStore.ts (2)
20-26: BrushstepSizeAPI is clear; double‑check the differing default values (10 vs 5)
stepSizeinbrushSettingsandsetBrushStepSizeis well‑defined:
- Initial state:
stepSize: 10.- Setter:
_.clamp(step, 1, 100)ensures spacing used by stroke resampling is always positive and bounded.- Reset paths:
resetBrushToDefault()setsstepSize = 5.resetState()also constructsbrushSettingswithstepSize: 5.This means:
- A fresh store instance starts with
stepSize = 10, but both “reset brush” and fullresetStatesnap it to5. If that’s intentional (e.g., you want a slightly different "session initial" vs "reset to ideal default"), it might be worth a short comment; if not, you probably want these three locations to share the same value via a single constant.Example to de‑duplicate:
const DEFAULT_BRUSH_STEP_SIZE = 5 const brushSettings = ref<Brush>({ // … stepSize: DEFAULT_BRUSH_STEP_SIZE }) function resetBrushToDefault(): void { // … brushSettings.value.stepSize = DEFAULT_BRUSH_STEP_SIZE } function resetState(): void { brushSettings.value = { // … stepSize: DEFAULT_BRUSH_STEP_SIZE } }Also applies to: 115-137, 183-190
53-54:clearTrigger/triggerClearandtgpuRootsurface look fine; consider tightening types laterThe new store additions:
const clearTrigger = ref<number>(0)withtriggerClear()incrementing it and both exported.const tgpuRoot = ref<any>(null)exported for GPU integration.This pattern (trigger ref + incrementing action) is a reasonable way to signal clear events to dependents without coupling to specific consumers.
tgpuRootbeinganyis acceptable for now, given it likely wraps a complex TypeGPU root object that would otherwise pull GPU types into the store.If/when the GPU integration stabilizes, you might later introduce a lightweight interface for
tgpuRootinstead ofany, but that’s non‑blocking.Also applies to: 72-75, 175-177, 211-273
src/composables/maskeditor/gpu/GPUBrushRenderer.ts (2)
13-38: Pipeline and buffer setup is coherent; avoid hard‑coding the instance strideOverall the constructor and pipeline setup look correct:
- Vertex layouts match
StrokePoint(vec2 + size + pressure) and the brush shaders.- Uniform and texture bind group layouts align with WGSL bindings.
- Separate pipelines for accumulation, composite, erase, and preview give you flexibility while sharing layouts.
One small fragility:
- You compute
const STROKE_STRIDE = d.sizeOf(StrokePoint) // 16, and use it for the instance buffer size, but the vertex buffer layout still hard‑codesarrayStride: 16and the instance data packing inrenderStrokeInternalassumes 4 floats per instance.If
StrokePointever changes (e.g., adding tilt/angle),STROKE_STRIDEwill update, but the hard‑coded16and the 4‑float packing will silently go out of sync.Consider wiring everything through
STROKE_STRIDEand a single source of truth for the instance layout, for example:this.instanceBuffer = device.createBuffer({ size: MAX_STROKES * STROKE_STRIDE, usage: GPUBufferUsage.VERTEX | GPUBufferUsage.COPY_DST }) // In pipeline vertex config: { arrayStride: STROKE_STRIDE, stepMode: 'instance', attributes: [ { shaderLocation: 1, offset: 0, format: 'float32x2' }, { shaderLocation: 2, offset: 8, format: 'float32' }, { shaderLocation: 3, offset: 12, format: 'float32' } ] }and keeping the packing in
renderStrokeInternalaligned with that layout.Also applies to: 63-224
404-446: Stroke preparation and batched rendering look correct; per‑call allocations could be reduced if needed
prepareStroke(width, height):
- Recreates the accumulation texture when size changes, invalidating dependent bind groups.
- Clears the texture via a short render pass, which is appropriate.
renderStrokeToAccumulatorsimply forwards torenderStrokeInternalusing the accumulate pipeline; logic is straightforward.renderStrokeInternal:
- Writes uniforms matching
BrushUniformsfor each call.- Chunks points into batches of
MAX_STROKES, writes instance data once per batch, and submits one render pass per batch.- Correctly binds quad + instance vertex buffers and index buffer, and uses instanced indexed drawing.
For very long strokes or high‑frequency updates, you may eventually want to reduce allocations:
new ArrayBuffer(UNIFORM_SIZE)andnew Float32Array(batchSize * 4)are created on every call/batch.- These could be reused via a small scratch buffer (or a pre‑allocated
Float32Arraysized toMAX_STROKES * 4) if profiling ever shows GC pressure here.This is purely a performance nit; functionally the implementation looks sound.
Also applies to: 448-627
src/composables/maskeditor/useBrushDrawing.ts (5)
417-483: Minor redundant dirty‑rect updates in CPU brush paths
drawShapecallsupdateDirtyRectafterdrawMaskShape, anddrawMaskShapeitself callsupdateDirtyRectin each branch. For mask‑layer CPU strokes this results in two identical updates per dab:// drawShape (mask path) drawMaskShape(...) updateDirtyRect(point.x, point.y, effectiveRadius) // drawMaskShape branches ctx.drawImage(...) updateDirtyRect(x, y, brushRadius)Functionally this is harmless (it just recomputes the same bounds), but it’s unnecessary work on tight inner loops, especially for large strokes or fast painting.
You can safely drop the extra call in
drawShapeand keep the per‑branch calls insidedrawMaskShape(anddrawRgbShapealready updates the rect internally):- drawMaskShape(..., effectiveRadius, effectiveHardness, opacity, isErasing) - - updateDirtyRect(point.x, point.y, effectiveRadius) + drawMaskShape(..., effectiveRadius, effectiveHardness, opacity, isErasing)This keeps dirty‑rect logic centralized per brush implementation and slightly reduces per‑dab overhead.
Also applies to: 494-539, 551-602
856-887: Dead / confusing path inhandleDrawingfordiff > 20 && !isDrawingIn
handleDrawing:if (diff > 20 && !isDrawing.value) { requestAnimationFrame(async () => { if (!isDrawing.value) return // Fix: Prevent race condition ... await gpuDrawPoint(coords_canvas) }) } else { requestAnimationFrame(async () => { if (!isDrawing.value) return ... await drawWithBetterSmoothing(coords_canvas) }) }Because the outer condition requires
!isDrawing.value, the innerif (!isDrawing.value) returnalways short‑circuits the branch, so this upper path effectively never does any drawing. That makes the intention (hover preview? delayed start?) hard to follow.If the goal was a hover/preview stroke when not actively drawing, this logic needs to be revisited. If it’s legacy smoothing code that’s no longer used, consider removing or simplifying it to a single path guarded by
isDrawing.value:async function handleDrawing(event: PointerEvent): Promise<void> { ... - if (diff > 20 && !isDrawing.value) { - requestAnimationFrame(async () => { - if (!isDrawing.value) return - ... - await gpuDrawPoint(coords_canvas) - }) - } else { - requestAnimationFrame(async () => { - if (!isDrawing.value) return - ... - await drawWithBetterSmoothing(coords_canvas) - }) - } + requestAnimationFrame(async () => { + if (!isDrawing.value) return + try { + if (currentTool === 'eraser' || event.buttons === 2) { + initShape(CompositionOperation.DestinationOut) + } else { + initShape(CompositionOperation.SourceOver) + } + await drawWithBetterSmoothing(coords_canvas) + } catch (error) { + console.error('[useBrushDrawing] Drawing error:', error) + } + }) }This will make the stroke path more predictable and easier to maintain.
269-287: Consider toning down verbose GPU initialization logging in production
initTypeGPUandinitGPUResourceslog quite a lot of information:console.warn('✅ TypeGPU initialized! Root:', root) console.warn('Device info:', root.device.limits) console.warn(`🎨 Initializing GPU resources for ${canvasWidth}x${canvasHeight} canvas`) console.warn('✅ GPU resources initialized successfully') console.warn('✅ Brush renderer initialized')These are very helpful during development and debugging, but at scale (especially on large images or frequent re‑init), they may spam the console for end users.
You might want to:
- Gate these logs behind a dev flag (e.g.,
if (import.meta.env.DEV)), or- Downgrade most of them to
console.debugand keep only one user‑actionable warning for error cases.This keeps the debugging value without cluttering production consoles.
Also applies to: 351-415
1074-1205: GPU readback path looks sound; consider future optimization around full‑frame copiesThe
copyGpuToCanvaspipeline is well‑structured: it lazily (re)allocates readback buffers, runs compute readback, copies into staging buffers, maps both, and then uses a dirty‑rect to limit the region written back to the 2D canvases.One thing to keep in mind for very large canvases:
- You always read back
width * height * 4bytes for both mask and RGB, even though only a subrect may be dirty.- WebGPU doesn’t let you map a subrange without mapping the full buffer, but you can restrict the
copyTextureToBuffer/copyBufferToBufferregion to a smaller area backed by a smaller buffer aligned to row‑pitch constraints.Not necessarily worth changing now, but if you profile and see readback dominating for huge images, a future improvement could be:
- Use a smaller staging/storage buffer sized to the dirty rect (with appropriate row padding).
- Copy only that rect from the textures into those buffers.
- Construct
ImageDatafor just that rect instead of the full frame.For now the implementation is correct and reasonably robust (good use of unmap after copying and guards for invalid dirty rects).
995-1011: Small cleanups: unnecessaryasyncand potential integration with brush setting persistenceA couple of minor nits in the brush‑adjustment handlers:
startBrushAdjustmentandhandleBrushAdjustmentare declaredasyncbut neverawaitanything. They can be plain synchronous functions, which also avoids any accidental unused promise warnings.- After computing
newSizeandnewHardness, you correctly push them into the store. If you want brush adjustments to be persisted immediately, you might consider callingsaveBrushSettings()here (or rely on an external watcher to do that).Example minimal cleanup:
-async function startBrushAdjustment(event: PointerEvent): Promise<void> { +function startBrushAdjustment(event: PointerEvent): void { ... -async function handleBrushAdjustment(event: PointerEvent): Promise<void> { +function handleBrushAdjustment(event: PointerEvent): void { ... store.setBrushSize(newSize) store.setBrushHardness(newHardness) + // Optionally: saveBrushSettings() }Purely optional, but it makes intent a bit clearer.
Also applies to: 1009-1057
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (23)
package.json(3 hunks)pnpm-workspace.yaml(2 hunks)src/components/maskeditor/BrushCursor.vue(3 hunks)src/components/maskeditor/BrushSettingsPanel.vue(3 hunks)src/components/maskeditor/MaskEditorContent.vue(5 hunks)src/components/maskeditor/dialog/TopBarHeader.vue(1 hunks)src/composables/maskeditor/ShiftClick.test.ts(1 hunks)src/composables/maskeditor/StrokeProcessor.test.ts(1 hunks)src/composables/maskeditor/StrokeProcessor.ts(1 hunks)src/composables/maskeditor/brushUtils.test.ts(1 hunks)src/composables/maskeditor/brushUtils.ts(1 hunks)src/composables/maskeditor/gpu/GPUBrushRenderer.ts(1 hunks)src/composables/maskeditor/gpu/brushShaders.ts(1 hunks)src/composables/maskeditor/gpu/gpuSchema.ts(1 hunks)src/composables/maskeditor/splineUtils.ts(1 hunks)src/composables/maskeditor/useBrushDrawing.ts(16 hunks)src/composables/maskeditor/useCanvasHistory.ts(5 hunks)src/composables/maskeditor/useMaskEditorLoader.ts(1 hunks)src/extensions/core/maskeditor/types.ts(1 hunks)src/stores/maskEditorStore.ts(9 hunks)tests-ui/tests/composables/maskeditor/useCanvasHistory.test.ts(2 hunks)tsconfig.json(1 hunks)vite.config.mts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
src/composables/maskeditor/brushUtils.test.ts (1)
src/composables/maskeditor/brushUtils.ts (2)
getEffectiveBrushSize(9-14)getEffectiveHardness(26-34)
src/composables/maskeditor/ShiftClick.test.ts (1)
src/composables/maskeditor/splineUtils.ts (1)
resampleSegment(78-126)
src/composables/maskeditor/StrokeProcessor.ts (1)
src/composables/maskeditor/splineUtils.ts (2)
catmullRomSpline(12-59)resampleSegment(78-126)
src/composables/maskeditor/StrokeProcessor.test.ts (1)
src/composables/maskeditor/StrokeProcessor.ts (1)
StrokeProcessor(4-115)
src/composables/maskeditor/gpu/GPUBrushRenderer.ts (2)
src/composables/maskeditor/gpu/gpuSchema.ts (1)
StrokePoint(13-17)src/composables/maskeditor/gpu/brushShaders.ts (5)
brushVertex(41-47)brushFragment(78-84)blitShader(103-106)compositeShader(126-131)readbackShader(168-171)
src/composables/maskeditor/useBrushDrawing.ts (6)
src/composables/maskeditor/gpu/GPUBrushRenderer.ts (2)
GPUBrushRenderer(20-805)destroy(789-804)src/extensions/core/maskeditor/types.ts (1)
Point(49-52)src/composables/maskeditor/StrokeProcessor.ts (1)
StrokeProcessor(4-115)src/composables/maskeditor/brushUtils.ts (2)
getEffectiveBrushSize(9-14)getEffectiveHardness(26-34)src/utils/colorUtil.ts (2)
hexToRgb(54-71)parseToRgb(132-183)src/composables/maskeditor/splineUtils.ts (1)
resampleSegment(78-126)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: collect
- GitHub Check: lint-and-format
- GitHub Check: setup
- GitHub Check: test
🔇 Additional comments (18)
src/composables/maskeditor/useCanvasHistory.ts (4)
7-9: LGTM: Type definition supports GPU acceleration.The updated type definition correctly supports both
ImageDataandImageBitmap, enabling GPU-accelerated rendering paths while maintaining backward compatibility with CPU-based rendering.
95-103: LGTM: Proper ImageBitmap memory cleanup.The explicit cleanup of
ImageBitmapinstances is essential to prevent memory leaks, asImageBitmapholds GPU resources that aren't automatically garbage collected. The implementation correctly closes both mask and rgb bitmaps when they're removed from the history.
126-147: LGTM: Correct handling of ImageBitmap restoration.The restoration logic appropriately handles both
ImageBitmap(usingclearRect+drawImage) andImageData(usingputImageData). TheclearRectcall beforedrawImageensures a clean slate.Minor edge case: If the canvas is resized between saving and restoring a state, the bitmap dimensions may not match the current canvas size. This is likely an acceptable limitation since canvas resizing would typically invalidate the entire history.
149-158: LGTM: Comprehensive cleanup on clear.The cleanup correctly iterates through all states and closes any
ImageBitmapinstances, preventing memory leaks when history is cleared.src/composables/maskeditor/useMaskEditorLoader.ts (1)
83-103: Widget value handling and mutable node image refs look solidSwitching
nodeImageUrl/nodeImageReftoletand then derivingwidgetFilenamefrom either a raw string or an object withfilenameis a clean way to let the widget override stale node images. The type guards aroundimageWidget.valuehandle the known shapes without risking runtime errors, and the logic leavesnodeImageRefuntouched when the widget value is absent or unsupported.No issues from a correctness or safety standpoint here.
tsconfig.json (1)
46-49: WebGPU type declarations correctly wired into TypeScript configAdding
"@webgpu/types"alongside"vitest/globals"makes WebGPU globals available to the codebase and aligns with the new GPU tooling used elsewhere. As long as the package is installed (it is referenced in package.json) andpnpm typecheckpasses, this looks good.Please confirm
pnpm typecheck(or your Nx typecheck target) succeeds after this change to validate the new type source is correctly picked up.pnpm-workspace.yaml (1)
45-45: Catalog entries for GPU tooling are consistent with package.jsonThe new catalog versions for
@webgpu/types,typegpu, andunplugin-typegpuline up with their"catalog:"usage in package.json, so workspace resolution should work as expected.Please run your usual
pnpm build/Nx pipeline to confirm there are no version/compatibility surprises betweentypegpuandunplugin-typegpuwith these catalog values.Also applies to: 86-86, 90-90
src/extensions/core/maskeditor/types.ts (1)
59-65: Brush API rename tostepSizelooks coherent; ensure all constructors are updatedReplacing the old smoothing parameter with a required
stepSize: numberonBrushmakes the intent (stroke sampling step) much clearer and aligns with the spline/resampling work described in the PR. Since this is part of a public-ish type, double‑check that all places creatingBrushinstances (store defaults, tests, any external integrations) now provide an appropriatestepSizevalue to avoid runtimeundefinedin GPU/CPU paths.package.json (1)
78-78: GPU tooling dependencies are declared consistently with workspace catalogAdding
@webgpu/typesandunplugin-typegpuas devDependencies plustypegpuas a runtime dependency matches how the rest of the tooling is wired (Vite plugin + TypeScript types + runtime GPU code). This should keep installation and resolution clean across the monorepo.After merging, it’s worth checking your production bundle (or existing bundle-size reports) to ensure
typegpuis only pulled into the GPU mask editor path and doesn’t significantly impact core flows.Also applies to: 116-116, 180-181
src/composables/maskeditor/brushUtils.test.ts (2)
5-23: getEffectiveBrushSize tests track the scaling formula wellThe three cases (hardness 1, 0, and 0.5) exactly pin down the current MAX_SCALE = 1.5 interpolation and give solid regression coverage for the utility.
26-45: getEffectiveHardness tests cover proportionality and zero-size edge casesYou verify unchanged hardness when effectiveSize === size, the proportional reduction when effectiveSize grows, and the effectiveSize = 0 guard; this closely matches the helper’s intended behaviour.
src/composables/maskeditor/ShiftClick.test.ts (2)
6-55: Connected-segment test nicely exercises remainder carry-overThis test validates both per-segment expectations and continuity across A→B→C, including checking the carried remainder and absolute x positions, which is exactly what resampleSegment’s contract depends on.
57-83: Short-segment behaviour is well specifiedThe second test clearly codifies that a segment shorter than spacing should emit no points but adjust the remainder, and that the next longer segment then produces a correctly placed sample and updated remainder, giving good coverage for shift-click strokes.
src/components/maskeditor/BrushSettingsPanel.vue (1)
55-62: Increasing brush thickness max to 500 looks safeThe slider still uses min=1 and step=1 and remains bound to store.brushSettings.size, so this should just extend the usable range while keeping existing behaviour intact.
src/composables/maskeditor/StrokeProcessor.test.ts (2)
76-95: 3-point and single-click tests give good coverage of StrokeProcessor edgesThe simple horizontal 3-point stroke validates spacing across a minimal spline, and the single-point click test ensures the processor still emits exactly one point for a tap-only stroke; together they provide strong regression coverage around start/end handling.
Also applies to: 97-107
35-73: The review is based on a misreading of the test codeThe review claims
prevXis hardcoded to 0 and the first distance will be ~0, but the actual test (lines 38–42) computes distances between consecutive output points (outputPoints[i] - outputPoints[i-1]for i ≥ 1), not from a fixed origin. The test correctly asserts all consecutive distances should be approximately equal to spacing.Given the StrokeProcessor and resampleSegment implementation:
- resampleSegment outputs samples at positions 0, spacing, 2*spacing, ... within each segment when startOffset=0
- The first output point is at the start of the first processed segment
- The second point is spacing away
- All consecutive distances between output points should be ~spacing
The test assertion is valid as written. No changes are needed.
Likely an incorrect or invalid review comment.
src/composables/maskeditor/gpu/brushShaders.ts (1)
86-131: Blit/composite/erase/readback shaders look consistent with premultiplied‑alpha workflow
- Blit and composite shaders correctly assume premultiplied textures and avoid double‑darkening by not re‑multiplying alpha.
- The erase variants use
srcFactor: 'zero', dstFactor: 'one-minus-src-alpha', which matches the “destination‑out” intent given the stroke mask in the source.- The readback compute shader safely:
- Bounds‑checks invocation vs
textureDimensions.- Un‑premultiplies RGB when
a > 0.0.- Clamps and packs channels into a little‑endian
u32in the expected RGBA order.Given that, the main remaining assumption is that all textures passed to these shaders are indeed premultiplied and created with formats compatible with
texture_2d<f32>andrgba8unormtargets, which appears to be the case inGPUBrushRenderer.Also applies to: 133-171
src/composables/maskeditor/gpu/GPUBrushRenderer.ts (1)
629-739: Preview blitting, readback, and destroy lifecycle look aligned with the shader contracts
blitToCanvas:
- Clears the destination canvas (with or without a background texture) and then composites the accumulated stroke using the appropriate preview pipeline (composite vs erase).
- Caches bind groups for background and stroke textures to avoid recreating them every frame.
- Updates the same uniform layout as in
compositeStroke, feeding screen size and brush parameters into the preview path.
clearPreviewcorrectly clears the current swapchain texture with a simple render pass.
prepareReadback:
- Reuses
readbackBindGroupbased on(texture, outputBuffer), minimizing churn.- Dispatches workgroups with the expected 8x8 workgroup size, matching the compute shader.
destroy:
- Destroys all owned GPU buffers and the accumulation texture.
- Nulls out cached bind groups and texture/buffer references to avoid stale reuse.
- Appropriately does not destroy external textures/buffers (background/readback targets), which are owned by callers.
From a lifecycle perspective this class looks self‑contained and safe to reuse across multiple strokes and sessions, assuming callers ensure
destroy()is invoked when the renderer is no longer needed (e.g., from the Vue component’s unmount hook).Also applies to: 741-757, 758-787, 789-804
| const brushOpacity = computed(() => { | ||
| return store.brushVisible ? '1' : '0' | ||
| return store.brushVisible ? 1 : 0 | ||
| }) |
There was a problem hiding this comment.
Aligns cursor preview with effective brush size/hardness; consider relaxing float equality
Using getEffectiveBrushSize/getEffectiveHardness for both the cursor radius and gradient makes the preview much closer to the actual brush behavior and the new soft‑edge model, and switching brushOpacity to numeric values is also cleaner for CSS.
One minor robustness point: if (effectiveHardness === 1) relies on an exact floating‑point comparison. If effectiveHardness is computed (rather than directly set to 1), you could occasionally get 0.999... and skip the solid‑brush shortcut unintentionally. Consider either:
- Checking the original
store.brushSettings.hardness === 1, or - Using a tolerance (e.g.,
if (effectiveHardness >= 0.999)).
This would make the “fully hard” visual consistent even in the face of small floating‑point rounding differences.
Also applies to: 46-51, 87-109
🤖 Prompt for AI Agents
In src/components/maskeditor/BrushCursor.vue around lines 42-44 (and also
affecting lines 46-51 and 87-109), the cursor preview should use
getEffectiveBrushSize and getEffectiveHardness for both the radius and gradient,
and brushOpacity should be numeric (1 or 0) for cleaner CSS; replace usages that
compute cursor geometry from raw settings with the effective helpers and set
brushOpacity to numeric values. Also replace the exact floating-point check if
(effectiveHardness === 1) with a robust test such as checking the original
store.brushSettings.hardness === 1 or using a tolerance (e.g., effectiveHardness
>= 0.999) so fully-hard brushes still use the solid shortcut despite tiny
rounding errors.
| export function catmullRomSpline( | ||
| p0: Point, | ||
| p1: Point, | ||
| p2: Point, | ||
| p3: Point, | ||
| t: number | ||
| ): Point { | ||
| // Centripetal Catmull-Rom Spline (alpha = 0.5) to prevent loops and overshoots | ||
| const alpha = 0.5 | ||
|
|
||
| const getT = (t: number, p0: Point, p1: Point) => { | ||
| const d = Math.hypot(p1.x - p0.x, p1.y - p0.y) | ||
| return t + Math.pow(d, alpha) | ||
| } | ||
|
|
||
| const t0 = 0 | ||
| const t1 = getT(t0, p0, p1) | ||
| const t2 = getT(t1, p1, p2) | ||
| const t3 = getT(t2, p2, p3) | ||
|
|
||
| // Map normalized t to parameter range | ||
| const tInterp = t1 + (t2 - t1) * t | ||
|
|
||
| // Safe interpolation for coincident points | ||
| const interp = ( | ||
| pA: Point, | ||
| pB: Point, | ||
| tA: number, | ||
| tB: number, | ||
| t: number | ||
| ): Point => { | ||
| if (Math.abs(tB - tA) < 0.0001) return pA | ||
| const k = (t - tA) / (tB - tA) | ||
| return add(mul(pA, 1 - k), mul(pB, k)) | ||
| } | ||
|
|
||
| // Barry-Goldman pyramidal interpolation | ||
| const A1 = interp(p0, p1, t0, t1, tInterp) | ||
| const A2 = interp(p1, p2, t1, t2, tInterp) | ||
| const A3 = interp(p2, p3, t2, t3, tInterp) | ||
|
|
||
| const B1 = interp(A1, A2, t0, t2, tInterp) | ||
| const B2 = interp(A2, A3, t1, t3, tInterp) | ||
|
|
||
| const C = interp(B1, B2, t1, t2, tInterp) | ||
|
|
||
| return C | ||
| } |
There was a problem hiding this comment.
Spline math looks solid; add an explicit guard for non‑positive spacing in resampleSegment
- The Catmull‑Rom implementation (centripetal α=0.5 with Barry‑Goldman interpolation and coincident‑point guard) looks correct and numerically safe for typical stroke input.
- In
resampleSegment, ifspacing <= 0the innerwhile (nextSampleDist <= …) { nextSampleDist += spacing }loops will never advance and can hang the app. Callers currently clamp brushstepSizeto[1, 100], but since this is a generic utility it would be safer to defensively bail out early.
Example minimal safeguard:
export function resampleSegment(
points: Point[],
spacing: number,
startOffset: number
): { points: Point[]; remainder: number } {
if (spacing <= 0) {
console.warn('[resampleSegment] spacing must be > 0')
return { points: [], remainder: startOffset }
}
// existing implementation…
}Also applies to: 78-125
🤖 Prompt for AI Agents
In src/composables/maskeditor/splineUtils.ts around lines 12-59 and additionally
78-125, add a defensive early-return in resampleSegment to guard against
non‑positive spacing (spacing <= 0) to prevent the inner while loop from never
advancing; log a concise warning (e.g. console.warn('[resampleSegment] spacing
must be > 0')) and return { points: [], remainder: startOffset } so callers get
a safe result and the function exits immediately.
| // Cleanup GPU resources on unmount | ||
| onUnmounted(() => { | ||
| if (renderer) { | ||
| renderer.destroy() | ||
| renderer = null | ||
| } | ||
| if (maskTexture) { | ||
| maskTexture.destroy() | ||
| maskTexture = null | ||
| } | ||
| if (rgbTexture) { | ||
| rgbTexture.destroy() | ||
| rgbTexture = null | ||
| } | ||
| if (readbackStorageMask) { | ||
| readbackStorageMask.destroy() | ||
| readbackStorageMask = null | ||
| } | ||
| if (readbackStorageRgb) { | ||
| readbackStorageRgb.destroy() | ||
| readbackStorageRgb = null | ||
| } | ||
| if (readbackStagingMask) { | ||
| readbackStagingMask.destroy() | ||
| readbackStagingMask = null | ||
| } | ||
| if (readbackStagingRgb) { | ||
| readbackStagingRgb.destroy() | ||
| readbackStagingRgb = null | ||
| } | ||
| // We do not destroy the device as it might be shared or managed by TGPU | ||
| }) |
There was a problem hiding this comment.
Unify GPU cleanup paths and clarify TypeGPU root ownership
There are two separate cleanup flows:
onUnmounted(Lines 236‑267) manually destroysrenderer, textures, and readback buffers but explicitly does not destroy the device /store.tgpuRoot.destroy()(Lines 1210‑1236) also destroysrendererand textures, cleans up readback buffers, and then destroysstore.tgpuRootand nullsdevice.
This creates a few risks:
- If the caller uses the exported
destroy()and Vue also runsonUnmounted, GPU resources (and possibly the TypeGPU root) may be destroyed twice. Whether the underlying WebGPU implementation tolerates double‑destroy is not guaranteed. - The comment in
onUnmountedsays “We do not destroy the device as it might be shared,” butdestroy()does destroystore.tgpuRoot, implying ownership of the global root here, which may break other consumers using the same store.
I’d strongly suggest:
- Deciding a single place that owns
store.tgpuRoot’s lifetime (likely the store or a higher‑level mask editor component). - Making
destroy()the single cleanup entry point for this composable and calling it fromonUnmountedinstead of duplicating logic, or removing thestore.tgpuRoot.destroy()responsibility from this composable.
Example sketch:
onUnmounted(() => {
- if (renderer) { ... }
- if (maskTexture) { ... }
- ...
- // We do not destroy the device as it might be shared or managed by TGPU
+ destroy()
})
function destroy(): void {
- renderer?.destroy()
- ...
- if (store.tgpuRoot) {
- store.tgpuRoot.destroy()
- store.tgpuRoot = null
- }
- device = null
+ // Either:
+ // 1) Only clean up resources owned by this composable, leave tgpuRoot/device
+ // lifetime to the store, OR
+ // 2) Clearly document that this composable owns tgpuRoot and is the only
+ // place allowed to destroy it.
}This will make resource ownership and lifetime easier to reason about and avoid subtle device‑lifecycle bugs.
Also applies to: 1210-1236
| // Mock ImageBitmap for test environment | ||
| if (typeof globalThis.ImageBitmap === 'undefined') { | ||
| globalThis.ImageBitmap = class ImageBitmap { | ||
| width: number | ||
| height: number | ||
| constructor(width = 100, height = 100) { | ||
| this.width = width | ||
| this.height = height | ||
| } | ||
| close() {} | ||
| } as any | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Make ImageBitmap.close() a spy to verify cleanup calls.
The mock close() method is currently a no-op. To properly test the ImageBitmap cleanup logic added in useCanvasHistory.ts (lines 99-100, 152-153), close() should be a spy so tests can verify it's called when states are removed or cleared.
// Mock ImageBitmap for test environment
if (typeof globalThis.ImageBitmap === 'undefined') {
globalThis.ImageBitmap = class ImageBitmap {
width: number
height: number
constructor(width = 100, height = 100) {
this.width = width
this.height = height
}
- close() {}
+ close = vi.fn()
} as any
}📝 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.
| // Mock ImageBitmap for test environment | |
| if (typeof globalThis.ImageBitmap === 'undefined') { | |
| globalThis.ImageBitmap = class ImageBitmap { | |
| width: number | |
| height: number | |
| constructor(width = 100, height = 100) { | |
| this.width = width | |
| this.height = height | |
| } | |
| close() {} | |
| } as any | |
| } | |
| // Mock ImageBitmap for test environment | |
| if (typeof globalThis.ImageBitmap === 'undefined') { | |
| globalThis.ImageBitmap = class ImageBitmap { | |
| width: number | |
| height: number | |
| constructor(width = 100, height = 100) { | |
| this.width = width | |
| this.height = height | |
| } | |
| close = vi.fn() | |
| } as any | |
| } |
🤖 Prompt for AI Agents
In tests-ui/tests/composables/maskeditor/useCanvasHistory.test.ts around lines
21-32, the ImageBitmap mock's close() is currently a no-op; replace it with a
test spy (e.g., a jest.fn() or equivalent) so tests can assert cleanup calls.
Update the mock so close is a spy on the prototype or instance, ensure the spy
is reset/cleared between tests, and then assert close() is called when states
are removed or cleared (corresponding to useCanvasHistory.ts lines 99-100 and
152-153).
| mockMaskCtx = { | ||
| getImageData: vi.fn(() => createMockImageData()), | ||
| putImageData: vi.fn() | ||
| putImageData: vi.fn(), | ||
| clearRect: vi.fn(), | ||
| drawImage: vi.fn() | ||
| } | ||
|
|
||
| mockRgbCtx = { | ||
| getImageData: vi.fn(() => createMockImageData()), | ||
| putImageData: vi.fn() | ||
| putImageData: vi.fn(), | ||
| clearRect: vi.fn(), | ||
| drawImage: vi.fn() | ||
| } |
There was a problem hiding this comment.
Add test coverage for new ImageBitmap functionality.
While the mock infrastructure now supports clearRect and drawImage, there are no tests that exercise the new ImageBitmap-related features:
- Missing: Tests for
saveStatewith providedImageDataorImageBitmapparameters - Missing: Tests that verify
ImageBitmaprestoration usesdrawImageinstead ofputImageData - Missing: Tests that verify
ImageBitmap.close()is called when states exceedmaxStates - Missing: Tests that verify
ImageBitmap.close()is called inclearStates - Missing: Tests for the exposed
currentStateIndexAPI
These features represent core functionality for GPU acceleration and memory management. Without tests, regressions could easily be introduced.
Example test to add:
it('should close ImageBitmap when exceeding maxStates', () => {
const history = useCanvasHistory(2)
const mockBitmap = new ImageBitmap(100, 100)
const closeSpy = vi.spyOn(mockBitmap, 'close')
history.saveInitialState()
history.saveState(mockBitmap, mockBitmap)
history.saveState() // This should trigger removal of oldest state
expect(closeSpy).toHaveBeenCalled()
})
it('should restore ImageBitmap using drawImage', () => {
const history = useCanvasHistory()
const mockBitmap = new ImageBitmap(100, 100)
history.saveInitialState()
history.saveState(mockBitmap, mockBitmap)
history.undo()
expect(mockMaskCtx.drawImage).toHaveBeenCalledWith(mockBitmap, 0, 0)
expect(mockRgbCtx.drawImage).toHaveBeenCalledWith(mockBitmap, 0, 0)
})🤖 Prompt for AI Agents
tests-ui/tests/composables/maskeditor/useCanvasHistory.test.ts lines 56-68: add
unit tests to cover ImageBitmap-related behavior: (1) add a test that saves an
ImageBitmap via saveState (and also one for ImageData), then undo/redo to assert
restoration uses ctx.drawImage (not putImageData) by checking
mockMaskCtx.drawImage and mockRgbCtx.drawImage were called with the bitmap; (2)
add a test that constructs or mocks an ImageBitmap with a spy on close(), set
useCanvasHistory(maxStates=2), call saveInitialState(), saveState(bitmap,
bitmap) twice to exceed maxStates and assert the oldest bitmap.close() was
called; (3) add a test that calls clearStates() after saving ImageBitmap states
and asserts close() was called on all bitmaps; and (4) add tests that assert the
exposed currentStateIndex updates correctly after saveInitialState, saveState,
undo, redo, and clearStates; when ImageBitmap is not constructible in the test
environment, use a simple object with a close() spy and ensure contexts accept
it for drawImage assertions.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/composables/maskeditor/useBrushDrawing.ts (1)
236-267: [Duplicate] GPU cleanup paths still inconsistent.This issue was flagged in a previous review and appears unresolved. The
onUnmountedhook and the exporteddestroy()function both clean up GPU resources, but with different scopes:
onUnmounteddestroys renderer and textures but explicitly does NOT destroystore.tgpuRoot(comment on Line 266)destroy()(Lines 1212-1238) destroys renderer, textures, ANDstore.tgpuRootIf
destroy()is called externally and Vue also runsonUnmounted, resources will be cleaned up twice, potentially causing issues with the underlying WebGPU implementation.As noted in the previous review, clarify ownership of
store.tgpuRootand either:
- Make
destroy()the single cleanup entry point and call it fromonUnmounted, or- Remove
store.tgpuRoot.destroy()from this composable and manage it at the store levelAlso applies to: 1212-1238
🧹 Nitpick comments (1)
src/composables/maskeditor/useBrushDrawing.ts (1)
272-287: Use appropriate log levels for success messages.Several success messages use
console.warninstead ofconsole.logorconsole.info:
- Line 282: "✅ TypeGPU initialized! Root:"
- Line 283: "Device info:"
- Line 374: Initializing GPU resources message
- Line 404: "✅ GPU resources initialized successfully"
- Line 408: "✅ Brush renderer initialized"
- Line 1324: "✅ Preview Canvas Initialized"
These are informational messages, not warnings.
- console.warn('✅ TypeGPU initialized! Root:', root) - console.warn('Device info:', root.device.limits) + console.log('✅ TypeGPU initialized! Root:', root) + console.log('Device info:', root.device.limits)Apply similar changes to Lines 374, 404, 408, and 1324.
Also applies to: 1311-1325
Also applies to: 351-415
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/composables/maskeditor/useBrushDrawing.ts(16 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/composables/maskeditor/useBrushDrawing.ts (6)
src/composables/maskeditor/gpu/GPUBrushRenderer.ts (2)
GPUBrushRenderer(20-805)destroy(789-804)src/extensions/core/maskeditor/types.ts (1)
Point(49-52)src/composables/maskeditor/StrokeProcessor.ts (1)
StrokeProcessor(4-115)src/composables/maskeditor/brushUtils.ts (2)
getEffectiveBrushSize(9-14)getEffectiveHardness(26-34)src/utils/colorUtil.ts (2)
hexToRgb(54-71)parseToRgb(132-183)src/composables/maskeditor/splineUtils.ts (1)
resampleSegment(78-126)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: setup
- GitHub Check: lint-and-format
- GitHub Check: collect
- GitHub Check: test
🔇 Additional comments (18)
src/composables/maskeditor/useBrushDrawing.ts (18)
20-32: LGTM: Debounced cache save with error handling.The debounced
saveBrushToCachefunction properly handles serialization errors and uses a reasonable 300ms delay to avoid excessive writes to localStorage.
92-151: LGTM: Improved brush texture generation with quality enhancements.The brush texture generation now uses:
- Integer dimensions to avoid artifacts
- Pixel center sampling for better accuracy
- Quadratic falloff for improved soft brush appearance
- Chebyshev distance for rectangular brushes
160-193: LGTM: Dirty rectangle tracking for optimization.The dirty rectangle tracking properly uses Infinity sentinels and includes 2px padding for anti-aliasing. This optimization reduces the amount of data transferred from GPU to CPU canvas.
202-209: LGTM: Backward-compatible cache loading.The cached brush settings loading properly falls back to
stepSize: 5for older cached settings that don't have this field, ensuring smooth migration.
212-234: LGTM: Proper watchers for external events and undo/redo.The watchers correctly:
- Clear GPU textures on external clear events
- Skip redundant GPU updates when history is being saved (via
isSavingHistoryflag)- Update GPU textures from canvas state on undo/redo
- Clear preview canvas to remove artifacts
422-483: LGTM: Proper layer and tool delegation in drawShape.The
drawShapefunction correctly determines whether to draw on the RGB or mask canvas based on the active layer and current tool, and properly calculates effective brush size and hardness.
651-683: LGTM: Gradient creation with safety checks.The
createBrushGradientfunction properly handles non-finite coordinates (Lines 661-667) by returning a degenerate gradient, preventing potential WebGL/Canvas errors.
685-714: LGTM: Proper stroke processing with GPU/CPU fallback.The
drawWithBetterSmoothingfunction correctly:
- Uses
StrokeProcessorto generate spline-smoothed, equidistant points- Passes
skipResampling=truetogpuRendersince points are already properly spaced- Provides CPU fallback when GPU is unavailable
- Updates preview on initial draw to ensure background visibility
749-775: LGTM: Eraser+shift-click issue appears resolved.The
drawLinefunction now:
- Always calls
initShape(compositionOp)to set the correctglobalCompositeOperation(Line 765)- Routes to GPU when
rendereris available, regardless of composition operation (Lines 767-768)- Provides CPU fallback when GPU is unavailable
This addresses the previous issue where eraser+shift-click lines were inconsistent between CPU and GPU paths.
781-852: LGTM: Proper drawing initialization with GPU/CPU handling.The
startDrawingfunction correctly:
- Initializes GPU stroke accumulator (Line 788)
- Calculates spacing based on
stepSizepercentage (Lines 802-807)- Handles shift-click line drawing vs normal strokes
- Manages canvas visibility for GPU preview mode (Lines 826-837)
- Initializes
StrokeProcessorwith proper spacing- Includes comprehensive error handling
858-892: LGTM: Proper race condition guards in handleDrawing.The
handleDrawingfunction includes race condition checks (Lines 866, 877) that prevent drawing operations from continuing after the pointer is released, avoiding potential errors.
898-991: LGTM: Comprehensive stroke finalization with proper cleanup.The
drawEndfunction properly:
- Flushes remaining points from
StrokeProcessor(Lines 909-921)- Composites the stroke accumulator with correct settings (Lines 924-952)
- Performs GPU readback with error handling (Lines 957-965)
- Uses
isSavingHistoryflag to prevent redundant GPU updates (Lines 967-972)- Clears preview and restores canvas visibility (Lines 975-989)
1076-1207: LGTM: Proper GPU readback with buffer lifecycle management.The
copyGpuToCanvasfunction correctly:
- Manages readback buffer lifecycle, recreating when size changes (Lines 1097-1131)
- Uses proper buffer usage flags (STORAGE for compute output, MAP_READ for CPU access)
- Copies mapped data with
slice(0)before unmapping (Lines 1163-1172)- Applies dirty rectangle optimization to minimize canvas updates (Lines 1177-1204)
- Provides fallback to full update if dirty rect is invalid
1245-1305: LGTM: GPU point rendering with Photoshop-like accumulation.The
gpuDrawPointfunction correctly implements Photoshop-like brush accumulation by:
- Rendering to accumulator with fixed 0.5 opacity (Line 1265) to avoid creases when overlapping
- Applying final user opacity in the blit/composite pass (Line 1292)
- Providing CPU fallback when GPU unavailable
1311-1325: LGTM: Proper WebGPU canvas configuration.The
initPreviewCanvasfunction correctly configures the WebGPU canvas context with the preferred format and premultiplied alpha mode, which is necessary for proper blending.
1332-1433: LGTM: Comprehensive GPU rendering with accumulation strategy.The
gpuRenderfunction implements a sophisticated brush accumulation strategy:
- Uses fixed 0.5 opacity for accumulator rendering (Lines 1395-1397, 1400) to prevent overlapping artifacts
- Applies final user opacity in the composite/blit pass
- Handles both pre-spaced points (from spline processor) and legacy resampling for shift-click
- Properly updates dirty rect and preview canvas
1438-1459: LGTM: Simple and effective GPU texture clearing.The
clearGPUfunction correctly clears both mask and RGB textures by writing zeros usingdevice.queue.writeTexture.
1461-1473: LGTM: Complete public API exported.The return statement properly exports all necessary functions for external use, including lifecycle management (
destroy,initGPUResources,initPreviewCanvas), drawing operations, brush adjustment, and GPU clearing.
|
@jtydhr88 all relevant Claude issues should be fixed |
|
|
||
| <SliderControl | ||
| :label="t('maskEditor.smoothingPrecision')" | ||
| label="Stepsize" |
There was a problem hiding this comment.
Can you add it into en/main.json for i18n?
3 participants
GPU accelerated brush engine for the mask editor
Tested Browsers
preview1.mp4
preview2.mp4
preview3.mp4
┆Issue is synchronized with this Notion page by Unito