Adopt CSS-style drop shadow mask #146
Conversation
| #define SHADOW_LUT_Y_LEN (CONFIG_VERTICAL_OFFSET + CONFIG_SHADOW_FADE_TAIL) | ||
|
|
||
| /* Fast lookup: 17-entry approximation of (1 - t^2)^2 * 0.92 + 0.08. */ | ||
| static inline twin_fixed_t shadow_gaussian_weight(twin_fixed_t t) |
There was a problem hiding this comment.
It seems that the shadow_gaussian_weight() function has the same effect as twin_stack_blur(). Removing the call to twin_stack_blur() still produces a gradient color effect.
There was a problem hiding this comment.
It seems that the shadow_gaussian_weight() function has the same effect as twin_stack_blur().
No, these two functions have completely different purposes and effects.
The purpose of twin_stack_blur:
- Blurs an existing image in-place
- Uses the Stack Blur algorithm (2-pass separable convolution)
- Horizontal scan + vertical scan
- Creates a temporary pixmap for two-directional processing
Complexity: O(2 × width × height × k)
The purpose of shadow_gaussian_weight:
- Looks up a single weight value from a precomputed LUT
- Input: t ∈ [0, 1] (fixed-point format)
- Output: Gaussian weight (16.16 fixed-point)
- Uses formula: (1 - t²)² × 0.92 + 0.08
Complexity: O(1) lookup
Comparisons:
twin_stack_bluris like taking a photograph and applying a Photoshop blur filter to it. You're processing what's already there.shadow_gaussian_weightis like looking up paint opacity values from a chart to paint a gradient from scratch. You're creating something new.
| twin_coord_t bottom_start = win_height; | ||
| if (bottom_start < 0) | ||
| bottom_start = 0; | ||
| twin_coord_t bottom_end = win_height + bottom_extent; |
There was a problem hiding this comment.
twin_coord_t win_height = shadow->height - shadow->window->shadow_y;
twin_coord_t bottom_extent = shadow->height - win_height;
Therefore, bottom_end == shadow->height
| if (bottom_start < 0) | ||
| bottom_start = 0; | ||
| twin_coord_t bottom_end = win_height + bottom_extent; | ||
| if (bottom_end > shadow->height) |
There was a problem hiding this comment.
There is no need to compare.
There was a problem hiding this comment.
There is no need to compare.
Scenario where bounds check matters:
CONFIG_VERTICAL_OFFSET = 6, FADE_TAIL = 2
∴ lut_y_len = 8
Case 1: shadow_y = 10
→ bottom_extent limited to 8
→ bottom_end = win_height + 8
→ Need bounds check if win_height + 8 > shadow->height
Case 2: shadow_y = 20 (large window)
→ bottom_extent limited to 8
→ bottom_end = win_height + 8 (definitely < shadow->height)
Bounds check is REQUIRED because bottom_extent gets clamped to LUT size.
| if (bottom_width <= 0) | ||
| continue; | ||
|
|
||
| twin_coord_t dist_y = y - win_height; |
There was a problem hiding this comment.
twin_coord_t dist_y = y - bottom_start; this is clearer.
There was a problem hiding this comment.
twin_coord_t dist_y = y - bottom_start;this is clearer.
Current implementation is correct for LUT indexing semantics.
- alpha_lut_y is built with indices [0, y_offset)
- LUT entry 0 = maximum alpha (at window edge)
- LUT entry y_offset-1 = minimum alpha (at shadow fade end)
- dist_y must represent distance from window edge (win_height)
- NOT distance from loop start (bottom_start)
|
Only need to keep one variable for the same function. |
This commit generates shadow_gaussian lookup table during the build, so the renderer stops depending on runtime math. It replaces the stack-blur border writes with a lookup-table renderer that matches CSS behavior and stays efficient. For performance considerations, it caches the vertical and bottom falloff tables and rely on memset() or pointer writes to trim per-frame work. It exposes a small configurable fade tail and update defaults plus the API, so it aligns with CSS box-shadow semantics.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 7 files
Prompt for AI agents (all 2 issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/draw-common.c">
<violation number="1" location="src/draw-common.c:418">
Bug: `alpha_bottom[0]` should be `alpha_bottom[x]`. The loop iterates with `x` but always reads from index 0, resulting in a uniform alpha across the bottom strip instead of the intended gradient falloff.</violation>
</file>
<file name="src/twin_private.h">
<violation number="1" location="src/twin_private.h:647">
The comment above this function still mentions "horizontal offset, and vertical offset" but these parameters have been removed. The comment should be updated to reflect that offsets are now configured through Kconfig rather than passed as function parameters.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
|
|
||
| if (alpha_bottom) { | ||
| for (twin_coord_t x = 0; x < bottom_width; x++) { | ||
| twin_a8_t alpha = (alpha_y * alpha_bottom[0]) >> 8; |
There was a problem hiding this comment.
Bug: alpha_bottom[0] should be alpha_bottom[x]. The loop iterates with x but always reads from index 0, resulting in a uniform alpha across the bottom strip instead of the intended gradient falloff.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/draw-common.c, line 418:
<comment>Bug: `alpha_bottom[0]` should be `alpha_bottom[x]`. The loop iterates with `x` but always reads from index 0, resulting in a uniform alpha across the bottom strip instead of the intended gradient falloff.</comment>
<file context>
@@ -177,43 +232,275 @@ void twin_shadow_border(twin_pixmap_t *shadow,
+
+ if (alpha_bottom) {
+ for (twin_coord_t x = 0; x < bottom_width; x++) {
+ twin_a8_t alpha = (alpha_y * alpha_bottom[0]) >> 8;
+ dst[x] = (alpha << 24) | base_rgb;
+ }
</file context>
| twin_a8_t alpha = (alpha_y * alpha_bottom[0]) >> 8; | |
| twin_a8_t alpha = (alpha_y * alpha_bottom[x]) >> 8; |
| twin_argb32_t color, | ||
| twin_coord_t shift_x, | ||
| twin_coord_t shift_y); | ||
| void twin_shadow_border(twin_pixmap_t *shadow, twin_argb32_t color); |
There was a problem hiding this comment.
The comment above this function still mentions "horizontal offset, and vertical offset" but these parameters have been removed. The comment should be updated to reflect that offsets are now configured through Kconfig rather than passed as function parameters.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/twin_private.h, line 647:
<comment>The comment above this function still mentions "horizontal offset, and vertical offset" but these parameters have been removed. The comment should be updated to reflect that offsets are now configured through Kconfig rather than passed as function parameters.</comment>
<file context>
@@ -644,10 +644,7 @@ typedef struct twin_backend {
- twin_argb32_t color,
- twin_coord_t shift_x,
- twin_coord_t shift_y);
+void twin_shadow_border(twin_pixmap_t *shadow, twin_argb32_t color);
#endif
</file context>
This commit generates shadow_gaussian lookup table during the build, so the renderer stops depending on runtime math. It replaces the stack-blur border writes with a lookup-table renderer that matches CSS behavior and stays efficient.
For performance considerations, it caches the vertical and bottom falloff tables and rely on memset() or pointer writes to trim per-frame work. It exposes a small configurable fade tail and update defaults plus the API, so it aligns with CSS box-shadow semantics.
Summary by cubic
Switches drop shadow rendering to a CSS-style box-shadow mask using a precomputed Gaussian LUT. Improves visual match and speeds up rendering while simplifying the shadow API.
New Features
Migration