test(effects): record why per-node effects need paint_pass, with numbers - #175
Merged
Conversation
Investigating the "per-node visual effects" gap produced a negative result rather than a feature, and this commit ships the evidence so the next attempt does not have to rediscover it. `effects` is a field of `Scene` only, so an effect covers the whole frame. The obvious workaround — render normally, then crop the effect to a node's rect from `render_scene_hits` — does not merely cost too much, it is wrong. A background node with `pixelate` applied that way contaminates **5904 of 10000 pixels** inside an overlapping sibling's own box; a sample pixel goes from pure blue to `[155, 0, 99]`. `buffer_crop_strategy_leaks_effect_onto_ overlapping_sibling` pins that: it passes, and what it documents is the rejection. The same approach also costs x1.47 on a real scenario (mega-showcase scene 0, 1920x1080, release: 6.0ms/frame becomes 8.8ms), but the leak is the disqualifying part, not the cost. The cost benchmark is kept `#[ignore]`d because its other measurement is the one that matters for whoever implements this properly: bounding the CPU effect pass to a node's real box instead of the full frame is **x14.57** faster at a 13.9x area ratio. That is the same argument PR #154 already made for the `save_layer` it bounded (42-60s down to 0.5s over 60 frames), and it says any real implementation must bound its layer to the node. No production file is touched. The feature is blocked on three things that sit outside one file: a node's device-space rect exists only inside `paint_node` (animation presets resolve into `CssStyle.transform`, so a rect computed from `run_layout` alone is silently wrong for most animated content); `post_effects.rs` lives in the crate that depends on the one holding `paint_pass.rs`, not the reverse; and `CssStyle` is the only per-node channel that reaches `BoxNode` without teaching `box_builder` a new field.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a diagnostic, not a feature. Investigating the "per-node visual effects" gap (High/L) produced a negative result, and this PR ships the evidence so the next attempt does not rediscover it. No production file is touched.
What was asked, and what happened
effectsis a field ofSceneonly, so an effect covers the whole frame: you can grey a scene, never a single card. The brief asked for per-node application first and a bigger catalogue second, withpaint_pass.rsread-only and an instruction to stop and explain if per-node application required touching it.It does. And the obvious way around it is not merely expensive — it is wrong.
The workaround is disqualified, with a number
Render normally, then crop the effect to a node's rect obtained from
render_scene_hits:59% contamination of a sibling that sits visually on top. That is the exact case the feature exists for — a treatment on a background image with text over it — and the workaround destroys the text.
buffer_crop_strategy_leaks_effect_onto_overlapping_siblingpins this. It passes; what it documents is the rejection.The same approach also costs x1.47 on a real scenario (
mega-showcasescene 0, 1920x1080, release: 6.0ms/frame → 8.8ms). But the leak is what disqualifies it, not the cost.The measurement that matters for whoever implements this
Bounding the CPU effect pass to a node's real box instead of the full frame, at a 13.9x area ratio:
That is the same argument PR #154 already made for the
save_layerit bounded — 42-60s down to 0.5s over 60 frames, the largest performance win of the whole chantier. Any real implementation must bound its layer to the node, and now there is a benchmark to prove it did.Why it is blocked, precisely
Three things, and only the first is the frozen file:
paint_node.BoxLayoutcarries neither transform nor camera;paint_nodeaccumulates the canvas matrix and derives the rect. This is not a corner case:css/animation.rsresolves animation presets intoCssStyle.transformoverrides, so most animated content carries a transform absent from the raw JSON. A rect computed fromrun_layoutalone would be silently wrong for it.post_effects.rslives inrustmotion, which depends onrustmotion-corewherepaint_pass.rslives — never the reverse. The pure effect functions have to move first.CssStyleis the only per-node channel that already reachesBoxNodewithout teachingbox_builder.rsa new field — which would be a second de-facto frozen file.A sketch of the patch is in the commit and in the issue filed alongside this PR.
Two findings worth keeping
style.filteralready covers part of the gap, today.FilterFnhas a real SkiaNoise { intensity, seed }, applied per node with correct layer isolation.style.filter: [{"fn":"noise","intensity":0.15,"seed":42}]gives "grain on a background, text spared" right now, without any of this work. What it cannot do is animated grain —noise_image_filteris deliberately frame-stable. So the grain half of the gap is narrower than the differential suggested; vignette, pixelate and progressive-blur have noFilterFnequivalent and are where the value is.No registry was built, deliberately. The repo's
enum+ exhaustivematchconvention has a property a dynamic registry does not: the compiler forces every site to be updated when a variant is added. A registry trades that static guarantee for a silent registration omission at runtime — worse, not better, for a catalogue of four to six variants. It would not even reduce the number of places to touch; it would move them.Verification
cargo test --workspace: green (the new leak proof included)cargo fmt --all --checkandcargo clippy --workspace --all-targets -- -D warnings: cleangit status: two test files added, nothing else