fix: Recompute sdf #142
fix: Recompute sdf #142
Conversation
WalkthroughRenames computeSDF→computeShape across WebGPU and runtime, adds a distance_scale uniform and bind-group binding to the compute shader, threads buffer/size and distance-scale parameters through Zig/WebGPU APIs, introduces SDF texture sizing and per-shape SDF caching/preview state, and expands initialization to accept max buffer size. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as App/UI
participant Logic as Logic (Zig)
participant Size as get_sdf_texture_size
participant WG as WebGPU computeShape
participant Shader as WGSL Shader
participant Tex as Storage Texture
App->>Logic: initState(width,height,maxTex,maxBuf)
App->>Logic: request SDF for shape
Logic->>Size: compute(bounds) -> TextureSize {w,h}
Size-->>Logic: {w,h}
Logic->>WG: compute_shape(points, w, h, distance_scale, texture_id)
WG->>Shader: dispatch (bindings: texture(0), curves(1), uniform(distance_scale@2))
Shader->>Tex: write sdf_value * distance_scale
WG-->>Logic: done
Logic-->>App: render uses updated texture
note over WG,Shader: New uniform @group(0) @binding(2): distance_scale
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Pull Request Overview
Refactors the SDF (Signed Distance Field) computation system by renaming functions and improving texture size management for better performance and scalability.
- Renames
computeSDFtocomputeShapefor clearer function naming - Improves texture size calculation with dynamic scaling based on device limits
- Optimizes SDF regeneration to only occur when necessary
Reviewed Changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/run.ts | Updates function call from computeSDF to computeShape and adds distance scale parameter |
| src/logic/utils.zig | Renames cmpF32 to equalF32 for clarity |
| src/logic/types.zig | Adds TextureSize struct for texture dimension management |
| src/logic/shared.zig | Adds global variables for buffer and texture size limits |
| src/logic/shapes/shapes.zig | Major refactor to improve SDF computation with better state management and scaling |
| src/logic/shapes/paths.zig | Fixes skeleton point size calculation |
| src/logic/index.zig | Updates initialization and shape cache management with new texture sizing |
| src/logic/get_sdf_texture_size.zig | New utility for calculating optimal SDF texture dimensions |
| src/index.ts | Passes device buffer limits to initialization |
| src/WebGPU/programs/initPrograms.ts | Renames computeSDF to computeShape |
| src/WebGPU/programs/computeShape/shader.wgsl | Adds distance scale uniform to shader |
| src/WebGPU/programs/computeShape/getProgram.ts | Updates program with distance scale parameter |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/logic/shapes/shapes.zig (1)
277-308: Only clear outdated_sdf when bounds update succeeds; avoid premature “cache valid”.
Ties into the earlier degenerate-bounds issue. With the update_bounds change, guard the reset:// function has side effect, marks texture as generated pub fn getNewSdfPoint(self: *Shape, allocator: std.mem.Allocator) !?[]Point { - if (self.outdated_sdf) { - try self.update_bounds(allocator, self.preview_point); - } + if (self.outdated_sdf) { + const updated = try self.update_bounds(allocator, self.preview_point); + if (!updated) { + // Keep outdated_sdf = true; wait for more input before emitting points + return null; + } + } const points = try self.getAllPoints( allocator, self.preview_point, active_path_index, ); if (points.len == 0) { return null; } const scale = Matrix3x3.scaling( self.bounds[0].distance(self.bounds[1]) / shared.render_scale, self.bounds[0].distance(self.bounds[3]) / shared.render_scale, ); const padding = (self.props.stroke_width / 2.0) / shared.render_scale; for (points) |*point| { const scaled = scale.get(point); point.x = padding + scaled.x; point.y = padding + scaled.y; } - self.outdated_sdf = false; + self.outdated_sdf = false; return points; }src/logic/index.zig (1)
712-751: Unit test broken by API changes (initState and callback types).
The test currently won’t compile:
- initState now needs (width, height, texture_max_size, max_buffer_size)
- connectOnAssetUpdateCallback expects ([]const AssetSerialized) → void
- resetAssets now takes []const AssetSerialized
Update the test as follows:
test "reset_assets does not call the real update callback" { // Setup initial state - initState(100, 100); + initState(100, 100, 8192, 16_777_216); // Ensure state is cleaned up after the test defer destroyState(); // Define a mock callback function locally, with its own static state. const MockCallback = struct { // This static variable will hold the state for our mock. // It's reset to false before each test run. var was_called: bool = false; - fn assets_update(_: []const images.Serialized) void { + fn assets_update(_: []const AssetSerialized) void { // Modify the static variable within the struct. was_called = true; } fn assets_selection(_: u32) void {} }; // Connect our mock callback. This is the "real" callback for this test. connectOnAssetUpdateCallback(MockCallback.assets_update); connectOnAssetSelectionCallback(MockCallback.assets_selection); // Call the function we are testing - const initial_assets = [_]images.Serialized{.{ - .points = [_]types.PointUV{ - types.PointUV{ .x = 0.0, .y = 0.0, .u = 0.0, .v = 0.0 }, - types.PointUV{ .x = 1.0, .y = 0.0, .u = 1.0, .v = 0.0 }, - types.PointUV{ .x = 1.0, .y = 1.0, .u = 1.0, .v = 1.0 }, - types.PointUV{ .x = 0.0, .y = 1.0, .u = 0.0, .v = 1.0 }, - }, - .texture_id = 1, - .id = 123, - }}; - resetAssets(&initial_assets, false); + const initial_assets = [_]AssetSerialized{ .{ + .img = .{ + .points = [_]types.PointUV{ + .{ .x = 0.0, .y = 0.0, .u = 0.0, .v = 0.0 }, + .{ .x = 1.0, .y = 0.0, .u = 1.0, .v = 0.0 }, + .{ .x = 1.0, .y = 1.0, .u = 1.0, .v = 1.0 }, + .{ .x = 0.0, .y = 1.0, .u = 0.0, .v = 1.0 }, + }, + .texture_id = 1, + .id = 123, + }, + }}; + resetAssets(&initial_assets, false); // for the duration of resetAssets, the update callback should NOT be called try std.testing.expect(!MockCallback.was_called); }If you’d like, I can open a follow-up PR to update any other tests and callsites similarly.
🧹 Nitpick comments (9)
src/logic/utils.zig (1)
18-21: Consider tolerance parameter or relative tolerance to avoid false negativesUsing floatEps(f32) as an absolute threshold is often too tight for world-space values far from 1.0. Optional: add a variant that accepts tolerance, or compute a mixed tolerance.
Option A (drop-in tweak):
-const EPSILON = math.floatEps(f32); +const EPSILON = math.floatEps(f32); // base epsilon pub fn equalF32(a: f32, b: f32) bool { - return @abs(a - b) < EPSILON; + const scale = @max(@abs(a), @abs(b)); + const tol = @max(EPSILON * scale, 1e-6); // floor tolerance at 1e-6 world units + return @abs(a - b) <= tol; }Option B (non-breaking API addition elsewhere in this file):
pub fn equalF32Tol(a: f32, b: f32, tol: f32) bool { return @abs(a - b) <= tol; }src/logic/shapes/paths.zig (1)
115-124: Correct fix: corner radius scales with render_scale nowPassing size / 2.0 makes the vertex marker consistently round at all zooms. This aligns with the control-point sizing and removes the non-scaled constant.
Minor: size is computed twice (Lines 104 and 115). You can compute once above the branch for clarity.
Example outside this hunk:
const size = SKELETON_POINT_SIZE * shared.render_scale; if (is_control_point) { ... } else { ... }src/WebGPU/programs/computeShape/shader.wgsl (1)
26-26: Semantics: negative distance_scale flips inside/outside; clamp or documentIf callers pass a negative value, the sign of signed_distance is inverted in the texture. Either document that distance_scale must be > 0, or clamp/abs it here.
Option (shader-side guard):
- shape_info.signed_distance * u.distance_scale, + shape_info.signed_distance * abs(u.distance_scale),Alternatively, validate on the host and default to 1.0 when unset.
src/index.ts (1)
88-93: Use a safe fallback fordevice.limits.maxBufferSizeinLogic.initStatePassing an undefined
maxBufferSizeinto the Zig sizing logic will yieldNaN. It’s safest to prefer the explicitmaxBufferSizewhen available, and otherwise fall back tomaxStorageBufferBindingSize.Locations to update:
- src/index.ts, around lines 88–93 (the
Logic.initStatecall)- Insert the fallback locals immediately after obtaining
device(just before the call toLogic.initState)Suggested diff in
src/index.ts:--- a/src/index.ts +++ b/src/index.ts @@ -87,6 +87,10 @@ async function main() { const device = await getDevice() + // Texture and buffer size limits (fallbacks for runtimes that omit maxBufferSize) + const maxTextureSize = device.limits.maxTextureDimension2D + const maxBufferSize = + (device.limits as any).maxBufferSize ?? device.limits.maxStorageBufferBindingSize Logic.initState( - projectWidth, - projectHeight, - device.limits.maxTextureDimension2D, - device.limits.maxBufferSize + projectWidth, + projectHeight, + maxTextureSize, + maxBufferSize )src/WebGPU/programs/initPrograms.ts (1)
61-64: Clear the per-frame buffer trash list to prevent unbounded growthcleanup() destroys buffers but does not clear buffersToDestroy, causing repeated iteration over already-destroyed entries and steady growth if cleanup is called per frame.
return function cleanup() { - buffersToDestroy.forEach((buffer) => buffer.destroy()) + buffersToDestroy.forEach((buffer) => buffer.destroy()) + // Reset the list to avoid re-destroying and unbounded growth across frames. + buffersToDestroy.length = 0 }src/WebGPU/programs/computeShape/getProgram.ts (1)
17-21: Nit: rename the returned function for consistencyThe returned function is still named
computeSDF. Rename tocomputeShapeto match file/module naming and reduce confusion.- return function computeSDF( + return function computeShape( passEncoder: GPUComputePassEncoder, curvesDataView: DataView, distanceScaleFactor: number, texture: GPUTexture ) {src/logic/index.d.ts (1)
63-69: Ensure TS definitions and call sites align with latest Zig API changes
- initState signature now includes
max_buffer_size(4th parameter) — verified call site insrc/index.ts:88uses five arguments matching the updated signature.- compute_shape signature should include a new
distance_scaleparameter beforetexture_id— please confirm the definition insrc/logic/index.d.ts(lines ~95–101) and update any TypeScript call sites (none were found via grep).- start_cache callback signature has been standardized to
texture_idas the first parameter — verify the definition insrc/logic/index.d.ts(lines ~116–119) and ensure any JS/TS callback implementations match.Nit: In TS ambient types, consider using camelCase (
distanceScale) for the new parameter to stay idiomatic, provided it doesn’t diverge from the Zig ABI/contracts.src/run.ts (1)
59-63: Threaded distanceScaleFactor looks correct; add basic guards for width/height.
The argument order matches the Zig side (points, w, h, distance_scale, texture). Consider fast‑failing on zero dimensions to avoid creating 0×0 textures or dispatching empty workgroups.Apply this minimal guard:
- compute_shape: (curves_data, width, height, distanceScaleFactor, textureId) => { + compute_shape: (curves_data, width, height, distanceScaleFactor, textureId) => { const curvesDataView = curves_data['*'].dataView - Textures.updateSDF(textureId, width, height) - computeShape(computePass, curvesDataView, distanceScaleFactor, Textures.getTexture(textureId)) + if (width === 0 || height === 0) return + Textures.updateSDF(textureId, width, height) + computeShape( + computePass, + curvesDataView, + distanceScaleFactor, + Textures.getTexture(textureId) + ) },src/logic/shapes/shapes.zig (1)
209-219: onReleasePointer clears active_path_index when closed — OK; consider clearing preview_point.
Stale preview_point may persist visually after release. Optional QoL: reset preview_point here.pub fn onReleasePointer(self: Shape) void { if (active_path_index) |i| { const active_path = self.paths.items[i]; if (active_path.closed) { active_path_index = null; } } is_handle_preview = false; + // Optional: avoid stale preview dot after release + // (safe no-op when editor will call update_preview_point(null) on commit) + // self.preview_point = null; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (13)
src/WebGPU/programs/computeShape/getProgram.ts(1 hunks)src/WebGPU/programs/computeShape/shader.wgsl(2 hunks)src/WebGPU/programs/initPrograms.ts(3 hunks)src/index.ts(1 hunks)src/logic/get_sdf_texture_size.zig(1 hunks)src/logic/index.d.ts(3 hunks)src/logic/index.zig(7 hunks)src/logic/shapes/paths.zig(1 hunks)src/logic/shapes/shapes.zig(14 hunks)src/logic/shared.zig(1 hunks)src/logic/types.zig(1 hunks)src/logic/utils.zig(1 hunks)src/run.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
src/logic/types.zig (1)
src/textures.ts (3)
TextureSource(23-28)add(30-79)img(49-76)
src/logic/get_sdf_texture_size.zig (2)
src/utils/getDefaultPoints.ts (2)
getDefaultTextureScale(26-42)getDefaultPoints(3-21)src/textures.ts (2)
updateSDF(115-126)createSDF(101-113)
src/WebGPU/programs/computeShape/getProgram.ts (2)
src/WebGPU/programs/computeSDF/getProgram.ts (2)
getComputeSDF(3-43)drawShape(17-42)src/WebGPU/programs/drawShape/getProgram.ts (2)
getDrawShape(3-98)drawShape(60-97)
src/WebGPU/programs/initPrograms.ts (2)
src/WebGPU/programs/computeShape/getProgram.ts (1)
getComputeShape(3-53)src/WebGPU/programs/computeSDF/getProgram.ts (2)
drawShape(17-42)getComputeSDF(3-43)
src/WebGPU/programs/computeShape/shader.wgsl (1)
src/WebGPU/programs/computeSDF/getProgram.ts (2)
drawShape(17-42)getComputeSDF(3-43)
src/logic/index.d.ts (2)
src/WebGPU/textureCache.ts (1)
startCache(15-85)src/WebGPU/programs/computeSDF/getProgram.ts (1)
drawShape(17-42)
src/run.ts (2)
src/WebGPU/programs/initPrograms.ts (1)
computeShape(27-27)src/WebGPU/programs/computeSDF/getProgram.ts (2)
drawShape(17-42)getComputeSDF(3-43)
src/logic/index.zig (3)
src/WebGPU/programs/pickShape/getProgram.ts (2)
pickShape(45-82)getDrawShape(3-83)src/WebGPU/programs/drawShape/getProgram.ts (2)
drawShape(60-97)getDrawShape(3-98)src/WebGPU/programs/computeSDF/getProgram.ts (2)
drawShape(17-42)getComputeSDF(3-43)
🔇 Additional comments (26)
src/logic/utils.zig (1)
18-21: No remainingcmpF32references—rename verifiedSearches for
cmpF32across the repository returned no matches, confirming all call sites have been updated toequalF32.src/WebGPU/programs/computeShape/shader.wgsl (2)
17-17: Bind group entries are consistent
The WGSL shader (@group(0) @binding(2) var<uniform> u: Uniforms;) expects bindings in the order 0: texture, 1: curves, 2: uniforms. Insrc/WebGPU/programs/computeShape/getProgram.ts(lines 44–48), the host code creates the bind group with:
- binding 0 →
texture.createView()- binding 1 →
{ buffer: curvesBuffer }- binding 2 →
{ buffer: uniformBuffer }This matches the shader’s binding indices exactly. No changes are needed.
11-14: Uniform buffer layout matches host-side usage—no changes requiredThe WGSL
Uniformsstruct is 4 bytes (onef32), and ingetComputeShapeyou create the GPUBuffer withsize: 4, so the shader layout and host-side buffer size align perfectly. Per the WGSL spec, a singlef32struct has 4 byte size and 4 byte alignment, so no implicit padding is needed. If you do encounter platform-specific junk or zero values indistance_scale, you can revisit padding then, but for now the current setup is correct and consistent.src/logic/shapes/shapes.zig (14)
4-4: New TextureSize import — OK.
Used for sdf_size; keeps type coupling explicit.
18-20: Clarifying comment about non‑struct state — OK.
Localizing “active” state outside Shape struct is an acceptable tradeoff here.
42-45: Skeleton uniform uses constant stroke width 2.0.
Intentional for editor overlay; just confirm it’s independent from render_scale by design.Would you like this to scale with render_scale for high‑DPI consistency?
64-68: Per‑shape preview_point and sdf_size fields — good additions.
- preview_point scoped to the shape eliminates global coupling.
- sdf_size caches the last texture size for smarter invalidation.
89-96: Shape.new: Initialize outdated_sdf = true — correct.
Ensures first SDF is computed without relying on external triggers.
108-114: Initial bounds offset by first absolute point.
This establishes local coordinates early. One caveat: until update_bounds runs, DEFAULT_BOUNDS may overestimate the SDF region.Confirm DEFAULT_BOUNDS is reasonably small to avoid oversized initial textures when only a point/segment exists.
128-134: Start new path vs. extend active path — OK.
Snap threshold is computed from current bounds; matches earlier logic.
137-144: updatePointPreview signature shift (Point instead of x,y) — good.
Cleaner callsites and less param churn; also marks outdated_sdf only when needed.
146-161: update_preview_point avoids redundant invalidation — good.
Comparison via Utils.equalF32 on x/y keeps noise down. Nice use of inf sentinel.
197-207: updateLastHandle — OK.
Converts to local path space correctly and marks outdated_sdf.
232-236: Preview point skeleton rendering — OK.
Appends only when not handling; keeps overlay clean.
271-275: Uniform scaling: stroke_width divided by render_scale — good.
Matches how bounds and SDF are scaled elsewhere.
310-327: getBoundsWithPadding(scale) — OK.
Padding is applied in world units, then scaled; that’s consistent with render_scale handling elsewhere.
333-334: getDrawBounds delegates to getBoundsWithPadding(1) — OK.
Keeps a single source of truth for padding logic.src/logic/index.zig (9)
2-4: Import updates — OK.
New modules (types, images, Msdf, shared, Utils, get_sdf_texture_size) are consistent with the refactor.Also applies to: 6-6, 8-8, 12-15
135-141: Smart SDF invalidation on render_scale changes — good.
Only marks outdated when texture size would grow; avoids churn.
327-330: onPointerUp: release shape pointers only when drawing — good.
Prevents unintended state resets in other tools.
336-337: onPointerMove: pass Point object to shape.updatePointPreview — good.
Matches new API; no issues.
393-394: commitChanges uses update_preview_point(null) — good.
Explicitly clears preview state on commit; pairs with shapes.resetState().
526-529: calculateShapesSDF: short‑circuit when !outdated_sdf — good.
Prevents unnecessary compute submissions.
531-541: SDF size/scale computation pipeline — looks correct.
- bounds = getBoundsWithPadding(1 / render_scale)
- init_width from bounds
- sdf_size via get_sdf_texture_size
- points scaled to texture size
- distance_scale = 1/scale
This is coherent with run.ts and WGSL uniform.
590-592: Editor overlay draw path updated — OK.
Uses new getSkeletonDrawVertexData API; no issues.
19-25: compute_shape signature verified across Zig and TypeScript
All callsites consistently use the five-parameter signature—no stragglers found.
• src/logic/index.zig
– Declaration (lines 19–25) and call (around line 541) both pass 5 arguments
• src/run.ts (lines 59–61)
• src/logic/index.d.ts (lines 95–100)
No further action required.
… int, update naming
|
🎉 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
Performance
Visual
Bug Fixes
Refactor