feat(compositor): text color, font selection, draggable layers, clipping fix#80
Conversation
…ing fix - Text color: add color picker (RGB + alpha slider) for text overlays - Font selection: add font_name field to TextOverlayConfig with 12 curated system fonts (DejaVu, Liberation, FreeFonts), dropdown in UI, warning when named font file is missing - Draggable layer list: replace z-index ▲/▼ buttons with drag-to-reorder using motion/react Reorder, reassigns z-indices on drop - Text clipping fix: expand bitmap height to ceil(font_size * 1.4) in rasterize_text_overlay and auto-expand UI rect height when font size increases Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
![staging-devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/870844?s=60&v=4){kind=small}
| <Reorder.Group | ||
| axis="y" | ||
| values={entries} | ||
| onReorder={handleReorder} | ||
| as="div" | ||
| style={{ listStyle: 'none', padding: 0, margin: 0 }} | ||
| > | ||
| {entries.map((entry) => ( | ||
| <Reorder.Item | ||
| key={entry.id} | ||
| value={entry} | ||
| as="div" | ||
| style={{ listStyle: 'none' }} | ||
| dragListener={!disabled} |
There was a problem hiding this comment.
📝 Info: Reorder.Group receives useMemo-derived values instead of direct state
The Reorder.Group component at ui/src/nodes/CompositorNode.tsx:731 receives values={entries} where entries is computed via useMemo (line 593) rather than held in direct useState. Motion's Reorder typically expects values to be state that onReorder updates directly (e.g. [items, setItems] = useState(...) with onReorder={setItems}). Here, handleReorder goes through onReorderLayers → reorderLayers → multiple setState calls → useMemo recomputation. This round-trip works correctly because React batches the state updates into a single render, and the entries memo recomputes with the new z-indices. During drag, motion manages visual positions internally and only uses values for the committed order. No data loss or incorrect behavior observed, but this pattern is worth noting for future maintainers since it deviates from motion's typical usage pattern.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
- Add reorderLayers() to useCompositorLayers that atomically updates z-index for all layer types (video, text, image) in a single commit, preventing race conditions from stale refs when handleReorder fired N individual update calls. - Add missing color array comparison to mergeOverlayState's hasExtraChanges comparator so server-echoed color changes are correctly detected. - Remove unused onZIndexChange prop from UnifiedLayerList since reorderLayers now handles all z-index mutations. Signed-off-by: Devin AI <devin@devin.ai> Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Replace fixed z-index bands (text: 100+, image: 200+) with maxZIndex() + 1 so that new overlays always stack on top even after drag-to-reorder has normalized z-indices to [0, n-1]. Signed-off-by: Devin AI <devin@devin.ai> Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
Add measure_text() that computes actual pixel width/height from font metrics, and use it in rasterize_text_overlay to expand the bitmap to fit the full rendered string. Previously only height was expanded via a 1.4× heuristic; now both width and height use exact font measurements. On the UI side, updateTextOverlay now auto-expands the rect width (~0.6× font_size per character) in addition to height when the text would overflow the current box. Signed-off-by: Devin AI <devin@devin.ai> Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
Signed-off-by: Devin AI <devin@devin.ai> Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
- Replace throttledConfigChange with immediate onParamChange in reorderLayers' onParamChange path so video layer and overlay z-index updates commit atomically in the same tick. - Extract serializeLayers() helper to avoid duplicating layer serialization logic between buildConfig and reorderLayers. - Add missing dejavu-serif-bold and dejavu-sans-mono-bold to the font_name doc comment in TextOverlayConfig. Signed-off-by: Devin AI <devin@devin.ai> Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
- Add FONT_FAMILY_MAP to CompositorCanvas that maps backend font names to CSS font-family values. Text overlays now preview the selected font in both display and edit mode on the canvas. - Bold font variants (e.g. dejavu-sans-bold) render with fontWeight 700. - Fix FontSelect contrast: use var(--sk-panel-bg) instead of undefined var(--sk-input-bg), add color-scheme hint, and style option elements explicitly for dark mode compatibility. Signed-off-by: Devin AI <devin@devin.ai> Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
There was a problem hiding this comment.
🟡 measure_text height includes above-origin pixels that blit_text_rgba clips, causing top-of-glyph clipping for tall ascenders
The new measure_text function (crates/nodes/src/video/mod.rs:56-89) computes height as max_bottom - max_top, where max_top can be negative for glyphs taller than 'A' (e.g. accented capitals like Á, É). However, rasterize_text_overlay (crates/nodes/src/video/compositor/overlay.rs:231-236) calls blit_text_rgba with origin_y: 0. In blit_text_rgba, any pixel at dst_y < 0 is clipped (crates/nodes/src/video/mod.rs:134). This means: for 'Á' with gy = -3, the top 3 pixel rows (containing the accent mark) are lost, while 3 unused rows are allocated at the bottom of the bitmap. The fix is to pass origin_y = -max_top (i.e., the absolute value of the most-negative gy) to shift rendering down so all glyphs fit. Since measure_text only returns (width, height), it would need to also return the y-offset.
(Refers to lines 231-236)
Prompt for agents
In crates/nodes/src/video/mod.rs, change measure_text to return (u32, u32, i32) where the third element is the y-offset (the negative of max_top, i.e. how many pixels above origin the tallest glyph extends). Then in crates/nodes/src/video/compositor/overlay.rs rasterize_text_overlay, use that offset as origin_y when calling blit_text_rgba (lines 231-236), replacing the hardcoded 0. This ensures glyphs with tall ascenders (like accented capitals) are rendered fully within the bitmap instead of being clipped at the top.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
There was a problem hiding this comment.
Good catch on the ascender clipping for accented characters. This is a pre-existing limitation (blit_text_rgba always used origin_y: 0), but now that we have measure_text it would be straightforward to return the y-offset and pass it through. Will address if requested.
- Add web-safe intermediate fonts (Verdana, Georgia, Arial, Times New Roman, Courier New) to the CSS font-family fallback stacks so the canvas preview shows a visible difference between sans-serif, serif, and monospace font groups even when the exact system fonts are not installed in the browser. - Remove the alpha slider from text color controls. Text opacity is already covered by the layer opacity slider, and a standalone alpha slider for a single channel was confusing. The color picker now always sends alpha=255. Signed-off-by: Devin AI <devin@devin.ai> Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Signed-off-by: Devin AI <devin@devin.ai> Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
2 participants
Summary
Four compositor UI/backend improvements on the
videobranch:1. Text Color Support
<input type="color">) for text overlay color[R, G, B, 255]— text opacity is controlled via the existing layer opacity slider2. Font Selection
font_name: Option<String>field toTextOverlayConfigwith a curated map of 12 system fonts (DejaVu, Liberation, FreeFonts families)load_font()resolution order:font_data_base64→font_name→font_path→ system defaultfontNamefield toTextOverlayState, font dropdown in text overlay controls, round-trip serialization3. Draggable Layer List
Reorderfrommotion/reactreorderLayers()which batches all layer type updates into a single config commit (prevents race conditions from stale refs)maxZIndex() + 1instead of fixed bands so they always stack on top after reordering4. Text Layer Clipping Fix
rasterize_text_overlayused the fixedrect.width/rect.height— text was clipped both horizontally and verticallymeasure_text()that computes exact pixel dimensions from font metrics. Bitmap is now expanded in both width and height to fit the full rendered stringupdateTextOverlayauto-expands the rect width (~0.6× font size per character) and height (~1.4× font size) when text/fontSize changes would overflowBug fixes (post-review)
colorarray comparison tomergeOverlayState'shasExtraChangescomparatorhandleReordercalls with batchedreorderLayers()functionreorderLayersonParamChangepath to use immediate commit instead of throttledfont_namedoc comment to list all 12 available font namesvar(--sk-panel-bg)withcolor-schemehint and explicit option stylingReview & Testing Checklist for Human
fonts-dejavu-core,fonts-liberation,fonts-freefont-ttf)Notes
/usr/share/fonts/truetype/...). Missing fonts produce a warning and fall back to DejaVu Sansmotion/reactReordercomponent is already available sincemotionv12 is a project dependency