fix: Refactor transform UI #138
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Caution Review failedThe pull request is closed. WalkthroughRefactors transformPoints to use a forward, matrix-centric transform built from rectangle bounds; changes the API to accept Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant Index as onPointerMove
participant TransformUI as transformPoints
participant Matrix as Matrix3x3
participant Geometry
User->>Index: pointer move (x,y)
Index->>TransformUI: transformPoints(ui_id, bounds, Point{x,y})
TransformUI->>Matrix: m = Matrix3x3.getMatrixFromRectangle(bounds.*)
TransformUI->>Matrix: i_m = m.inverse()
TransformUI->>Geometry: raw_in_bounds = i_m.get(raw_pointer)
alt corner/edge adjustments
TransformUI->>Matrix: matrix.pivotScale(...) per-case
else rotation
TransformUI->>Geometry: center = bounds[0].mid(bounds[2])
TransformUI->>Geometry: compute asset_new_angle (center.angleTo(raw_pointer) - ...)
TransformUI->>Matrix: if matrix.isMirrored() adjust angle
TransformUI->>Matrix: matrix.rotateScaled(asset_new_angle, aspect)
end
TransformUI->>Geometry: map DEFAULT_BOUNDS via matrix -> new_bounds
TransformUI->>Geometry: clamp extents to >=1.0 using angle cos/sin
TransformUI->>Index: write updated bounds
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
✨ 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
This PR refactors the transform UI code to improve readability and maintainability by simplifying the transformation logic and reducing code duplication. The changes focus on streamlining how UI component transformations are applied to point bounds.
- Renamed parameter from
pointstoboundsfor clarity - Simplified matrix transformation logic by combining rotation and translation operations
- Replaced manual point transformation with a loop-based approach
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: 0
🧹 Nitpick comments (7)
src/logic/transform_ui.zig (7)
57-62: ConfirmMatrix3x3.getaccepts PointUV inputs (type interop).
pointsis built as[4]Pointwhileboundsis[4]PointUV, andmatrix.get(bounds[i])impliesgetis generic over both or can acceptPointUV. Ifgetreturns aPoint, this is fine; if it returns aPointUV, the explicit[4]Pointtype onpointsmay be unnecessary or could mismatch.
- If
getis generic, consider declaringpointsas[4]@TypeOf(matrix.get(bounds[0]))for clarity.- Otherwise, be explicit about the conversion to avoid accidental copies or method resolution surprises.
67-92: Corner/edge handlers look correct in local space.Assignments for cases 1–8 are consistent and minimal. Good symmetry and no off-by-one pitfalls observed. Consider a small helper to reduce duplication if you plan to extend constraints (e.g., aspect-ratio lock with Shift, symmetric scale with Alt), but not required for this PR.
Also applies to: 95-114
118-123: Rotation semantics: verify “absolute vs. delta” and pivot; simplify loop.
- Angle: Using
atan2(pointer.y, pointer.x) + π/2as the new rotation in local space is plausible, but it effectively sets an absolute local-space angle, not a delta from the current one. Given you first aligned to the object’s frame, this should behave as intended. Still, please verify expected behavior when the user rotates, stops, then resumes rotation from a different handle position—the result should not “snap” unexpectedly.- Pivot: The loop applies
new_rotationaround the origin. This is OK only if the center is at (0,0) in local space (see the earlier comment). If center is not at origin due to composition order, rotation will orbit the rectangle.Minor clean-up: you can assign the whole struct rather than per-field.
Apply this tiny refactor:
- for (&points) |*p| { - const new_point = new_rotation.get(p); - p.x = new_point.x; - p.y = new_point.y; - } + for (&points) |*p| { + p.* = new_rotation.get(p.*); + }Also applies to: 126-130
135-143: Min-size clamp: use axis deltas instead of Euclidean distance.Because the local-space rect is axis-aligned, checking Euclidean distance is overkill and slightly less explicit. Using axis deltas removes a sqrt and better communicates intent.
Apply this refactor:
- // make sure bounds is not smaller tan 1x1(it removed tons of edge cases) - if (points[0].distance(points[1]) < 1.0) { + // make sure bounds is not smaller than 1x1 (this removed tons of edge cases) + if (@abs(points[1].x - points[0].x) < 1.0) { points[1].x = points[0].x + 1.0; points[2].x = points[3].x + 1.0; } - if (points[0].distance(points[3]) < 1.0) { + if (@abs(points[3].y - points[0].y) < 1.0) { points[3].y = points[0].y + 1.0; points[2].y = points[1].y + 1.0; }
49-51: Fix typos in comments.Nit-level, but worth cleaning up in a refactor PR.
- var matrix = Matrix3x3.rotation(-asset_angle_y); // transfor matrix - matrix.translate(-asset_center.x, -asset_center.y); // useufl for angle and mirrored scaling(with shift/alt) + var matrix = Matrix3x3.rotation(-asset_angle_y); // transform matrix + matrix.translate(-asset_center.x, -asset_center.y); // useful for angle and mirrored scaling (with Shift/Alt) @@ - // make sure bounds is not smaller tan 1x1(it removed tons of edge cases) + // make sure bounds is not smaller than 1x1 (this removed tons of edge cases)Also applies to: 135-135
146-151: Inverse on an orthonormal + translation matrix: consider a specialized inverse and NaN guard.
matrixis rotation + translation, so it should be orthonormal. A specialized inverse (transpose of rotation and translated by the negative rotated offset) is faster and numerically stable if available, e.g.,inverseOrthonormal(). If not implemented, currentinverse()is fine.Also consider guarding against NaNs/Infs during interactive drags to avoid corrupting
boundson rare numerical spikes.Optionally:
- const i_matrix = matrix.inverse(); + const i_matrix = matrix.inverse(); // or matrix.inverseOrthonormal() if available for (bounds, points) |*b, p| { const t_p = i_matrix.get(p); + if (!std.math.isFinite(t_p.x) or !std.math.isFinite(t_p.y)) continue; b.x = t_p.x; b.y = t_p.y; }
44-151: Add quick tests for degenerate and reflection edge cases.The integration tests cover rotation, scaling, reflect-X via top-middle, and rotate-after-reflect (great). To lock in this refactor, add cases for:
- Reflect-Y via middle-left/middle-right, then rotate.
- Drag to within <1px width/height to exercise the clamp branches.
- Rotate, then scale with a corner handle, then rotate again (accumulated transforms).
I can draft Playwright steps mirroring your existing
integration-tests/init.tsutilities if helpful.
📜 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 (1)
src/logic/transform_ui.zig(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/logic/transform_ui.zig (2)
integration-tests/init.ts (2)
resizeAsset(127-162)getRotationHandle(99-113)integration-tests/tests/asset-basic-transform.spec.ts (1)
testinfo(6-36)
🔇 Additional comments (1)
src/logic/transform_ui.zig (1)
44-63: Matrix composition order confirmed correct (pivot at center)I verified that
Matrix3x3.translatedoes a post-multiplication (self.* = multiply(self.*, translation)), so your code:var matrix = Matrix3x3.rotation(-asset_angle_y); matrix.translate(-asset_center.x, -asset_center.y);produces
R * T. When you callmatrix.get(p), it effectively doesR * (T * p), i.e. translate first, then rotate—exactly what you want for a rotation aboutasset_center. No swap in the call order is needed.Optional safeguard: you can still include the debug-only assertions to catch any future regressions, but functionally the current composition is correct.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/logic/matrix.zig (2)
332-335: Tests won’t compile: inverse() returns Matrix3x3, not ?Matrix3x3inverse() panics on non-invertible matrices; it does not return an optional. The test uses
orelse, which is invalid and breaks the build.Apply this diff to fix the test:
- const inv = m.inverse() orelse @panic("Matrix not invertible"); + const inv = m.inverse();
346-355: Tests won’t compile: getTransformBetween() returns Matrix3x3, not ?Matrix3x3Similar to the previous comment, the test incorrectly expects an optional.
Apply this diff to fix the test:
- const transform = Matrix3x3.getTransformBetween(matrix1, matrix2) orelse @panic("Transform failed"); + const transform = Matrix3x3.getTransformBetween(matrix1, matrix2);
🧹 Nitpick comments (5)
src/logic/matrix.zig (1)
223-231: Guard against aspect = 0 and document behavior for extreme aspect ratiosrotateScaled divides by aspect; if height collapses to ~0 the call site will feed in aspect ≈ ∞ or 0, leading to Inf/NaN. Even if upstream clamps after the transform, a degenerate input can still happen (e.g., newly imported zero-height bounds). Add a small epsilon guard to keep the matrix well-defined.
Apply this diff to make rotateScaled numerically robust:
pub fn rotateScaled(self: *Matrix3x3, angle_rad: f32, aspect: f32) void { - const c = std.math.cos(angle_rad); - const s = std.math.sin(angle_rad); + const c = std.math.cos(angle_rad); + const s = std.math.sin(angle_rad); + const eps: f32 = 1e-6; + const safe_aspect = if (@abs(aspect) < eps) (if (aspect < 0) -eps else eps) else aspect; self.* = Matrix3x3.multiply(self.*, Matrix3x3.from([_]f32{ - c, -s / aspect, 0.0, - s * aspect, c, 0.0, + c, -s / safe_aspect, 0.0, + s * safe_aspect, c, 0.0, 0.0, 0.0, 1.0, })); }Note: The anisotropic rotation form [c, -s/aspect; s*aspect, c] matches S(w,h)^-1 · R(θ) · S(w,h) with aspect = w/h.
src/logic/transform_ui.zig (4)
44-47: Pointer mapping via inverse() is elegant—add a safety net for singular boundsBuilding the local-space pointer with
matrix.inverse().get(raw_pointer)is the right approach. However, if bounds become near-degenerate (e.g., imported as 0×H, or from a prior bug), inverse() will panic. Suggest an early guard or a fallback.Minimal guard idea:
- var matrix = Matrix3x3.getMatrixFromRectangle(bounds.*); - const pointer = matrix.inverse().get(raw_pointer); + var matrix = Matrix3x3.getMatrixFromRectangle(bounds.*); + // Avoid inverse panic on degenerate rectangles. + const w = bounds[0].distance(bounds[1]); + const h = bounds[0].distance(bounds[3]); + if (w < 1e-6 or h < 1e-6) return; // or: skip update / clamp to minimal safe size + const pointer = matrix.inverse().get(raw_pointer);If you prefer not to early-return, we can clamp w/h to eps and still proceed.
48-53: Hoist DEFAULT_RECT to file scope (comptime) to avoid reinitializationDEFAULT_RECT is constant and used on each pointer move. Moving it to file scope (comptime const) avoids repeated initialization and makes intent explicit.
Example:
comptime const DEFAULT_RECT: [4]PointUV = .{ .{ .x = 0.0, .y = 1.0, .u = 0.0, .v = 1.0 }, .{ .x = 1.0, .y = 1.0, .u = 1.0, .v = 1.0 }, .{ .x = 1.0, .y = 0.0, .u = 1.0, .v = 0.0 }, .{ .x = 0.0, .y = 0.0, .u = 0.0, .v = 0.0 }, };Then use it directly in the function.
66-78: Rotation path: good approach—make aspect robust and confirm mirrored semanticsThe center-based translate → rotateScaled → translate flow is solid, and the mirrored sign flip is a pragmatic fix for reflected states. Two suggestions:
- Use a safe aspect to avoid 1/0 in rotateScaled when height is ~0.
- Add a quick comment clarifying why angle is measured vs the Y-axis (0→3), which makes the top rotation handle behavior easier to reason about after reflections.
Apply this diff to harden aspect:
- const aspect = bounds[0].distance(bounds[1]) / bounds[0].distance(bounds[3]); + const width = bounds[0].distance(bounds[1]); + const height = bounds[0].distance(bounds[3]); + const aspect = width / @max(height, 1e-6);
94-106: Min-size clamp is helpful—consider centralizing the threshold and preserving new edge anglesThe clamp enforces ≥1px along both axes by nudging endpoints using pre-transform angles. It works, but:
- Extract the 1.0 literal to a
const MIN_SIZE: f32 = 1.0;at file scope for consistency and future tuning.- Using pre-transform angles can produce slight visual discontinuities after large flips. Optionally recompute the current edge directions post-transform to nudge along the final orientation.
Potential tweak:
- if (bounds[0].distance(bounds[1]) < 1.0) { + const MIN_SIZE: f32 = 1.0; + if (bounds[0].distance(bounds[1]) < MIN_SIZE) { bounds[1].x = bounds[0].x + @cos(angle_x); bounds[1].y = bounds[0].y + @sin(angle_x); bounds[2].x = bounds[3].x + @cos(angle_x); bounds[2].y = bounds[3].y + @sin(angle_x); } - if (bounds[0].distance(bounds[3]) < 1.0) { + if (bounds[0].distance(bounds[3]) < MIN_SIZE) { bounds[3].x = bounds[0].x + @cos(angle_y); bounds[3].y = bounds[0].y + @sin(angle_y); bounds[2].x = bounds[1].x + @cos(angle_y); bounds[2].y = bounds[1].y + @sin(angle_y); }If you want, I can provide a variant that derives the nudge direction from the post-transform edges instead.
📜 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 (3)
src/logic/index.zig(1 hunks)src/logic/matrix.zig(1 hunks)src/logic/transform_ui.zig(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/logic/matrix.zig (1)
src/WebGPU/m3.ts (4)
rotate(88-90)scale(92-94)rotation(67-75)scaling(77-82)
src/logic/index.zig (2)
src/WebGPU/pointer.ts (2)
updatePointer(84-87)updatePointer(47-52)src/logic/index.d.ts (1)
PointUV(6-11)
src/logic/transform_ui.zig (2)
integration-tests/tests/asset-basic-transform.spec.ts (1)
testinfo(6-36)integration-tests/init.ts (2)
getRotationHandle(99-113)resizeAsset(127-162)
🔇 Additional comments (5)
src/logic/matrix.zig (2)
212-219: pivotScale math is correct (T(p) * S(sx, sy) * T(-p))The composition encodes scaling around an arbitrary pivot via px*(1 - sx) / py*(1 - sy). Clean and side-effect free.
233-237: isMirrored check is appropriateUsing the 2x2 linear-part determinant (m00m11 - m01m10) is the standard reflection test for 2D affine transforms. Good choice.
src/logic/transform_ui.zig (2)
55-64: Corner/edge scale formulas and pivots look correctNice use of pivotScale: each handler maps to the expected unit-space pivot and scale factors (e.g., TL uses sx=1 - pointer.x, sy=pointer.y, pivot=(1,0)). This matches the derivation x' = px + sx(x - px), y' = py + sy(y - py).
82-92: Mapping DEFAULT_RECT through the updated matrix is the right, rotation-safe write-backThis avoids per-corner mutation bugs and keeps the transform pipeline consistent. Good call.
src/logic/index.zig (1)
365-369: API callsite updated correctly to pass a PointThe new signature
transformPoints(u32, *[4]PointUV, Point)is respected. No other callsites appear in this file. LGTM.
|
🎉 This PR is included in version 0.1.0-next.22 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Summary by CodeRabbit
Bug Fixes
Refactor