-
Notifications
You must be signed in to change notification settings - Fork 808
Improve rounded corner rendering with squircles #1038
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Refactored the rounded rectangle SDF to support squircle corners using a power metric, improving visual quality and flexibility. The fragment shader now applies rounded corners with smoother edge softness and masking, and handles cases where rounding is zero.
WalkthroughRefactors rounded-corner masking in the composite video frame shader by adding a super-ellipse metric (squircle), introducing SQUIRCLE_POWER, clamping radius, guarding zero/negative rounding, and replacing a binary mask with a smooth, edge-softened blend for color and alpha. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant VS as Vertex Shader
participant FS as Fragment Shader
participant SDF as sdf_rounded_rect
participant Blend as Edge Mask & Blend
VS->>FS: Interpolated position, uv, inputs
FS->>SDF: Compute distance with clamped radius (squircle power)
SDF-->>FS: Composite distance metric
alt rounding_px <= 0
FS->>Blend: Bypass masking, use original color/alpha
else rounding_px > 0
FS->>Blend: smoothstep(distance, edge_softness)
Blend-->>FS: Masked color/alpha (soft edges)
end
FS-->>Output: Final fragment color
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal). Please share your feedback with us on this Discord post. 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/rendering/src/shaders/composite-video-frame.wgsl (1)
60-61
: Consider caching power calculations for performance.The nested
pow()
calls are computationally expensive and executed per fragment. For better performance, consider pre-calculating1.0 / SQUIRCLE_POWER
as a constant.Apply this diff to optimize the power calculations:
-const SQUIRCLE_POWER: f32 = 4.5; +const SQUIRCLE_POWER: f32 = 4.5; +const SQUIRCLE_POWER_INV: f32 = 1.0 / 4.5;- let super_len = pow(pow(normalized.x, SQUIRCLE_POWER) + pow(normalized.y, SQUIRCLE_POWER), 1.0 / SQUIRCLE_POWER); + let super_len = pow(pow(normalized.x, SQUIRCLE_POWER) + pow(normalized.y, SQUIRCLE_POWER), SQUIRCLE_POWER_INV);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/rendering/src/shaders/composite-video-frame.wgsl
(3 hunks)
⏰ 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). (3)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (4)
crates/rendering/src/shaders/composite-video-frame.wgsl (4)
33-33
: LGTM! Well-chosen constant for aesthetic rounding.The squircle power of 4.5 provides a good balance between rounded corners and straight edges, creating visually pleasing results similar to those used in modern UI design systems.
49-64
: Excellent squircle implementation with proper edge case handling.The refactored SDF function correctly implements a super-ellipse (squircle) with appropriate safeguards:
- Proper radius clamping prevents visual artifacts
- Zero-radius guard maintains correct behavior for sharp corners
- The super-ellipse formula creates smooth, aesthetically pleasing rounded corners
The mathematical approach is sound and the implementation handles edge cases well.
240-242
: LGTM! Consistent early exit optimization.The guard clause efficiently skips rounded corner processing when rounding is disabled, improving performance for rectangular content.
244-252
: Excellent smooth masking implementation.The refactored masking logic provides several improvements over a binary mask:
fwidth()
provides proper derivative-based edge detection- The fallback minimum of 0.75 ensures adequate softening on all hardware
- Smooth blending for both color and alpha creates professional-quality anti-aliased edges
This approach will eliminate jagged edges and provide consistent visual quality across different display densities.
if clamped_r <= 0.0 { | ||
return length(outside) + inside; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
Verify numerical stability for very small radius values.
While the zero-radius guard is good, consider whether very small positive values (e.g., 0.001) might cause numerical instability in the power calculations on Line 60-61.
🏁 Script executed:
#!/bin/bash
# Search for any related numerical stability handling or similar guards in shader code
rg -n --type=rust -C3 "clamp.*0\.0.*min"
rg -n -g "*.wgsl" -C3 "pow.*pow"
Length of output: 671
Guard clamped_r with a small epsilon before dividing to avoid overflow/NaN
The code only returns when clamped_r <= 0.0, but normalized = outside / vec2(clamped_r) and the subsequent pow(...) (crates/rendering/src/shaders/composite-video-frame.wgsl, ~lines 59–61) can produce huge values or overflow/NaN for very small positive radii (e.g., 1e-3). Clamp clamped_r to a safe minimum or early-return when clamped_r < EPS (or use max(clamped_r, EPS)) before the division.
🤖 Prompt for AI Agents
In crates/rendering/src/shaders/composite-video-frame.wgsl around lines 55 to
57, the branch only checks clamped_r <= 0.0 but later divides by clamped_r and
uses pow(...), which can overflow/NaN for very small positive radii; fix by
guarding the divisor with a small EPS (e.g., const EPS = 1e-6) — either
early-return when clamped_r < EPS or replace the divisor with max(clamped_r,
EPS) before normalizing outside and before any pow calls so divisions and
exponentiation use a safe minimum value.
Refactored the rounded rectangle SDF to support squircle corners using a power metric, improving visual quality and flexibility. The fragment shader now applies rounded corners with smoother edge softness and masking, and handles cases where rounding is zero.
Demo:
https://Cap.link/2zwsqmfvtaznb1n
Summary by CodeRabbit
New Features
Style