fix: Rotate icon #19
fix: Rotate icon #19
Conversation
WalkthroughThis update introduces significant refactoring and modularization across the rendering pipeline, especially for triangle and MSDF (multi-channel signed distance field) drawing. It adds new Zig modules for triangle and MSDF logic, updates shaders and WebGPU programs for instanced and rounded-corner rendering, and separates vertex buffer management for different rendering types. Type definitions and utility functions are also extended or reorganized. Changes
Sequence Diagram(s)sequenceDiagram
participant Logic as Logic (Zig)
participant Triangle as Triangle Module
participant Msdf as Msdf Module
participant WebGPU as WebGPU Draw Program
participant Shader as GPU Shader
Logic->>Triangle: get_vertex_data(...) / get_round_corner_vector(...)
Logic->>Msdf: get_msdf_vertex_data(...)
Logic->>WebGPU: drawTriangle(..., triangleBuffer)
Logic->>WebGPU: drawMSDF(..., msdfBuffer)
WebGPU->>Shader: Vertex/Fragment shader execution (instanced)
Shader-->>WebGPU: Rendered output (rounded corners/MSDF icons)
Possibly related PRs
Suggested labels
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
src/logic/index.zig (1)
158-219: Memory leak: ArrayLists are not freed.The ArrayLists created for vertex data are never freed, causing a memory leak. The TODO comment acknowledges this but it should be addressed.
fn get_border() struct { []f32, []f32 } { // { triangle vertex, msdf vertex } var triangle_vertex_data = std.ArrayList(f32).init(std.heap.page_allocator); var msdf_vertex_data = std.ArrayList(f32).init(std.heap.page_allocator); + defer triangle_vertex_data.deinit(); + defer msdf_vertex_data.deinit(); // ... existing code ... return .{ - triangle_vertex_data.items, - msdf_vertex_data.items, + triangle_vertex_data.toOwnedSlice() catch &[_]f32{}, + msdf_vertex_data.toOwnedSlice() catch &[_]f32{}, }; }Note: Using
toOwnedSlice()transfers ownership of the memory to the caller. The caller (canvas_render) will need to free these slices when done, or you should consider using a different memory management strategy.
🧹 Nitpick comments (5)
src/WebGPU/programs/drawTriangle/getProgram.ts (1)
3-4: Fix typo in comment.- 4 * 3 /* positon */ + 4 /* color */ + 3 /* value of roudned corner for each of three positions */ + 4 * 3 /* position */ + 4 /* color */ + 3 /* value of rounded corner for each of three positions */src/WebGPU/programs/drawMSDF/getProgram.ts (1)
3-4: Fix typos in comment.- 3 /*3 verticies*/ * (4 /*destinatio position*/ + 2) /*source position*/ + 4 /*color*/ + 3 /*3 vertices*/ * (4 /*destination position*/ + 2) /*source position*/ + 4 /*color*/src/logic/msdf.zig (1)
28-29: Fix typo in constant names.The constant names contain a typo: "VERTICIES" should be "VERTICES".
-const TRIANGLE_DRAW_VERTICIES_COUNT = (4 + 2) * 3 + 4; // 4 -> x,y,z,w, 2 -> u,v, 3 verticies, 4 -> color -pub const DRAW_VERTICIES_COUNT = TRIANGLE_DRAW_VERTICIES_COUNT * 2; +const TRIANGLE_DRAW_VERTICES_COUNT = (4 + 2) * 3 + 4; // 4 -> x,y,z,w, 2 -> u,v, 3 vertices, 4 -> color +pub const DRAW_VERTICES_COUNT = TRIANGLE_DRAW_VERTICES_COUNT * 2;src/logic/line.zig (1)
35-64: Consider aligning pick buffer generation with the 4-point approach.While the current implementation is correct, the draw function now uses 4 points to define the quad, whereas the pick function still uses 6 points. Consider refactoring for consistency.
This would make the pick buffer generation consistent with the draw buffer approach, though the current implementation is functionally correct.
src/WebGPU/programs/drawTriangle/shader.wgsl (1)
67-107: Consider early-out optimization for distant pixels.The fragment shader implementation is correct. For potential performance improvement, consider early-out logic when the pixel is far from all corners.
You could add an early check:
// Early out if pixel is far from all corners if (min_circle_dist > max(in.radius_list.x, max(in.radius_list.y, in.radius_list.z))) { return in.color; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
src/WebGPU/programs/drawMSDF/getProgram.ts(5 hunks)src/WebGPU/programs/drawMSDF/shader.wgsl(3 hunks)src/WebGPU/programs/drawTriangle/getProgram.ts(4 hunks)src/WebGPU/programs/drawTriangle/shader.wgsl(2 hunks)src/logic/index.zig(9 hunks)src/logic/line.zig(1 hunks)src/logic/msdf.zig(1 hunks)src/logic/transform_ui.zig(3 hunks)src/logic/triangle.zig(1 hunks)src/logic/types.zig(1 hunks)src/logic/utils.zig(1 hunks)src/run.ts(1 hunks)src/utils/distancePointToLine.ts(0 hunks)src/utils/getAngle.ts(0 hunks)src/utils/getBezierPos.ts(1 hunks)src/utils/getBezierTan.ts(1 hunks)src/utils/getCurveLength.ts(1 hunks)src/utils/getDistance.ts(0 hunks)src/utils/normalizeVec2.ts(0 hunks)
💤 Files with no reviewable changes (4)
- src/utils/normalizeVec2.ts
- src/utils/distancePointToLine.ts
- src/utils/getDistance.ts
- src/utils/getAngle.ts
🧰 Additional context used
🧬 Code Graph Analysis (3)
src/run.ts (1)
src/WebGPU/programs/initPrograms.ts (1)
drawMSDF(21-21)
src/utils/getBezierTan.ts (1)
src/utils/normalizeVec2.ts (1)
normalizeVec2(1-7)
src/WebGPU/programs/drawMSDF/getProgram.ts (1)
src/WebGPU/programs/initPrograms.ts (1)
drawMSDF(21-21)
🔇 Additional comments (21)
src/logic/utils.zig (2)
3-7: LGTM: Mathematically correct angle midpoint calculation.The vector addition approach correctly handles angle wraparound by summing unit vectors and computing the resultant angle.
9-12: Verify the mathematical correctness of adding π to the angle difference.The function adds π to the normalized angle difference, shifting the result from [-π, π] to [0, 2π]. This is unusual for an angle difference function, which typically returns signed differences or absolute differences in [0, π].
Please verify this is the intended behavior. If you need the absolute angular difference, consider this approach:
- return math.atan2(math.sin(delta), math.cos(delta)) + math.pi; + return @fabs(math.atan2(math.sin(delta), math.cos(delta)));If you need the angular difference in [0, 2π] range, the current implementation is correct but the function name might be misleading.
src/utils/getCurveLength.ts (1)
1-1: LGTM: Consistent import style.The change from double quotes to single quotes improves code style consistency.
src/run.ts (1)
34-38: LGTM: Good defensive programming practice.The null check prevents runtime errors when textures are undefined or not properly loaded, improving the robustness of the MSDF rendering pipeline.
src/utils/getBezierPos.ts (1)
1-1: LGTM: Improved function signature formatting.The single-line function signature is more concise while maintaining readability.
src/logic/types.zig (1)
7-12: LGTM: Correct midpoint calculation.The implementation correctly calculates the midpoint between two points using the standard midpoint formula. The method integrates well with the existing Point struct.
src/utils/getBezierTan.ts (1)
1-11: LGTM! Clean formatting improvements.The formatting changes improve code readability by consolidating the function signature and return statement while maintaining clarity.
src/logic/triangle.zig (2)
6-26: Well-structured vertex data packing.The function efficiently packs vertex data into the buffer with a clear layout that matches the shader expectations. The organization of positions, color, and radius values is logical and maintainable.
28-46: Solid implementation of rounded corner calculations.The mathematical approach using angle bisectors and circular offsets is correct and well-implemented. The circular indexing with modulo arithmetic properly handles all corner cases.
src/WebGPU/programs/drawTriangle/getProgram.ts (2)
18-31: Correct implementation of instanced vertex attributes.The vertex buffer configuration properly sets up instanced rendering with accurate offsets and formats matching the vertex data layout.
35-93: Proper setup for instanced triangle rendering with alpha blending.The fragment target blend state and instanced draw call are correctly configured. The instance count calculation properly divides by the stride.
src/WebGPU/programs/drawMSDF/shader.wgsl (2)
1-44: Well-designed instanced vertex shader.The vertex shader correctly handles instanced rendering by using vertex index to select the appropriate position and UV data. The flat interpolation for color ensures consistent coloring across the triangle.
50-57: Improved flexibility with per-instance coloring.The fragment shader now properly supports per-instance colors while maintaining MSDF-based opacity calculation.
src/WebGPU/programs/drawMSDF/getProgram.ts (2)
24-36: Correctly configured vertex attributes for instanced MSDF rendering.The vertex attributes properly match the shader's input structure with accurate offsets and clear documentation.
83-117: Proper implementation of instanced MSDF rendering.The function rename to
drawMSDFbetter reflects its purpose, and the instanced draw call is correctly configured with 3 vertices per instance.src/logic/msdf.zig (1)
57-82: LGTM! Well-structured vertex data generation.The function correctly generates vertex data for two triangles forming a rectangle, with proper texture coordinate mapping and Y-axis flip for standard texture rendering.
src/logic/line.zig (1)
9-33: Excellent refactoring to leverage Triangle module.The function has been successfully refactored to use the Triangle module's rounded corner calculations, reducing code duplication and improving modularity.
src/logic/transform_ui.zig (1)
207-254: Well-implemented buffer separation and icon handling.The function successfully separates triangle and MSDF vertex data, with appropriate special handling for the rotation icon including hover state visualization.
src/logic/index.zig (2)
42-43: Good change to use floating-point dimensions.Changing width and height to f32 allows for sub-pixel precision, which is beneficial for smooth graphics rendering and transformations.
Also applies to: 61-65
74-77: Improved error handling for asset removal.Good change from
catch unreachableto ignoring the error, preventing unnecessary panics.src/WebGPU/programs/drawTriangle/shader.wgsl (1)
29-65: Well-implemented instanced vertex shader.The vertex shader correctly handles instanced rendering with vertex index modulo 3, and the threshold calculations for rounded corners are mathematically sound.
| var i: usize = 0; | ||
| while (i < data.len) : (i += 7) { | ||
| const icon_id: u32 = @intFromFloat(data[i]); | ||
| std.debug.print("icon_id: {}\n", .{icon_id}); | ||
| const icon = IconData{ | ||
| .id = std.meta.intToEnum(IconId, icon_id) catch unreachable, | ||
| .x = data[i + 1], | ||
| .y = data[i + 2], | ||
| .width = data[i + 3], | ||
| .height = data[i + 4], | ||
| .real_width = data[i + 5], | ||
| .real_height = data[i + 6], | ||
| }; | ||
| icons.put(icon.id, icon) catch unreachable; | ||
| } |
There was a problem hiding this comment.
Add input validation and improve error handling.
The function lacks input validation and uses catch unreachable which will panic on errors.
pub fn init_icons(data: []const f32) void {
icons = std.AutoHashMap(IconId, IconData).init(std.heap.page_allocator);
+ if (data.len % 7 != 0) {
+ std.log.err("Invalid icon data length: {} (must be multiple of 7)", .{data.len});
+ return;
+ }
+
var i: usize = 0;
while (i < data.len) : (i += 7) {
const icon_id: u32 = @intFromFloat(data[i]);
- std.debug.print("icon_id: {}\n", .{icon_id});
const icon = IconData{
- .id = std.meta.intToEnum(IconId, icon_id) catch unreachable,
+ .id = std.meta.intToEnum(IconId, icon_id) catch {
+ std.log.warn("Unknown icon ID: {}, skipping", .{icon_id});
+ continue;
+ },
.x = data[i + 1],
.y = data[i + 2],
.width = data[i + 3],
.height = data[i + 4],
.real_width = data[i + 5],
.real_height = data[i + 6],
};
- icons.put(icon.id, icon) catch unreachable;
+ icons.put(icon.id, icon) catch |err| {
+ std.log.err("Failed to add icon {}: {}", .{ icon_id, err });
+ };
}
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/logic/msdf.zig around lines 36 to 50, the code lacks input validation and
uses `catch unreachable` which causes panics on errors. Add checks to ensure the
data slice length is a multiple of 7 before processing, and validate that the
icon_id can be safely converted to the IconId enum. Replace `catch unreachable`
with proper error handling that returns or logs an error instead of panicking.
|
🎉 This PR is included in version 0.1.0-next.4 🎉 The release is available on: Your semantic-release bot 📦🚀 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/logic/index.zig (2)
75-75: Consider logging the removal result.While ignoring the error is safer than panicking, consider logging when asset removal fails to aid debugging.
- _ = state.assets.remove(state.active_asset_id); + if (!state.assets.remove(state.active_asset_id)) { + // Log: Asset removal failed for ID: state.active_asset_id + }
239-258: Remove commented-out test code.The large block of commented code appears to be leftover from testing/development and should be removed to keep the codebase clean.
- // const points = [_]Types.Point{ - // Types.Point{ .x = 100.0, .y = 70.0 }, // - // Types.Point{ .x = 300.0, .y = 100.0 }, // - // Types.Point{ .x = 300.0, .y = 250.0 }, // - // Types.Point{ .x = 100.0, .y = 150.0 }, // - // }; - // const p0_v = Triangle.get_round_corner_vector(0, points, 10.0); - // const p1_v = Triangle.get_round_corner_vector(1, points, 20.0); - // const p2_v = Triangle.get_round_corner_vector(2, points, 80.0); - // const p3_v = Triangle.get_round_corner_vector(3, points, 20.0); - - // var shape_vertex_data: [2 * Triangle.DRAW_VERTICES_COUNT]f32 = undefined; - // const color = [_]f32{ 0.0, 1.0, 1.0, 1.0 }; - // Triangle.get_vertex_data(shape_vertex_data[0..Triangle.DRAW_VERTICES_COUNT], p0_v, p1_v, p2_v, color); - // Triangle.get_vertex_data(shape_vertex_data[Triangle.DRAW_VERTICES_COUNT .. 2 * Triangle.DRAW_VERTICES_COUNT], p0_v, p2_v, p3_v, color); - - // web_gpu_programs.draw_triangle(&shape_vertex_data); - - // const msdf_vertex_data = Msdf.get_msdf_vertex_data(Msdf.IconId.rotate, 10.0, 10.0, 100.0, [_]f32{ 1.0, 0.0, 0.0, 1.0 }); - // web_gpu_programs.draw_msdf(&msdf_vertex_data, 0);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/logic/index.zig(9 hunks)src/logic/line.zig(1 hunks)src/logic/msdf.zig(1 hunks)src/logic/transform_ui.zig(3 hunks)src/logic/triangle.zig(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/logic/triangle.zig
- src/logic/msdf.zig
- src/logic/line.zig
⏰ 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). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (12)
src/logic/index.zig (6)
7-11: Module import refactoring looks good.The import changes align with the modularization approach, moving from specific imports to full module imports and adding the new Triangle and Msdf modules.
158-219: Excellent refactoring of vertex buffer separation.The separation of triangle and MSDF vertex data into distinct buffers is a clean architectural improvement that aligns with the rendering pipeline changes. The function now properly handles both rendering types.
231-237: Clean separation of rendering calls.The canvas rendering now properly separates triangle and MSDF rendering with appropriate buffer checks. This is a solid improvement over the previous approach.
280-290: Proper delegation to Msdf module.The icon management is now properly delegated to the Msdf module, which is a good architectural decision for separation of concerns.
42-43: Consistency Verified: width/height fields now use f32
Allu32occurrences forwidthandheighthave been removed—no residual usages detected.
201-212: ✅ TransformUI & Msdf integration verifiedBuffer sizes (
DRAW_VERTICES_COUNT) in bothsrc/logic/transform_ui.zigandsrc/logic/msdf.zigmatch, and the signature ofget_transform_uialigns with how it’s called insrc/logic/index.zig. No changes needed.src/logic/transform_ui.zig (6)
1-7: Import path updates look correct.The import changes align with the module refactoring, moving from relative imports to direct module imports and adding the Msdf module.
18-18: Constant renaming improves clarity.The rename from
UI_NUM_VERTICIES_BORDERtoUI_VERTICIES_COUNT_BORDERis more descriptive and follows better naming conventions.
204-212: Function signature changes support dual buffer architecture.The updated function signature properly separates triangle and MSDF vertex data, which aligns with the rendering pipeline refactoring.
214-232: Excellent implementation of rotation icon with MSDF.The special handling for the rotation icon (id 9) with MSDF data generation is well implemented. The color inversion logic for hover states and proper sizing calculations look correct.
256-266: Pick function updates are consistent.The updates to use
Line.PICK_VERTICIES_COUNTare consistent with the module refactoring and maintain the same logic flow.
234-252: Buffer indexing confirmed correctVerified that
HALF_BUFFER = DRAW_VERTICES_COUNT / 2triangle_buffer[i..][0..Line.DRAW_VERTICES_COUNT]andtriangle_buffer[(HALF_BUFFER + i)..][0..Line.DRAW_VERTICES_COUNT]together cover the full buffer exactly (no overflow or gaps) as
DRAW_VERTICES_COUNT = UI_VERTICIES_COUNT_BORDER * Line.DRAW_VERTICES_COUNT * 2. No changes required.
| var buffer: [Line.DRAW_VERTICES_COUNT]f32 = undefined; | ||
|
|
||
| Line.get_vertex_data( | ||
| // buffer[0..LINE_NUM_VERTICIES], | ||
| buffer[0..][0..LINE_NUM_VERTICIES], | ||
| // buffer[0..LINE_VERTICIES_COUNT], | ||
| buffer[0..Line.DRAW_VERTICES_COUNT], | ||
| point, | ||
| next_point, | ||
| 10.0, | ||
| red, | ||
| 5.0, | ||
| ); | ||
|
|
||
| vertex_data.appendSlice(&buffer) catch unreachable; | ||
| triangle_vertex_data.appendSlice(&buffer) catch unreachable; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify Line module API changes.
The code now uses Line.DRAW_VERTICES_COUNT and Line.get_vertex_data with updated parameters. Ensure these API changes are consistent with the Line module refactoring.
🏁 Script executed:
#!/bin/bash
# Description: Verify Line module API consistency
# Expected: Line module should export DRAW_VERTICES_COUNT and get_vertex_data function
echo "Checking Line module exports..."
rg -A 10 "pub const DRAW_VERTICES_COUNT" src/logic/line.zig
rg -A 5 "pub fn get_vertex_data" src/logic/line.zig
echo "Checking Line module function signatures..."
ast-grep --pattern 'pub fn get_vertex_data($$$) $$$'Length of output: 1729
Align get_vertex_data call with refactored Line API
The get_vertex_data function now expects a pointer to a fixed‐length array of DRAW_VERTICES_COUNT vertices, so you should pass &buffer instead of slicing.
- File:
src/logic/index.zig(lines 168–180)
Suggested diff:
- Line.get_vertex_data(
- buffer[0..Line.DRAW_VERTICES_COUNT],
+ Line.get_vertex_data(
+ &buffer,
point,
next_point,
10.0,
red,
5.0,
);This change ensures the call matches:
pub fn get_vertex_data(
buffer: *[DRAW_VERTICES_COUNT]f32,
start: anytype,
end: anytype,
width: f32,
color: [4]f32,
rounded: f32,
) void { … }🤖 Prompt for AI Agents
In src/logic/index.zig around lines 168 to 180, the call to Line.get_vertex_data
incorrectly passes a slice of the buffer instead of a pointer to the
fixed-length array as required by the updated API. Fix this by passing &buffer
directly to get_vertex_data instead of buffer[0..Line.DRAW_VERTICES_COUNT],
ensuring the argument matches the expected *[DRAW_VERTICES_COUNT]f32 type.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores