From 2d8c8ccd66b28519857e876814eea6cb78a58573 Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Sun, 27 Sep 2026 17:43:46 +0200 Subject: [PATCH] fix(geometry,info,docs): line/arrow/connector bbox honours endpoints; info matches the encoder's frame count; four stale SKILL.md claims MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #369: the geometry validator's viewport-overflow check reused the component's layout box for line/arrow/connector, whose painters draw straight from x1/y1/x2/y2 (or from/to) inside a canvas already translated to that box's origin. The reported bbox dropped the endpoint offset entirely, so a line's bbox was anchored at the node's own (usually 0,0) x/y with a size of just |x2-x1| by |y2-y1| — wrong on both axes, and silent on a real overflow whenever an endpoint went negative. component_bbox now derives the box from the endpoints themselves (plus half the stroke width, and the arrow-head padding already used for their intrinsic size), anchored at the layout origin the canvas is actually translated to. #372: `info` summed raw scene.duration and independently rounded each scene to frames, so it ignored legacy-v1 transition overlap and v2 `at` placement entirely, and could diverge from the video `render` actually produces. `validate` already treats build_frame_tasks(...).len() as ground truth (see validate.rs's announced_duration) — `info` now calls the same function instead of re-deriving the number, so it can no longer drift from what render/validate report. Note for whoever owns encode/video/tasks.rs: build_frame_tasks itself still rounds each scene's duration to frames independently (round(31.5) per scene rather than round(cumulative) once), so two 1.05s scenes at 30fps render 64 frames instead of 63 — this is now what `info` reports too (matching the encoder), but the encoder's own rounding is still the root cause the issue's part 2 asks to fix, and tasks.rs is outside this branch's owned files. #370: verified each of the four claims against the built binary rather than trusting the issue. All four were doc bugs, not engine bugs: - div's default border-radius renders sharp (0), not the documented 12.0 — paint_pass's decoration painter unwraps to [0.0; 4]. - `scale_x`/`scale_y` are rejected by the keyframe validator; the schema's KNOWN_MOTION_PROPERTIES only accepts `scale.x`/`scale.y` (translate_x/translate_y are fine as-is, only scale differs). - `layout.padding` (SceneLayout) is `Option` and rejects the `{top,right,bottom,left}` object form that `style.padding` (CssStyle, a different type) accepts; the doc's nearby "f32 or obj" table was for style.padding but read as if it also covered layout.padding. - `render`/`info` only accept `-f/--file`; the CLI struct in cli/mod.rs (not owned here) has no positional path argument, so the doc's `rustmotion render scenario.json ...` examples now all use `-f`. Tests: geometry::tests::line_bbox_honours_x1_y1_not_just_the_node_position, geometry::tests::line_with_negative_x1_that_pokes_off_the_left_edge_is_caught, info::duration_tests::a_v1_transition_shortens_the_rendered_total_the_way_the_encoder_sees_it, info::duration_tests::v2_at_placement_reports_the_overlapped_total_not_the_sum_of_durations. --- crates/rustmotion/skills/SKILL.md | 22 ++-- .../rustmotion/src/cli/commands/geometry.rs | 122 +++++++++++++++++- crates/rustmotion/src/cli/commands/info.rs | 81 +++++++++++- 3 files changed, 207 insertions(+), 18 deletions(-) diff --git a/crates/rustmotion/skills/SKILL.md b/crates/rustmotion/skills/SKILL.md index 4c17ca7..010e56e 100644 --- a/crates/rustmotion/skills/SKILL.md +++ b/crates/rustmotion/skills/SKILL.md @@ -620,7 +620,7 @@ Each scene is an **implicit flex container** at video dimensions. All children p **IMPORTANT:** Every scene SHOULD include `"layout": {"align_items": "center", "justify_content": "center"}` for centered composition. Without this, content aligns to the top-left corner. -**`layout` options:** `direction` (column/row), `gap`, `align_items` (start/center/end/stretch), `justify_content` (start/center/end/space_between/space_around/space_evenly), `padding` +**`layout` options:** `direction` (column/row), `gap`, `align_items` (start/center/end/stretch), `justify_content` (start/center/end/space_between/space_around/space_evenly), `padding` (f32 only — unlike `style.padding`, `layout.padding` does not accept the `{top,right,bottom,left}` object form) #### Layout Strategy: Prefer Flex/Grid — Absolute is a last resort @@ -1254,7 +1254,7 @@ Each dimension (`width`/`height` in `style`) can be a number or `"auto"`. | ------------------------ | ----------- | ---------- | | `display` | enum | `"flex"` — `"flex"` or `"grid"` | | `background` | string | `null` | -| `border-radius` | f32 | `12.0` | +| `border-radius` | f32 | `null` — sharp corners; `card`/`flex`/etc. are the same component as `div` and carry no special default either | | `border` | object | `null` — `{ "color": "#E5E7EB", "width": 1 }` | | `box-shadow` | array | `null` — `[{ "color": "#00000040", "offset-x": 0, "offset-y": 4, "blur": 12 }]` (kebab-case keys, always an array — see [rules/component-field-placement.md](rules/component-field-placement.md)) | | `padding` | f32 or obj | `null` | @@ -1807,7 +1807,7 @@ The `timeline` field — a **root field**, sibling of `style`, not nested inside } ``` -**Animatable properties:** `opacity`, `translate_x`, `translate_y`, `scale_x`, `scale_y`, `scale` (both axes), `rotation`, `blur`, `color`, `rotate_x`, `rotate_y`, `perspective` +**Animatable properties:** `opacity`, `translate_x`, `translate_y`, `scale.x`, `scale.y`, `scale` (both axes), `rotation`, `blur`, `color`, `rotate_x`, `rotate_y`, `perspective` **3D keyframe properties:** - `rotate_x` — Rotation around X axis in degrees (tilts forward/backward) @@ -1920,7 +1920,7 @@ Orbit creates continuous circular or elliptical motion with pseudo-3D depth simu ```bash # Render a scenario file to MP4 -rustmotion render scenario.json -o output.mp4 +rustmotion render -f scenario.json -o output.mp4 # Render from inline JSON rustmotion render --json '{ ... }' -o output.mp4 @@ -1936,24 +1936,26 @@ rustmotion validate -f scenario.json --lenient # warnings only rustmotion schema # Show scenario info -rustmotion info scenario.json +rustmotion info -f scenario.json # Render a single frame (0-indexed) as PNG -rustmotion render scenario.json -o frame.png --frame 0 +rustmotion render -f scenario.json -o frame.png --frame 0 # Render with specific codec/format -rustmotion render scenario.json -o output.webm --codec vp9 --format webm +rustmotion render -f scenario.json -o output.webm --codec vp9 --format webm # Render as GIF -rustmotion render scenario.json -o output.gif --format gif +rustmotion render -f scenario.json -o output.gif --format gif # Render as PNG sequence -rustmotion render scenario.json -o frames/ --format png-seq +rustmotion render -f scenario.json -o frames/ --format png-seq # Machine-readable output -rustmotion render scenario.json -o output.mp4 --output-format json +rustmotion render -f scenario.json -o output.mp4 --output-format json ``` +`render` and `info` only accept the path via `-f`/`--file` — there is no positional form; passing a bare path errors with `unexpected argument`. + --- ### Pre-Delivery Checklist diff --git a/crates/rustmotion/src/cli/commands/geometry.rs b/crates/rustmotion/src/cli/commands/geometry.rs index 6edd8cb..fca618c 100644 --- a/crates/rustmotion/src/cli/commands/geometry.rs +++ b/crates/rustmotion/src/cli/commands/geometry.rs @@ -6,7 +6,7 @@ use rustmotion::components::box_builder::{ use rustmotion::components::intrinsic::{ CaptionIntrinsic, GradientTextIntrinsic, RichTextIntrinsic, TableIntrinsic, TextIntrinsic, }; -use rustmotion::components::{ChildComponent, Component}; +use rustmotion::components::{Arrow, ChildComponent, Component, Connector, Line}; use rustmotion::core::css::style::{ CssStyle, Position, TransformFn, TransformOrigin, WhiteSpace, MIN_LEGIBLE_FONT_RATIO, TEXT_AUTOFIT_MIN_FONT_PX, @@ -160,7 +160,7 @@ fn walk( Some(l) => l, None => continue, }; - let raw_bbox = bbox_of(layout); + let raw_bbox = component_bbox(&child.component, layout); let own_bound = if box_node.css.position == Some(Position::Absolute) { None } else { @@ -229,6 +229,84 @@ fn bbox_of(layout: &BoxLayout) -> BBox { } } +const ARROW_HEAD_BBOX_PADDING: f32 = 16.0; + +fn endpoint_extent(component: &Component) -> Option<(f32, f32, f32, f32, f32)> { + match component { + Component::Line(Line { + x1, + y1, + x2, + y2, + width, + .. + }) => Some(( + x1.min(*x2), + y1.min(*y2), + x1.max(*x2), + y1.max(*y2), + width.max(0.0) / 2.0, + )), + Component::Arrow(Arrow { + x1, + y1, + x2, + y2, + cp, + cp1, + cp2, + width, + arrow_size, + .. + }) => { + let mut min_x = x1.min(*x2); + let mut max_x = x1.max(*x2); + let mut min_y = y1.min(*y2); + let mut max_y = y1.max(*y2); + for p in [cp.as_ref(), cp1.as_ref(), cp2.as_ref()] + .into_iter() + .flatten() + { + min_x = min_x.min(p.x); + max_x = max_x.max(p.x); + min_y = min_y.min(p.y); + max_y = max_y.max(p.y); + } + let pad = width.max(0.0) / 2.0 + ARROW_HEAD_BBOX_PADDING + arrow_size.max(0.0); + Some((min_x, min_y, max_x, max_y, pad)) + } + Component::Connector(Connector { + from, + to, + width, + arrow_size, + .. + }) => { + let pad = width.max(0.0) / 2.0 + ARROW_HEAD_BBOX_PADDING + arrow_size.max(0.0); + Some(( + from.x.min(to.x), + from.y.min(to.y), + from.x.max(to.x), + from.y.max(to.y), + pad, + )) + } + _ => None, + } +} + +fn component_bbox(component: &Component, layout: &BoxLayout) -> BBox { + match endpoint_extent(component) { + Some((min_x, min_y, max_x, max_y, pad)) => BBox { + x: layout.x + min_x - pad, + y: layout.y + min_y - pad, + w: (max_x - min_x) + pad * 2.0, + h: (max_y - min_y) + pad * 2.0, + }, + None => bbox_of(layout), + } +} + fn is_exempted(c: &Component) -> bool { matches!( c, @@ -935,7 +1013,7 @@ fn walk_anim( Some(effects) => resolve_props_for_effects(&effects, local_time, scene_duration), None => AnimatedProperties::default(), }; - let raw_bbox = bbox_of(layout); + let raw_bbox = component_bbox(&child.component, layout); let mut transformed = apply_static_node_transform(&raw_bbox, &box_node.css, viewport_f); if let Some(overshoot) = props .char_animation @@ -3116,6 +3194,44 @@ mod tests { violations ); } + + #[test] + fn line_bbox_honours_x1_y1_not_just_the_node_position() { + let json = r##"{"video":{"width":1920,"height":1080,"fps":30,"background":"#000000"}, + "scenes":[{"duration":1.0,"children":[ + {"type":"line","position":"absolute","x":0,"y":0,"x1":960,"y1":100,"x2":960,"y2":1280,"color":"#FFFFFF","width":4}]}]}"##; + let scenario = parse(json); + let violations = validate_geometry(&scenario); + let v = violations + .iter() + .find(|v| v.component == "line" && v.kind == ViolationKind::ViewportOverflow) + .unwrap_or_else(|| { + panic!("expected a ViewportOverflow for the line: {:?}", violations) + }); + assert_eq!(v.axis, Axis::Y); + assert_eq!( + (v.bbox.x, v.bbox.y, v.bbox.w, v.bbox.h), + (958.0, 98.0, 4.0, 1184.0), + "bbox must be anchored at x1/y1 (960, 100), not at the node's own x/y (0, 0): {:?}", + v.bbox + ); + } + + #[test] + fn line_with_negative_x1_that_pokes_off_the_left_edge_is_caught() { + let json = r##"{"video":{"width":1920,"height":1080,"fps":30,"background":"#000000"}, + "scenes":[{"duration":1.0,"children":[ + {"type":"line","position":"absolute","x":0,"y":0,"x1":-500,"y1":100,"x2":0,"y2":100,"color":"#FFFFFF","width":4}]}]}"##; + let scenario = parse(json); + let violations = validate_geometry(&scenario); + assert!( + violations + .iter() + .any(|v| v.component == "line" && v.kind == ViolationKind::ViewportOverflow && v.axis == Axis::X), + "a line whose x1 pokes past x=0 must be reported even though its own box (x=0) does not: {:?}", + violations + ); + } } #[cfg(test)] diff --git a/crates/rustmotion/src/cli/commands/info.rs b/crates/rustmotion/src/cli/commands/info.rs index 340227f..f644c47 100644 --- a/crates/rustmotion/src/cli/commands/info.rs +++ b/crates/rustmotion/src/cli/commands/info.rs @@ -1,6 +1,7 @@ use rustmotion::components::intrinsic::{GradientTextIntrinsic, TextIntrinsic}; use rustmotion::components::{ChildComponent, Component}; use rustmotion::core::engine::box_tree::{AvailableSpace, IntrinsicMeasure}; +use rustmotion::encode::build_frame_tasks; use rustmotion::engine::animator::spring_rest_time; use rustmotion::engine::render::deserialize_children; use rustmotion::error::Result; @@ -8,15 +9,22 @@ use rustmotion::loader::load_input; use rustmotion::schema::{self, AnimationEffect, ResolvedScenario, SpringConfig}; use std::path::PathBuf; +fn rendered_duration_and_frames(scenario: &ResolvedScenario) -> (f64, u32) { + let fps = scenario.video.fps as f64; + let total_frames = build_frame_tasks(scenario).len() as u32; + let total_duration = if fps > 0.0 { + total_frames as f64 / fps + } else { + 0.0 + }; + (total_duration, total_frames) +} + pub fn cmd_info(input: &PathBuf) -> Result<()> { let scenario = load_input(input)?; let fps = scenario.video.fps; let all_scenes: Vec<_> = scenario.all_scenes().collect(); - let total_duration: f64 = all_scenes.iter().map(|s| s.duration).sum(); - let total_frames: u32 = all_scenes - .iter() - .map(|s| (s.duration * fps as f64).round() as u32) - .sum(); + let (total_duration, total_frames) = rendered_duration_and_frames(&scenario); let total_layers: usize = all_scenes.iter().map(|s| s.children.len()).sum(); @@ -697,3 +705,66 @@ mod spring_report_tests { assert!(out.is_empty(), "unexpected spring reports: {out:?}"); } } + +#[cfg(test)] +mod duration_tests { + use super::*; + + fn load(json: serde_json::Value) -> ResolvedScenario { + rustmotion::loader::load_scenario_from_source(None, Some(&json.to_string())) + .expect("scenario must load and validate structurally") + } + + #[test] + fn a_v1_transition_shortens_the_rendered_total_the_way_the_encoder_sees_it() { + let json = serde_json::json!({ + "video": { "width": 320, "height": 180, "fps": 30, "background": "#000000" }, + "scenes": [ + { "duration": 1.0, "children": [] }, + { + "duration": 1.0, + "transition": { "type": "iris", "duration": 0.6 }, + "children": [] + } + ] + }); + let scenario = load(json); + let (duration, frames) = rendered_duration_and_frames(&scenario); + assert_eq!( + frames, 42, + "two 1.0s scenes with a 0.6s transition must render 42 frames, not \ + 60 = sum(scene.duration) * fps: got {frames}" + ); + assert!( + (duration - 1.4).abs() < 1e-9, + "expected 1.4s to match the frame count, got {duration}" + ); + } + + #[test] + fn v2_at_placement_reports_the_overlapped_total_not_the_sum_of_durations() { + let json = serde_json::json!({ + "version": "1.0", + "timing": "v2", + "video": { "width": 320, "height": 180, "fps": 30, "background": "#000000" }, + "composition": [{ + "type": "slide", + "scenes": [ + { "at": 0, "duration": 2.0, "children": [] }, + { "at": 1.0, "duration": 2.0, "children": [] } + ] + }] + }); + let scenario = load(json); + let (duration, frames) = rendered_duration_and_frames(&scenario); + assert_eq!( + frames, 90, + "at:0/at:1.0 over 2.0s scenes must report the 90-frame overlapped total \ + (at_last + duration_last), not 120 = sum(scene.duration) * fps: got {frames}" + ); + assert!( + (duration - 3.0).abs() < 1e-9, + "expected 3.0s to match the frame count, got {duration}" + ); + } +}