Introduce a tracing eDSL for mirage kernels - #156
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds a typed shader EDSL, traces shader bodies into AST modules, emits AGSL/SKSL runtime effects, migrates built-in shaders, adds RainyWindow, and standardizes diagnostics with identifier-aware linting and parity tests. ChangesMirage EDSL pipeline
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant ShaderDefinition
participant MirageShader
participant TraceContext
participant RuntimeEffectEmitter
participant MirageCompiler
participant SkiaRuntimeEffect
ShaderDefinition->>MirageShader: provide paramsFactory and traced body
MirageShader->>TraceContext: execute body and collect ShaderModule
TraceContext-->>RuntimeEffectEmitter: return expressions, statements, and helpers
RuntimeEffectEmitter-->>MirageShader: emit AGSL/SKSL kernel
MirageShader->>MirageCompiler: lint and compile generated kernel
MirageCompiler-->>SkiaRuntimeEffect: provide validated runtime-effect source
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
7c041ab to
1da0c72
Compare
d7bb4ab to
8d205d0
Compare
1da0c72 to
9f67d52
Compare
ef15ef2 to
ce3da24
Compare
4700c1b to
82b920f
Compare
bcce4cf to
58f1314
Compare
…otone off hand-written AGSL/SkSL strings
…L/SkSL strings onto the tracing eDSL
…body lambdas with bare uniform handles
…eserved-name guard to the mirage eDSL, and rename comparison infixes to their GLSL builtin spellings
…Init's per-call allocation
…AKE_CASE abbreviation
…int and standard-uniform scans
…int tests at MirageDiagnosticException directly
…d expand its DSL wildcard import
58f1314 to
548a363
Compare
Snapshot diff report✅ 8 screenshots verified, no changes.
Full screenshot report (artifact) ScreenshotsCommitted reference screenshots each spec verifies against (current expected state).
Device renderingActual pixels captured on the emulator per API band (API 30 = GLES mirage + legacy blur, API 34 = AGSL + RenderEffect). Render evidence, not a golden diff.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
cloudy/src/desktopTest/kotlin/com/skydoves/cloudy/internal/edsl/RasterTestUtils.kt (1)
29-37: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winClose Skia native resources with
useblocks.
Surface,Paint, andBitmapareorg.jetbrains.skiaManaged/AutoCloseableobjects. Not closing them leaks native memory; relying on GC finalization is unreliable and can cause pressure in CI when tests accumulate. Wrap each in.use { }.♻️ Proposed refactor with resource management
internal fun rasterize(shader: Shader, size: Int): ByteArray { val info = ImageInfo(size, size, ColorType.RGBA_8888, ColorAlphaType.PREMUL) - val surface = Surface.makeRaster(info) - val canvas: Canvas = surface.canvas - canvas.drawPaint(Paint().apply { this.shader = shader }) - - val bitmap = Bitmap().apply { allocPixels(info) } - surface.readPixels(bitmap, 0, 0) - return bitmap.readPixels() ?: error("readPixels returned null") + val bitmap = Bitmap().apply { allocPixels(info) } + Surface.makeRaster(info).use { surface -> + Paint().use { paint -> + paint.shader = shader + surface.canvas.drawPaint(paint) + } + surface.readPixels(bitmap, 0, 0) + } + return bitmap.use { it.readPixels() } ?: error("readPixels returned null") }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cloudy/src/desktopTest/kotlin/com/skydoves/cloudy/internal/edsl/RasterTestUtils.kt` around lines 29 - 37, Update rasterize to wrap the Surface, Paint used by drawPaint, and Bitmap allocations in nested use blocks so every Skia native resource is closed deterministically. Preserve the existing rasterization flow and return the bitmap pixel data only after readPixels succeeds.cloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/edsl/TraceContext.kt (1)
55-61: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: collapse the manual drain into a
subList(...).clear().
bodyis already a defensivetoList()copy, so the trailingrepeat { removeAt(last) }can be the idiomatic sublist clear — same effect, no index arithmetic.♻️ Optional tidy-up
fun ifBlock(condition: Expression, block: () -> Unit) { val start = statements.size block() val body = statements.subList(start, statements.size).toList() - repeat(statements.size - start) { statements.removeAt(statements.size - 1) } + statements.subList(start, statements.size).clear() statements += IfBlock(condition, body) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/edsl/TraceContext.kt` around lines 55 - 61, In ifBlock, replace the manual repeat/removeAt draining of statements after creating the defensive body copy with clearing the corresponding statements sublist directly. Preserve the existing body extraction and subsequent IfBlock append behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@cloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/edsl/MirageDiagnostics.kt`:
- Around line 46-50: Update MirageDiagnosticException so its message property no
longer overrides IllegalArgumentException.message: rename the constructor
property to rawMessage and pass it into the formatted superclass message with
the diagnostic code and hint. Preserve access to the raw text for existing tests
while ensuring toString() and stack traces expose the formatted diagnostic.
In
`@cloudy/src/desktopTest/kotlin/com/skydoves/cloudy/internal/edsl/RasterTestUtils.kt`:
- Line 28: Update the KDoc for the raster-drawing utility to say RGBA_8888
instead of ARGB_8888, and correct the related downstream test comment while
leaving the existing pixel indexing and implementation unchanged.
---
Nitpick comments:
In
`@cloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/edsl/TraceContext.kt`:
- Around line 55-61: In ifBlock, replace the manual repeat/removeAt draining of
statements after creating the defensive body copy with clearing the
corresponding statements sublist directly. Preserve the existing body extraction
and subsequent IfBlock append behavior.
In
`@cloudy/src/desktopTest/kotlin/com/skydoves/cloudy/internal/edsl/RasterTestUtils.kt`:
- Around line 29-37: Update rasterize to wrap the Surface, Paint used by
drawPaint, and Bitmap allocations in nested use blocks so every Skia native
resource is closed deterministically. Preserve the existing rasterization flow
and return the bitmap pixel data only after readPixels succeeds.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f5bed1ef-923f-4a4d-aed5-5f30a2f830c3
⛔ Files ignored due to path filters (1)
cloudy/api/cloudy.klib.apiis excluded by!cloudy/api/**/*.api
📒 Files selected for processing (21)
cloudy/src/commonMain/kotlin/com/skydoves/cloudy/MirageParams.ktcloudy/src/commonMain/kotlin/com/skydoves/cloudy/MirageShader.ktcloudy/src/commonMain/kotlin/com/skydoves/cloudy/MirageShaders.ktcloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/MirageCompiler.ktcloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/MiragePreamble.ktcloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/edsl/Expression.ktcloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/edsl/MirageDiagnostics.ktcloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/edsl/MirageReservedNames.ktcloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/edsl/RuntimeEffectEmitter.ktcloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/edsl/ShaderValues.ktcloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/edsl/TraceContext.ktcloudy/src/commonTest/kotlin/com/skydoves/cloudy/MirageCompilerTest.ktcloudy/src/desktopTest/kotlin/com/skydoves/cloudy/MirageChromaticRasterTest.ktcloudy/src/desktopTest/kotlin/com/skydoves/cloudy/internal/edsl/MirageDiagnosticsTest.ktcloudy/src/desktopTest/kotlin/com/skydoves/cloudy/internal/edsl/MirageEdslChromaticTest.ktcloudy/src/desktopTest/kotlin/com/skydoves/cloudy/internal/edsl/MirageEdslDuotoneTest.ktcloudy/src/desktopTest/kotlin/com/skydoves/cloudy/internal/edsl/MirageEdslFoilTest.ktcloudy/src/desktopTest/kotlin/com/skydoves/cloudy/internal/edsl/MirageEdslSpecularTest.ktcloudy/src/desktopTest/kotlin/com/skydoves/cloudy/internal/edsl/MirageReservedNamesTest.ktcloudy/src/desktopTest/kotlin/com/skydoves/cloudy/internal/edsl/RasterTestUtils.ktcloudy/src/desktopTest/kotlin/com/skydoves/cloudy/internal/edsl/ShaderLiteralsTest.kt
…ception's stack trace shows the formatted diagnostic
…bList().clear() instead of a manual loop
…he RGBA_8888 KDoc typo
…texture sampling, and port RainyWindow's Heartfelt drop field onto it
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@cloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/edsl/ShaderValues.kt`:
- Around line 666-676: Add KDoc to the new public overloads max(Float2, Float2),
min(Float1, Float), and max(Half1, Float) in ShaderValues.kt, matching the
surrounding builtin documentation style and describing each function’s
parameters and result.
- Around line 729-744: Add individual KDoc blocks to the public swizzle
properties Float2.yx, Float3.y, Float3.z, Float3.xy, and Float3.yzx, replacing
the shared plain comment as appropriate. Describe each property’s swizzle result
consistently with the existing KDoc style used by nearby public APIs such as
Half4.r and Float3.plus.
- Around line 688-691: Update the Half1 overload of clamp to wrap a.e in the
existing float-cast expression before constructing the Call, while keeping the
lo/hi literals and Float1 result unchanged. Ensure the emitted shader expression
is clamp(float(a.e), lo, hi) and does not pass the half expression directly.
In
`@cloudy/src/desktopTest/kotlin/com/skydoves/cloudy/internal/edsl/MirageEdslRainyWindowTest.kt`:
- Around line 49-56: Update the parity assertion in the RainyWindow test to
rasterize and compare buildEdslRainyWindowShader() against the actual legacy
RAINY_WINDOW_KERNEL source, rather than handRolledRainyWindowShader(). Preserve
the existing meanAbsDiff threshold and raster configuration while ensuring the
comparison is anchored to the legacy kernel.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ab4fce9a-dfbc-4490-8c0f-79c3ca3411f0
⛔ Files ignored due to path filters (1)
cloudy/api/cloudy.klib.apiis excluded by!cloudy/api/**/*.api
📒 Files selected for processing (7)
cloudy/src/commonMain/kotlin/com/skydoves/cloudy/RainyWindowShader.ktcloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/edsl/Expression.ktcloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/edsl/MirageReservedNames.ktcloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/edsl/RuntimeEffectEmitter.ktcloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/edsl/ShaderValues.ktcloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/edsl/TraceContext.ktcloudy/src/desktopTest/kotlin/com/skydoves/cloudy/internal/edsl/MirageEdslRainyWindowTest.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- cloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/edsl/Expression.kt
- cloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/edsl/RuntimeEffectEmitter.kt
| test( | ||
| "RainyWindow rasterizes like the hand-written RAINY_WINDOW_KERNEL through skiko RuntimeEffect", | ||
| ) { | ||
| meanAbsDiff( | ||
| rasterize(buildEdslRainyWindowShader(), RASTER), | ||
| rasterize(handRolledRainyWindowShader(), RASTER), | ||
| ).shouldBeLessThanOrEqual(0.5) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the scale used by meanAbsDiff/rasterize.
fd RasterTestUtils.kt --exec cat {}
# Compare against the legacy RAINY_WINDOW_KERNEL constant (if still present) for fidelity.
rg -n -A2 'RAINY_WINDOW_KERNEL' --type kotlinRepository: skydoves/Cloudy
Length of output: 2050
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the test and the relevant kernel/reference definitions.
rg -n -A120 -B20 'handRolledRainyWindowShader|buildEdslRainyWindowShader|RAINY_WINDOW_KERNEL' cloudy/src
# If the legacy kernel exists, print its exact definition block.
python3 - <<'PY'
from pathlib import Path
import re
root = Path("cloudy/src")
for path in root.rglob("*.kt"):
text = path.read_text()
if "RAINY_WINDOW_KERNEL" in text or "handRolledRainyWindowShader" in text:
print(f"\n=== {path} ===")
for name in ["RAINY_WINDOW_KERNEL", "handRolledRainyWindowShader", "buildEdslRainyWindowShader"]:
m = re.search(rf'(?s)(.*?{name}.*?)(?=\n\S|\Z)', text)
if m:
print(m.group(1)[:8000])
PYRepository: skydoves/Cloudy
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re, difflib
repo = Path('.')
test_file = repo / 'cloudy/src/desktopTest/kotlin/com/skydoves/cloudy/internal/edsl/MirageEdslRainyWindowTest.kt'
app_file = repo / 'app/src/commonMain/kotlin/demo/shader/RainyWindowShader.kt'
def extract_triple_quoted_after_marker(text, marker):
i = text.find(marker)
if i < 0:
raise SystemExit(f"marker not found: {marker}")
start = text.find('"""', i)
if start < 0:
raise SystemExit(f"opening triple quotes not found after {marker}")
start += 3
end = text.find('"""', start)
if end < 0:
raise SystemExit(f"closing triple quotes not found after {marker}")
return text[start:end]
test_text = test_file.read_text()
app_text = app_file.read_text()
handrolled = extract_triple_quoted_after_marker(test_text, "private fun handRolledRainyWindowShader(): Shader")
legacy = extract_triple_quoted_after_marker(app_text, "RAINY_WINDOW_KERNEL: String =")
print("handrolled_len", len(handrolled))
print("legacy_len", len(legacy))
print("identical", handrolled == legacy)
if handrolled != legacy:
for line in difflib.unified_diff(
legacy.splitlines(), handrolled.splitlines(),
fromfile='legacy', tofile='handrolled', lineterm=''
):
print(line)
# keep output compact
# stop after first 120 diff lines
PYRepository: skydoves/Cloudy
Length of output: 10704
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import difflib
repo = Path('.')
test_file = repo / 'cloudy/src/desktopTest/kotlin/com/skydoves/cloudy/internal/edsl/MirageEdslRainyWindowTest.kt'
app_file = repo / 'app/src/commonMain/kotlin/demo/shader/RainyWindowShader.kt'
def extract_after_marker(text, marker):
i = text.find(marker)
if i < 0:
raise SystemExit(f"marker not found: {marker}")
start = text.find('"""', i)
if start < 0:
raise SystemExit(f"opening triple quotes not found after {marker}")
start += 3
end = text.find('"""', start)
if end < 0:
raise SystemExit(f"closing triple quotes not found after {marker}")
return text[start:end]
test_text = test_file.read_text()
app_text = app_file.read_text()
handrolled = extract_after_marker(test_text, "private fun handRolledRainyWindowShader(): Shader")
legacy = extract_after_marker(app_text, "RAINY_WINDOW_KERNEL: String =")
print("handrolled_len", len(handrolled))
print("legacy_len", len(legacy))
print("identical", handrolled == legacy)
if handrolled != legacy:
diff = difflib.unified_diff(
legacy.splitlines(),
handrolled.splitlines(),
fromfile='legacy',
tofile='handrolled',
lineterm=''
)
for idx, line in enumerate(diff):
print(line)
if idx >= 120:
break
PYRepository: skydoves/Cloudy
Length of output: 5937
Anchor the parity gate to the actual legacy kernel. The 0.5 bound is on a 0–255 byte scale, so it can still fail; the real issue is that the handwritten shader here is not byte-identical to the legacy RAINY_WINDOW_KERNEL source. Compare the eDSL output against the actual legacy kernel, otherwise this test only tracks the transcription in this file.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@cloudy/src/desktopTest/kotlin/com/skydoves/cloudy/internal/edsl/MirageEdslRainyWindowTest.kt`
around lines 49 - 56, Update the parity assertion in the RainyWindow test to
rasterize and compare buildEdslRainyWindowShader() against the actual legacy
RAINY_WINDOW_KERNEL source, rather than handRolledRainyWindowShader(). Preserve
the existing meanAbsDiff threshold and raster configuration while ensuring the
comparison is anchored to the legacy kernel.
…pers, bounded loops, and branch/When expressions
…rnal modules can author RainyWindow-style nested helpers
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/edsl/ShaderFunction.kt (1)
1-1: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd KDocs for the new public EDSL APIs and regenerate the api dump.
ShaderFunction4,ShaderFunction5, and the 4- and 5-parametershaderFunction(...)overloads incloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/edsl/ShaderFunction.ktare public API and need KDoc. The matchingcloudy/api/**/*.apidump should be updated in the same PR.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/edsl/ShaderFunction.kt` at line 1, Add KDoc documentation to the public ShaderFunction4 and ShaderFunction5 types and the four- and five-parameter shaderFunction(...) overloads in ShaderFunction.kt, then regenerate the matching cloudy/api/**/*.api dump so it reflects the documented public EDSL APIs.Sources: Path instructions, Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@cloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/edsl/ShaderFunction.kt`:
- Line 1: Add KDoc documentation to the public ShaderFunction4 and
ShaderFunction5 types and the four- and five-parameter shaderFunction(...)
overloads in ShaderFunction.kt, then regenerate the matching cloudy/api/**/*.api
dump so it reflects the documented public EDSL APIs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b94ccacb-d9dc-422a-b4e0-3411af8c1521
⛔ Files ignored due to path filters (1)
cloudy/api/cloudy.klib.apiis excluded by!cloudy/api/**/*.api
📒 Files selected for processing (2)
cloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/edsl/ShaderFunction.ktcloudy/src/desktopTest/kotlin/com/skydoves/cloudy/internal/edsl/MirageShaderFunctionTest.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- cloudy/src/desktopTest/kotlin/com/skydoves/cloudy/internal/edsl/MirageShaderFunctionTest.kt


















Summary
internal/edsl/) that builds a typed expression/statement IR at shader-construction time and emits it to a single AGSL/SkSL text, replacing the hand-written string pairsMirageKernels.kt's hand-written strings: Duotone, Foil, Specular, Chromatic (and its five preset looks)RuntimeEffectVerification
Screen_recording_20260716_115048.mp4
Summary by CodeRabbit
colorize,composite, andgenerateso shaders can be built from DSL lambdas.