Repository navigation
fix(effects): address the review of the shader effects integration - #1223
Merged
EtienneLescot merged 1 commit intoOct 14, 2026
Merged
Conversation
- A loop's counter starts from its value at THAT loop: the last store before it in its block, else its initialiser. Any other write to the counter (another block, after the loop, through a pointer) is refused, so the safety ceiling can never under-count trips. - The compiled-effect cache never evicts an effect the current scene shows (Windows and Linux). - setEffect clamps a moved pill against its own lane only, the one the overlap check used. - checkShaderEffect returns null (not checked) when the addon throws. - Workbench: shaderEffect is an EditFamily, and the ripple scenario only counts ranges of an effect that reads ctx.clicks. - Docs: checkShaderEffect in the spec, effects in ai-agent.md's removeModifier and time-base lines.
Member
Author
|
Linux, on lavapipe under WSL, at ee5e02e: |
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
6 tasks done
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 free
to 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.
Summary
CodeRabbit's eight comments on #1210, all on the shader effects rows.
if (…) { i = -100000; } for (; i < 4; i++)count 4 trips for 100 004, under the 4096 safety ceiling.shader_fx::eviction_victim, Windows and Linux).set_scenestores the scene before warming the cache so eviction can see it. feat(compositor): the macOS shader effects pass (Metal) #1206 (macOS) will want the same.setEffectclamps a moved pill against its own lane only, the one the overlap check used.checkShaderEffectreturnsnull("not checked") when the addon throws.shaderEffectis anEditFamily; the ripple scenario counts only ranges of an effect that readsctx.clicks.checkShaderEffectin the spec (E.6 now says the in-app agent has the checker); effects inai-agent.md'sremoveModifierrow and time-base note.Related issue
None.
Type of change
Release impact
Desktop impact
Testing
the_counter_starts_from_its_value_at_the_loopandeviction_spares_the_effects_the_scene_shows. Windowscargo test -p openscreen-compositor -p compositor-view-napi --lib: 448 + 1 passed. Linux on lavapipe under WSL: see the comment below.moves a region against its own lane only, which fails on the old code (start clamped to 6 s instead of 4 s).agent-tools.test.tsandelectron/mcp: 318 passed.npx tsc --noEmit,-p tsconfig.test.json,-p tsconfig.workbench.json, Biome on the touched files.🤖 Generated with Claude Code