Unify mirage + blur nodes onto one EffectNode spine - #151
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (76)
💤 Files with no reviewable changes (7)
🚧 Files skipped from review as they are similar to previous changes (66)
📝 WalkthroughWalkthroughThe PR migrates Mirage from optic-based plans to shader-based pipelines, introduces unified effect rendering, replaces platform blur routing, adds Android acyclic backdrop capture, and expands screenshot, raster, compiler, and crash-regression coverage. ChangesMirage shader and effect migration
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Modifier.mirage
participant EffectNode
participant BlurStrategy
participant MirageBackdropSnapshot
participant MirageEffect
Modifier.mirage->>EffectNode: Build EffectElement and pipeline stages
EffectNode->>BlurStrategy: Draw platform blur stage
BlurStrategy->>MirageBackdropSnapshot: Capture acyclic backdrop snapshot on Android
EffectNode->>MirageEffect: Draw shader filters and overlays
MirageEffect->>EffectNode: Report rendered state
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 |
2e803cd to
325d31c
Compare
7c041ab to
1da0c72
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.
|
1da0c72 to
9f67d52
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
cloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/MirageProgramCache.kt (1)
120-129: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider renaming
planRenderstopipelineRenders.The function name still uses the old "plan" vocabulary while the rest of the codebase has migrated to "pipeline." The
whendispatch onStagesubtypes is correct —PlatformFilterreturningfalseis the right behavior since blur stages render on their own path and shouldn't gateMirageFallback.♻️ Optional rename
-internal fun planRenders(stages: List<Stage>, dialect: Dialect): Boolean = stages.any { stage -> +internal fun pipelineRenders(stages: List<Stage>, dialect: Dialect): Boolean = stages.any { stage ->Also update the call site in
MirageModifier.kt:- if (!planRenders(stages, currentDialect())) { + if (!pipelineRenders(stages, currentDialect())) {🤖 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/MirageProgramCache.kt` around lines 120 - 129, Rename the function planRenders to pipelineRenders in MirageProgramCache.kt and update its call site in MirageModifier.kt to use the new name. Preserve the existing Stage dispatch and rendering behavior unchanged.cloudy/src/desktopTest/kotlin/com/skydoves/cloudy/SkyBackdropRasterTest.kt (1)
159-172: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUnclosed Skia
Image/Bitmapper render call.
scene.render()returns a native-backedImagethat's never closed, and theBitmap()allocated inimageBytesis never closed either — only the outerImageComposeSceneis wrapped in.use. Skia bindings (Skija/Skiko) manage native memory throughAutoCloseable-style disposal; leaving these unclosed leaks native buffers on everyrenderScene()call (6+ per test run here).♻️ Proposed fix to close Image/Bitmap
private fun renderScene(content: `@Composable` () -> Unit): ByteArray = ImageComposeScene(width = SURFACE, height = SURFACE, density = Density(1f), content = content) .use { scene -> scene.render() - imageBytes(scene.render()) + scene.render().use { image -> imageBytes(image) } } private fun imageBytes(image: Image): ByteArray { val info = ImageInfo(SURFACE, SURFACE, ColorType.RGBA_8888, ColorAlphaType.PREMUL) - val bitmap = Bitmap().apply { allocPixels(info) } - require(image.readPixels(bitmap)) { "Image.readPixels returned false" } - return bitmap.readPixels() ?: error("Bitmap.readPixels returned null") + Bitmap().apply { allocPixels(info) }.use { bitmap -> + require(image.readPixels(bitmap)) { "Image.readPixels returned false" } + return bitmap.readPixels() ?: error("Bitmap.readPixels returned null") + } }Please confirm
org.jetbrains.skia.ImageandBitmapimplementAutoCloseable/usein the Skiko version this project targets before applying.🤖 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/SkyBackdropRasterTest.kt` around lines 159 - 172, Update renderScene and imageBytes to dispose every native-backed Skia resource: wrap the Image returned by scene.render() in use before passing it to imageBytes, and wrap the allocated Bitmap in imageBytes with use while preserving the existing pixel-read validation and byte conversion. Confirm the targeted Skiko APIs support AutoCloseable/use before applying.cloudy/src/androidDeviceTest/kotlin/com/skydoves/cloudy/SkyBackdropScreenshotTest.kt (1)
153-160: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winFixed
Thread.sleep(600)risks CI flakiness — poll for a stable capture instead.A hard-coded 600ms wait for the async rasterized backdrop snapshot is brittle: too slow on fast devices (wasted CI time across 3 tests × multiple captures) and potentially too short on a loaded/cold emulator (flaky failures). Since there's no public idling hook for the snapshot, a bounded retry loop that captures repeatedly until two consecutive captures match (or a timeout) would be more robust than a blind sleep.
🔁 Illustrative refactor — poll instead of fixed sleep
- composeTestRule.mainClock.autoAdvance = true - Thread.sleep(600) - composeTestRule.waitForIdle() - val map = composeTestRule.onNodeWithTag(ROOT_TAG).captureToImage().toPixelMap() + composeTestRule.mainClock.autoAdvance = true + var previous: IntArray? = null + var map = composeTestRule.onNodeWithTag(ROOT_TAG).captureToImage().toPixelMap() + val deadline = System.currentTimeMillis() + 3_000 + while (System.currentTimeMillis() < deadline) { + composeTestRule.waitForIdle() + val current = composeTestRule.onNodeWithTag(ROOT_TAG).captureToImage().toPixelMap() + val flat = IntArray(current.width * current.height) { i -> current[i % current.width, i / current.width].toArgb() } + if (previous != null && flat.contentEquals(previous)) { map = current; break } + previous = flat + map = current + Thread.sleep(50) + }🤖 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/androidDeviceTest/kotlin/com/skydoves/cloudy/SkyBackdropScreenshotTest.kt` around lines 153 - 160, Replace the fixed Thread.sleep(600) in SkyBackdropScreenshotTest with a bounded polling loop that repeatedly captures the ROOT_TAG image until two consecutive pixel maps match or the timeout is reached. Keep autoAdvance enabled and call waitForIdle between attempts, then use the stable/latest capture for assertions while retaining a finite timeout to prevent hangs.
🤖 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/androidMain/kotlin/com/skydoves/cloudy/internal/LegacyForegroundBlurrer.kt`:
- Around line 294-303: Update captureSmallLayer so the original HARDWARE bitmap
is explicitly recycled after a successful copy to ARGB_8888, while retaining the
copied bitmap as the return value; ensure recycling occurs only after the copy
succeeds and preserve the existing failure behavior.
- Around line 186-243: Update the blur processing flow in the coroutine around
RenderScriptToolkit.blur and iterativeBlur to catch LinkageError alongside
Exception in both relevant catch blocks. Convert the error to CloudyState.Error,
invalidate node, and preserve CancellationException propagation so missing
native libraries degrade without crashing.
In `@cloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/EffectElement.kt`:
- Around line 73-113: Update EffectElement.equals and hashCode to include the
onStateChanged callback in element identity, or alternatively ensure EffectNode
refreshes its callback during update. Preserve callback replacement across
recompositions so update() is not skipped and the node invokes the latest
handler.
In `@cloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/EffectNode.kt`:
- Around line 136-145: Update EffectNode.update()’s overlay migration logic to
detect every sky change using previous !== sky, including null↔non-null
transitions. Before adopting the new sky, remove reblur from the previous
frameDriver when previous is non-null and add it to the new sky’s frameDriver
when sky is non-null, while preserving the isAttached guard and existing
snapshot/GLES invalidation for replacements.
In `@docs/src/wasmJsMain/kotlin/docs/screen/ApiMirageScreen.kt`:
- Around line 59-63: Update the descriptive text in ApiMirageScreen so the
shader sentence uses the grammatically correct article “a” before “shader,”
preserving the rest of the documentation unchanged.
---
Nitpick comments:
In
`@cloudy/src/androidDeviceTest/kotlin/com/skydoves/cloudy/SkyBackdropScreenshotTest.kt`:
- Around line 153-160: Replace the fixed Thread.sleep(600) in
SkyBackdropScreenshotTest with a bounded polling loop that repeatedly captures
the ROOT_TAG image until two consecutive pixel maps match or the timeout is
reached. Keep autoAdvance enabled and call waitForIdle between attempts, then
use the stable/latest capture for assertions while retaining a finite timeout to
prevent hangs.
In
`@cloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/MirageProgramCache.kt`:
- Around line 120-129: Rename the function planRenders to pipelineRenders in
MirageProgramCache.kt and update its call site in MirageModifier.kt to use the
new name. Preserve the existing Stage dispatch and rendering behavior unchanged.
In `@cloudy/src/desktopTest/kotlin/com/skydoves/cloudy/SkyBackdropRasterTest.kt`:
- Around line 159-172: Update renderScene and imageBytes to dispose every
native-backed Skia resource: wrap the Image returned by scene.render() in use
before passing it to imageBytes, and wrap the allocated Bitmap in imageBytes
with use while preserving the existing pixel-read validation and byte
conversion. Confirm the targeted Skiko APIs support AutoCloseable/use before
applying.
🪄 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: f9bf469d-88b3-4443-836d-6d662b936c30
⛔ Files ignored due to path filters (1)
cloudy/api/cloudy.klib.apiis excluded by!cloudy/api/**/*.api
📒 Files selected for processing (76)
app/src/androidMain/kotlin/demo/shader/DropletMapImage.android.ktapp/src/commonMain/kotlin/demo/Route.ktapp/src/commonMain/kotlin/demo/screen/MenuHomeScreen.ktapp/src/commonMain/kotlin/demo/screen/MirageScreen.ktapp/src/commonMain/kotlin/demo/screen/MirageSkyScreen.ktapp/src/commonMain/kotlin/demo/shader/DropletMap.ktapp/src/commonMain/kotlin/demo/shader/RainyWindowShader.ktapp/src/skikoMain/kotlin/demo/shader/DropletMapImage.skiko.ktcloudy/src/androidDeviceTest/kotlin/com/skydoves/cloudy/BackdropCaptureCrashRegressionTest.ktcloudy/src/androidDeviceTest/kotlin/com/skydoves/cloudy/GlProgramMatchTest.ktcloudy/src/androidDeviceTest/kotlin/com/skydoves/cloudy/GlesRoundtripBenchmark.ktcloudy/src/androidDeviceTest/kotlin/com/skydoves/cloudy/Issue112RegressionTest.ktcloudy/src/androidDeviceTest/kotlin/com/skydoves/cloudy/MainPixelCopyCrashReproTest.ktcloudy/src/androidDeviceTest/kotlin/com/skydoves/cloudy/MirageBandScreenshotTest.ktcloudy/src/androidDeviceTest/kotlin/com/skydoves/cloudy/SkyBackdropScreenshotTest.ktcloudy/src/androidHostTest/kotlin/com/skydoves/cloudy/MirageFilterChainTest.ktcloudy/src/androidHostTest/kotlin/com/skydoves/cloudy/MiragePipelineModifierTest.ktcloudy/src/androidHostTest/kotlin/com/skydoves/cloudy/ScreenshotTestSupport.ktcloudy/src/androidMain/kotlin/com/skydoves/cloudy/Cloudy.android.ktcloudy/src/androidMain/kotlin/com/skydoves/cloudy/CloudyBackground.android.ktcloudy/src/androidMain/kotlin/com/skydoves/cloudy/CloudyBlurStrategy.ktcloudy/src/androidMain/kotlin/com/skydoves/cloudy/CloudyLegacyBlurStrategy.ktcloudy/src/androidMain/kotlin/com/skydoves/cloudy/CloudyRenderEffectStrategy.ktcloudy/src/androidMain/kotlin/com/skydoves/cloudy/MirageModifier.android.ktcloudy/src/androidMain/kotlin/com/skydoves/cloudy/internal/BackdropClearBlurrer.ktcloudy/src/androidMain/kotlin/com/skydoves/cloudy/internal/BlurStrategy.android.ktcloudy/src/androidMain/kotlin/com/skydoves/cloudy/internal/BlurTier.android.ktcloudy/src/androidMain/kotlin/com/skydoves/cloudy/internal/LegacyBackdropBlurrer.ktcloudy/src/androidMain/kotlin/com/skydoves/cloudy/internal/LegacyForegroundBlurrer.ktcloudy/src/androidMain/kotlin/com/skydoves/cloudy/internal/MirageBackendProgram.android.ktcloudy/src/androidMain/kotlin/com/skydoves/cloudy/internal/MirageNode.android.ktcloudy/src/commonMain/kotlin/com/skydoves/cloudy/ExperimentalMirage.ktcloudy/src/commonMain/kotlin/com/skydoves/cloudy/LiquidGlass.ktcloudy/src/commonMain/kotlin/com/skydoves/cloudy/LiquidGlassExperimental.ktcloudy/src/commonMain/kotlin/com/skydoves/cloudy/MirageModifier.ktcloudy/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/CompiledProgram.ktcloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/Effect.ktcloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/EffectElement.ktcloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/EffectNode.ktcloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/MirageBackdropElement.ktcloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/MirageBackdropNode.ktcloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/MirageBackdropSnapshot.ktcloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/MirageBackend.ktcloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/MirageColorGrade.ktcloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/MirageCompiler.ktcloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/MirageEffect.ktcloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/MirageFilterChain.ktcloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/MirageGlslEs.ktcloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/MirageKernels.ktcloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/MirageNode.ktcloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/MiragePipeline.ktcloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/MiragePreamble.ktcloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/MirageProgramCache.ktcloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/MirageUniformBinding.ktcloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/PostProcess.ktcloudy/src/commonTest/kotlin/com/skydoves/cloudy/MirageCompilerTest.ktcloudy/src/commonTest/kotlin/com/skydoves/cloudy/MirageElementEqualityTest.ktcloudy/src/commonTest/kotlin/com/skydoves/cloudy/MirageGlslEsTest.ktcloudy/src/commonTest/kotlin/com/skydoves/cloudy/MiragePresetTest.ktcloudy/src/desktopTest/kotlin/com/skydoves/cloudy/MirageChromaticRasterTest.ktcloudy/src/desktopTest/kotlin/com/skydoves/cloudy/MirageColorGradeRasterTest.ktcloudy/src/desktopTest/kotlin/com/skydoves/cloudy/MirageProgramCacheTest.ktcloudy/src/desktopTest/kotlin/com/skydoves/cloudy/RainyWindowRasterTest.ktcloudy/src/desktopTest/kotlin/com/skydoves/cloudy/SkyBackdropRasterTest.ktcloudy/src/skikoMain/kotlin/com/skydoves/cloudy/Cloudy.skiko.ktcloudy/src/skikoMain/kotlin/com/skydoves/cloudy/CloudyBackground.skiko.ktcloudy/src/skikoMain/kotlin/com/skydoves/cloudy/MirageModifier.skiko.ktcloudy/src/skikoMain/kotlin/com/skydoves/cloudy/internal/BlurStrategy.skiko.ktcloudy/src/skikoMain/kotlin/com/skydoves/cloudy/internal/MirageNode.skiko.ktdocs/src/wasmJsMain/kotlin/docs/screen/ApiMirageScreen.ktdocs/src/wasmJsMain/kotlin/docs/screen/GettingStartedScreen.ktdocs/src/wasmJsMain/kotlin/docs/screen/HomeScreen.ktdocs/src/wasmJsMain/kotlin/docs/screen/PlaygroundScreen.kt
💤 Files with no reviewable changes (7)
- cloudy/src/androidMain/kotlin/com/skydoves/cloudy/CloudyBlurStrategy.kt
- cloudy/src/androidMain/kotlin/com/skydoves/cloudy/CloudyRenderEffectStrategy.kt
- cloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/MirageBackdropElement.kt
- cloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/MirageBackdropNode.kt
- cloudy/src/androidMain/kotlin/com/skydoves/cloudy/CloudyLegacyBlurStrategy.kt
- cloudy/src/commonMain/kotlin/com/skydoves/cloudy/internal/MirageNode.kt
- cloudy/src/androidDeviceTest/kotlin/com/skydoves/cloudy/MainPixelCopyCrashReproTest.kt
9f67d52 to
4700c1b
Compare
…d instrumented devices
…de spine with a BlurTier ladder
…o break the PixelCopy RenderNode cycle
…ics terms and collapse the source type to a nullable Sky
…quality into a data class
…across the mirage API
…lign the docs shader vocabulary
…cabulary consistency
…line rename fallout
…n-specific comments
…the content blur bloom stays free
…kdrop snapshot inline to fix scroll staleness
… same scroll staleness the blur path had
…den legacy blur cleanup
4700c1b to
82b920f
Compare


















Summary
Unifies the four separate modifier nodes behind
Modifier.mirage,Modifier.mirage(sky),Modifier.cloudy(radius), andModifier.cloudy(sky, ...)into oneEffectNodespine. Before this branch, the mirage self-content node and the mirage backdrop node were ~90% duplicated, and the foreground/background blur ran through separate strategy/factory classes. They now share oneModifier.Nodethat owns the plan, clock, layer pool, positioning, and post-processing, and delegates the per-effect draw to anEffect(eitherMirageEffectfor the shader pipeline orBlurStrategyfor the platform blur).No public API change.
Modifier.mirage/Modifier.cloudy/Modifier.sky/liquidGlass/CloudyStateand their KDoc contracts are unchanged; the klib and JVM ABI dumps show only internal-symbol churn.This is opened as a draft: it is the first slice of a larger unification (a shared
forecast { }plan DSL, a light rig, and the liquidGlass strangle are planned on top of this spine), and I'd like a read on the internal shape before building further.What changed
EffectNode(commonMain, internal) replacesMirageNode+MirageBackdropNode+ the two blur nodes. It branches on a nullableSky?for the stage-0 source (null = self content, non-null = backdrop).Effectinterface — two implementers:MirageEffect(shader filter/overlay pipeline) andBlurStrategy(android + skiko). The Android blur resolves aBlurTierper draw: GPURenderEffect(API 31+), CPU legacy blur (API < 31 withcpuBlurEnabled), or scrim (API < 31 without). The legacy CPU blur machines are extracted from the old nodes unchanged, not rewritten.MirageFilterChain(the layer pool + stage chaining) is owned by the node and used byMirageEffect; blur bypasses it.Effect,EffectNode,Source→Sky?,BlurTier,Stage,MirageFilterChain) rather than domain metaphors, so the internals read the way a graphics reviewer expects. The public concept names (mirage,sky,cloudy) stay.Included fix — backdrop
captureToImage/ PixelCopy crashWhile building the test safety net for this refactor I found a pre-existing crash (reproduces on unmodified
main, tracked separately in #150):captureToImage()of aModifier.skytree with a descendantModifier.cloudy(sky = ...)backdrop SIGSEGVs the RenderThread with an unboundedRenderNode::prepareTreeImplrecursion. The backdrop blur recordsdrawLayer(sky.backgroundLayer), and since the backdrop node is a descendant of the sky recorder, that closes a cyclic RenderNode reference. On-screen rendering survives it (theSky.isCapturingguard, damage-scoped walk), but the full-tree re-walkcaptureToImageforces does not. The fix samples the sky through a rasterized snapshot (drawImage) instead of the live layer, which is the acyclic structure the API < 31 CPU path already used. It carries onto this branch as commit6a42ed6.Test plan
SkyBackdropRasterTest(desktopTest,ImageComposeScene) — the first test that executes the skiko backdrop render path, asserting blur / tint / mirage pixel effects.Issue112RegressionTest+BackdropCaptureCrashRegressionTest(instrumented) — the four capture variants that crashed before the fix now pass.MirageElementEqualityTest,MiragePlanModifierTest,MirageFilterChainTest,MirageChromaticRasterTestupdated to the unified types, intent unchanged.desktopTest,testAndroidHostTest,spotlessCheck,checkLegacyAbigreen.iosArm64+wasmJscompile (commonMain purity).Summary by CodeRabbit