fix: split fill and stroke #167
Conversation
|
Caution Review failedAn error occurred during the review process. Please try again later. ✨ 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 refactors the shape rendering system by consolidating fill and stroke properties into a unified SDF (Signed Distance Field) effects system. The change unifies fill and stroke rendering into a single system that can handle multiple layers of effects.
- Replaced separate fill/stroke properties with an array of SDF effects that each specify distance ranges and colors
- Updated shader uniforms and GPU programs to use the new unified effect-based rendering approach
- Modified serialization and type definitions to support the new effect structure
Reviewed Changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/svgToShapes/index.ts | Converts fill/stroke parsing to create SDF effects with distance ranges |
| src/svgToShapes/definitions.ts | Adds stroke-width property to definition types |
| src/sanitizeFill.ts | Updates type signature to work with SDF effect fills |
| src/run.ts | Updates pick_shape callback to use uniform data instead of stroke width |
| src/logic/shapes/shapes.zig | Major refactor replacing fill/stroke with SDF effects array and new uniform types |
| src/logic/index.zig | Updates function signatures and adds test SDF effects for new shapes |
| src/index.ts | Updates serialization to handle SDF effects array |
| src/WebGPU/programs/pickShape/shader.wgsl | Changes distance checking logic to use start/end ranges |
| src/WebGPU/programs/pickShape/getProgram.ts | Updates to use uniform data instead of single stroke width value |
| src/WebGPU/programs/initPrograms.ts | Updates buffer size calculations for new uniform structures |
| src/WebGPU/programs/drawShape/*.wgsl | Unifies color functions and updates uniform structures across shaders |
Comments suppressed due to low confidence (2)
src/logic/shapes/shapes.zig:1
- These lines are attempting to deinitialize
strokeandfillproperties that no longer exist after the refactor to SDF effects. This will cause compilation errors. The deinit should iterate oversdf_effectsand calldeinit()on each effect's fill.
const Utils = @import("../utils.zig");
src/WebGPU/programs/pickShape/getProgram.ts:1
- The comment still references 'stroke width' but the uniform structure now uses dist_start and dist_end. Update the comment to reflect the new uniform structure.
import { delayedDestroy } from '../initPrograms'
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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/WebGPU/programs/drawShape/radial-gradient.wgsl (1)
49-56: Clamp stops_count to avoid OOB when > 10If host ever sends >10, indexing u.stops[i] is undefined behavior on GPU.
Example patch outside the changed lines (illustrative):
let count = min(u.stops_count, 10u); let last_index = count - 1u; ... for (var i: u32 = 0u; i < count; i = i + 1u) { ... }I can submit a precise diff if desired.
src/WebGPU/programs/drawShape/linear-gradient.wgsl (1)
45-52: Clamp stops_count to the array size to prevent OOB reads.If u.stops_count > 10, current loop and last_index will read past u.stops. Clamp locally.
- let last_index = u.stops_count - 1u; + let count = min(u.stops_count, 10u); + let last_index = count - 1u; var lower_t = -1.0; var lower_color = u.stops[0u].color; var upper_t = 2.0; var upper_color = u.stops[last_index].color; - for (var i: u32 = 0u; i < u.stops_count; i = i + 1u) { + for (var i: u32 = 0u; i < count; i = i + 1u) {Also applies to: 63-75
src/logic/index.zig (1)
221-274: Potential leak: serialized shapes snapshot not deeply freed on “no changes” path.shape.serialize(std.heap.page_allocator) allocates; when all_match is true, new allocations aren’t deinitialized before clearAndFree/deinit, and last_assets_update’s previous internals aren’t freed anywhere. Consider owning snapshots with an Arena per snapshot and deinit the old arena when replacing, or introduce AssetSerialized.deinit() and call it on both the discarded new list and the old last_assets_update before freeing the slice.
src/logic/shapes/shapes.zig (2)
385-397: Guard against stop count overflow and zero-initialize stopsCurrent loops will write past the fixed-size [10] arrays if gradients have >10 stops, and leave unused entries uninitialized.
- var stops: [10]UniformGradientStop = undefined; - for (gradient.stops.items, 0..) |stop, i| { + var stops: [10]UniformGradientStop = std.mem.zeroes([10]UniformGradientStop); + const count_lin: usize = @min(gradient.stops.items.len, stops.len); + for (gradient.stops.items[0..count_lin], 0..) |stop, i| { stops[i] = UniformGradientStop{ .offset = stop.offset, .color = .{ stop.color[0] * self.props.opacity, stop.color[1] * self.props.opacity, stop.color[2] * self.props.opacity, stop.color[3] * self.props.opacity, }, }; } ... - .stops_count = gradient.stops.items.len, + .stops_count = @as(u32, @intCast(count_lin)),- var stops: [10]UniformGradientStop = undefined; - for (gradient.stops.items, 0..) |stop, i| { + var stops: [10]UniformGradientStop = std.mem.zeroes([10]UniformGradientStop); + const count_rad: usize = @min(gradient.stops.items.len, stops.len); + for (gradient.stops.items[0..count_rad], 0..) |stop, i| { stops[i] = UniformGradientStop{ .offset = stop.offset, .color = .{ stop.color[0] * self.props.opacity, stop.color[1] * self.props.opacity, stop.color[2] * self.props.opacity, stop.color[3] * self.props.opacity, }, }; } ... - .stops_count = gradient.stops.items.len, + .stops_count = @as(u32, @intCast(count_rad)),Also applies to: 410-421
595-603: Fix deinit: references removed fields and leaks per-effect fills
self.props.stroke/fillno longer exist; also need to deinit each effect’s fill and the effects list.pub fn deinit(self: *Shape) void { for (self.paths.items) |path| { path.deinit(); } self.paths.deinit(); - self.props.stroke.deinit(); - self.props.fill.deinit(); + for (self.props.sdf_effects.items) |effect| { + effect.fill.deinit(); + } + self.props.sdf_effects.deinit(); }
🧹 Nitpick comments (18)
src/WebGPU/programs/pickShape/shader.wgsl (2)
37-37: Reduce texel addressing bias at edgesRounding avoids systematic floor bias when casting to u32.
- let dist = textureLoad(texture, vec2u(in.uv)).r; + let dist = textureLoad(texture, vec2u(round(in.uv))).r;
39-41: Fix comment to reflect actual discard conditionCurrent comment mentions only “outer boundary”.
- discard; // outside the outer boundary + discard; // outside [dist_end, dist_start] bandsrc/sanitizeFill.ts (1)
1-1: Ensure SdfEffect type is imported or globally declaredIf SdfEffect isn’t ambient, add an explicit type-only import to prevent TS errors.
+import type { SdfEffect } from 'logic/index' export default function sanitizeFill(fill: SdfEffect['fill']): SdfEffect['fill'] {If the path differs, adjust accordingly.
src/run.ts (1)
120-126: Guard uniform shape and align namingAt runtime, ensure uniform has dataView; minor naming consistency helps readability.
- pick_shape: (bound_box_data, uniform, textureId) => { + pick_shape: (bound_box_data, uniform, textureId) => { + if (!uniform || !('dataView' in uniform)) { + throw Error('pick_shape: missing uniform.dataView') + } pickShape( pickPass, bound_box_data['*'].dataView, uniform.dataView, Textures.getTexture(textureId) ) },src/WebGPU/programs/pickShape/getProgram.ts (2)
12-12: Remove dead constantuniformBufferSize is no longer used.
- const uniformBufferSize = (1 /*stroke width*/ + /*padding*/ 3) * 4
41-46: Nit: function name mismatch with file roleOptional rename to improve clarity (importers use default, so this is cosmetic): getPickShape.
src/WebGPU/programs/initPrograms.ts (2)
77-78: Solid uniform layout: confirm host packing matches WGSLCounts assume 2 f32 (dist), 2 padding, 4 color = 32B. Verify host writes 2 dummy floats after dist_end.
Would you like a small shared helper computeUniformByteLength(...) to centralize these layouts?
83-90: Linear gradient layout: double-check paddingYou reduced padding to 1; ensure WGSL struct keeps 16B alignment before vec2 fields to avoid misreads on some drivers.
I can generate a table mapping WGSL fields → float slots to prevent drift across files.
src/WebGPU/programs/drawShape/solid.wgsl (1)
3-7: Uniform layout OK; keep offsets consistent across effects.dist_start/dist_end first with vec2 padding ensures color at 16B alignment. Mirror this exact layout in all drawShape shaders and keep TS/Zig packing in sync.
Optionally document intended offsets:
// Offsets (bytes): dist_start=0, dist_end=4, padding=8, color=16src/index.ts (1)
184-185: Finish the TODO: strip internal properties from effect.fill.Provide a dedicated sanitizer for public payloads.
I can add sanitizeFillPublic(effect.fill) and replace this call. Want me to open a PR note with an implementation?
src/WebGPU/programs/drawShape/linear-gradient.wgsl (1)
35-42: Degenerate-axis: comment and code disagree; pick one.Comment says “horizontal gradient across world_uv.x” but code uses uv.x. Either use world_uv.x or update the comment for clarity.
Apply one of:
- // Degenerate axis: fall back to horizontal gradient across world_uv.x - t_uv = clamp(uv.x, 0.0, 1.0); + // Degenerate axis: fall back to horizontal gradient across world_uv.x + t_uv = clamp(world_uv.x, 0.0, 1.0);or
- // Degenerate axis: fall back to horizontal gradient across world_uv.x + // Degenerate axis: fall back to horizontal gradient across uv.x (normalized)src/logic/index.zig (3)
309-327: Avoid hardcoded demo effects in pointer-down path.These defaults (three effects with fixed distances/colors) look like test/demo data; move behind a feature flag or inject from UI to avoid surprising new users.
- const props = shapes.SerializedProps{ + const props = getDefaultDrawProps() orelse shapes.SerializedProps{
720-727: Effect draw order may affect visual blending.If effects overlap, ordering changes output. Consider sorting by abs(dist_start) descending (outer to inner) or defining a deterministic z-order.
784-790: Same per-effect ordering concern applies during direct draw.Keep order consistent with cache rendering to avoid cache vs. live draw mismatches.
src/svgToShapes/index.ts (3)
193-197: Initialize sdf_effects with capacity if large SVGs are expected.Minor: if many shapes include both fill and stroke, pre-sizing can reduce reallocs.
227-253: Stroke effect: guard against non-finite/negative widths.Ensure width is finite and ≥ 0 before pushing the effect; clamp or skip otherwise to avoid NaNs in uniforms.
- const width = ensureNumber(props['stroke-width'], 1) + let width = ensureNumber(props['stroke-width'], 1) + if (!Number.isFinite(width) || width < 0) width = 0
55-75: Gradient units/transform assumptions need verification.The code always normalizes to objectBoundingBox after applying gradientTransform. If defs can specify gradientUnits='userSpaceOnUse', this will be wrong. Either honor gradientUnits or assert we only support objectBoundingBox here.
Also applies to: 80-108
src/logic/index.d.ts (1)
152-156: Consider a Pick uniform alias for parityFor symmetry with Draw: introduce a TS alias and use it here.
+type ShapePickUniform = PointerDataView ... - pick_shape: ( + pick_shape: ( bound_box_data: ArrayPointerDataView, - uniformData: PointerDataView, + uniformData: ShapePickUniform, sdf_texture_id: number ) => void
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
src/WebGPU/programs/drawShape/base.wgsl(1 hunks)src/WebGPU/programs/drawShape/linear-gradient.wgsl(1 hunks)src/WebGPU/programs/drawShape/radial-gradient.wgsl(1 hunks)src/WebGPU/programs/drawShape/solid.wgsl(1 hunks)src/WebGPU/programs/initPrograms.ts(2 hunks)src/WebGPU/programs/pickShape/getProgram.ts(2 hunks)src/WebGPU/programs/pickShape/shader.wgsl(2 hunks)src/index.ts(1 hunks)src/logic/index.d.ts(4 hunks)src/logic/index.zig(6 hunks)src/logic/shapes/shapes.zig(9 hunks)src/run.ts(1 hunks)src/sanitizeFill.ts(1 hunks)src/svgToShapes/definitions.ts(1 hunks)src/svgToShapes/index.ts(3 hunks)
🔇 Additional comments (25)
src/svgToShapes/definitions.ts (1)
44-45: LGTM: typing addition is safe and usefulOptional 'stroke-width' on Def is consistent with other AttrValue fields.
src/WebGPU/programs/pickShape/getProgram.ts (1)
57-64: Good call on delayedDestroy for vertexBufferPrevents piling up transient buffers during picking.
src/WebGPU/programs/initPrograms.ts (1)
95-102: Radial gradient layout: include radius_ratio here is correctMatches WGSL (u32 stops_count + f32 radius_ratio). Looks consistent.
src/WebGPU/programs/drawShape/radial-gradient.wgsl (2)
7-14: Uniform header change looks correctdist_start/dist_end placement before counts keeps the first 16B nicely packed.
18-18: Unified getColor signature: OKMatches the per-effect approach; sdf/world_uv/uv order is consistent with other shaders.
src/WebGPU/programs/drawShape/solid.wgsl (1)
11-13: getColor passthrough looks good.Matches the new single-color path.
src/index.ts (1)
181-185: Confirm external API change for onAssetsUpdate
No internal references toprops.fill/strokeremain in the serialized output—onlyprops.sdf_effectsis emitted. Ensure all downstream consumers ofonAssetsUpdateare updated to parseprops.sdf_effectsinstead of the oldprops.fill/stroke.src/WebGPU/programs/drawShape/linear-gradient.wgsl (1)
6-14: Confirm CPU→GPU uniform layout matches WGSL (Stop stride = 32 bytes).With Stop{ vec4f + f32 } the struct alignment is 16 and size rounds up to 32 bytes per element. Ensure the TS/Zig packing writes 32-byte strides for stops, and that the padding u32 keeps start/end at the expected offsets.
src/logic/index.zig (2)
26-36: ABI check: passing DrawUniform/PickUniform by value across JS↔Zig.zigar interop can be picky with non-primitive params. Ensure shapes.DrawUniform/PickUniform are extern-friendly (no slices/optionals) and that the JS side passes a compatible DataView/byte buffer.
877-883: Pick path: verify discard thresholds match draw path.Ensure getPickUniform(effect) encodes the same dist_start/dist_end and tolerance as draw to prevent pick/invisible mismatches around the ramps.
src/svgToShapes/index.ts (1)
31-35: Return type change LGTM; check ambient type availability.SdfEffect['fill'] must be in scope via types from logic/index.d.ts. If not globally visible, import the type explicitly to avoid TS tooling drift.
src/logic/index.d.ts (5)
40-44: SdfEffect API looks goodShape matches Zig’s SdfEffect and the new per-effect model.
46-50: Props change to sdf_effects is clearGood, aligns with per-effect uniforms.
64-65: cache_texture_id nullabilityNice improvement to reflect absence of cache.
91-95: ShapeDrawUniform alias is fineConsistent with DrawUniform variants.
146-150: draw_shape signature aligns with per-effect uniformsNo issues.
src/logic/shapes/shapes.zig (9)
50-55: SdfEffect struct LGTMMatches the per-effect rendering model.
56-61: SerializedSdfEffect LGTMKeeps dist and fill payloads clearly separated.
66-75: Props/SerializedProps migration LGTMArrayList for effects is appropriate here.
115-123: Effect construction LGTMCorrectly allocates fill instances per effect.
308-317: Skeleton uniform computation LGTMUses render_scale and sdf_scale correctly.
361-366: PickUniform generation LGTMScales distances as expected.
368-436: Opacity handling looks consistentPremultiplies color channels; if pipeline expects premultiplied alpha, this is correct.
Please confirm shaders treat incoming colors as premultiplied. If not, leave alpha as-is and only multiply RGB.
563-593: Serialization path LGTMEffects are serialized with distances and fill payloads.
638-648: Public DrawUniform/PickUniform LGTMType surface matches TS declarations and program plumbing.
| sdf_effects: [...shape.props.sdf_effects].map((effect) => ({ | ||
| dist_start: effect.dist_start, | ||
| dist_end: effect.dist_end, | ||
| fill: sanitizeFill(effect.fill), // TODO: correctly filter out zigar added properties | ||
| })), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden against missing/legacy effects array.
Use a safe fallback to avoid runtime errors when sdf_effects is undefined (older assets).
Apply this diff:
- sdf_effects: [...shape.props.sdf_effects].map((effect) => ({
+ sdf_effects: [...(shape.props.sdf_effects ?? [])].map((effect) => ({
dist_start: effect.dist_start,
dist_end: effect.dist_end,
fill: sanitizeFill(effect.fill), // TODO: correctly filter out zigar added properties
})),📝 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.
| sdf_effects: [...shape.props.sdf_effects].map((effect) => ({ | |
| dist_start: effect.dist_start, | |
| dist_end: effect.dist_end, | |
| fill: sanitizeFill(effect.fill), // TODO: correctly filter out zigar added properties | |
| })), | |
| sdf_effects: [...(shape.props.sdf_effects ?? [])].map((effect) => ({ | |
| dist_start: effect.dist_start, | |
| dist_end: effect.dist_end, | |
| fill: sanitizeFill(effect.fill), // TODO: correctly filter out zigar added properties | |
| })), |
🤖 Prompt for AI Agents
In src/index.ts around lines 181 to 185, the code assumes
shape.props.sdf_effects always exists and spreads it before mapping, which
throws if sdf_effects is undefined for older assets; change to use a safe
fallback array (e.g. (shape.props.sdf_effects || []) or
Array.from(shape.props.sdf_effects || [])) before mapping so the map runs on an
empty array when missing, and keep the sanitizeFill call inside the map as-is.
| fn getSdfPadding(self: Shape) Point { | ||
| return Point{ | ||
| .x = 1.0 + self.props.stroke_width / 2.0, | ||
| .y = 1.0 + self.props.stroke_width / 2.0, | ||
| }; | ||
| var padding = consts.POINT_ZERO; | ||
| // because of skeleton render, we cannot od less than zero | ||
|
|
||
| for (self.props.sdf_effects.items) |effect| { | ||
| if (std.math.isInf(effect.dist_end)) { | ||
| std.debug.print("SDF effect dist_end cannot be infinite!\nShape ID: {d}, effect: {any}\n", .{ self.id, effect }); | ||
| @panic("SDF effect dist_end cannot be infinite!"); | ||
| } | ||
| padding.x = @max(padding.x, -effect.dist_end); | ||
| padding.y = @max(padding.y, -effect.dist_end); | ||
| } | ||
|
|
||
| // we do smoothing in shaders wit fwidth(), so it's 1px to make sure we wont cut it out | ||
| padding.x += 1.0; | ||
| padding.y += 1.0; | ||
|
|
||
| return padding; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Padding should consider both dist_start and -dist_end; also fix typos and validate dist_start
Currently only -dist_end is used; outward extent can be max(dist_start, -dist_end). Also validate dist_start, and fix comments.
- var padding = consts.POINT_ZERO;
- // because of skeleton render, we cannot od less than zero
+ var padding = consts.POINT_ZERO;
+ // because of skeleton render, we cannot do less than zero
for (self.props.sdf_effects.items) |effect| {
- if (std.math.isInf(effect.dist_end)) {
+ if (std.math.isInf(effect.dist_end) or std.math.isInf(effect.dist_start)) {
std.debug.print("SDF effect dist_end cannot be infinite!\nShape ID: {d}, effect: {any}\n", .{ self.id, effect });
@panic("SDF effect dist_end cannot be infinite!");
}
- padding.x = @max(padding.x, -effect.dist_end);
- padding.y = @max(padding.y, -effect.dist_end);
+ const eff_pad = @max(effect.dist_start, -effect.dist_end);
+ const eff_non_neg = @max(0.0, eff_pad);
+ padding.x = @max(padding.x, eff_non_neg);
+ padding.y = @max(padding.y, eff_non_neg);
}
- // we do smoothing in shaders wit fwidth(), so it's 1px to make sure we wont cut it out
+ // we do smoothing in shaders with fwidth(), so add 1px to avoid cutting it out
padding.x += 1.0;
padding.y += 1.0;
return padding;📝 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.
| fn getSdfPadding(self: Shape) Point { | |
| return Point{ | |
| .x = 1.0 + self.props.stroke_width / 2.0, | |
| .y = 1.0 + self.props.stroke_width / 2.0, | |
| }; | |
| var padding = consts.POINT_ZERO; | |
| // because of skeleton render, we cannot od less than zero | |
| for (self.props.sdf_effects.items) |effect| { | |
| if (std.math.isInf(effect.dist_end)) { | |
| std.debug.print("SDF effect dist_end cannot be infinite!\nShape ID: {d}, effect: {any}\n", .{ self.id, effect }); | |
| @panic("SDF effect dist_end cannot be infinite!"); | |
| } | |
| padding.x = @max(padding.x, -effect.dist_end); | |
| padding.y = @max(padding.y, -effect.dist_end); | |
| } | |
| // we do smoothing in shaders wit fwidth(), so it's 1px to make sure we wont cut it out | |
| padding.x += 1.0; | |
| padding.y += 1.0; | |
| return padding; | |
| } | |
| fn getSdfPadding(self: Shape) Point { | |
| var padding = consts.POINT_ZERO; | |
| // because of skeleton render, we cannot do less than zero | |
| for (self.props.sdf_effects.items) |effect| { | |
| if (std.math.isInf(effect.dist_end) or std.math.isInf(effect.dist_start)) { | |
| std.debug.print("SDF effect dist_end cannot be infinite!\nShape ID: {d}, effect: {any}\n", .{ self.id, effect }); | |
| @panic("SDF effect dist_end cannot be infinite!"); | |
| } | |
| const eff_pad = @max(effect.dist_start, -effect.dist_end); | |
| const eff_non_neg = @max(0.0, eff_pad); | |
| padding.x = @max(padding.x, eff_non_neg); | |
| padding.y = @max(padding.y, eff_non_neg); | |
| } | |
| // we do smoothing in shaders with fwidth(), so add 1px to avoid cutting it out | |
| padding.x += 1.0; | |
| padding.y += 1.0; | |
| return padding; | |
| } |
🤖 Prompt for AI Agents
In src/logic/shapes/shapes.zig around lines 543 to 561, the padding computation
only uses -effect.dist_end and neglects outward extent from effect.dist_start,
and comments contain typos; update the loop to validate both effect.dist_end and
effect.dist_start (check for infinity and panic/log with shape id if invalid),
compute per-axis padding using the maximum of existing padding and
max(effect.dist_start, -effect.dist_end) (i.e., consider outward and inward
extents), then add the 1.0 pixel safety margin; also fix comment typos ("do"
instead of "od", "with" instead of "wit", "won't" instead of "wont") and ensure
debug messages reference the correct field when logging errors.
| const UniformLinearGradient = extern struct { | ||
| stroke_width: f32, | ||
| dist_start: f32, | ||
| dist_end: f32, | ||
| stops_count: u32, | ||
| padding: [2]f32 = .{ 0.0, 0.0 }, // Padding for alignment | ||
| padding: u32 = 0.0, | ||
| start: Point, | ||
| end: Point, | ||
| stops: [10]UniformGradientStop, | ||
| }; | ||
|
|
There was a problem hiding this comment.
Invalid default literal type for padding
padding: u32 = 0.0 won’t compile; use integer zero.
- padding: u32 = 0.0,
+ padding: u32 = 0,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const UniformLinearGradient = extern struct { | |
| stroke_width: f32, | |
| dist_start: f32, | |
| dist_end: f32, | |
| stops_count: u32, | |
| padding: [2]f32 = .{ 0.0, 0.0 }, // Padding for alignment | |
| padding: u32 = 0.0, | |
| start: Point, | |
| end: Point, | |
| stops: [10]UniformGradientStop, | |
| }; | |
| const UniformLinearGradient = extern struct { | |
| dist_start: f32, | |
| dist_end: f32, | |
| stops_count: u32, | |
| padding: u32 = 0, | |
| start: Point, | |
| end: Point, | |
| stops: [10]UniformGradientStop, | |
| }; |
🤖 Prompt for AI Agents
In src/logic/shapes/shapes.zig around lines 618 to 627 the UniformLinearGradient
field declaration uses a floating-point default literal for a u32 field
("padding: u32 = 0.0"), which is a type mismatch and won't compile; change the
default to an integer zero (e.g., = 0) or remove the default so the literal
matches the u32 type.
| let serializedFill: SdfEffect['fill'] | null = null | ||
|
|
||
| if (m) { | ||
| const def = defs[m[1]] | ||
| if (def) { | ||
| const grad = toRuntimeGradient(def, boundingBox, fillOpacity) | ||
| if (grad) serializedProps.fill = grad | ||
| if (grad) { | ||
| serializedFill = grad | ||
| } | ||
| } | ||
| } else { | ||
| const rgba = parseColor(fill, fillOpacity) | ||
| serializedProps.fill = { solid: rgba } | ||
| serializedFill = { solid: rgba } | ||
| } | ||
|
|
||
| if (serializedFill) { | ||
| serializedProps.sdf_effects.push({ | ||
| dist_start: Infinity, | ||
| dist_end: 0, | ||
| fill: serializedFill, | ||
| }) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Don’t emit Infinity for dist_start; it can explode padding/texture sizing.
Infinity may propagate into Zig padding and texture-size calculations. Use a large but finite extent (e.g., shape diagonal) for fill’s outer threshold.
- serializedProps.sdf_effects.push({
- dist_start: Infinity,
+ const w = boundingBox.max_x - boundingBox.min_x
+ const h = boundingBox.max_y - boundingBox.min_y
+ const dist = Math.hypot(w, h) || 1
+ serializedProps.sdf_effects.push({
+ dist_start: dist,
dist_end: 0,
fill: serializedFill,
})📝 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.
| let serializedFill: SdfEffect['fill'] | null = null | |
| if (m) { | |
| const def = defs[m[1]] | |
| if (def) { | |
| const grad = toRuntimeGradient(def, boundingBox, fillOpacity) | |
| if (grad) serializedProps.fill = grad | |
| if (grad) { | |
| serializedFill = grad | |
| } | |
| } | |
| } else { | |
| const rgba = parseColor(fill, fillOpacity) | |
| serializedProps.fill = { solid: rgba } | |
| serializedFill = { solid: rgba } | |
| } | |
| if (serializedFill) { | |
| serializedProps.sdf_effects.push({ | |
| dist_start: Infinity, | |
| dist_end: 0, | |
| fill: serializedFill, | |
| }) | |
| } | |
| if (serializedFill) { | |
| const w = boundingBox.max_x - boundingBox.min_x | |
| const h = boundingBox.max_y - boundingBox.min_y | |
| const dist = Math.hypot(w, h) || 1 | |
| serializedProps.sdf_effects.push({ | |
| dist_start: dist, | |
| dist_end: 0, | |
| fill: serializedFill, | |
| }) | |
| } |
|
|
||
| let dist = sdf.r; | ||
| let width = fwidth(dist); | ||
| let width = fwidth(dist) * 0.5; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid AA band collapse at extreme zoom.
Guarantee a minimal width to keep edges stable.
Apply this diff:
- let width = fwidth(dist) * 0.5;
+ let width = max(fwidth(dist) * 0.5, 1e-4);📝 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.
| let width = fwidth(dist) * 0.5; | |
| let width = max(fwidth(dist) * 0.5, 1e-4); |
🤖 Prompt for AI Agents
In src/WebGPU/programs/drawShape/base.wgsl around line 52, the AA width is
computed as "let width = fwidth(dist) * 0.5;" which can collapse at extreme
zoom; change it to enforce a minimum width (e.g. let width = max(fwidth(dist) *
0.5, MIN_AA_WIDTH)) and define MIN_AA_WIDTH as a small constant or a value
derived from viewport scale (for example 1.0 / max(viewportDimension, 1024.0))
so edges remain stable at high zoom.
| let inner_alpha = smoothstep(u.dist_start - width, u.dist_start + width, dist); | ||
| let outer_alpha = smoothstep(u.dist_end - width, u.dist_end + width, dist); | ||
| let alpha = outer_alpha - inner_alpha; | ||
| let color = getColor(sdf, vsOut.uv, vsOut.norm_uv); | ||
|
|
||
| let fill_alpha = smoothstep(hs - width, hs + width, dist); | ||
| let fill_color = getFillColor(sdf, vsOut.uv, vsOut.norm_uv); | ||
| let stroke_color = getStrokeColor(sdf, vsOut.uv, vsOut.norm_uv); | ||
| let color = mix(stroke_color, fill_color, fill_alpha); | ||
|
|
||
| let total_alpha = smoothstep(-hs - width, -hs + width, dist); | ||
|
|
||
| // return vec4f(color.rgb, color.a * total_alpha); | ||
| return vec4f(fill_color.rgb, fill_color.a * fill_alpha); | ||
| return vec4f(color.rgb, color.a * alpha); |
There was a problem hiding this comment.
Clamp alpha and tolerate reversed thresholds (prevents negative/NaN blending).
Sort the thresholds and clamp the difference to [0,1] to avoid artifacts when dist_start > dist_end or due to precision.
Apply this diff:
- let inner_alpha = smoothstep(u.dist_start - width, u.dist_start + width, dist);
- let outer_alpha = smoothstep(u.dist_end - width, u.dist_end + width, dist);
- let alpha = outer_alpha - inner_alpha;
+ let start = min(u.dist_start, u.dist_end);
+ let end = max(u.dist_start, u.dist_end);
+ let inner_alpha = smoothstep(start - width, start + width, dist);
+ let outer_alpha = smoothstep(end - width, end + width, dist);
+ let alpha = clamp(outer_alpha - inner_alpha, 0.0, 1.0);🤖 Prompt for AI Agents
In src/WebGPU/programs/drawShape/base.wgsl around lines 54 to 59, the alpha
calculation can produce negative or NaN results when u.dist_start > u.dist_end
or due to precision; ensure the two thresholds are ordered (use min/max or swap)
before calling smoothstep, compute outer_alpha and inner_alpha with those sorted
thresholds, then compute alpha = clamp(outer_alpha - inner_alpha, 0.0, 1.0) so
the final returned color uses a safe, clamped alpha.
| let inner_alpha = smoothstep(u.dist_start - width, u.dist_start + width, dist); | ||
| let outer_alpha = smoothstep(u.dist_end - width, u.dist_end + width, dist); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify u binding and presence across all effect shaders.
This module references u.dist_start/end and getColor(...). Ensure every effect shader defines @group(0) @binding(0) var<uniform> u: Uniforms; with those fields and the same binding indices.
Run:
🏁 Script executed:
#!/bin/bash
# Check that all drawShape shaders define `u: Uniforms` and include dist_start/dist_end
rg -nC2 --type wgsl -g 'src/WebGPU/programs/drawShape/*.wgsl' \
-e '@group\(0\)\s*@binding\(0\)\s*var<uniform>\s*u:\s*Uniforms' \
-e 'struct\s+Uniforms\s*\{[^}]*dist_start:\s*f32,[^}]*dist_end:\s*f32,'
# Sanity: ensure `getColor` exists in each effect
rg -n --type wgsl -g 'src/WebGPU/programs/drawShape/*.wgsl' 'fn\s+getColor\s*\('Length of output: 57
🏁 Script executed:
#!/bin/bash
# Check that all drawShape shaders define `u: Uniforms` with the proper binding
echo "Files missing uniform binding:"
rg -L -n -g 'src/WebGPU/programs/drawShape/*.wgsl' '@group\(0\)\s*@binding\(0\)\s*var<uniform>\s*u:\s*Uniforms'
# Check that all shaders include dist_start and dist_end in Uniforms
echo "Files missing dist_start or dist_end:"
for file in src/WebGPU/programs/drawShape/*.wgsl; do
missing=false
if ! grep -q 'dist_start\s*:' "$file"; then
echo " Missing dist_start in $file"; missing=true
fi
if ! grep -q 'dist_end\s*:' "$file"; then
echo " Missing dist_end in $file"; missing=true
fi
$missing && continue || echo " Both fields present in $file"
done
# Check for getColor function in each shader
echo "Files missing getColor function:"
rg -L -n -g 'src/WebGPU/programs/drawShape/*.wgsl' 'fn\s+getColor\s*\('Length of output: 1143
🏁 Script executed:
#!/usr/bin/env bash
set -e
echo "=== Checking uniform binding in drawShape shaders ==="
for f in src/WebGPU/programs/drawShape/*.wgsl; do
if grep -q '@group(0) @binding(0) var<uniform> u:' "$f"; then
echo " OK: $f"
else
echo " MISSING uniform binding in $f"
fi
done
echo
echo "=== Checking Uniforms struct with dist_start and dist_end ==="
for f in src/WebGPU/programs/drawShape/*.wgsl; do
if grep -q 'struct\s\+Uniforms' "$f"; then
if grep -q 'dist_start\s*:' "$f" && grep -q 'dist_end\s*:' "$f"; then
echo " OK: $f"
else
echo " MISSING dist_start or dist_end in Uniforms of $f"
fi
else
echo " MISSING struct Uniforms in $f"
fi
done
echo
echo "=== Checking getColor function presence ==="
for f in src/WebGPU/programs/drawShape/*.wgsl; do
if grep -q 'fn\s\+getColor' "$f"; then
echo " OK: $f"
else
echo " MISSING getColor in $f"
fi
doneLength of output: 890
Add missing uniform and struct definitions in drawShape shaders
- src/WebGPU/programs/drawShape/base.wgsl: add
@group(0) @binding(0) var<uniform> u: Uniforms; struct Uniforms { dist_start: f32; dist_end: f32; // …other fields as needed }; fn getColor(sdf: vec4f, uv: vec2f, norm_uv: vec2f) -> vec4f { /*…*/ }
- src/WebGPU/programs/drawShape/linear-gradient.wgsl & radial-gradient.wgsl: ensure
struct Uniformsincludesdist_startanddist_endalongside the existing uniform binding - solid.wgsl already defines both the uniform and struct as required
🤖 Prompt for AI Agents
In src/WebGPU/programs/drawShape/base.wgsl around lines 54-55, the shader uses
u.dist_start and u.dist_end but the uniform binding and Uniforms struct are
missing; add a @group(0) @binding(0) var<uniform> u: Uniforms; and define struct
Uniforms with at least dist_start: f32; dist_end: f32; (and any other existing
fields required by other codepaths), and ensure a getColor(sdf: vec4f, uv:
vec2f, norm_uv: vec2f) -> vec4f function is present or referenced; also update
src/WebGPU/programs/drawShape/linear-gradient.wgsl and radial-gradient.wgsl to
include dist_start and dist_end in their Uniforms definitions to match usage.
| const uniformBuffer = device.createBuffer({ | ||
| label: 'drawShape uniforms', | ||
| size: uniformBufferSize, | ||
| size: uniformData.byteLength, | ||
| usage: GPUBufferUsage.UNIFORM | GPUBufferUsage.COPY_DST, | ||
| }) | ||
| device.queue.writeBuffer(uniformBuffer, 0, new Float32Array([strokeWidth])) | ||
| device.queue.writeBuffer(uniformBuffer, 0, uniformData) | ||
| delayedDestroy(uniformBuffer) | ||
|
|
There was a problem hiding this comment.
Uniform buffer must be 16-byte aligned
Allocate a padded size to avoid validation errors when uniformData.byteLength is 8 (two f32).
- const uniformBuffer = device.createBuffer({
- label: 'drawShape uniforms',
- size: uniformData.byteLength,
+ const paddedSize = (uniformData.byteLength + 15) & ~15; // 16B align
+ const uniformBuffer = device.createBuffer({
+ label: 'pickShape uniforms',
+ size: paddedSize,
usage: GPUBufferUsage.UNIFORM | GPUBufferUsage.COPY_DST,
})
device.queue.writeBuffer(uniformBuffer, 0, uniformData)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const uniformBuffer = device.createBuffer({ | |
| label: 'drawShape uniforms', | |
| size: uniformBufferSize, | |
| size: uniformData.byteLength, | |
| usage: GPUBufferUsage.UNIFORM | GPUBufferUsage.COPY_DST, | |
| }) | |
| device.queue.writeBuffer(uniformBuffer, 0, new Float32Array([strokeWidth])) | |
| device.queue.writeBuffer(uniformBuffer, 0, uniformData) | |
| delayedDestroy(uniformBuffer) | |
| // Ensure uniform buffer size is 16-byte aligned to satisfy WebGPU validation | |
| const paddedSize = (uniformData.byteLength + 15) & ~15; // round up to next multiple of 16 | |
| const uniformBuffer = device.createBuffer({ | |
| label: 'pickShape uniforms', | |
| size: paddedSize, | |
| usage: GPUBufferUsage.UNIFORM | GPUBufferUsage.COPY_DST, | |
| }); | |
| device.queue.writeBuffer(uniformBuffer, 0, uniformData); | |
| delayedDestroy(uniformBuffer); |
🤖 Prompt for AI Agents
In src/WebGPU/programs/pickShape/getProgram.ts around lines 49 to 56, the
uniform buffer size uses uniformData.byteLength which can be 8 and must be
16-byte aligned for WebGPU; allocate a paddedSize =
Math.ceil(uniformData.byteLength / 16) * 16 and use that size when creating the
buffer, call device.queue.writeBuffer(uniformBuffer, 0, uniformData) as before
(writing only the actual data), and keep delayedDestroy(uniformBuffer) unchanged
so the buffer is properly aligned and avoids validation errors.
| struct Uniforms { | ||
| stroke_width: f32, | ||
| dist_start: f32, | ||
| dist_end: f32, | ||
| }; |
There was a problem hiding this comment.
Pad uniforms to 16 bytes to avoid cross-driver UB
Uniforms with only two f32 can cause validation/layout issues on some backends. Add explicit padding to 16 bytes.
Apply:
struct Uniforms {
- dist_start: f32,
- dist_end: f32,
+ dist_start: f32,
+ dist_end: f32,
+ _pad0: f32,
+ _pad1: f32,
};📝 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.
| struct Uniforms { | |
| stroke_width: f32, | |
| dist_start: f32, | |
| dist_end: f32, | |
| }; | |
| struct Uniforms { | |
| dist_start: f32, | |
| dist_end: f32, | |
| _pad0: f32, | |
| _pad1: f32, | |
| }; |
🤖 Prompt for AI Agents
In src/WebGPU/programs/pickShape/shader.wgsl around lines 9–12, the Uniforms
struct currently contains only two f32 fields which can cause validation/layout
issues on some drivers; fix by expanding the struct to 16-byte size by adding
explicit padding (e.g. add a vec2<f32> or two additional f32 fields named as
unused/pad) so the struct is 16-byte aligned and update any host-side uniform
buffer creation if needed to match the new size.
|
🎉 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