feat: Resize and rotation #14
Conversation
WalkthroughThis update introduces a transformation UI for graphical assets, enabling resizing and rotation through interactive handles. It adds new Zig modules for 2D matrix math and transform UI logic, updates picking and rendering to support these features, and revises coordinate conventions. Tests and architectural decision records document these design choices. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Canvas
participant ZigLogic
participant WebGPU
participant TransformUI
User->>Canvas: Pointer down on transform handle
Canvas->>ZigLogic: on_pointer_down(x, y)
ZigLogic->>TransformUI: is_transform_ui(hovered_id)
alt Transform UI handle
ZigLogic->>ZigLogic: Set ongoing_action = transform
else Not a handle
ZigLogic->>ZigLogic: Set ongoing_action = move/none
end
User->>Canvas: Pointer move
Canvas->>ZigLogic: on_pointer_move(x, y)
alt ongoing_action == transform
ZigLogic->>TransformUI: tranform_points(handle_id, points, x, y)
TransformUI-->>ZigLogic: Updated points
else
ZigLogic->>ZigLogic: Move logic
end
User->>Canvas: Pointer up
Canvas->>ZigLogic: on_pointer_up()
ZigLogic->>ZigLogic: Reset ongoing_action
ZigLogic->>WebGPU: Render transform UI (get_transform_ui)
WebGPU->>Canvas: Draw handles and borders
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: 9
🧹 Nitpick comments (14)
ADRs/y-axis coords (1)
1-7: Minor typos in ADR
missunderstanding→misunderstanding
(1,1) i top right corner→(1,1) is top-right cornerPolish the ADR for future readers.
ADRs/points vs matrix (1)
1-10: Fix typos & unclear wording in ADRSeveral small language issues make the ADR harder to follow (e.g., positon, 3v3, matricies, transition). A quick sweep keeps it professional and unambiguous.
-We have two options how we can store assets positon/scale/rotation. +We have two options for storing an asset’s position / scale / rotation. -a) four points, each represents one corner -b) width, height + 3v3 matrix +a) four points – each represents one corner +b) width, height + 3×3 matrix -Seems like matrix could simplify a lot of transformations but at the same time -UV coordinates would be to be stored somewhere as well. +Using a matrix would simplify many transformations, but UV coordinates would still need to be stored somewhere. -Thats why for now we stick with storing assets as four points, each point is x,y,u,v. -For transformations like all scaling, rotations we are going to use matricies. -For transition is not useful since it's super easy to just add cursor position to points. +Therefore, for now we store assets as four points (x, y, u, v each). +We will apply scaling and rotation via matrices, while translation is handled by simply adding the cursor delta to the points.integration-tests/index.ts (1)
4-8: Remove stale comment & tighten namingThe comment “// is the string "Jonathan"” is unrelated and confusing; also
new_asset_idcould be shortened tonextAssetId.-const isSampleParam = params.has('sample') // is the string "Jonathan" +const isSampleParam = params.has('sample')src/WebGPU/programs/initPrograms.ts (1)
1-10: Consistent camel-case for factory names
getDrawtexturedeviates from thegetDrawXpattern used elsewhere. Renaming togetDrawTexturekeeps the surface API clean.integration-tests/tests/creator.spec.ts (1)
28-31: Drop console-spam helper in test suitePermanent
mousemovelogging makes local runs noisy and can flood Playwright output when the CI-skip guard is removed later.- await page.evaluate(() => - window.addEventListener('mousemove', (e) => console.log(e.clientX, e.clientY)) - ) // to display cursor position during debugging - // helps to copy position here + // Debug helper removed – re-enable locally if needed.src/logic/types.zig (1)
7-11:anytypemay be wider than needed.
angle_toacceptsother: anytype. Unless you intentionally want duck-typing, constraining this toPoint(or acomptimeinterface) will give clearer errors and better inlining.- pub fn angle_to(self: Point, other: anytype) f32 { + pub fn angle_to(self: Point, other: Point) f32 {src/WebGPU/programs/pickTriangle/getProgram.ts (3)
69-76: Vertex buffer recreated every draw – consider re-use or a staging buffer.Creating & writing a GPUBuffer each frame is expensive and incurs validation overhead. Reuse a pre-allocated buffer or keep one per mesh and update with
queue.writeBuffer/copyBuffer.
77-82: BindGroup churn hurts performance.
device.createBindGroupinside the render loop allocates & validates every frame. Cache one bind group per pipeline layout (or per canvas) and reuse it unless the uniform layout changes.
64-68: Matrix length not validated.
worldProjectionMatrixmust have 16 floats; an assert (dev-only) would prevent subtle GPU garbage writes.if (import.meta.env?.DEV && worldProjectionMatrix.length !== 16) throw new Error('worldProjectionMatrix must be length-16 Float32Array')src/WebGPU/programs/pickTriangle/shader.wgsl (1)
25-27: Casting can be removed after switching tou32.After changing the attribute type, the fragment shader can simply
return in.id;.src/WebGPU/programs/pickTexture/getProgram.ts (2)
79-88: Avoid per-frame BindGroup creation.See comment in
pickTriangle. The same caching applies here.
54-55: Minor: usestd140-aligned byte size constant for readability.64 bytes is a magic number; a named constant clarifies intent.
const MAT4_SIZE = 16 * 4; // 64 bytes const uniformBufferSize = MAT4_SIZE;src/logic/matrix.zig (1)
129-137: Consider adding type constraints or documentation for the point parameter.Using
anytypeprovides flexibility but lacks compile-time type safety. Consider either:
- Using a specific type or union type
- Adding a doc comment specifying the expected interface (must have
.xand.yfields)src/logic/index.zig (1)
152-156: Remove commented-out code.Line.get_vertex_data( - // buffer[0..LINE_NUM_VERTICIES], buffer[0..][0..LINE_NUM_VERTICIES], point, next_point,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (13)
integration-tests/tests/creator.spec.ts-snapshots/after-hover-chromium.pngis excluded by!**/*.pngintegration-tests/tests/creator.spec.ts-snapshots/after-move-chromium.pngis excluded by!**/*.pngintegration-tests/tests/creator.spec.ts-snapshots/after-selection-chromium.pngis excluded by!**/*.pngintegration-tests/tests/creator.spec.ts-snapshots/hover-image-chromium.pngis excluded by!**/*.pngintegration-tests/tests/creator.spec.ts-snapshots/hover-rotation-ui-chromium.pngis excluded by!**/*.pngintegration-tests/tests/creator.spec.ts-snapshots/hover-top-left-scale-ui-chromium.pngis excluded by!**/*.pngintegration-tests/tests/creator.spec.ts-snapshots/hover-top-scale-ui-chromium.pngis excluded by!**/*.pngintegration-tests/tests/creator.spec.ts-snapshots/move-image-chromium.pngis excluded by!**/*.pngintegration-tests/tests/creator.spec.ts-snapshots/rotate-image-chromium.pngis excluded by!**/*.pngintegration-tests/tests/creator.spec.ts-snapshots/select-image-chromium.pngis excluded by!**/*.pngintegration-tests/tests/creator.spec.ts-snapshots/upload-image-chromium.pngis excluded by!**/*.pngintegration-tests/tests/creator.spec.ts-snapshots/use-top-left-scale-ui-chromium.pngis excluded by!**/*.pngintegration-tests/tests/creator.spec.ts-snapshots/use-top-scale-ui-chromium.pngis excluded by!**/*.png
📒 Files selected for processing (20)
ADRs/points vs matrix(1 hunks)ADRs/y-axis coords(1 hunks)integration-tests/index.ts(1 hunks)integration-tests/tests/creator.spec.ts(2 hunks)src/WebGPU/pointer.ts(2 hunks)src/WebGPU/programs/initPrograms.ts(2 hunks)src/WebGPU/programs/pickTexture/getProgram.ts(6 hunks)src/WebGPU/programs/pickTexture/shader.wgsl(0 hunks)src/WebGPU/programs/pickTriangle/getProgram.ts(1 hunks)src/WebGPU/programs/pickTriangle/shader.wgsl(1 hunks)src/getCanvasMatrix.ts(1 hunks)src/index.ts(1 hunks)src/logic/index.d.ts(2 hunks)src/logic/index.zig(6 hunks)src/logic/line.zig(2 hunks)src/logic/matrix.zig(1 hunks)src/logic/transform_ui.zig(1 hunks)src/logic/types.zig(1 hunks)src/run.ts(2 hunks)src/utils/getDefaultPoints.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/WebGPU/programs/pickTexture/shader.wgsl
🧰 Additional context used
🧬 Code Graph Analysis (5)
src/WebGPU/pointer.ts (1)
src/logic/index.d.ts (2)
on_pointer_move(24-24)on_pointer_down(22-22)
src/run.ts (1)
src/WebGPU/programs/initPrograms.ts (1)
pickTriangle(19-19)
src/index.ts (3)
src/WebGPU/getTexture/index.ts (1)
createTextureFromSource(106-117)src/logic/index.d.ts (1)
add_texture(18-18)src/utils/getDefaultPoints.ts (1)
getDefaultPoints(3-19)
integration-tests/index.ts (1)
src/index.ts (1)
initCreator(29-102)
src/WebGPU/programs/pickTriangle/getProgram.ts (2)
src/WebGPU/programs/pickTexture/getProgram.ts (1)
getProgram(5-101)src/WebGPU/programs/initPrograms.ts (1)
pickTriangle(19-19)
🔇 Additional comments (21)
src/index.ts (1)
78-82: Possible missingflipYoption support + silent failure risk
createTextureFromSourceis now called with{ flipY: true }.
The helper insrc/WebGPU/getTexture/index.tsforwardsoptionstocopySourceToTexture, but the latter must actually honourflipY; if that util silently ignores unknown keys, textures will appear upside-down and the bug will be hard to trace.Verify that:
copySourceToTexturerespects theflipYflag.- Call-sites that relied on the old default (
false) are not unintentionally flipped.If the flag is not yet implemented, add explicit handling or at least a runtime assertion to surface the mistake.
src/run.ts (1)
31-32: Looks good – integration point is clearThe new
pick_triangleplumbing cleanly mirrors existing texture picking and keeps theconnectWebGPUProgramscontract symmetrical. No issues spotted.src/utils/getDefaultPoints.ts (1)
14-18: Point order may contradict the new up-growing Y axisADR states (0,0) is bottom-left, yet the first entry here is
(paddingX, paddingY + scaledHeight)– the top-left corner.
If downstream code assumes the array starts with bottom-left (e.g., for winding order or hit-tests) this mismatch will break transformations.Please confirm the expected ordering after the coordinate-system flip and adjust if necessary.
Likely an incorrect or invalid review comment.
src/getCanvasMatrix.ts (1)
4-11:✅ Verification successful
Confirm near/far ordering for mat4.ortho
mat4.ortho(left, right, bottom, top, near, far)now receivesnear = 1,far = -1.
Some matrix helpers expectnear < far; others flip internally. A mismatch silently inverts depth.
🏁 Script executed:
#!/bin/bash # Grep the implementation of mat4.ortho to double-check the expected ordering fd --type f mat4 | xargs rg -n 'function .*ortho'Length of output: 51
🏁 Script executed:
#!/bin/bash # Show how mat4 is imported in getCanvasMatrix.ts grep -n "import.*mat4" src/getCanvasMatrix.ts # Install gl-matrix to inspect its implementation npm install gl-matrix # Locate the ortho function definition in gl-matrix grep -R "function[[:space:]]*ortho" -n node_modules/gl-matrixLength of output: 2152
🏁 Script executed:
#!/bin/bash # Locate the project’s mat4 wrapper module echo "Searching for utils/mat4 module..." fd --type f mat4.ts | grep -v node_modules # If found, inspect the file for how ortho is implemented or forwarded FILE=$(fd --type f mat4.ts | grep -v node_modules | head -n1) echo "Inspecting $FILE" grep -n "ortho" "$FILE" && sed -n '1,200p' "$FILE"Length of output: 6780
Near/Far ordering is correct in mat4.ortho
The custom ortho implementation uses 1/(near – far), so supplying near = 1 and far = –1 (yielding 1/(1−(−1)) > 0) produces the intended depth range. This matches the existing projection helper (which passes depth, –depth). No changes required.
src/WebGPU/programs/initPrograms.ts (1)
29-31:✅ Verification successful
Verify updated signature of
getPickTextureThe second argument (
presentationFormat) was dropped. Make sure every other call site — especially in shader-setup modules — was adjusted; otherwise you’ll get a run-time bind-group mismatch.
🏁 Script executed:
#!/bin/bash # Find any remaining calls that still pass two arguments. rg --line-number 'getPickTexture\([^,]*,[^)]*\)'Length of output: 50
No remaining two-argument calls to
getPickTextureI ran a global search for any calls still passing two arguments to
getPickTextureand found none. All call sites—including in shader-setup modules—have been updated to the new single-parameter signature, so there’s no risk of a bind-group mismatch at runtime.src/logic/index.d.ts (2)
18-18: Rename looks good – check downstream TS/JS callers.
add_texturenow expectstexture_id. Make sure every caller (e.g.src/index.ts, tests) was updated or compilation will break.
26-31: Declaration matches runtime code – no concerns.
The newpick_trianglecallback mirrors the snake-case naming used for the other GPU programs and the function type is consistent.src/logic/types.zig (2)
20-25: Return type inconsistency – UV info discarded.
midreturns plainPoint; callers that expect.u/.vwill silently lose those components. If you ever need mid-UV coordinates, consider returningPointUVor providing a second helper.
33-35: Nice addition.
distancewrapsstd.math.hypotcleanly.src/logic/matrix.zig (3)
1-48: Matrix constructors are well-implemented.The basic matrix constructors (identity, scaling, translation, rotation) are correctly implemented for 2D affine transformations.
50-60: Verify the transformation order is as intended.The current implementation applies transformations in local space (post-multiplication). This means transformations are applied relative to the current orientation. Ensure this matches your intended behavior, as pre-multiplication would apply transformations in global space.
62-127: Matrix operations are correctly implemented.The transpose, inverse, multiply, and approxEq methods are all mathematically correct. Good practice checking for singular matrices in the inverse method.
src/logic/line.zig (2)
1-40: Good refactoring to use caller-provided buffers.The change from dynamic allocation to stack-based buffers improves performance and memory safety. The use of fixed-size array types ensures buffer size correctness at compile time.
41-70: Picking vertex data method is well-implemented.The new
get_vertex_data_pickmethod correctly generates vertex data for GPU-based picking with the appropriate vertex format (5 floats per vertex).src/logic/index.zig (4)
1-35: Transform UI integration setup looks good.The new imports, function pointer, and action type are properly integrated for the transform UI feature.
62-64: Good parameter naming consistency.Changing from
texture_indextotexture_idimproves consistency with the rest of the codebase.
79-140: Transform UI interaction handling is well-implemented.The pointer event handlers correctly detect and handle transform UI elements, maintaining proper state transitions between move and transform actions.
214-218: Transform UI picking integration looks good.The addition of transform UI picking for the active asset is properly implemented using the new
pick_triangleGPU program.src/logic/transform_ui.zig (3)
1-41: Transform UI structure is well-designed.The
TransformLinestruct and line configurations provide a clean representation of the transform UI elements (corners, edges, and rotation handle).
44-145: Transformation logic is well-implemented.The approach of transforming to unrotated space, applying modifications, and transforming back is mathematically correct. Each UI component is handled appropriately for its intended behavior.
147-238: Rendering functions are well-structured.The functions correctly calculate line endpoints and generate vertex data for both rendering and picking. The hover state highlighting is a nice touch for user feedback.
| updatePointer(e) | ||
| on_pointer_move(pointer.x, pointer.y) | ||
| }) | ||
|
|
||
| canvas.addEventListener('click', () => { | ||
| on_pointer_click() | ||
| on_pointer_move(pointer.x, canvas.height - pointer.y) | ||
| }) |
There was a problem hiding this comment.
Y-flip uses mismatching spaces
pointer.y is measured in CSS pixels (rect.height) whereas canvas.height is in device pixels.
On HiDPI screens or if the canvas is scaled via CSS, canvas.height - pointer.y yields incorrect coordinates.
Compute everything in the same space, e.g.:
- on_pointer_move(pointer.x, canvas.height - pointer.y)
+ const scale = canvas.height / rect.height
+ on_pointer_move(pointer.x * scale, canvas.height - pointer.y * scale)This keeps math consistent regardless of DPR or CSS zoom.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/WebGPU/pointer.ts around lines 18 to 20, the calculation for the Y-flip
uses mismatched units by subtracting pointer.y in CSS pixels from canvas.height
in device pixels, causing incorrect coordinates on HiDPI or scaled canvases. Fix
this by converting pointer.y to device pixels before subtracting, typically by
multiplying pointer.y by the device pixel ratio or scaling factor so both values
are in the same coordinate space before performing the subtraction.
| arrayStride: STRIDE * 4, | ||
| attributes: [ | ||
| { shaderLocation: 0, offset: 0, format: 'float32x4' }, // destination position | ||
| { shaderLocation: 1, offset: 16, format: 'float32' }, // id | ||
| ] as const, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
ID should be uint32, not float32, to avoid precision loss.
Large IDs (>2²⁴) cannot be represented exactly as float32.
Use a uint32 vertex attribute and store u32 in the shader.
- { shaderLocation: 1, offset: 16, format: 'float32' }, // id
+ { shaderLocation: 1, offset: 16, format: 'uint32' }, // id📝 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.
| arrayStride: STRIDE * 4, | |
| attributes: [ | |
| { shaderLocation: 0, offset: 0, format: 'float32x4' }, // destination position | |
| { shaderLocation: 1, offset: 16, format: 'float32' }, // id | |
| ] as const, | |
| arrayStride: STRIDE * 4, | |
| attributes: [ | |
| { shaderLocation: 0, offset: 0, format: 'float32x4' }, // destination position | |
| { shaderLocation: 1, offset: 16, format: 'uint32' }, // id | |
| ] as const, |
🤖 Prompt for AI Agents
In src/WebGPU/programs/pickTriangle/getProgram.ts around lines 19 to 23, the
vertex attribute for the ID is incorrectly set as 'float32', which can cause
precision loss for large IDs. Change the format of the ID attribute from
'float32' to 'uint32' to ensure accurate representation of large integer IDs.
Also, update the shader code to expect a 'u32' type for this attribute.
| struct Vertex { | ||
| @location(0) position: vec4f, | ||
| @location(1) id: f32, | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use u32 for the vertex ID.
Storing the ID as f32 and casting to u32 wastes precision and ALU. Define the field as u32 in both vertex buffer and shader:
struct Vertex {
@location(0) position: vec4f,
@location(1) id: u32,
};Make the corresponding change in the TS pipeline vertex attribute (uint32) and remove the cast in fs.
🤖 Prompt for AI Agents
In src/WebGPU/programs/pickTriangle/shader.wgsl lines 1 to 4, change the Vertex
struct's id field type from f32 to u32 to avoid precision loss and unnecessary
casting. Also update the TypeScript pipeline vertex attribute to use uint32
instead of float32 for the id, and remove any casting from u32 to float in the
fragment shader code to keep types consistent and efficient.
| { shaderLocation: 0, offset: 0, format: 'float32x4' }, // destination position | ||
| { shaderLocation: 1, offset: 16, format: 'float32x2' }, // source position | ||
| { shaderLocation: 2, offset: 16 + 8, format: 'float32' }, // id | ||
| // {shaderLocation: 3, offset: 16 + 8, format: 'float32x3'}, // id |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Same precision issue as pick-triangle – store IDs as uint32.
- { shaderLocation: 2, offset: 16 + 8, format: 'float32' }, // id
+ { shaderLocation: 2, offset: 16 + 8, format: 'uint32' }, // idRemember to update the WGSL shader and the CPU-side vertex packing.
📝 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.
| { shaderLocation: 0, offset: 0, format: 'float32x4' }, // destination position | |
| { shaderLocation: 1, offset: 16, format: 'float32x2' }, // source position | |
| { shaderLocation: 2, offset: 16 + 8, format: 'float32' }, // id | |
| // {shaderLocation: 3, offset: 16 + 8, format: 'float32x3'}, // id | |
| { shaderLocation: 0, offset: 0, format: 'float32x4' }, // destination position | |
| { shaderLocation: 1, offset: 16, format: 'float32x2' }, // source position | |
| { shaderLocation: 2, offset: 16 + 8, format: 'uint32' }, // id | |
| // { shaderLocation: 3, offset: 16 + 8, format: 'float32x3'}, // id |
🤖 Prompt for AI Agents
In src/WebGPU/programs/pickTexture/getProgram.ts around lines 26 to 29, the ID
attribute is currently stored as a 'float32', which can cause precision issues.
Change the format of the ID attribute to 'uint32' to ensure correct precision.
Additionally, update the corresponding WGSL shader code and the CPU-side vertex
packing logic to handle the ID as a uint32 instead of a float32.
| return id >= 1 and id <= 9; | ||
| } | ||
|
|
||
| pub fn tranform_points(ui_component_id: u32, points: *[4]PointUV, raw_x: f32, raw_y: f32) void { |
There was a problem hiding this comment.
Fix typo in function name.
-pub fn tranform_points(ui_component_id: u32, points: *[4]PointUV, raw_x: f32, raw_y: f32) void {
+pub fn transform_points(ui_component_id: u32, points: *[4]PointUV, raw_x: f32, raw_y: f32) void {This typo also needs to be fixed in src/logic/index.zig line 136 where this function is called.
📝 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.
| pub fn tranform_points(ui_component_id: u32, points: *[4]PointUV, raw_x: f32, raw_y: f32) void { | |
| pub fn transform_points(ui_component_id: u32, points: *[4]PointUV, raw_x: f32, raw_y: f32) void { |
🤖 Prompt for AI Agents
In src/logic/transform_ui.zig at line 43, correct the typo in the function name
from "tranform_points" to "transform_points". Then, in src/logic/index.zig at
line 136, update the function call to use the corrected name "transform_points"
to ensure consistency and proper linkage.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/logic/matrix.zig (4)
71-101: Extract magic epsilon and re-use across the file
inversehard-codes1e-6for singularity detection, whileapproxEqalso acceptseps.
Expose a single constant (e.g.pub const EPS:f32 = 1e-6;) or pass the tolerance as a parameter to avoid divergent thresholds in the future.- if (@abs(det) < 1e-6) return null; + if (@abs(det) < EPS) return null;This keeps numerical heuristics in one place.
121-127: Micro-nit: useforfor clarityThe
while … : (i += 1)idiom is perfectly valid, but the conventionalforloop is a tad clearer and removes the explicit counter:-var i: usize = 0; -while (i < 9) : (i += 1) { - if (@abs(a.values[i] - b.values[i]) > eps) return false; -} +for (a.values, b.values) |lhs, rhs| { + if (@abs(lhs - rhs) > eps) return false; +}No behaviour change, just readability.
129-137: Narrow the generic parameter oftransform_pointAccepting
anytypeallows passing in a struct that merely happens to have.xand.y, but:
- Compilation errors will be harder to read if a field is misspelled.
- It hides intent from the call-site.
Consider either:
- pub fn transform_point(self: Matrix3x3, p: anytype) Point { + pub fn transform_point(self: Matrix3x3, p: Point) Point {or making the function generic with an interface guarantee:
fn transform_point(self: Matrix3x3, p: anytype) @TypeOf(p) comptime { _ = .{ p.x, p.y }; } // compile-time field checkExplicit is safer here.
8-16: Expose a compile-time identity constant
identity()constructs a new array every call. Declaring:pub const IDENTITY = Matrix3x3{ .values = [_]f32{ 1,0,0, 0,1,0, 0,0,1 }, };and returning
IDENTITYavoids repeated initialisation and conveys immutability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
ADRs/points vs matrix(1 hunks)ADRs/y-axis coords(1 hunks)src/logic/index.zig(6 hunks)src/logic/matrix.zig(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- ADRs/points vs matrix
🚧 Files skipped from review as they are similar to previous changes (2)
- ADRs/y-axis coords
- src/logic/index.zig
🔇 Additional comments (1)
src/logic/matrix.zig (1)
50-60:❓ Verification inconclusive
Check intended transform-composition order
self.* = Matrix3x3.multiply(self.*, Matrix3x3.{scale/rotation/translation}(...))post-multiplies the new transform.
With the current row-major, row-vector-on-left convention this means:p' = p * self * T // after self.translate(...)Many APIs (e.g. CSS, SVG, OpenGL column-major) expect the opposite order (
T * self).
If the rest of the engine assumes intuitive “last call is applied last” semantics, the current order is correct; otherwise every high-level call will appear reversed. Please double-check the calling sites before this lands.
🏁 Script executed:
#!/bin/bash # Inspect definition of multiply, scaling, rotation, translation to confirm composition semantics rg -n "pub fn multiply" -A5 -B2 src/logic/matrix.zig rg -n "pub fn scaling" -A5 -B2 src/logic/matrix.zig rg -n "pub fn rotation" -A5 -B2 src/logic/matrix.zig rg -n "pub fn translation" -A5 -B2 src/logic/matrix.zigLength of output: 1201
Verify transform-composition order
The
scale,rotate, andtranslatemethods all do:self.* = Matrix3x3.multiply(self.*, T)which post-multiplies the new transform. Under our row-major, row-vector-on-left convention this yields:
p' = p * self * Tso the last call is applied last. If elsewhere your code uses column-vector (
M * v) or expectsT * self, the transform order will be reversed.Please confirm that all matrix–vector multiplications and rendering code use the same convention and that this ordering matches your intended “last call is last applied” semantics.
• File:
src/logic/matrix.zig
– Methods:scale,rotate,translate
|
🎉 This PR is included in version 0.1.0-next.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation
Tests