Repository navigation
fix(registry): clamp filmed-screen's curve and vignette - #5501
Merged
Merged
Conversation
Post-merge review of #5499: - curve is clamped at 0: a negative value samples outside the frame and folds over below about -0.32. - the vignette factor is clamped at 0: above vignette ~1.11 it went negative in the corners. - README: a "previous look" line with the pre-#5499 values, noting that the LED fade-in threshold and the trace of fringing on the focused element are hard-coded. The demo renders the same: at the defaults neither clamp is reached. Co-Authored-By: Claude Opus 5.5 <[email protected]>
jrusso1020
approved these changes
Oct 12, 2026
jrusso1020
left a comment
jrusso1020
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed at ae9aab0c. This covers all three follow-ups from my #5499 review.
- Curve.
Math.max(0, …)onuCurvein both the snippet and the demo copy. Removing it fails two tests: the newcurve: -1test, and the existing check that the demo carries the same library. - Vignette.
max(0.0, 1.0 - uVig * dot(v, v))in both shaders. At the default 1.0 the factor stays above 0 everywhere in a 16:9 frame (the corners sit at about 0.10), so the default render doesn't change. The clamp only matters from about 1.11 up. It isn't tested, since the tests stub WebGL. - README. I checked the "previous look" values against
071ce804: blur 0.9 and rack 0.7 were the JS defaults, vignette 0.42 and LED 0.9 were constants in the shader, curve didn't exist, and aberration was an unscaled 1×. The two hard-coded changes are described correctly: the LED fade (smoothstep(4.0, 6.5)on cell size) and the 0.25 of the fringing left at full focus (1.0 - 0.75 * sharp).
Nit: curve: NaN or a string still goes through, because Math.max(0, NaN) is NaN. Every other option takes values the same way, so this isn't new.
Tests pass 7/7 locally. The required checks are green at this head. I didn't render anything.
— Rames
…view The catalog builds this item's payload as unsupported (it needs canvas-draw-element), and without preview.video the page fell back to the flag notice: "Needs chrome://flags/#canvas-draw-element to render live" under a "Recorded preview" caption, with no video, whatever the visitor's flags. The manifest now names the CDN video and poster the way the liquid-glass blocks do; the regenerated page carries video= and poster= instead of needsFlag. Co-Authored-By: Claude Opus 5.5 <[email protected]>
jrusso1020
approved these changes
Oct 12, 2026
jrusso1020
left a comment
jrusso1020
left a comment
Collaborator
There was a problem hiding this comment.
Re-approving at 86d86e0a. The only change since ae9aab0c is one commit adding preview.video and preview.poster to registry-item.json.
- Both URLs return 200: the video is 1280×720, 8 s,
video/mp4, and the poster isimage/png. A frame from the video has the strong corner vignette, so it shows the new look rather than the #5487 one. - The shape matches other components'
previewblocks (e.g.grain-overlay).catalog-detail.jsxpicks the recorded video over theneedsFlagnotice whenvideois set, so the page caption becomes "Recorded preview". - The generated
docs/catalog/components/filmed-screen.mdxstill hasneedsFlagat this head. The publish bot regenerates it after merge, as the PR says.
My review at ae9aab0c otherwise stands. The required checks are green at this head.
— Rames
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.
Fix-forward from Rames's post-merge review of #5499 (review).
curveclamped at 0. A negative value samples outside the frame and folds over below about −0.32. A new test mounts withcurve: -1and checks that the shader gets 0; it fails without the clamp.Vignette factor clamped at 0 (
max(0.0, 1.0 - uVig * dot(v, v))). Abovevignette≈ 1.11 it went negative in the corners.README: a "previous look" line with the values from before feat(registry): filmed-screen curves, orbits and follows typing by default #5499 (
blur: 0.9,rack: 0.7,vignette: 0.42,aberration: 1,led: 0.9,curve: 0). It notes that the LED fade-in threshold and the trace of fringing on the focused element are hard-coded and can't be restored by options.Catalog page preview:
registry-item.jsonnow setspreview.videoandpreview.poster(the CDN files). The catalog builds this item's payload as needingcanvas-draw-element. With no recorded video it showed the flag notice, under a "Recorded preview" caption, with nothing playing, whatever the visitor's flags were. Regenerated locally, the page now getsvideo=/poster=instead ofneedsFlag; the generated files themselves are left to the publish bot.The demo's output is unchanged: at the defaults neither clamp is reached (the 16:9 corner factor is about 0.10).
filmed-screen.test.mjspasses 7/7, and lint-registry-items, oxlint and oxfmt are clean.🤖 Generated with Claude Code