diff --git a/crates/rustmotion-cli/build.rs b/crates/rustmotion-cli/build.rs new file mode 100644 index 0000000..2525a63 --- /dev/null +++ b/crates/rustmotion-cli/build.rs @@ -0,0 +1,104 @@ +//! Generates the `SKILL_FILES` table embedded into the `rustmotion` binary by +//! `src/skills.rs`. +//! +//! `rustmotion skills install` is the only channel through which an LLM agent +//! receives the generation rules under `.claude/skills/rustmotion/`. Before +//! this build script existed, the embedded table was a hand-maintained +//! literal list: any rule file added to disk was invisible to `install` +//! until someone remembered to add a matching entry. That silent gap let 17 +//! of 47 rule files — including ones explicitly named by CLAUDE.md — never +//! reach an installed project (issue #165). +//! +//! Walking the directory at build time instead of listing files by hand +//! makes "every `.md` file under `.claude/skills/rustmotion/` is embedded" +//! true by construction, not by memory. `cargo:rerun-if-changed` on the +//! directory (Cargo scans it recursively) means adding, removing, or editing +//! a rule file triggers a rebuild of the generated table on the next build. + +use std::env; +use std::fs; +use std::path::{Path, PathBuf}; + +/// Recursively collect every `.md` file under `dir`, sorted by file name at +/// each directory level so the generated table has a stable, reproducible +/// order across machines and OSes. +fn collect_md_files(dir: &Path, out: &mut Vec) { + let mut entries: Vec<_> = fs::read_dir(dir) + .unwrap_or_else(|e| panic!("failed to read directory {}: {e}", dir.display())) + .filter_map(|e| e.ok()) + .collect(); + entries.sort_by_key(|e| e.file_name()); + + for entry in entries { + let path = entry.path(); + if path.is_dir() { + collect_md_files(&path, out); + } else if path.extension().and_then(|e| e.to_str()) == Some("md") { + out.push(path); + } + } +} + +fn main() { + let manifest_dir = + PathBuf::from(env::var("CARGO_MANIFEST_DIR").expect("CARGO_MANIFEST_DIR is set by cargo")); + // crates/rustmotion-cli -> crates -> + let workspace_root = manifest_dir + .parent() + .and_then(Path::parent) + .unwrap_or_else(|| { + panic!( + "expected {} to live at /crates/rustmotion-cli", + manifest_dir.display() + ) + }) + .to_path_buf(); + + let skills_root = workspace_root.join(".claude/skills/rustmotion"); + + // Rerun whenever any file under the skills tree is added, removed, or + // edited — Cargo scans directories given to rerun-if-changed recursively. + println!("cargo:rerun-if-changed={}", skills_root.display()); + // Once any rerun-if-changed is emitted, Cargo stops rebuilding on build.rs + // changes implicitly, so it must be listed explicitly too. + println!("cargo:rerun-if-changed=build.rs"); + + let skill_md = skills_root.join("SKILL.md"); + assert!( + skill_md.is_file(), + "expected {} to exist — is the workspace layout intact?", + skill_md.display() + ); + + let rules_dir = skills_root.join("rules"); + let mut rule_files = Vec::new(); + collect_md_files(&rules_dir, &mut rule_files); + + // SKILL.md first — `skills::show("skill")` and `skills::list()` treat it + // as the main skill definition — followed by every rule file, + // alphabetically. This mirrors the order of the previous hand-written + // list, but nothing downstream is allowed to depend on it: `show()` + // locates SKILL.md by path, not by position. + let mut all_files = vec![skill_md]; + all_files.extend(rule_files); + + let mut generated = String::from("&[\n"); + for path in &all_files { + let rel_path = path + .strip_prefix(&workspace_root) + .unwrap_or(path) + .to_str() + .unwrap_or_else(|| panic!("non-UTF-8 skill file path: {}", path.display())) + .replace('\\', "/"); // normalize separators if built on Windows + let abs_path = path + .to_str() + .unwrap_or_else(|| panic!("non-UTF-8 skill file path: {}", path.display())); + generated.push_str(&format!( + " SkillFile {{ path: {rel_path:?}, content: include_str!({abs_path:?}) }},\n" + )); + } + generated.push_str("]\n"); + + let out_dir = PathBuf::from(env::var("OUT_DIR").expect("OUT_DIR is set by cargo")); + fs::write(out_dir.join("skill_files.rs"), generated).expect("failed to write skill_files.rs"); +} diff --git a/crates/rustmotion-cli/src/commands/batch.rs b/crates/rustmotion-cli/src/commands/batch.rs index f1f358c..1220601 100644 --- a/crates/rustmotion-cli/src/commands/batch.rs +++ b/crates/rustmotion-cli/src/commands/batch.rs @@ -426,6 +426,10 @@ fn render_row( crf, format.map(str::to_string), transparent, + // `batch` has no `--hardware-acceleration` flag of its own (Lot D's + // scope stops at commands/render.rs; wiring one into `cmd_batch` + // touches this file). Always software here. + false, ) } diff --git a/crates/rustmotion-cli/src/commands/info.rs b/crates/rustmotion-cli/src/commands/info.rs index 76920b0..7edfd53 100644 --- a/crates/rustmotion-cli/src/commands/info.rs +++ b/crates/rustmotion-cli/src/commands/info.rs @@ -1,6 +1,9 @@ +use rustmotion::components::{ChildComponent, Component}; +use rustmotion::engine::animator::spring_rest_time; +use rustmotion::engine::render::deserialize_children; use rustmotion::error::Result; use rustmotion::loader::load_input; -use rustmotion::schema; +use rustmotion::schema::{self, AnimationEffect, ResolvedScenario, SpringConfig}; use std::path::PathBuf; pub fn cmd_info(input: &PathBuf) -> Result<()> { @@ -55,5 +58,195 @@ pub fn cmd_info(input: &PathBuf) -> Result<()> { } } + let springs = collect_springs(&scenario); + if !springs.is_empty() { + println!("Springs:"); + for report in &springs { + println!(" {}", report.describe()); + } + } + Ok(()) } + +/// Where a `SpringConfig` was found, and the settle time computed for it — +/// the "measure du repos" issue #167 lot E asks `rustmotion info` to +/// surface, so an author can size the enclosing animation's `duration` +/// around a spring instead of guessing (see `SpringConfig::duration`'s doc +/// comment for why the two are not automatically kept in sync). +#[derive(Debug)] +struct SpringReport { + label: String, + rest_seconds: f64, + duration_was_set: bool, +} + +impl SpringReport { + fn describe(&self) -> String { + if self.duration_was_set { + format!( + "{}: settles at {:.3}s (spring.duration set explicitly)", + self.label, self.rest_seconds + ) + } else { + format!( + "{}: settles at {:.3}s (natural — no spring.duration set; \ + pin the enclosing animation's duration to at least this to \ + avoid cutting the spring short)", + self.label, self.rest_seconds + ) + } + } +} + +fn collect_springs(scenario: &ResolvedScenario) -> Vec { + let mut out = Vec::new(); + for (vi, view) in scenario.views.iter().enumerate() { + for (si, scene) in view.scenes.iter().enumerate() { + let children = deserialize_children(scene); + let path = format!("view {} / scene {}", vi + 1, si + 1); + collect_springs_in_children(&children, &path, &mut out); + } + } + out +} + +fn collect_springs_in_children( + children: &[ChildComponent], + path: &str, + out: &mut Vec, +) { + for (i, child) in children.iter().enumerate() { + let p = format!("{path} / layer {}", i + 1); + if let Some(anim) = child.component.as_animatable() { + for effect in anim.animation_effects() { + if let Some((_, timing)) = effect.as_preset() { + if let Some(spring) = &timing.spring { + out.push(spring_report(&p, spring)); + } + } + if let AnimationEffect::Keyframes(k) = effect { + for kf_anim in &k.keyframes { + if let Some(spring) = &kf_anim.spring { + out.push(spring_report(&p, spring)); + } + } + } + } + } + match &child.component { + Component::Card(c) => collect_springs_in_children(&c.children, &p, out), + Component::Flex(c) => collect_springs_in_children(&c.children, &p, out), + Component::Grid(c) => collect_springs_in_children(&c.children, &p, out), + Component::Positioned(c) => collect_springs_in_children(&c.children, &p, out), + Component::Container(c) => collect_springs_in_children(&c.children, &p, out), + _ => {} + } + } +} + +fn spring_report(label: &str, spring: &SpringConfig) -> SpringReport { + SpringReport { + label: label.to_string(), + rest_seconds: spring_rest_time(spring), + duration_was_set: spring.duration.is_some(), + } +} + +#[cfg(test)] +mod spring_report_tests { + //! Issue #167 lot E: `rustmotion info` must surface the settle time of + //! every spring it finds, recursing into containers the same way + //! `validate_schema::validate_children` already does. + use super::*; + use rustmotion::components::ChildComponent; + + #[test] + fn finds_a_spring_on_a_top_level_preset() { + let child: ChildComponent = serde_json::from_value(serde_json::json!({ + "type": "text", + "content": "hi", + "style": { + "animation": [{ "name": "bounce_in", "duration": 0.6, "spring": { "damping": 12, "stiffness": 100, "mass": 1 } }] + } + })) + .unwrap(); + let mut out = Vec::new(); + collect_springs_in_children(&[child], "test", &mut out); + assert_eq!( + out.len(), + 1, + "expected exactly one spring report: {:?}", + out.iter().map(|r| &r.label).collect::>() + ); + assert!(out[0].rest_seconds > 0.0); + assert!(!out[0].duration_was_set); + } + + #[test] + fn finds_a_spring_nested_inside_a_card() { + let child: ChildComponent = serde_json::from_value(serde_json::json!({ + "type": "card", + "children": [{ + "type": "text", + "content": "hi", + "style": { + "animation": [{ "name": "fade_in_up", "duration": 0.6, "spring": { "damping": 15, "stiffness": 100, "mass": 1 } }] + } + }] + })) + .unwrap(); + let mut out = Vec::new(); + collect_springs_in_children(&[child], "test", &mut out); + assert_eq!( + out.len(), + 1, + "spring nested inside a card must be found: {out:?}" + ); + assert!( + out[0].label.contains("layer 1"), + "expected the nested layer to be labelled: {}", + out[0].label + ); + } + + #[test] + fn reports_the_pinned_duration_when_spring_duration_is_set() { + let child: ChildComponent = serde_json::from_value(serde_json::json!({ + "type": "text", + "content": "hi", + "style": { + "animation": [{ + "name": "bounce_in", + "duration": 0.6, + "spring": { "damping": 6, "stiffness": 120, "mass": 1, "duration": 0.8 } + }] + } + })) + .unwrap(); + let mut out = Vec::new(); + collect_springs_in_children(&[child], "test", &mut out); + assert_eq!(out.len(), 1); + assert!(out[0].duration_was_set); + assert!( + (out[0].rest_seconds - 0.8).abs() < 1e-9, + "spring.duration must be reported verbatim as the settle time, got {}", + out[0].rest_seconds + ); + } + + #[test] + fn no_springs_produces_an_empty_report() { + let child: ChildComponent = serde_json::from_value(serde_json::json!({ + "type": "text", + "content": "hi", + "style": { + "animation": [{ "name": "fade_in_up", "duration": 0.6 }] + } + })) + .unwrap(); + let mut out = Vec::new(); + collect_springs_in_children(&[child], "test", &mut out); + assert!(out.is_empty(), "unexpected spring reports: {out:?}"); + } +} diff --git a/crates/rustmotion-cli/src/commands/render.rs b/crates/rustmotion-cli/src/commands/render.rs index 6aff1f4..5e13a95 100644 --- a/crates/rustmotion-cli/src/commands/render.rs +++ b/crates/rustmotion-cli/src/commands/render.rs @@ -40,6 +40,7 @@ fn load_for_watch( Ok(loaded.scenario) } +#[allow(clippy::too_many_arguments)] pub fn cmd_render( scenario: ResolvedScenario, output: &Path, @@ -50,6 +51,7 @@ pub fn cmd_render( crf: Option, format: Option, transparent: bool, + hardware_acceleration: bool, ) -> Result<()> { let start = std::time::Instant::now(); @@ -106,6 +108,13 @@ pub fn cmd_render( .ok() }; + if hardware_acceleration && matches!(fmt, "png-seq" | "gif" | "raw") && !quiet { + eprintln!( + "Warning: --hardware-acceleration has no effect on `{fmt}` output — only the \ + ffmpeg-driven video encode step (mp4/webm/mov) can use a hardware encoder." + ); + } + match fmt { "png-seq" => { let mut tui = make_tui("png"); @@ -166,19 +175,27 @@ pub fn cmd_render( } } }; - encode::encode_with_ffmpeg( + encode::video::encode_with_ffmpeg_hw( &scenario, output_str, quiet, codec_str, crf, transparent, + hardware_acceleration, Some(&mut cb), )?; if let Some(ref mut t) = tui { t.finish("Done!"); } } else { + if hardware_acceleration && !quiet { + eprintln!( + "Hardware acceleration requested but ffmpeg was not found on PATH \ + (only ffmpeg can drive a hardware encoder); continuing with the \ + bundled software encoder." + ); + } let mut tui = make_tui("h264"); let mut cb = |p: encode::EncodeProgress| { if let Some(ref mut t) = tui { @@ -226,6 +243,7 @@ pub fn cmd_watch( crf: Option, format: Option, transparent: bool, + hardware_acceleration: bool, no_validate: bool, lenient: bool, strict_anim: bool, @@ -234,13 +252,20 @@ pub fn cmd_watch( use notify::{RecursiveMode, Watcher}; use std::sync::mpsc; - // Determine if we can use incremental rendering (native h264 only) + // Determine if we can use incremental rendering (native h264 only). + // Hardware acceleration is an ffmpeg-only feature (see + // `encode::video::encode_with_ffmpeg_hw`): the incremental/native path + // never shells out to ffmpeg at all, so routing a hardware-acceleration + // request there would silently do nothing. Forcing `use_ffmpeg` here + // keeps that request meaningful under `--watch` too, same as it already + // is for `codec`/`format`/`transparent`. let fmt = format .as_deref() .unwrap_or_else(|| output.extension().and_then(|e| e.to_str()).unwrap_or("mp4")); let use_ffmpeg = codec.as_deref().is_some_and(|c| c != "h264") || matches!(fmt, "webm" | "mov") - || transparent; + || transparent + || hardware_acceleration; let can_incremental = frame.is_none() && !matches!(fmt, "png-seq" | "gif" | "raw") && !use_ffmpeg; @@ -337,6 +362,7 @@ pub fn cmd_watch( crf, format.clone(), transparent, + hardware_acceleration, ) { eprintln!("Render error: {}", e); } @@ -491,6 +517,7 @@ pub fn cmd_watch( crf, format.clone(), transparent, + hardware_acceleration, ) { eprintln!("Render error: {}", e); } diff --git a/crates/rustmotion-cli/src/commands/validate_schema.rs b/crates/rustmotion-cli/src/commands/validate_schema.rs index 6b30b5b..3cb2985 100644 --- a/crates/rustmotion-cli/src/commands/validate_schema.rs +++ b/crates/rustmotion-cli/src/commands/validate_schema.rs @@ -378,6 +378,14 @@ fn check_color_str(s: &str, label: &str, path: &str, errors: &mut Vec) { /// floors these defensively (belt and suspenders — see `spring_value`'s doc /// comment), but catching it here gives the author an actionable error /// instead of a silently broken render. +/// +/// Issue #167 lot E adds `duration`/`rest_threshold`: a non-positive +/// `duration` would make `spring_value`'s remap divide by zero or invert +/// time (both silently ignored by the solver rather than rejected — see its +/// `Some(duration) if duration > 0.0` guard), and a `rest_threshold` outside +/// `(0.0, 1.0)` is either meaningless (<=0: never satisfied except in the +/// limit) or vacuous (>=1.0: satisfied from t=0, before the spring has +/// moved at all — the whole 0→1 travel is "close enough"). fn check_spring_config(spring: &SpringConfig, path: &str, errors: &mut Vec) { if spring.mass <= 0.0 { errors.push(format!( @@ -400,6 +408,28 @@ fn check_spring_config(spring: &SpringConfig, path: &str, errors: &mut Vec 0 when set (got {duration}) — a zero or \ + negative duration cannot be mapped to a settle time" + )); + } + } + if let Some(rest_threshold) = spring.rest_threshold { + if rest_threshold <= 0.0 { + errors.push(format!( + "{path}: spring.rest_threshold must be > 0 when set (got {rest_threshold}) — a \ + zero or negative threshold is never satisfied, so the spring would never be \ + considered at rest" + )); + } else if rest_threshold >= 1.0 { + errors.push(format!( + "{path}: spring.rest_threshold must be < 1.0 when set (got {rest_threshold}) — \ + a threshold this large is satisfied at t=0, before the spring has moved" + )); + } + } } /// The `time_scale` declared on a container component, if any. @@ -716,6 +746,108 @@ mod style_warning_tests { assert!(errors.is_empty(), "unexpected errors: {errors:?}"); } + // ---- issue #167 lot E: `spring.duration`/`spring.rest_threshold` ---- + + #[test] + fn zero_spring_duration_is_an_error() { + let child: ChildComponent = serde_json::from_value(serde_json::json!({ + "type": "text", + "content": "hi", + "style": { + "animation": [{ "name": "fade_in_up", "duration": 0.6, "spring": { "damping": 15, "stiffness": 100, "mass": 1, "duration": 0.0 } }] + } + })) + .unwrap(); + let mut errors = Vec::new(); + let mut warnings = Vec::new(); + validate_children(&[child], "test", 4.0, &mut errors, &mut warnings); + assert!( + errors + .iter() + .any(|e| e.contains("spring.duration") && e.contains("> 0")), + "missing spring.duration error: {errors:?}" + ); + } + + #[test] + fn negative_spring_duration_is_an_error() { + let child: ChildComponent = serde_json::from_value(serde_json::json!({ + "type": "text", + "content": "hi", + "style": { + "animation": [{ "name": "bounce_in", "duration": 0.6, "spring": { "damping": 15, "stiffness": 100, "mass": 1, "duration": -0.5 } }] + } + })) + .unwrap(); + let mut errors = Vec::new(); + let mut warnings = Vec::new(); + validate_children(&[child], "test", 4.0, &mut errors, &mut warnings); + assert!( + errors.iter().any(|e| e.contains("spring.duration")), + "missing spring.duration error: {errors:?}" + ); + } + + #[test] + fn zero_or_negative_rest_threshold_is_an_error() { + let child: ChildComponent = serde_json::from_value(serde_json::json!({ + "type": "text", + "content": "hi", + "style": { + "animation": [{ "name": "fade_in_up", "duration": 0.6, "spring": { "damping": 15, "stiffness": 100, "mass": 1, "rest_threshold": -0.01 } }] + } + })) + .unwrap(); + let mut errors = Vec::new(); + let mut warnings = Vec::new(); + validate_children(&[child], "test", 4.0, &mut errors, &mut warnings); + assert!( + errors.iter().any(|e| e.contains("spring.rest_threshold")), + "missing spring.rest_threshold error: {errors:?}" + ); + } + + #[test] + fn absurdly_large_rest_threshold_is_an_error() { + // >= 1.0 is satisfied at t=0, before the spring has moved at all — + // "at rest" from the first frame is not a meaningful measurement. + let child: ChildComponent = serde_json::from_value(serde_json::json!({ + "type": "text", + "content": "hi", + "style": { + "animation": [{ "name": "fade_in_up", "duration": 0.6, "spring": { "damping": 15, "stiffness": 100, "mass": 1, "rest_threshold": 1.0 } }] + } + })) + .unwrap(); + let mut errors = Vec::new(); + let mut warnings = Vec::new(); + validate_children(&[child], "test", 4.0, &mut errors, &mut warnings); + assert!( + errors.iter().any(|e| e.contains("spring.rest_threshold")), + "missing spring.rest_threshold error: {errors:?}" + ); + } + + #[test] + fn positive_spring_duration_and_rest_threshold_are_accepted() { + let child: ChildComponent = serde_json::from_value(serde_json::json!({ + "type": "text", + "content": "hi", + "style": { + "animation": [{ + "name": "fade_in_up", + "duration": 0.8, + "spring": { "damping": 15, "stiffness": 100, "mass": 1, "duration": 0.8, "rest_threshold": 0.01 } + }] + } + })) + .unwrap(); + let mut errors = Vec::new(); + let mut warnings = Vec::new(); + validate_children(&[child], "test", 4.0, &mut errors, &mut warnings); + assert!(errors.is_empty(), "unexpected errors: {errors:?}"); + } + #[test] fn positive_time_scale_is_accepted() { let child: ChildComponent = serde_json::from_value(serde_json::json!({ diff --git a/crates/rustmotion-cli/src/commands/validation.rs b/crates/rustmotion-cli/src/commands/validation.rs index 6477447..8561014 100644 --- a/crates/rustmotion-cli/src/commands/validation.rs +++ b/crates/rustmotion-cli/src/commands/validation.rs @@ -300,14 +300,32 @@ pub fn check_codec(codec: Option<&str>) -> Result<()> { Ok(()) } -/// Validate `--crf` is in the H.264/H.265 valid range. Defaults to OK if None. -pub fn check_crf(crf: Option) -> Result<()> { +/// Validate `--crf` is in the H.264/H.265 valid range, and flag when it is +/// paired with `--hardware-acceleration`. Returns `Ok(Some(warning))` for +/// that combination: hardware encoders (VideoToolbox/NVENC/QSV/AMF) are +/// bitrate/quality-driven, not CRF-driven, and `ffmpeg_args`'s hardware +/// branch does not emit `-crf` at all — passing `--crf` there silently does +/// nothing unless ffmpeg falls back to the software encoder, in which case +/// it applies after all. The caller decides whether/how to print the +/// warning (e.g. respecting `--quiet`); this function stays pure so the +/// combination is testable without capturing stderr. +/// +/// Still returns `Err` for an out-of-range value regardless of +/// `hardware_acceleration` — an invalid CRF is invalid on any path. +pub fn check_crf(crf: Option, hardware_acceleration: bool) -> Result> { if let Some(v) = crf { if v > 51 { return Err(RustmotionError::InvalidCrf { value: v }); } + if hardware_acceleration { + return Ok(Some(format!( + "--crf {v} has no effect if a hardware encoder ends up being used under \ + --hardware-acceleration (VideoToolbox/NVENC/QSV/AMF are bitrate/quality-driven, \ + not CRF-driven); it still applies if ffmpeg falls back to the software encoder." + ))); + } } - Ok(()) + Ok(None) } /// Print a report to stderr in the same format as `cmd_validate`. @@ -493,3 +511,38 @@ pub fn warn_strict_attrs_is_now_default() { checker was hardened; you can drop the flag." ); } + +#[cfg(test)] +mod check_crf_tests { + use super::check_crf; + + #[test] + fn no_crf_is_always_fine() { + assert_eq!(check_crf(None, false).unwrap(), None); + assert_eq!(check_crf(None, true).unwrap(), None); + } + + #[test] + fn crf_without_hardware_acceleration_warns_about_nothing() { + assert_eq!(check_crf(Some(23), false).unwrap(), None); + } + + #[test] + fn crf_with_hardware_acceleration_returns_a_warning_naming_the_value() { + let warning = check_crf(Some(23), true).unwrap().expect("must warn"); + assert!( + warning.contains("23"), + "warning should name the ignored value: {warning}" + ); + assert!( + warning.contains("--hardware-acceleration"), + "warning should name the flag responsible: {warning}" + ); + } + + #[test] + fn out_of_range_crf_still_errors_even_with_hardware_acceleration() { + assert!(check_crf(Some(52), true).is_err()); + assert!(check_crf(Some(52), false).is_err()); + } +} diff --git a/crates/rustmotion-cli/src/lib.rs b/crates/rustmotion-cli/src/lib.rs index be15b12..860ff06 100644 --- a/crates/rustmotion-cli/src/lib.rs +++ b/crates/rustmotion-cli/src/lib.rs @@ -76,6 +76,17 @@ enum Commands { #[arg(long)] transparent: bool, + /// Try this machine's hardware video encoder (VideoToolbox on macOS, + /// NVENC/QSV/AMF elsewhere) for h264/h265, probed live via `ffmpeg + /// -encoders` — never assumed from the platform this binary was + /// built for. Falls back to the software encoder, with a message, + /// when unavailable, unsupported for the chosen codec, or combined + /// with --transparent (no hardware encoder here produces an alpha + /// channel). `--crf` has no effect once a hardware encoder is + /// actually used; see the warning `check_crf` prints for that case. + #[arg(long)] + hardware_acceleration: bool, + /// Watch the input file for changes and re-render automatically #[arg(short, long)] watch: bool, @@ -452,6 +463,7 @@ pub fn run() -> Result<()> { crf, format, transparent, + hardware_acceleration, watch, no_validate, lenient, @@ -462,7 +474,11 @@ pub fn run() -> Result<()> { } => { // Validate codec / CRF up-front so we never spawn an encoder with bad args. commands::validation::check_codec(codec.as_deref())?; - commands::validation::check_crf(crf)?; + if let Some(warning) = commands::validation::check_crf(crf, hardware_acceleration)? { + if !cli.quiet { + eprintln!("Warning: {warning}"); + } + } let overrides = build_overrides(props.as_ref(), &var)?; @@ -483,6 +499,7 @@ pub fn run() -> Result<()> { crf, format, transparent, + hardware_acceleration, no_validate, lenient, strict_anim, @@ -531,6 +548,7 @@ pub fn run() -> Result<()> { crf, format, transparent, + hardware_acceleration, ) } } @@ -596,7 +614,10 @@ pub fn run() -> Result<()> { jobs, } => { commands::validation::check_codec(codec.as_deref())?; - commands::validation::check_crf(crf)?; + // `batch` has no `--hardware-acceleration` flag of its own (out of + // this workstream's file scope: wiring it into `cmd_batch` touches + // commands/batch.rs), so this combination can never fire here. + commands::validation::check_crf(crf, false)?; commands::cmd_batch( &file, &data, diff --git a/crates/rustmotion-cli/src/skills.rs b/crates/rustmotion-cli/src/skills.rs index 1267d67..7fbced6 100644 --- a/crates/rustmotion-cli/src/skills.rs +++ b/crates/rustmotion-cli/src/skills.rs @@ -13,135 +13,17 @@ struct SkillFile { const CLAUDE_MD: &str = include_str!("../../../CLAUDE.md"); /// All skill files embedded at compile time. -const SKILL_FILES: &[SkillFile] = &[ - SkillFile { - path: ".claude/skills/rustmotion/SKILL.md", - content: include_str!("../../../.claude/skills/rustmotion/SKILL.md"), - }, - // Rules - SkillFile { - path: ".claude/skills/rustmotion/rules/3d-perspective.md", - content: include_str!("../../../.claude/skills/rustmotion/rules/3d-perspective.md"), - }, - SkillFile { - path: ".claude/skills/rustmotion/rules/captions-workflow.md", - content: include_str!("../../../.claude/skills/rustmotion/rules/captions-workflow.md"), - }, - SkillFile { - path: ".claude/skills/rustmotion/rules/card-flex-layout.md", - content: include_str!("../../../.claude/skills/rustmotion/rules/card-flex-layout.md"), - }, - SkillFile { - path: ".claude/skills/rustmotion/rules/chart-types.md", - content: include_str!("../../../.claude/skills/rustmotion/rules/chart-types.md"), - }, - SkillFile { - path: ".claude/skills/rustmotion/rules/continuous-presets.md", - content: include_str!("../../../.claude/skills/rustmotion/rules/continuous-presets.md"), - }, - SkillFile { - path: ".claude/skills/rustmotion/rules/counter-standalone.md", - content: include_str!("../../../.claude/skills/rustmotion/rules/counter-standalone.md"), - }, - SkillFile { - path: ".claude/skills/rustmotion/rules/data-viz-components.md", - content: include_str!("../../../.claude/skills/rustmotion/rules/data-viz-components.md"), - }, - SkillFile { - path: ".claude/skills/rustmotion/rules/dot-map-coordinates.md", - content: include_str!("../../../.claude/skills/rustmotion/rules/dot-map-coordinates.md"), - }, - SkillFile { - path: ".claude/skills/rustmotion/rules/easing-guidelines.md", - content: include_str!("../../../.claude/skills/rustmotion/rules/easing-guidelines.md"), - }, - SkillFile { - path: ".claude/skills/rustmotion/rules/even-dimensions.md", - content: include_str!("../../../.claude/skills/rustmotion/rules/even-dimensions.md"), - }, - SkillFile { - path: ".claude/skills/rustmotion/rules/gradient-quality.md", - content: include_str!("../../../.claude/skills/rustmotion/rules/gradient-quality.md"), - }, - SkillFile { - path: ".claude/skills/rustmotion/rules/grid-card-height.md", - content: include_str!("../../../.claude/skills/rustmotion/rules/grid-card-height.md"), - }, - SkillFile { - path: ".claude/skills/rustmotion/rules/hex-colors.md", - content: include_str!("../../../.claude/skills/rustmotion/rules/hex-colors.md"), - }, - SkillFile { - path: ".claude/skills/rustmotion/rules/icon-format.md", - content: include_str!("../../../.claude/skills/rustmotion/rules/icon-format.md"), - }, - SkillFile { - path: ".claude/skills/rustmotion/rules/layer-order.md", - content: include_str!("../../../.claude/skills/rustmotion/rules/layer-order.md"), - }, - SkillFile { - path: ".claude/skills/rustmotion/rules/module-structure.md", - content: include_str!("../../../.claude/skills/rustmotion/rules/module-structure.md"), - }, - SkillFile { - path: ".claude/skills/rustmotion/rules/notification-stacking.md", - content: include_str!("../../../.claude/skills/rustmotion/rules/notification-stacking.md"), - }, - SkillFile { - path: ".claude/skills/rustmotion/rules/paint-context.md", - content: include_str!("../../../.claude/skills/rustmotion/rules/paint-context.md"), - }, - SkillFile { - path: ".claude/skills/rustmotion/rules/prefer-presets.md", - content: include_str!("../../../.claude/skills/rustmotion/rules/prefer-presets.md"), - }, - SkillFile { - path: ".claude/skills/rustmotion/rules/responsive-device-sizing.md", - content: include_str!( - "../../../.claude/skills/rustmotion/rules/responsive-device-sizing.md" - ), - }, - SkillFile { - path: ".claude/skills/rustmotion/rules/stagger-animations.md", - content: include_str!("../../../.claude/skills/rustmotion/rules/stagger-animations.md"), - }, - SkillFile { - path: ".claude/skills/rustmotion/rules/stat-cards.md", - content: include_str!("../../../.claude/skills/rustmotion/rules/stat-cards.md"), - }, - SkillFile { - path: ".claude/skills/rustmotion/rules/text-background.md", - content: include_str!("../../../.claude/skills/rustmotion/rules/text-background.md"), - }, - SkillFile { - path: ".claude/skills/rustmotion/rules/timeline-sequencing.md", - content: include_str!("../../../.claude/skills/rustmotion/rules/timeline-sequencing.md"), - }, - SkillFile { - path: ".claude/skills/rustmotion/rules/timing-constraints.md", - content: include_str!("../../../.claude/skills/rustmotion/rules/timing-constraints.md"), - }, - SkillFile { - path: ".claude/skills/rustmotion/rules/ui-controls.md", - content: include_str!("../../../.claude/skills/rustmotion/rules/ui-controls.md"), - }, - SkillFile { - path: ".claude/skills/rustmotion/rules/validate-json.md", - content: include_str!("../../../.claude/skills/rustmotion/rules/validate-json.md"), - }, - SkillFile { - path: ".claude/skills/rustmotion/rules/vertical-align.md", - content: include_str!("../../../.claude/skills/rustmotion/rules/vertical-align.md"), - }, - SkillFile { - path: ".claude/skills/rustmotion/rules/video-wizard.md", - content: include_str!("../../../.claude/skills/rustmotion/rules/video-wizard.md"), - }, - SkillFile { - path: ".claude/skills/rustmotion/rules/wiggle-additive.md", - content: include_str!("../../../.claude/skills/rustmotion/rules/wiggle-additive.md"), - }, -]; +/// +/// Generated by `build.rs`, which walks `.claude/skills/rustmotion/` at +/// build time and embeds every `.md` file it finds via `include_str!`. This +/// is intentionally not a hand-maintained literal: a manually curated list +/// silently drops any rule file nobody remembered to add (issue #165). See +/// `tests/skill_files_match_disk.rs` for the guard that keeps this table and +/// the on-disk rule set from drifting apart again. +/// +/// SKILL.md is always the first entry (see `build.rs`), but code here must +/// not rely on that position — locate it by path instead. +const SKILL_FILES: &[SkillFile] = include!(concat!(env!("OUT_DIR"), "/skill_files.rs")); /// Resolve the target directory for skill installation. fn resolve_target(global: bool) -> Result { @@ -271,10 +153,14 @@ pub fn show(name: &str) -> Result<()> { } } - // Special case: SKILL.md + // Special case: SKILL.md. Matched by path rather than position — the + // generated table happens to put SKILL.md first, but nothing here should + // depend on that ordering to keep working if it ever changes. if needle.eq_ignore_ascii_case("skill") { - print!("{}", SKILL_FILES[0].content); - return Ok(()); + if let Some(sf) = SKILL_FILES.iter().find(|sf| sf.path.ends_with("/SKILL.md")) { + print!("{}", sf.content); + return Ok(()); + } } Err(RustmotionError::UnknownSkill { diff --git a/crates/rustmotion-cli/tests/skill_files_match_disk.rs b/crates/rustmotion-cli/tests/skill_files_match_disk.rs new file mode 100644 index 0000000..98f4bf5 --- /dev/null +++ b/crates/rustmotion-cli/tests/skill_files_match_disk.rs @@ -0,0 +1,177 @@ +//! Guards `rustmotion skills install` against silently dropping rule files. +//! +//! `SKILL_FILES` (embedded by `build.rs`, see `src/skills.rs`) is meant to +//! mirror every `.md` file under `.claude/skills/rustmotion/` exactly. +//! Before `build.rs` existed, that table was a hand-maintained literal list +//! that fell 17 files behind disk without ever failing a build or a test +//! (issue #165) — including `geometry-safety.md`, `world-view.md` and +//! `audio-reactive.md`, the three rules CLAUDE.md cites by name. +//! +//! This test runs the actual compiled `rustmotion` binary's +//! `skills install` against a scratch directory and diffs the resulting +//! file set + content against the source tree, so any future drift between +//! disk and the embedded table fails loudly and names the missing files — +//! rather than requiring someone to remember to update a list by hand. + +use std::collections::BTreeSet; +use std::fs; +use std::path::{Path, PathBuf}; +use std::process::Command; + +/// `crates/rustmotion-cli` -> `crates` -> ``. +fn workspace_root() -> PathBuf { + Path::new(env!("CARGO_MANIFEST_DIR")) + .parent() + .and_then(Path::parent) + .expect("rustmotion-cli is expected at /crates/rustmotion-cli") + .to_path_buf() +} + +/// Recursively collect every file under `dir`, as paths relative to `dir`. +fn collect_files_relative(dir: &Path) -> BTreeSet { + fn walk(base: &Path, dir: &Path, out: &mut BTreeSet) { + for entry in fs::read_dir(dir).unwrap_or_else(|e| panic!("read_dir {}: {e}", dir.display())) + { + let entry = + entry.unwrap_or_else(|e| panic!("read_dir entry in {}: {e}", dir.display())); + let path = entry.path(); + if path.is_dir() { + walk(base, &path, out); + } else { + out.insert( + path.strip_prefix(base) + .unwrap_or_else(|_| { + panic!("{} not under {}", path.display(), base.display()) + }) + .to_path_buf(), + ); + } + } + } + let mut out = BTreeSet::new(); + walk(dir, dir, &mut out); + out +} + +/// Minimal RAII scratch directory — avoids pulling in a `tempfile` +/// dev-dependency for two tests. +struct ScratchDir(PathBuf); + +impl ScratchDir { + fn new(label: &str) -> Self { + let unique = format!( + "rustmotion-cli-test-{label}-{}-{}", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .expect("system clock before UNIX epoch") + .as_nanos() + ); + let path = std::env::temp_dir().join(unique); + fs::create_dir_all(&path).expect("create scratch dir"); + Self(path) + } +} + +impl Drop for ScratchDir { + fn drop(&mut self) { + let _ = fs::remove_dir_all(&self.0); + } +} + +fn run_skills_install(cwd: &Path) -> std::process::Output { + Command::new(env!("CARGO_BIN_EXE_rustmotion")) + .args(["skills", "install"]) + .current_dir(cwd) + .output() + .expect("failed to spawn `rustmotion skills install`") +} + +/// The red-phase test for issue #165: what `skills install` writes must be +/// byte-for-byte identical (file set + content) to +/// `.claude/skills/rustmotion/` on disk. Before `build.rs`, this failed by +/// naming exactly the 17 rule files missing from the hand-written list. +#[test] +fn skills_install_matches_source_tree() { + let root = workspace_root(); + let source_skills_dir = root.join(".claude/skills/rustmotion"); + let source_files = collect_files_relative(&source_skills_dir); + assert!( + !source_files.is_empty(), + "expected {} to contain rule files", + source_skills_dir.display() + ); + + let scratch = ScratchDir::new("skills-install"); + let output = run_skills_install(&scratch.0); + assert!( + output.status.success(), + "`rustmotion skills install` failed: stdout={} stderr={}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + + let installed_skills_dir = scratch.0.join(".claude/skills/rustmotion"); + assert!( + installed_skills_dir.is_dir(), + "`rustmotion skills install` did not create {}", + installed_skills_dir.display() + ); + let installed_files = collect_files_relative(&installed_skills_dir); + + let missing: Vec<_> = source_files.difference(&installed_files).collect(); + let extra: Vec<_> = installed_files.difference(&source_files).collect(); + + assert!( + missing.is_empty() && extra.is_empty(), + "`rustmotion skills install` diverges from .claude/skills/rustmotion/:\n\ + missing ({} files on disk but never installed): {:#?}\n\ + extra ({} files installed but absent from disk): {:#?}", + missing.len(), + missing, + extra.len(), + extra + ); + + // Matching file sets isn't enough on its own: also confirm content is + // byte-for-byte what's on disk, so a build.rs path-mapping bug (e.g. two + // files swapped with each other's content) fails here too. + for rel in &source_files { + let source_content = fs::read(source_skills_dir.join(rel)) + .unwrap_or_else(|e| panic!("read source {}: {e}", rel.display())); + let installed_content = fs::read(installed_skills_dir.join(rel)) + .unwrap_or_else(|e| panic!("read installed {}: {e}", rel.display())); + assert_eq!( + source_content, + installed_content, + "installed content for {} does not match .claude/skills/rustmotion/{}", + rel.display(), + rel.display() + ); + } +} + +/// Sanity check that `skills install` actually writes a non-trivial number +/// of files into a fresh, empty target — guards against the comparison test +/// above passing vacuously (e.g. both sides empty because the binary +/// silently failed to run). +#[test] +fn skills_install_writes_files_into_empty_target() { + let scratch = ScratchDir::new("skills-install-count"); + let output = run_skills_install(&scratch.0); + assert!( + output.status.success(), + "`rustmotion skills install` failed: stdout={} stderr={}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + + let installed_skills_dir = scratch.0.join(".claude/skills/rustmotion"); + let installed_files = collect_files_relative(&installed_skills_dir); + assert!( + installed_files.len() >= 47, + "expected at least 47 files installed (1 SKILL.md + 46+ rules), got {}: {:#?}", + installed_files.len(), + installed_files + ); +} diff --git a/crates/rustmotion-components/src/badge.rs b/crates/rustmotion-components/src/badge.rs index 04ea5e2..cf94b2e 100644 --- a/crates/rustmotion-components/src/badge.rs +++ b/crates/rustmotion-components/src/badge.rs @@ -112,35 +112,42 @@ rustmotion_core::impl_traits!(Badge { }); impl Badge { - fn resolved_font_size(&self) -> f32 { - self.style.font_size_px_or(self.badge_size.params().0) + /// Resolves `font-size` against a real per-frame viewport (`rem`/`vw`/ + /// `vh` now resolve instead of silently dropping to 0px — lot B, wave + /// S). `em`/`%` on `font-size` itself remain approximate — see + /// `crate::intrinsic::font_size_ctx`'s doc comment. + fn resolved_font_size(&self, ctx: &PaintCtx) -> f32 { + self.style.font_size_px_ctx( + &crate::intrinsic::font_size_ctx(ctx.video_width as f32, ctx.video_height as f32, 0.0), + self.badge_size.params().0, + ) } /// Returns (h_padding, v_padding, icon_size) scaled proportionally /// to the resolved font size. If style.font_size overrides the default, /// padding and icon scale with it. - fn resolved_params(&self) -> (f32, f32, f32) { + fn resolved_params(&self, ctx: &PaintCtx) -> (f32, f32, f32) { let (default_fs, h_pad, v_pad, icon_size) = self.badge_size.params(); - let actual_fs = self.resolved_font_size(); + let actual_fs = self.resolved_font_size(ctx); let ratio = actual_fs / default_fs; (h_pad * ratio, v_pad * ratio, icon_size * ratio) } - fn make_font(&self) -> Option { + fn make_font(&self, ctx: &PaintCtx) -> Option { let font_style = skia_safe::FontStyle::normal(); let family = self.style.font_family.as_deref().unwrap_or("Inter"); let typeface = typeface_with_fallback(family, font_style).ok()?; Some(skia_safe::Font::from_typeface( typeface, - self.resolved_font_size(), + self.resolved_font_size(ctx), )) } } impl Badge { - fn paint(&self, canvas: &Canvas, layout_w: f32, layout_h: f32, time: f64) { + fn paint(&self, canvas: &Canvas, layout_w: f32, layout_h: f32, time: f64, ctx: &PaintCtx) { let color = self.style.background_color_str().unwrap_or("#3B82F6"); - let (h_pad, _v_pad, icon_size) = self.resolved_params(); + let (h_pad, _v_pad, icon_size) = self.resolved_params(ctx); let w = layout_w; let h = layout_h; @@ -224,7 +231,7 @@ impl Badge { let dst = Rect::from_xywh(x_offset, icon_y, icon_size, icon_size); canvas.draw_image_rect(img, None, dst, &Paint::default()); - let ratio = self.resolved_font_size() / self.badge_size.params().0; + let ratio = self.resolved_font_size(ctx) / self.badge_size.params().0; x_offset += icon_size + 6.0 * ratio; } @@ -234,10 +241,10 @@ impl Badge { } else { color }; - let Some(font) = self.make_font() else { + let Some(font) = self.make_font(ctx) else { return; }; - let font_size = self.resolved_font_size(); + let font_size = self.resolved_font_size(ctx); let emoji_font = emoji_typeface().map(|tf| skia_safe::Font::from_typeface(tf, font_size)); let mut text_paint = paint_from_hex(text_color); text_paint.set_anti_alias(true); @@ -348,7 +355,7 @@ impl Painter for Badge { _props: &AnimatedProperties, ctx: &PaintCtx, ) { - self.paint(canvas, layout.width, layout.height, ctx.time); + self.paint(canvas, layout.width, layout.height, ctx.time, ctx); } } @@ -386,4 +393,59 @@ mod tests { let badge = parse(r#"{"type":"badge","text":"v1","style":{"align-self":"center"}}"#); assert_eq!(badge.style.align_self, Some(AlignSelf::Center)); } + + // ─── Lot B, wave S: relative `font-size` units ───────────────────────── + + fn test_ctx() -> PaintCtx { + PaintCtx { + time: 0.0, + scene_duration: 1.0, + frame_index: 0, + fps: 30, + video_width: 400, + video_height: 200, + stagger_offset: 0.0, + } + } + + #[test] + fn rem_font_size_paints_visible_ink() { + // Reproduction: `font-size: "2rem"` used to resolve to 0px via the + // context-free `font_size_px_or`. + let mut badge = parse(r#"{"type":"badge","text":"v1"}"#); + badge.style.font_size = Some(rustmotion_core::css::Length::String("2rem".into())); + const W: i32 = 200; + const H: i32 = 100; + let mut surface = skia_safe::surfaces::raster_n32_premul((W, H)).expect("raster surface"); + { + let canvas = surface.canvas(); + badge.paint(canvas, 150.0, 60.0, 0.0, &test_ctx()); + } + let snapshot = surface.image_snapshot(); + let info = skia_safe::ImageInfo::new( + (W, H), + skia_safe::ColorType::RGBA8888, + skia_safe::AlphaType::Premul, + None, + ); + let mut buf = vec![0u8; (W * H * 4) as usize]; + let ok = snapshot.read_pixels( + &info, + &mut buf, + (W * 4) as usize, + skia_safe::IPoint::new(0, 0), + skia_safe::image::CachingHint::Disallow, + ); + assert!(ok, "pixel read should succeed"); + // Solid variant text is always white — probe for white ink + // specifically, since the pill background paints regardless. + let text_ink = buf + .chunks_exact(4) + .filter(|p| p[3] > 0 && p[0] > 200 && p[1] > 200 && p[2] > 200) + .count(); + assert!( + text_ink > 5, + "badge at font-size: 2rem must paint visible text, got {text_ink} pixels" + ); + } } diff --git a/crates/rustmotion-components/src/callout.rs b/crates/rustmotion-components/src/callout.rs index ab9e880..bcf9d07 100644 --- a/crates/rustmotion-components/src/callout.rs +++ b/crates/rustmotion-components/src/callout.rs @@ -68,8 +68,15 @@ impl Callout { self.style.border_radius_px_or(8.0) } - fn font_size(&self) -> f32 { - self.style.font_size_px_or(16.0) + /// Resolves `font-size` against a real per-frame viewport (`rem`/`vw`/ + /// `vh` now resolve instead of silently dropping to 0px — lot B, wave + /// S). `em`/`%` on `font-size` itself remain approximate — see + /// `crate::intrinsic::font_size_ctx`'s doc comment. + fn font_size(&self, ctx: &PaintCtx) -> f32 { + self.style.font_size_px_ctx( + &crate::intrinsic::font_size_ctx(ctx.video_width as f32, ctx.video_height as f32, 0.0), + 16.0, + ) } fn bubble_rect(&self, w: f32, h: f32) -> Rect { @@ -127,11 +134,11 @@ impl Callout { } impl Callout { - fn paint(&self, canvas: &Canvas, layout_w: f32, layout_h: f32) -> Result<()> { + fn paint(&self, canvas: &Canvas, layout_w: f32, layout_h: f32, ctx: &PaintCtx) -> Result<()> { let w = layout_w; let h = layout_h; let radius = self.radius(); - let font_size = self.font_size(); + let font_size = self.font_size(ctx); // Draw bubble body let bubble = self.bubble_rect(w, h); @@ -188,8 +195,83 @@ impl Painter for Callout { canvas: &Canvas, layout: &BoxLayout, _props: &AnimatedProperties, - _ctx: &PaintCtx, + ctx: &PaintCtx, ) { - let _ = self.paint(canvas, layout.width, layout.height); + let _ = self.paint(canvas, layout.width, layout.height, ctx); + } +} + +#[cfg(test)] +mod tests { + use super::*; + use rustmotion_core::css::CssStyle; + use rustmotion_core::css::Length; + + fn test_ctx() -> PaintCtx { + PaintCtx { + time: 0.0, + scene_duration: 1.0, + frame_index: 0, + fps: 30, + video_width: 400, + video_height: 200, + stagger_offset: 0.0, + } + } + + // ─── Lot B, wave S: relative `font-size` units ───────────────────────── + + #[test] + fn rem_font_size_paints_visible_ink() { + // Reproduction: `font-size: "2rem"` used to resolve to 0px via the + // context-free `font_size_px_or`. + let callout = Callout { + text: "hello".to_string(), + arrow_direction: ArrowDirection::default(), + arrow_size: default_arrow_size(), + timing: Default::default(), + style: CssStyle { + font_size: Some(Length::String("2rem".into())), + ..Default::default() + }, + timeline: Vec::new(), + stagger: None, + }; + const W: i32 = 400; + const H: i32 = 200; + let mut surface = skia_safe::surfaces::raster_n32_premul((W, H)).expect("raster surface"); + { + let canvas = surface.canvas(); + callout + .paint(canvas, W as f32, H as f32, &test_ctx()) + .expect("paint succeeds"); + } + let snapshot = surface.image_snapshot(); + let info = skia_safe::ImageInfo::new( + (W, H), + skia_safe::ColorType::RGBA8888, + skia_safe::AlphaType::Premul, + None, + ); + let mut buf = vec![0u8; (W * H * 4) as usize]; + let ok = snapshot.read_pixels( + &info, + &mut buf, + (W * 4) as usize, + skia_safe::IPoint::new(0, 0), + skia_safe::image::CachingHint::Disallow, + ); + assert!(ok, "pixel read should succeed"); + // Text is white (#FFFFFF default) on a dark #333333 bubble — probe + // for near-white ink specifically, since the bubble background + // paints regardless of font-size. + let text_ink = buf + .chunks_exact(4) + .filter(|p| p[3] > 0 && p[0] > 200 && p[1] > 200 && p[2] > 200) + .count(); + assert!( + text_ink > 10, + "callout at font-size: 2rem must paint visible text, got {text_ink} pixels" + ); } } diff --git a/crates/rustmotion-components/src/caption.rs b/crates/rustmotion-components/src/caption.rs index 056ebe6..4e6cb10 100644 --- a/crates/rustmotion-components/src/caption.rs +++ b/crates/rustmotion-components/src/caption.rs @@ -49,7 +49,19 @@ rustmotion_core::impl_traits!(Caption { impl Caption { fn paint(&self, canvas: &Canvas, layout_width: f32, layout_height: f32, ctx: &PaintCtx) { let time = ctx.time; - let font_size = self.style.font_size_px_or(48.0); + // #9 / lot B (wave S): `font-size` itself now resolves through the + // same context-aware machinery as `letter-spacing`/`line-height` + // below — it used to stay on the context-free `font_size_px_or`, + // silently dropping `rem`/`vw`/`vh` font-size to 0px. `em`/`%` on + // `font-size` itself remain approximate (see + // `crate::intrinsic::font_size_ctx`'s doc comment) — cascade.rs + // doesn't track the real parent font-size. + let base_ctx = crate::intrinsic::font_size_ctx( + ctx.video_width as f32, + ctx.video_height as f32, + layout_width.max(0.0), + ); + let font_size = self.style.font_size_px_ctx(&base_ctx, 48.0); let color = self.style.color_str_or("#FFFFFF"); let font_family = self.style.font_family_or("Inter"); @@ -58,11 +70,8 @@ impl Caption { // the real viewport, available here via `ctx` (mirrors // `text.rs::paint`'s `type_ctx`). let type_ctx = LengthContext { - viewport_width: ctx.video_width as f32, - viewport_height: ctx.video_height as f32, - parent_size: layout_width.max(0.0), font_size, - root_font_size: 16.0, + ..base_ctx }; // #9: derive weight/slant from `style.font-weight`/`font-style` @@ -714,6 +723,29 @@ mod tests { ); } + // ─── Lot B, wave S: relative `font-size` units ───────────────────────── + + #[test] + fn rem_font_size_paints_visible_ink() { + // Reproduction: `font-size: "2rem"` used to resolve to 0px on the + // context-free `font_size_px_or` path — `CaptionIntrinsic` (via + // `TextIntrinsic`) measured a 0-height box and nothing painted. + let mut caption = make_caption_with_max_width("hello world", None, Some(300.0)); + caption.style.font_size = Some(Length::String("2rem".into())); + const W: i32 = 400; + const H: i32 = 200; + let mut surface = skia_safe::surfaces::raster_n32_premul((W, H)).expect("raster surface"); + { + let canvas = surface.canvas(); + caption.paint(canvas, 300.0, H as f32, &test_ctx(0.5)); + } + let bounds = ink_bounds(&mut surface, W, H); + assert!( + bounds.is_some(), + "caption at font-size: 2rem must paint visible ink" + ); + } + // `Caption::resolve_font_style` is the exact weight/slant computation // `paint` uses; testing it directly is deterministic regardless of // whether the system's resolved "bold" and "normal" typefaces happen to diff --git a/crates/rustmotion-components/src/codeblock/dimensions.rs b/crates/rustmotion-components/src/codeblock/dimensions.rs index 3175a85..4b02e8d 100644 --- a/crates/rustmotion-components/src/codeblock/dimensions.rs +++ b/crates/rustmotion-components/src/codeblock/dimensions.rs @@ -15,11 +15,18 @@ pub(crate) struct CodeDimensions { pub(crate) fn compute_code_dimensions( code: &str, font: &Font, + font_size: f32, padding: (f32, f32, f32, f32), chrome_height: f32, layer: &Codeblock, ) -> CodeDimensions { - let font_size = layer.style.font_size_px_or(14.0); + // `font_size` is now a caller-supplied parameter instead of being + // re-derived here from `layer.style` — the caller (`render.rs`, + // `intrinsic.rs`) already resolved it once (with the real `LengthContext` + // where one is available) to build `font`; re-deriving it a second time + // with the context-free accessor was exactly the kind of duplicate + // computation that let this and the caller's value silently diverge for + // relative units (lot B, wave S). let actual_line_height = layer.style.line_height_for(font_size); let lines: Vec<&str> = code.lines().collect(); let line_count = lines.len().max(1); diff --git a/crates/rustmotion-components/src/codeblock/render.rs b/crates/rustmotion-components/src/codeblock/render.rs index 1c98c46..ed7da56 100644 --- a/crates/rustmotion-components/src/codeblock/render.rs +++ b/crates/rustmotion-components/src/codeblock/render.rs @@ -23,7 +23,15 @@ pub(super) fn render_codeblock( ) { let time = ctx.time; let font_family = layer.style.font_family_or("JetBrains Mono"); - let font_size = layer.style.font_size_px_or(14.0); + // Resolved once, against the real per-frame viewport (`rem`/`vw`/`vh` on + // `font-size` now resolve instead of silently dropping to 0px — lot B, + // wave S) and threaded through every `compute_code_dimensions` call + // below instead of each one re-deriving its own (previously identical + // only by coincidence) value. + let font_size = layer.style.font_size_px_ctx( + &crate::intrinsic::font_size_ctx(ctx.video_width as f32, ctx.video_height as f32, 0.0), + 14.0, + ); let font_weight = match &layer.style.font_weight { Some(CssFontWeight::Keyword(FontWeightKw::Bold | FontWeightKw::Bolder)) => FontWeight::Bold, Some(CssFontWeight::Number(n)) if *n >= 600 => FontWeight::Bold, @@ -68,25 +76,69 @@ pub(super) fn render_codeblock( // still computed (so we know whether to auto-scroll), but the box footprint // is taken from the laid-out BoxLayout, not the legacy `layer.size`. let natural_height = if let Some(ref trans) = transition { - let dims_a = compute_code_dimensions(&trans.code_a, &font, padding, chrome_height, layer); - let dims_b = compute_code_dimensions(&trans.code_b, &font, padding, chrome_height, layer); + let dims_a = compute_code_dimensions( + &trans.code_a, + &font, + font_size, + padding, + chrome_height, + layer, + ); + let dims_b = compute_code_dimensions( + &trans.code_b, + &font, + font_size, + padding, + chrome_height, + layer, + ); lerp( dims_a.total_height, dims_b.total_height, trans.progress as f32, ) } else { - compute_code_dimensions(¤t_code, &font, padding, chrome_height, layer).total_height + compute_code_dimensions( + ¤t_code, + &font, + font_size, + padding, + chrome_height, + layer, + ) + .total_height }; let gutter_width = if max_gutter_width > 0.0 { max_gutter_width } else if let Some(ref trans) = transition { - let dims_a = compute_code_dimensions(&trans.code_a, &font, padding, chrome_height, layer); - let dims_b = compute_code_dimensions(&trans.code_b, &font, padding, chrome_height, layer); + let dims_a = compute_code_dimensions( + &trans.code_a, + &font, + font_size, + padding, + chrome_height, + layer, + ); + let dims_b = compute_code_dimensions( + &trans.code_b, + &font, + font_size, + padding, + chrome_height, + layer, + ); f32::max(dims_a.gutter_width, dims_b.gutter_width) } else { - compute_code_dimensions(¤t_code, &font, padding, chrome_height, layer).gutter_width + compute_code_dimensions( + ¤t_code, + &font, + font_size, + padding, + chrome_height, + layer, + ) + .gutter_width }; let total_width = layout.width.round(); diff --git a/crates/rustmotion-components/src/counter.rs b/crates/rustmotion-components/src/counter.rs index d9511e4..dd8935b 100644 --- a/crates/rustmotion-components/src/counter.rs +++ b/crates/rustmotion-components/src/counter.rs @@ -90,7 +90,18 @@ impl Counter { ) -> Result<()> { use rustmotion_core::engine::animator::ease; - let font_size = self.style.font_size_px_or(48.0); + // Lot B (wave S): `font-size` now resolves through the same + // context-aware machinery `text-shadow` already used below (see + // `lctx`) — it used to stay on the context-free `font_size_px_or`, + // silently dropping `rem`/`vw`/`vh` font-size to 0px. `em`/`%` on + // `font-size` itself remain approximate — see + // `crate::intrinsic::font_size_ctx`'s doc comment. + let base_ctx = crate::intrinsic::font_size_ctx( + ctx.video_width as f32, + ctx.video_height as f32, + layout_width.max(0.0), + ); + let font_size = self.style.font_size_px_ctx(&base_ctx, 48.0); // Animated color (timeline style-state transitions) overrides the // static style color. let color = props @@ -146,7 +157,14 @@ impl Counter { let mut paint = paint_from_hex(color); paint.set_alpha_f(1.0); - let letter_spacing = self.style.letter_spacing_px(); + // This element's own resolved font-size as the `em`/`%` base for + // `letter-spacing` and (below) `text-shadow` — same context reused + // for both instead of each rebuilding an equivalent one. + let own_ctx = rustmotion_core::css::units::LengthContext { + font_size, + ..base_ctx + }; + let letter_spacing = self.style.letter_spacing_px_ctx(&own_ctx); let advance_width = measure_text_with_fallback(&content, &font, &emoji_font, letter_spacing); @@ -195,14 +213,7 @@ impl Counter { let shadows: Vec = if let Some(s) = &self.text_shadow { vec![s.clone()] } else if let Some(list) = &self.style.text_shadow { - let lctx = rustmotion_core::css::units::LengthContext { - viewport_width: ctx.video_width as f32, - viewport_height: ctx.video_height as f32, - parent_size: layout_width.max(0.0), - font_size, - root_font_size: 16.0, - }; - list.iter().map(|s| s.to_schema(&lctx)).collect() + list.iter().map(|s| s.to_schema(&own_ctx)).collect() } else { Vec::new() }; @@ -346,4 +357,55 @@ mod tests { let c = counter(Some(0.0), None); assert!((c.ramp_progress(2.0, 4.0) - 0.5).abs() < 1e-9); } + + // ─── Lot B, wave S: relative `font-size` units ───────────────────────── + + #[test] + fn rem_font_size_paints_visible_ink() { + // Reproduction: `font-size: "2rem"` used to resolve to 0px on the + // context-free `font_size_px_or` path. + let mut c = counter(None, None); + c.style.font_size = Some(rustmotion_core::css::Length::String("2rem".into())); + c.style.color = Some(rustmotion_core::css::style::Color::String("#FFFFFF".into())); + + const W: i32 = 300; + const H: i32 = 150; + let mut surface = skia_safe::surfaces::raster_n32_premul((W, H)).expect("raster surface"); + let ctx = PaintCtx { + time: 1.0, + scene_duration: 2.0, + frame_index: 30, + fps: 30, + video_width: 300, + video_height: 150, + stagger_offset: 0.0, + }; + let props = AnimatedProperties::default(); + { + let canvas = surface.canvas(); + c.paint(canvas, 200.0, 1.0, 2.0, &props, &ctx) + .expect("paint succeeds"); + } + let snapshot = surface.image_snapshot(); + let info = skia_safe::ImageInfo::new( + (W, H), + skia_safe::ColorType::RGBA8888, + skia_safe::AlphaType::Premul, + None, + ); + let mut buf = vec![0u8; (W * H * 4) as usize]; + let ok = snapshot.read_pixels( + &info, + &mut buf, + (W * 4) as usize, + skia_safe::IPoint::new(0, 0), + skia_safe::image::CachingHint::Disallow, + ); + assert!(ok, "pixel read should succeed"); + let lit = buf.chunks_exact(4).filter(|p| p[3] > 0).count(); + assert!( + lit > 20, + "counter at font-size: 2rem must paint visible ink, got {lit} lit pixels" + ); + } } diff --git a/crates/rustmotion-components/src/gradient_text.rs b/crates/rustmotion-components/src/gradient_text.rs index 086ec30..cfb68c0 100644 --- a/crates/rustmotion-components/src/gradient_text.rs +++ b/crates/rustmotion-components/src/gradient_text.rs @@ -56,8 +56,13 @@ rustmotion_core::impl_traits!(GradientText { }); impl GradientText { - fn resolve_font(&self) -> Option<(Font, Option)> { - let font_size = self.style.font_size_px_or(48.0); + /// `font_size` is resolved once by the caller (`paint`, against a real + /// `LengthContext`) and passed in here — this used to independently + /// recompute it via the context-free `font_size_px_or`, a second site + /// that could silently diverge from `paint`'s own resolution once one of + /// the two learned to handle relative units and the other didn't (lot B, + /// wave S). + fn resolve_font(&self, font_size: f32) -> Option<(Font, Option)> { let font_family = self.style.font_family_or("Inter"); let slant = match self.style.font_style { @@ -88,14 +93,21 @@ impl GradientText { return; } - let Some((font, emoji_font)) = self.resolve_font() else { + // `font-size` itself now resolves through `LengthContext` too (lot B, + // wave S) — it used to stay context-free (`font_size_px_or`), + // silently dropping `rem`/`vw`/`vh` font-size to 0px. `em`/`%` on + // `font-size` itself remain approximate — see + // `crate::intrinsic::font_size_ctx`'s doc comment (cascade.rs + // doesn't track the real parent font-size). + let base_ctx = crate::intrinsic::font_size_ctx( + ctx.video_width as f32, + ctx.video_height as f32, + layout_width.max(0.0), + ); + let font_size = self.style.font_size_px_ctx(&base_ctx, 48.0); + let Some((font, emoji_font)) = self.resolve_font(font_size) else { return; }; - // `font-size` stays context-free — see the identical note in - // `text.rs`'s `paint`: resolving its own `em`/`%` correctly needs a - // cascade.rs change (parent font-size as a resolved px value, not a - // raw `Length`), which is out of scope here (issue #125 §2). - let font_size = self.style.font_size_px_or(48.0); // `letter-spacing`/`line-height`'s `em`/`%` are relative to this // element's own font-size, no cascade dependency — a real // `LengthContext` is available here, so use the context-aware @@ -107,11 +119,8 @@ impl GradientText { // #125 §1 describes elsewhere. It's threaded through consistently // now. let type_ctx = rustmotion_core::css::units::LengthContext { - viewport_width: ctx.video_width as f32, - viewport_height: ctx.video_height as f32, - parent_size: layout_width.max(0.0), font_size, - root_font_size: 16.0, + ..base_ctx }; let line_height_val = self.style.line_height_for_ctx(font_size, &type_ctx); let letter_spacing = self.style.letter_spacing_px_ctx(&type_ctx); @@ -349,4 +358,36 @@ mod tests { "wrapped gradient_text must spill onto a second line within the box width" ); } + + #[test] + fn rem_font_size_paints_visible_ink() { + // Reproduction: `font-size: "2rem"` used to resolve to 0px via the + // context-free `font_size_px_or`, so `resolve_font` built a 0px font + // and nothing measurable painted. + let gt = GradientText { + content: "HELLO".into(), + colors: default_colors(), + angle: default_angle(), + animate_angle: false, + speed: default_speed(), + timing: Default::default(), + style: CssStyle { + font_size: Some(Length::String("2rem".into())), + ..Default::default() + }, + timeline: Vec::new(), + stagger: None, + }; + const W: i32 = 400; + const H: i32 = 200; + let mut surface = skia_safe::surfaces::raster_n32_premul((W, H)).expect("raster surface"); + let canvas = surface.canvas(); + gt.paint(canvas, 300.0, 0.0, &test_ctx()); + let grid = alpha_grid(&mut surface, W, H); + + assert!( + has_ink_in(&grid, W, 0, W, 0, 60), + "gradient_text at font-size: 2rem must paint visible ink" + ); + } } diff --git a/crates/rustmotion-components/src/icon.rs b/crates/rustmotion-components/src/icon.rs index 3110eb6..473d97f 100644 --- a/crates/rustmotion-components/src/icon.rs +++ b/crates/rustmotion-components/src/icon.rs @@ -5,7 +5,7 @@ use skia_safe::{Canvas, ColorType, ImageInfo, Paint, Rect, SamplingOptions}; use rustmotion_core::css::CssStyle; use rustmotion_core::engine::animator::AnimatedProperties; use rustmotion_core::engine::layout_pass::BoxLayout; -use rustmotion_core::engine::renderer::{asset_cache, fetch_icon_svg}; +use rustmotion_core::engine::renderer::{asset_cache, fetch_icon_svg, icon_cache_key}; use rustmotion_core::schema::TimelineStep; use rustmotion_core::traits::{PaintCtx, Painter, TimingConfig}; @@ -38,22 +38,36 @@ impl Painter for Icon { _ctx: &PaintCtx, ) { let color = self.style.color_str_or("#FFFFFF"); - // Oversample 2× so the rasterized SVG stays crisp under sub-pixel - // positioning and minor scale animations. Skia's high-quality - // sampling downscales to layout size without softening edges. - const OVERSAMPLE: u32 = 2; let target_w = (layout.width as u32).max(1); let target_h = (layout.height as u32).max(1); - let render_w = target_w * OVERSAMPLE; - let render_h = target_h * OVERSAMPLE; - - let cache_key = format!("icon:{}:{}:{}x{}", self.icon, color, render_w, render_h); + // Oversampling (crisp edges under sub-pixel positioning / scale + // animation) and the cache key are computed together by + // `icon_cache_key` — see its doc for why that matters (issue #166): + // this used to be a local `const OVERSAMPLE` here plus an + // independent `format!` in `preload.rs`'s prefetcher, and the two + // could never agree. + let (render_w, render_h, cache_key) = icon_cache_key(&self.icon, color, target_w, target_h); let cache = asset_cache(); let img = if let Some(cached) = cache.get(&cache_key) { cached.clone() } else { let Ok(svg_data) = fetch_icon_svg(&self.icon, color, render_w, render_h) else { + // Preload (issue #167 item 2) already hard-fails when an + // icon cannot be resolved via disk cache or network, so this + // branch is defense in depth (paint_content can run without + // a preceding prefetch, or the layout-derived target size + // here can differ from the preloader's style-based + // estimate, producing a genuine cache miss). Guarded so a + // single offline/typo'd icon does not spam once per frame + // over a render that can be 1000+ frames long. + if crate::warn_once_for(&format!("icon-fetch-failed:{}", self.icon)) { + eprintln!( + "Warning: icon '{}' could not be loaded (checked the disk cache and \ + the network) — nothing will be painted for it.", + self.icon + ); + } return; }; @@ -100,3 +114,100 @@ impl Painter for Icon { ); } } + +#[cfg(test)] +mod tests { + use super::*; + + fn base_ctx() -> PaintCtx { + PaintCtx { + time: 0.0, + scene_duration: 1.0, + frame_index: 0, + fps: 30, + video_width: 100, + video_height: 100, + stagger_offset: 0.0, + } + } + + fn solid_image() -> skia_safe::Image { + let px = [255u8, 0, 255, 255]; + let mut data = Vec::with_capacity(4 * 4); + for _ in 0..4 { + data.extend_from_slice(&px); + } + let img_info = ImageInfo::new( + (2, 2), + ColorType::RGBA8888, + skia_safe::AlphaType::Premul, + None, + ); + let skia_data = skia_safe::Data::new_copy(&data); + skia_safe::images::raster_from_data(&img_info, skia_data, 2 * 4).expect("sentinel image") + } + + /// Regression for issue #166: proves the painter's cache-key formula is + /// literally `icon_cache_key`. Pre-populate `asset_cache()` under the + /// exact key `preload.rs`'s prefetcher now computes for a 40×40 target, + /// then confirm the painter finds and paints it — instead of falling + /// through to `fetch_icon_svg` for a nonsense icon id (which pre-fix, on + /// a key mismatch, is exactly what would have happened every time). + #[test] + fn painter_finds_the_entry_preload_would_have_written() { + let icon = Icon { + icon: "test-suite:icon-cache-key-agreement".to_string(), + timing: Default::default(), + style: CssStyle::default(), + timeline: Vec::new(), + stagger: None, + }; + let target_w = 40u32; + let target_h = 40u32; + let color = icon.style.color_str_or("#FFFFFF"); + let (_, _, key) = icon_cache_key(&icon.icon, color, target_w, target_h); + + asset_cache().insert(key.clone(), solid_image()); + + let layout = BoxLayout { + width: target_w as f32, + height: target_h as f32, + ..Default::default() + }; + let ctx = base_ctx(); + let props = AnimatedProperties::default(); + let mut surface = + skia_safe::surfaces::raster_n32_premul((target_w as i32, target_h as i32)).unwrap(); + { + let canvas = surface.canvas(); + icon.paint_content(canvas, &layout, &props, &ctx); + } + + // Cleanup so this entry does not leak into other tests sharing the + // process-global asset_cache. + asset_cache().remove(&key); + + let snapshot = surface.image_snapshot(); + let info = ImageInfo::new( + (target_w as i32, target_h as i32), + ColorType::RGBA8888, + skia_safe::AlphaType::Premul, + None, + ); + let mut buf = vec![0u8; (target_w * target_h * 4) as usize]; + let ok = snapshot.read_pixels( + &info, + &mut buf, + (target_w * 4) as usize, + skia_safe::IPoint::new(0, 0), + skia_safe::image::CachingHint::Disallow, + ); + assert!(ok, "pixel read should succeed"); + let has_ink = buf.chunks(4).any(|px| px[3] > 0); + assert!( + has_ink, + "painter must have found and painted the cache entry preload.rs would have \ + written under the same key — if the keys disagree, nothing paints" + ); + } +} diff --git a/crates/rustmotion-components/src/intrinsic.rs b/crates/rustmotion-components/src/intrinsic.rs index 5eb0215..ac5e7b9 100644 --- a/crates/rustmotion-components/src/intrinsic.rs +++ b/crates/rustmotion-components/src/intrinsic.rs @@ -24,6 +24,70 @@ use crate::gradient_text::GradientText; use crate::kbd::Kbd; use crate::text::Text; +// ─── Shared `font-size` context resolution (deployment of `font_size_px_ctx` +// / `typography_px_ctx`, css/style.rs, across every component that still +// resolved `font-size` with the context-free `font_size_px_or`) ─────────── +// +// `font_size_px_or`/`.px()` cannot resolve `%`/`em`/`rem`/`vw`/`vh` — for a +// `Some(Length::String(_))` that parses as one of those units, `.px()` warns +// and returns `0.0`, and since the field itself is `Some`, the `_or` +// fallback default never kicks in either. A `text` with `"font-size": +// "2rem"` therefore measured *and* painted at 0px: `validate` passed (only +// warnings), but the rendered frame had no visible text (paint_pass's +// `height <= 0.0` guard skips the node once the intrinsic measures it at +// zero). +// +// `rustmotion_core::css::style::CssStyle::font_size_px_ctx` (and +// `typography_px_ctx`, which resolves `font-size`, `letter-spacing`, and +// `line-height` together, honouring CSS's two different `em` bases) already +// exist and are tested — nothing in the engine called them. These two +// helpers build the `LengthContext` every call site below feeds them, +// so the context-building logic lives in exactly one place instead of being +// copied into ~15 components. +use rustmotion_core::css::units::LengthContext; + +/// `LengthContext` for resolving `font-size` (and, through +/// [`CssStyle::typography_px_ctx`], `letter-spacing`/`line-height` derived +/// from it) against a real, per-frame viewport. Use from `Painter:: +/// paint_content` and friends, which have a real `PaintCtx` (`video_width`/ +/// `video_height`) on hand. +/// +/// `rem`/`vw`/`vh` resolve correctly through this. `em`/`%` on `font-size` +/// itself do not: per CSS they're relative to the *parent's* computed +/// font-size, but `cascade.rs` inherits `font-size` down the tree as a raw, +/// unresolved `Length`, not a resolved px value (see the module note above +/// `CssStyle::font_size_px_ctx`) — no caller in this workstream's scope can +/// supply the real cascaded value. `font_size: 16.0` here is the CSS root +/// default used as the best available stand-in; it makes `em`/`%` on +/// `font-size` *resolve* (no longer silently drop to 0px) without making +/// them *correct* against an actual parent font-size. Fixing that fully +/// needs a `cascade.rs` change, out of scope here. +pub fn font_size_ctx(viewport_width: f32, viewport_height: f32, parent_size: f32) -> LengthContext { + LengthContext { + viewport_width, + viewport_height, + parent_size, + font_size: 16.0, + root_font_size: 16.0, + } +} + +/// Same as [`font_size_ctx`], for the `Intrinsic` measurers in this module: +/// they run at `box_builder`/`geometry` construction time, before layout, so +/// there is no real per-frame viewport to hand (see the pre-existing note on +/// `TextIntrinsic::from_parts`, which has the same limitation for +/// `letter-spacing`/`line-height`). Falls back to the engine-wide default +/// 1920×1080 (same as `LengthContext::default()`) so `rem` — which does not +/// depend on the viewport at all — still resolves exactly, and `vw`/`vh` get +/// a reasonable non-zero approximation instead of silently dropping to 0. +/// This can diverge from what `Painter::paint_content` resolves via +/// [`font_size_ctx`] for `vw`/`vh` specifically, on videos that aren't +/// 1920×1080 — closing that fully needs the real `VideoConfig` threaded +/// through `box_builder.rs`/`geometry.rs`, both outside this workstream. +pub fn measure_time_font_size_ctx(parent_size: f32) -> LengthContext { + font_size_ctx(1920.0, 1080.0, parent_size) +} + /// Skia-backed intrinsic measurer for [`Text`] (audit #10: despite the name /// this module's doc header suggests, this uses `skia_safe::Font:: /// measure_str` via `engine::renderer::text`'s fallback-aware helpers — the @@ -71,30 +135,24 @@ impl TextIntrinsic { // wave; the geometry validator re-measures via this exact type and // must keep agreeing with it byte-for-byte, so changing what it // needs to pass in is not a call to make unilaterally here). + // `measure_time_font_size_ctx` falls back to the engine-wide default + // viewport (1920×1080) for this reason — see its doc comment. // - // But `letter-spacing`'s and `line-height`'s `em`/`%` resolve - // against this element's *own* font-size (not the parent's, not the - // viewport) — CSS spec, also documented on - // `CssStyle::letter_spacing_px_ctx`/`line_height_for_ctx` — and that - // own font-size is already known right here, with zero signature - // change needed. Building a `LengthContext` carrying just that - // resolved `font_size` (defaults for everything else) and using the - // `_ctx` resolvers closes the measure-vs-paint divergence for `em`/ - // `%` specifically (`Text`/`Caption`'s painters already resolve - // these two properties with the real `PaintCtx`'s viewport, but - // `em`/`%` on them don't read the viewport at all, so the two agree - // regardless of what viewport this default carries). `vw`/`vh`/ - // `rem` on `letter-spacing`/`line-height` remain unresolved against - // the *real* viewport here (they fall back to this struct's default - // 1920×1080/16px root) — closing that fully needs the real - // `VideoConfig` plumbed through `box_builder.rs`/`geometry.rs`, - // still out of scope for the reasons above. - let font_size = style.font_size_px_or(48.0); - let own_ctx = rustmotion_core::css::units::LengthContext { - font_size, - ..rustmotion_core::css::units::LengthContext::default() - }; - let line_height_resolved = style.line_height_for_ctx(font_size, &own_ctx); + // `font-size` itself, and `letter-spacing`/`line-height`'s `em`/`%` + // (relative to this element's *own*, just-resolved font-size — CSS + // spec, also documented on `CssStyle::letter_spacing_px_ctx`/ + // `line_height_for_ctx`) are resolved together by + // `typography_px_ctx`, which re-derives the right context between + // the two steps. `Text`/`Caption`'s painters resolve the same three + // properties with the real `PaintCtx`'s viewport (lot B, wave S), so + // `rem` (viewport-independent) always agrees between measure and + // paint; `vw`/`vh` can diverge on videos that aren't 1920×1080 — + // closing that fully needs the real `VideoConfig` plumbed through + // `box_builder.rs`/`geometry.rs`, still out of scope for the reasons + // above. + let base_ctx = measure_time_font_size_ctx(0.0); + let (font_size, letter_spacing, line_height_resolved) = + style.typography_px_ctx(&base_ctx, 48.0); Self { content: content.to_string(), font_family: style.font_family.clone(), @@ -102,7 +160,7 @@ impl TextIntrinsic { line_height_resolved, weight: weight_to_u16(style.font_weight.as_ref()), italic: matches!(style.font_style, Some(CssFontStyle::Italic)), - letter_spacing: style.letter_spacing_px_ctx(&own_ctx), + letter_spacing, max_width, wrap: true, } @@ -287,7 +345,9 @@ pub struct KbdIntrinsic { impl KbdIntrinsic { pub fn from_kbd(k: &Kbd) -> Self { - let fs = k.style.font_size_px_or(k.font_size); + let fs = k + .style + .font_size_px_ctx(&measure_time_font_size_ctx(0.0), k.font_size); let synthetic_style = synthesize_text_style(&k.style, fs, "SF Mono"); Self { text: TextIntrinsic::from_parts_with_wrap(&k.key, &synthetic_style, None, false), @@ -354,7 +414,9 @@ pub struct BadgeIntrinsic { impl BadgeIntrinsic { pub fn from_badge(b: &Badge) -> Self { let (default_fs, h_pad, v_pad, icon_size) = badge_size_params(&b.badge_size); - let font_size = b.style.font_size_px_or(default_fs); + let font_size = b + .style + .font_size_px_ctx(&measure_time_font_size_ctx(0.0), default_fs); let ratio = font_size / default_fs; let h_padding = h_pad * ratio; let v_padding = v_pad * ratio; @@ -451,7 +513,9 @@ pub struct TerminalIntrinsic { impl TerminalIntrinsic { pub fn from_terminal(t: &Terminal) -> Self { - let font_size = t.style.font_size_px_or(TERM_FONT_SIZE); + let font_size = t + .style + .font_size_px_ctx(&measure_time_font_size_ctx(0.0), TERM_FONT_SIZE); let line_height = (font_size * TERM_LINE_HEIGHT / TERM_FONT_SIZE).ceil(); let chrome_height = if t.show_chrome { CHROME_HEIGHT } else { 0.0 }; @@ -530,7 +594,9 @@ pub struct TableIntrinsic { impl TableIntrinsic { pub fn from_table(t: &Table) -> Self { - let font_size = t.style.font_size_px_or(TABLE_FONT_SIZE); + let font_size = t + .style + .font_size_px_ctx(&measure_time_font_size_ctx(0.0), TABLE_FONT_SIZE); let row_height = font_size * DEFAULT_ROW_HEIGHT_RATIO; let total_width = Self::compute_width(t, font_size); @@ -625,7 +691,9 @@ pub struct CodeblockIntrinsic { impl CodeblockIntrinsic { pub fn from_codeblock(c: &Codeblock) -> Self { let font_family = c.style.font_family_or("JetBrains Mono"); - let font_size = c.style.font_size_px_or(14.0); + let font_size = c + .style + .font_size_px_ctx(&measure_time_font_size_ctx(0.0), 14.0); let font_weight = match &c.style.font_weight { Some(CssFontWeight2::Keyword(CssFontWeightKw2::Bold | CssFontWeightKw2::Bolder)) => { FontWeight::Bold @@ -657,7 +725,7 @@ impl CodeblockIntrinsic { 0.0 }; - let dims = compute_code_dimensions(&c.code, &font, padding, chrome_height, c); + let dims = compute_code_dimensions(&c.code, &font, font_size, padding, chrome_height, c); Self { natural_width: dims.total_width, @@ -733,7 +801,8 @@ impl IntrinsicMeasure for RichTextIntrinsic { } }; - let layout = RichText::compute_layout(&self.spans, &self.style, max_width, -1.0); + let layout = + RichText::compute_layout(&self.spans, &self.style, 1920.0, 1080.0, max_width, -1.0); let line_count = layout.lines.len().max(1) as f32; (layout.max_width, line_count * layout.line_height) } @@ -1010,7 +1079,7 @@ mod tests { (None, None), (AvailableSpace::MaxContent, AvailableSpace::MaxContent), ); - let layout = RichText::compute_layout(&spans, &style, None, -1.0); + let layout = RichText::compute_layout(&spans, &style, 1920.0, 1080.0, None, -1.0); assert_eq!(w, layout.max_width); assert_eq!(h, layout.lines.len().max(1) as f32 * layout.line_height); } diff --git a/crates/rustmotion-components/src/kbd.rs b/crates/rustmotion-components/src/kbd.rs index 28d0cb7..c383e45 100644 --- a/crates/rustmotion-components/src/kbd.rs +++ b/crates/rustmotion-components/src/kbd.rs @@ -87,8 +87,19 @@ rustmotion_core::impl_traits!(Kbd { }); impl Kbd { - fn make_font(&self) -> Option { - let fs = self.style.font_size_px_or(self.font_size); + /// Resolves `font-size` against a real per-frame viewport (`rem`/`vw`/ + /// `vh` now resolve instead of silently dropping to 0px — lot B, wave + /// S). `em`/`%` on `font-size` itself remain approximate — see + /// `crate::intrinsic::font_size_ctx`'s doc comment. + fn resolved_font_size(&self, ctx: &PaintCtx) -> f32 { + self.style.font_size_px_ctx( + &crate::intrinsic::font_size_ctx(ctx.video_width as f32, ctx.video_height as f32, 0.0), + self.font_size, + ) + } + + fn make_font(&self, ctx: &PaintCtx) -> Option { + let fs = self.resolved_font_size(ctx); let font_style = skia_safe::FontStyle::normal(); let family = self.style.font_family.as_deref().unwrap_or("SF Mono"); let typeface = typeface_with_fallback(family, font_style).ok()?; @@ -97,7 +108,7 @@ impl Kbd { } impl Kbd { - fn paint(&self, canvas: &Canvas, layout_w: f32, layout_h: f32) { + fn paint(&self, canvas: &Canvas, layout_w: f32, layout_h: f32, ctx: &PaintCtx) { let w = layout_w; let h = layout_h; let radius = 6.0; @@ -138,10 +149,10 @@ impl Kbd { canvas.draw_rrect(face_rrect, &border_paint); // Text centered - let Some(font) = self.make_font() else { + let Some(font) = self.make_font(ctx) else { return; }; - let fs = self.style.font_size_px_or(self.font_size); + let fs = self.resolved_font_size(ctx); let emoji_font = emoji_typeface().map(|tf| skia_safe::Font::from_typeface(tf, fs)); let text_color = self.style.color_str().unwrap_or(&self.text_color); @@ -172,9 +183,9 @@ impl Painter for Kbd { canvas: &Canvas, layout: &BoxLayout, _props: &AnimatedProperties, - _ctx: &PaintCtx, + ctx: &PaintCtx, ) { - self.paint(canvas, layout.width, layout.height); + self.paint(canvas, layout.width, layout.height, ctx); } } @@ -186,6 +197,18 @@ mod tests { serde_json::from_str(json).expect("kbd should deserialize") } + fn test_ctx() -> PaintCtx { + PaintCtx { + time: 0.0, + scene_duration: 1.0, + frame_index: 0, + fps: 30, + video_width: 400, + video_height: 200, + stagger_offset: 0.0, + } + } + #[test] fn style_defaults_to_flex_start_when_absent() { // #127: same fix as `badge` — a keycap must keep its intrinsic @@ -236,7 +259,7 @@ mod tests { { let canvas = surface.canvas(); canvas.translate((20.0, 5.0)); - kbd.paint(canvas, box_w, box_h); + kbd.paint(canvas, box_w, box_h, &test_ctx()); } let snapshot = surface.image_snapshot(); let info = skia_safe::ImageInfo::new( @@ -267,4 +290,60 @@ mod tests { } } } + + // ─── Lot B, wave S: relative `font-size` units ───────────────────────── + + #[test] + fn rem_font_size_paints_visible_ink() { + // Reproduction: `font-size: "2rem"` used to resolve to 0px via the + // context-free `font_size_px_or`. + let kbd = Kbd { + key: "K".to_string(), + font_size: default_font_size(), + background_color: default_bg_color(), + border_color: default_border_color(), + text_color: default_text_color(), + timing: Default::default(), + style: CssStyle { + font_size: Some(rustmotion_core::css::Length::String("2rem".into())), + ..Default::default() + }, + timeline: Vec::new(), + stagger: None, + }; + const W: i32 = 200; + const H: i32 = 100; + let mut surface = skia_safe::surfaces::raster_n32_premul((W, H)).expect("raster surface"); + { + let canvas = surface.canvas(); + kbd.paint(canvas, 100.0, 60.0, &test_ctx()); + } + let snapshot = surface.image_snapshot(); + let info = skia_safe::ImageInfo::new( + (W, H), + skia_safe::ColorType::RGBA8888, + skia_safe::AlphaType::Premul, + None, + ); + let mut buf = vec![0u8; (W * H * 4) as usize]; + let ok = snapshot.read_pixels( + &info, + &mut buf, + (W * 4) as usize, + skia_safe::IPoint::new(0, 0), + skia_safe::image::CachingHint::Disallow, + ); + assert!(ok, "pixel read should succeed"); + // Text is near-white (#E2E8F0 default `text_color`) on a dark key + // face — probe for near-white ink specifically, since the face/ + // border/shadow paint regardless of font-size. + let text_ink = buf + .chunks_exact(4) + .filter(|p| p[3] > 0 && p[0] > 180 && p[1] > 180 && p[2] > 180) + .count(); + assert!( + text_ink > 5, + "kbd at font-size: 2rem must paint visible text, got {text_ink} pixels" + ); + } } diff --git a/crates/rustmotion-components/src/list.rs b/crates/rustmotion-components/src/list.rs index 12ab7f7..80e677a 100644 --- a/crates/rustmotion-components/src/list.rs +++ b/crates/rustmotion-components/src/list.rs @@ -80,17 +80,24 @@ rustmotion_core::impl_traits!(List { }); impl List { - fn resolved_font_size(&self) -> f32 { - self.style.font_size_px_or(16.0) + /// Resolves `font-size` against a real per-frame viewport (`rem`/`vw`/ + /// `vh` now resolve instead of silently dropping to 0px — lot B, wave + /// S). `em`/`%` on `font-size` itself remain approximate — see + /// `crate::intrinsic::font_size_ctx`'s doc comment. + fn resolved_font_size(&self, ctx: &PaintCtx) -> f32 { + self.style.font_size_px_ctx( + &crate::intrinsic::font_size_ctx(ctx.video_width as f32, ctx.video_height as f32, 0.0), + 16.0, + ) } - fn make_font(&self) -> Option { + fn make_font(&self, ctx: &PaintCtx) -> Option { let font_style = skia_safe::FontStyle::normal(); let family = self.style.font_family.as_deref().unwrap_or("Inter"); let typeface = typeface_with_fallback(family, font_style).ok()?; Some(skia_safe::Font::from_typeface( typeface, - self.resolved_font_size(), + self.resolved_font_size(ctx), )) } @@ -153,11 +160,11 @@ impl List { } impl List { - fn paint(&self, canvas: &Canvas) -> Result<()> { - let Some(font) = self.make_font() else { + fn paint(&self, canvas: &Canvas, ctx: &PaintCtx) -> Result<()> { + let Some(font) = self.make_font(ctx) else { return Ok(()); }; - let font_size = self.resolved_font_size(); + let font_size = self.resolved_font_size(ctx); let emoji_font = emoji_typeface().map(|tf| skia_safe::Font::from_typeface(tf, font_size)); let text_color = self.style.color_str_or("#FFFFFF"); let mut text_paint = paint_from_hex(text_color); @@ -251,8 +258,84 @@ impl Painter for List { canvas: &Canvas, _layout: &BoxLayout, _props: &AnimatedProperties, - _ctx: &PaintCtx, + ctx: &PaintCtx, ) { - let _ = self.paint(canvas); + let _ = self.paint(canvas, ctx); + } +} + +#[cfg(test)] +mod tests { + use super::*; + use rustmotion_core::css::CssStyle; + use rustmotion_core::css::Length; + + fn test_ctx() -> PaintCtx { + PaintCtx { + time: 0.0, + scene_duration: 1.0, + frame_index: 0, + fps: 30, + video_width: 400, + video_height: 200, + stagger_offset: 0.0, + } + } + + // ─── Lot B, wave S: relative `font-size` units ───────────────────────── + + #[test] + fn rem_font_size_paints_visible_ink() { + // Reproduction: `font-size: "2rem"` used to resolve to 0px via the + // context-free `font_size_px_or`. + let list = List { + items: vec![ListItem { + text: "hello".to_string(), + icon: None, + checked: None, + }], + variant: ListVariant::Bullet, + gap: default_gap(), + icon_size: default_icon_size(), + icon_color: default_icon_color(), + unchecked_color: default_unchecked_color(), + width: default_width(), + timing: Default::default(), + style: CssStyle { + font_size: Some(Length::String("2rem".into())), + color: Some(rustmotion_core::css::style::Color::String("#FFFFFF".into())), + ..Default::default() + }, + timeline: Vec::new(), + stagger: None, + }; + const W: i32 = 400; + const H: i32 = 200; + let mut surface = skia_safe::surfaces::raster_n32_premul((W, H)).expect("raster surface"); + { + let canvas = surface.canvas(); + list.paint(canvas, &test_ctx()).expect("paint succeeds"); + } + let snapshot = surface.image_snapshot(); + let info = skia_safe::ImageInfo::new( + (W, H), + skia_safe::ColorType::RGBA8888, + skia_safe::AlphaType::Premul, + None, + ); + let mut buf = vec![0u8; (W * H * 4) as usize]; + let ok = snapshot.read_pixels( + &info, + &mut buf, + (W * 4) as usize, + skia_safe::IPoint::new(0, 0), + skia_safe::image::CachingHint::Disallow, + ); + assert!(ok, "pixel read should succeed"); + let lit = buf.chunks_exact(4).filter(|p| p[3] > 0).count(); + assert!( + lit > 20, + "list at font-size: 2rem must paint visible ink, got {lit} lit pixels" + ); } } diff --git a/crates/rustmotion-components/src/marquee.rs b/crates/rustmotion-components/src/marquee.rs index abd88dc..a512f28 100644 --- a/crates/rustmotion-components/src/marquee.rs +++ b/crates/rustmotion-components/src/marquee.rs @@ -64,11 +64,25 @@ rustmotion_core::impl_traits!(Marquee { }); impl Marquee { - fn paint(&self, canvas: &Canvas, layout_w: f32, layout_h: f32, time: f64) -> Result<()> { + fn paint( + &self, + canvas: &Canvas, + layout_w: f32, + layout_h: f32, + time: f64, + ctx: &PaintCtx, + ) -> Result<()> { let w = layout_w; let h = layout_h; - let fs = self.style.font_size_px_or(self.font_size); + // Resolved against a real per-frame viewport (`rem`/`vw`/`vh` now + // resolve instead of silently dropping to 0px — lot B, wave S). + // `em`/`%` on `font-size` itself remain approximate — see + // `crate::intrinsic::font_size_ctx`'s doc comment. + let fs = self.style.font_size_px_ctx( + &crate::intrinsic::font_size_ctx(ctx.video_width as f32, ctx.video_height as f32, 0.0), + self.font_size, + ); let font_style = skia_safe::FontStyle::normal(); let family = self.style.font_family_or("Inter"); let typeface = typeface_with_fallback(family, font_style)?; @@ -141,6 +155,78 @@ impl Painter for Marquee { _props: &AnimatedProperties, ctx: &PaintCtx, ) { - let _ = self.paint(canvas, layout.width, layout.height, ctx.time); + let _ = self.paint(canvas, layout.width, layout.height, ctx.time, ctx); + } +} + +#[cfg(test)] +mod tests { + use super::*; + use rustmotion_core::css::CssStyle; + use rustmotion_core::css::Length; + + fn test_ctx() -> PaintCtx { + PaintCtx { + time: 0.0, + scene_duration: 1.0, + frame_index: 0, + fps: 30, + video_width: 400, + video_height: 200, + stagger_offset: 0.0, + } + } + + // ─── Lot B, wave S: relative `font-size` units ───────────────────────── + + #[test] + fn rem_font_size_paints_visible_ink() { + // Reproduction: `font-size: "2rem"` used to resolve to 0px via the + // context-free `font_size_px_or`. + let marquee = Marquee { + content: "hello world".to_string(), + speed: default_speed(), + direction: MarqueeDirection::default(), + font_size: default_font_size(), + color: default_color(), + separator: None, + timing: Default::default(), + style: CssStyle { + font_size: Some(Length::String("2rem".into())), + ..Default::default() + }, + timeline: Vec::new(), + stagger: None, + }; + const W: i32 = 400; + const H: i32 = 100; + let mut surface = skia_safe::surfaces::raster_n32_premul((W, H)).expect("raster surface"); + { + let canvas = surface.canvas(); + marquee + .paint(canvas, W as f32, H as f32, 0.0, &test_ctx()) + .expect("paint succeeds"); + } + let snapshot = surface.image_snapshot(); + let info = skia_safe::ImageInfo::new( + (W, H), + skia_safe::ColorType::RGBA8888, + skia_safe::AlphaType::Premul, + None, + ); + let mut buf = vec![0u8; (W * H * 4) as usize]; + let ok = snapshot.read_pixels( + &info, + &mut buf, + (W * 4) as usize, + skia_safe::IPoint::new(0, 0), + skia_safe::image::CachingHint::Disallow, + ); + assert!(ok, "pixel read should succeed"); + let lit = buf.chunks_exact(4).filter(|p| p[3] > 0).count(); + assert!( + lit > 20, + "marquee at font-size: 2rem must paint visible ink, got {lit} lit pixels" + ); } } diff --git a/crates/rustmotion-components/src/notification.rs b/crates/rustmotion-components/src/notification.rs index f597d5b..1f26d9f 100644 --- a/crates/rustmotion-components/src/notification.rs +++ b/crates/rustmotion-components/src/notification.rs @@ -101,12 +101,19 @@ impl Notification { .unwrap_or_else(|| self.variant.default_color()) } - fn title_font_size(&self) -> f32 { - self.style.font_size_px_or(16.0) + /// Resolves `font-size` against a real per-frame viewport (`rem`/`vw`/ + /// `vh` now resolve instead of silently dropping to 0px — lot B, wave + /// S). `em`/`%` on `font-size` itself remain approximate — see + /// `crate::intrinsic::font_size_ctx`'s doc comment. + fn title_font_size(&self, ctx: &PaintCtx) -> f32 { + self.style.font_size_px_ctx( + &crate::intrinsic::font_size_ctx(ctx.video_width as f32, ctx.video_height as f32, 0.0), + 16.0, + ) } - fn message_font_size(&self) -> f32 { - self.style.font_size_px_or(16.0) * 0.85 + fn message_font_size(&self, ctx: &PaintCtx) -> f32 { + self.title_font_size(ctx) * 0.85 } fn make_font(&self, bold: bool, size: f32) -> Option { @@ -216,7 +223,14 @@ impl Notification { } impl Notification { - fn paint(&self, canvas: &Canvas, layout_w: f32, layout_h: f32, time: f64) -> Result<()> { + fn paint( + &self, + canvas: &Canvas, + layout_w: f32, + layout_h: f32, + time: f64, + ctx: &PaintCtx, + ) -> Result<()> { let w = layout_w; let h = layout_h; let opacity = self.compute_opacity(time); @@ -227,7 +241,7 @@ impl Notification { // Resolve the title font before any canvas.save() so an early return // on font failure keeps save/restore balanced. - let Some(title_font) = self.make_font(true, self.title_font_size()) else { + let Some(title_font) = self.make_font(true, self.title_font_size(ctx)) else { return Ok(()); }; @@ -286,7 +300,7 @@ impl Notification { // Content area let h_pad = 16.0; let v_pad = 16.0; - let icon_size = self.title_font_size() * 1.5; + let icon_size = self.title_font_size(ctx) * 1.5; let mut content_x = accent_width + h_pad; // Icon @@ -297,7 +311,7 @@ impl Notification { } // Title - let title_fs = self.title_font_size(); + let title_fs = self.title_font_size(ctx); let emoji_font_title = emoji_typeface().map(|tf| skia_safe::Font::from_typeface(tf, title_fs)); let title_color = self.style.color_str_or("#FFFFFF"); @@ -320,7 +334,7 @@ impl Notification { // Message if let Some(message) = &self.message { - let msg_fs = self.message_font_size(); + let msg_fs = self.message_font_size(ctx); if let Some(msg_font) = self.make_font(false, msg_fs) { let emoji_font_msg = emoji_typeface().map(|tf| skia_safe::Font::from_typeface(tf, msg_fs)); @@ -359,6 +373,89 @@ impl Painter for Notification { _props: &AnimatedProperties, ctx: &PaintCtx, ) { - let _ = self.paint(canvas, layout.width, layout.height, ctx.time); + let _ = self.paint(canvas, layout.width, layout.height, ctx.time, ctx); + } +} + +#[cfg(test)] +mod tests { + use super::*; + use rustmotion_core::css::CssStyle; + use rustmotion_core::css::Length; + + fn test_ctx() -> PaintCtx { + PaintCtx { + time: 1.0, + scene_duration: 2.0, + frame_index: 30, + fps: 30, + video_width: 400, + video_height: 200, + stagger_offset: 0.0, + } + } + + // ─── Lot B, wave S: relative `font-size` units ───────────────────────── + + #[test] + fn rem_font_size_paints_visible_ink() { + // Reproduction: `font-size: "2rem"` used to resolve to 0px via the + // context-free `font_size_px_or`. + let notification = Notification { + title: "Hello".to_string(), + message: None, + icon: None, + variant: NotificationVariant::Info, + width: default_width(), + slide_in_at: 0.0, + slide_out_at: None, + slide_duration: default_slide_duration(), + accent_color: None, + push_at: Vec::new(), + stack_gap: default_stack_gap(), + wait_for_push: false, + timing: Default::default(), + style: CssStyle { + font_size: Some(Length::String("2rem".into())), + ..Default::default() + }, + timeline: Vec::new(), + stagger: None, + }; + const W: i32 = 400; + const H: i32 = 200; + let mut surface = skia_safe::surfaces::raster_n32_premul((W, H)).expect("raster surface"); + { + let canvas = surface.canvas(); + notification + .paint(canvas, 360.0, 100.0, 1.0, &test_ctx()) + .expect("paint succeeds"); + } + let snapshot = surface.image_snapshot(); + let info = skia_safe::ImageInfo::new( + (W, H), + skia_safe::ColorType::RGBA8888, + skia_safe::AlphaType::Premul, + None, + ); + let mut buf = vec![0u8; (W * H * 4) as usize]; + let ok = snapshot.read_pixels( + &info, + &mut buf, + (W * 4) as usize, + skia_safe::IPoint::new(0, 0), + skia_safe::image::CachingHint::Disallow, + ); + assert!(ok, "pixel read should succeed"); + // Title text is white (#FFFFFF default) on a dark #1E293B card — + // probe for near-white ink specifically. + let text_ink = buf + .chunks_exact(4) + .filter(|p| p[3] > 0 && p[0] > 200 && p[1] > 200 && p[2] > 200) + .count(); + assert!( + text_ink > 10, + "notification at font-size: 2rem must paint visible text, got {text_ink} pixels" + ); } } diff --git a/crates/rustmotion-components/src/pill_nav.rs b/crates/rustmotion-components/src/pill_nav.rs index b0c3dac..1d4ef9d 100644 --- a/crates/rustmotion-components/src/pill_nav.rs +++ b/crates/rustmotion-components/src/pill_nav.rs @@ -83,11 +83,18 @@ rustmotion_core::impl_traits!(PillNav { }); impl PillNav { - fn resolved_font_size(&self) -> f32 { - self.style.font_size_px_or(14.0) + /// Resolves `font-size` against a real per-frame viewport (`rem`/`vw`/ + /// `vh` now resolve instead of silently dropping to 0px — lot B, wave + /// S). `em`/`%` on `font-size` itself remain approximate — see + /// `crate::intrinsic::font_size_ctx`'s doc comment. + fn resolved_font_size(&self, ctx: &PaintCtx) -> f32 { + self.style.font_size_px_ctx( + &crate::intrinsic::font_size_ctx(ctx.video_width as f32, ctx.video_height as f32, 0.0), + 14.0, + ) } - fn make_font(&self, bold: bool) -> Option { + fn make_font(&self, bold: bool, ctx: &PaintCtx) -> Option { let font_style = if bold { skia_safe::FontStyle::bold() } else { @@ -97,13 +104,13 @@ impl PillNav { let typeface = typeface_with_fallback(family, font_style).ok()?; Some(skia_safe::Font::from_typeface( typeface, - self.resolved_font_size(), + self.resolved_font_size(ctx), )) } - fn compute_tab_layout(&self) -> Option<(f32, Vec, Vec)> { - let font = self.make_font(false)?; - let font_size = self.resolved_font_size(); + fn compute_tab_layout(&self, ctx: &PaintCtx) -> Option<(f32, Vec, Vec)> { + let font = self.make_font(false, ctx)?; + let font_size = self.resolved_font_size(ctx); let emoji_font = emoji_typeface().map(|tf| skia_safe::Font::from_typeface(tf, font_size)); let h_pad = font_size * 1.2; @@ -158,7 +165,7 @@ impl PillNav { } impl PillNav { - fn paint(&self, canvas: &Canvas, layout_w: f32, layout_h: f32, time: f64) { + fn paint(&self, canvas: &Canvas, layout_w: f32, layout_h: f32, time: f64, ctx: &PaintCtx) { if self.items.is_empty() { return; } @@ -174,7 +181,7 @@ impl PillNav { bg_paint.set_anti_alias(true); canvas.draw_rrect(outer_rrect, &bg_paint); - let Some((_total_w, tab_positions, tab_widths)) = self.compute_tab_layout() else { + let Some((_total_w, tab_positions, tab_widths)) = self.compute_tab_layout(ctx) else { return; }; let (active, transition_info) = self.active_at_time(time); @@ -206,10 +213,10 @@ impl PillNav { canvas.draw_rrect(pill_rrect, &pill_paint); // Tab labels - let Some(font) = self.make_font(false) else { + let Some(font) = self.make_font(false, ctx) else { return; }; - let font_size = self.resolved_font_size(); + let font_size = self.resolved_font_size(ctx); let emoji_font = emoji_typeface().map(|tf| skia_safe::Font::from_typeface(tf, font_size)); let (_, metrics) = font.metrics(); let text_y = (h + (-metrics.ascent)) / 2.0; @@ -249,6 +256,81 @@ impl Painter for PillNav { _props: &AnimatedProperties, ctx: &PaintCtx, ) { - self.paint(canvas, layout.width, layout.height, ctx.time); + self.paint(canvas, layout.width, layout.height, ctx.time, ctx); + } +} + +#[cfg(test)] +mod tests { + use super::*; + use rustmotion_core::css::CssStyle; + use rustmotion_core::css::Length; + + fn test_ctx() -> PaintCtx { + PaintCtx { + time: 0.0, + scene_duration: 1.0, + frame_index: 0, + fps: 30, + video_width: 400, + video_height: 200, + stagger_offset: 0.0, + } + } + + // ─── Lot B, wave S: relative `font-size` units ───────────────────────── + + #[test] + fn rem_font_size_paints_visible_ink() { + // Reproduction: `font-size: "2rem"` used to resolve to 0px via the + // context-free `font_size_px_or`. + let nav = PillNav { + items: vec!["Home".to_string(), "About".to_string()], + active_index: 0, + transitions: Vec::new(), + pill_color: default_pill_color(), + text_color: default_text_color(), + inactive_text_color: default_inactive_text_color(), + background_color: default_background_color(), + height: default_height(), + border_radius: default_border_radius(), + gap: default_gap(), + transition_duration: default_transition_duration(), + timing: Default::default(), + style: CssStyle { + font_size: Some(Length::String("2rem".into())), + ..Default::default() + }, + timeline: Vec::new(), + stagger: None, + }; + const W: i32 = 400; + const H: i32 = 100; + let mut surface = skia_safe::surfaces::raster_n32_premul((W, H)).expect("raster surface"); + { + let canvas = surface.canvas(); + nav.paint(canvas, 300.0, 44.0, 0.0, &test_ctx()); + } + let snapshot = surface.image_snapshot(); + let info = skia_safe::ImageInfo::new( + (W, H), + skia_safe::ColorType::RGBA8888, + skia_safe::AlphaType::Premul, + None, + ); + let mut buf = vec![0u8; (W * H * 4) as usize]; + let ok = snapshot.read_pixels( + &info, + &mut buf, + (W * 4) as usize, + skia_safe::IPoint::new(0, 0), + skia_safe::image::CachingHint::Disallow, + ); + assert!(ok, "pixel read should succeed"); + let lit = buf.chunks_exact(4).filter(|p| p[3] > 0).count(); + assert!( + lit > 20, + "pill_nav at font-size: 2rem must paint visible ink, got {lit} lit pixels" + ); } } diff --git a/crates/rustmotion-components/src/rich_text.rs b/crates/rustmotion-components/src/rich_text.rs index 86bc0ba..e2e3d4f 100644 --- a/crates/rustmotion-components/src/rich_text.rs +++ b/crates/rustmotion-components/src/rich_text.rs @@ -88,8 +88,18 @@ struct SpanFontInfo { /// component-level style for anything a span doesn't override. `None` at /// index `i` means the span's font failed to load — that span is skipped /// during tokenization (same behaviour as the original `filter_map`). -fn resolve_span_fonts(spans: &[RichTextSpan], style: &CssStyle) -> Vec> { - let default_size = style.font_size_px_or(48.0); +/// +/// `default_size` is resolved by the caller (once, against a real +/// `LengthContext` where one is available) rather than re-derived here — +/// this used to call the context-free `style.font_size_px_or(48.0)` +/// independently of `compute_layout`'s own resolution of the same value, a +/// duplicate computation that silently diverged for relative units (lot B, +/// wave S). +fn resolve_span_fonts( + spans: &[RichTextSpan], + style: &CssStyle, + default_size: f32, +) -> Vec> { let default_color = style.color_str_or("#FFFFFF"); let default_family = style.font_family_or("Inter"); let default_weight = match &style.font_weight { @@ -163,15 +173,30 @@ impl RichText { /// behaviour); the exact inter-word spacing of source whitespace is not /// preserved, matching how `wrap_text_with_fallback` already treats /// plain `text` content. + /// + /// `viewport_width`/`viewport_height` resolve `rem`/`vw`/`vh` on + /// `style.font-size` (lot B, wave S — this used to go through the + /// context-free `font_size_px_or`, which silently resolved those units + /// to 0px). Callers with no real per-frame viewport (intrinsic + /// measurement, which runs before layout) should pass a stand-in — see + /// `intrinsic::measure_time_font_size_ctx`'s doc comment for why 0px is + /// worse than an approximation. pub fn compute_layout( spans: &[RichTextSpan], style: &CssStyle, + viewport_width: f32, + viewport_height: f32, wrap_width: Option, visible_chars_progress: f32, ) -> RichTextLayout { - let default_size = style.font_size_px_or(48.0); - let line_height_val = style.line_height_for(default_size); - let span_fonts = resolve_span_fonts(spans, style); + let base_ctx = crate::intrinsic::font_size_ctx( + viewport_width, + viewport_height, + wrap_width.unwrap_or(0.0), + ); + let (default_size, _letter_spacing_unused, line_height_val) = + style.typography_px_ctx(&base_ctx, 48.0); + let span_fonts = resolve_span_fonts(spans, style, default_size); let emoji_tf = emoji_typeface(); // Typewriter truncation operates on each span's text by char count @@ -305,7 +330,13 @@ impl RichText { } } - fn paint(&self, canvas: &Canvas, layout_width: f32, props: &AnimatedProperties) { + fn paint( + &self, + canvas: &Canvas, + layout_width: f32, + props: &AnimatedProperties, + ctx: &PaintCtx, + ) { let align = match self.style.text_align { Some(CssTextAlign::Center) => TextAlign::Center, Some(CssTextAlign::Right | CssTextAlign::End) => TextAlign::Right, @@ -324,6 +355,8 @@ impl RichText { let layout = RichText::compute_layout( &self.spans, &self.style, + ctx.video_width as f32, + ctx.video_height as f32, wrap_width, props.visible_chars_progress, ); @@ -331,7 +364,19 @@ impl RichText { return; } - let span_fonts = resolve_span_fonts(&self.spans, &self.style); + // Same `default_size` resolution `compute_layout` used above (real + // viewport, same `wrap_width`-derived parent size) — kept as a + // second call rather than threading `span_fonts` back out of + // `RichTextLayout`, but now via the same context-aware accessor so + // the two can no longer diverge on a relative `font-size` the way + // they structurally could before (lot B, wave S). + let base_ctx = crate::intrinsic::font_size_ctx( + ctx.video_width as f32, + ctx.video_height as f32, + wrap_width.unwrap_or(0.0), + ); + let default_size = self.style.font_size_px_ctx(&base_ctx, 48.0); + let span_fonts = resolve_span_fonts(&self.spans, &self.style, default_size); let emoji_tf = emoji_typeface(); let align_width = if layout_width.is_finite() && layout_width > 0.0 { @@ -380,9 +425,9 @@ impl Painter for RichText { canvas: &Canvas, layout: &BoxLayout, props: &AnimatedProperties, - _ctx: &PaintCtx, + ctx: &PaintCtx, ) { - self.paint(canvas, layout.width, props); + self.paint(canvas, layout.width, props, ctx); } } @@ -418,14 +463,14 @@ mod tests { "the quick brown fox jumps over the lazy dog and keeps going", )]; let s = style(24.0); - let unconstrained = RichText::compute_layout(&spans, &s, None, -1.0); + let unconstrained = RichText::compute_layout(&spans, &s, 1920.0, 1080.0, None, -1.0); assert_eq!( unconstrained.lines.len(), 1, "unconstrained width must fit on one line" ); - let constrained = RichText::compute_layout(&spans, &s, Some(150.0), -1.0); + let constrained = RichText::compute_layout(&spans, &s, 1920.0, 1080.0, Some(150.0), -1.0); assert!( constrained.lines.len() > 1, "a single long span must wrap into multiple lines at 150px, got {} line(s)", @@ -448,7 +493,7 @@ mod tests { // already in the third span. let glued = vec![span("Total:"), span("42"), span(" items")]; let s = style(20.0); - let layout = RichText::compute_layout(&glued, &s, None, -1.0); + let layout = RichText::compute_layout(&glued, &s, 1920.0, 1080.0, None, -1.0); assert_eq!(layout.lines.len(), 1); let tokens = &layout.lines[0].tokens; assert_eq!( @@ -471,9 +516,9 @@ mod tests { fn typewriter_truncation_hides_tail_tokens() { let spans = vec![span("Hello "), span("world")]; let s = style(20.0); - let full = RichText::compute_layout(&spans, &s, None, -1.0); - let half = RichText::compute_layout(&spans, &s, None, 0.5); - let none = RichText::compute_layout(&spans, &s, None, 0.0); + let full = RichText::compute_layout(&spans, &s, 1920.0, 1080.0, None, -1.0); + let half = RichText::compute_layout(&spans, &s, 1920.0, 1080.0, None, 0.5); + let none = RichText::compute_layout(&spans, &s, 1920.0, 1080.0, None, 0.0); let full_tokens: usize = full.lines.iter().map(|l| l.tokens.len()).sum(); let half_tokens: usize = half.lines.iter().map(|l| l.tokens.len()).sum(); @@ -491,7 +536,7 @@ mod tests { fn empty_spans_produce_one_empty_line_not_a_panic() { let spans: Vec = vec![]; let s = style(20.0); - let layout = RichText::compute_layout(&spans, &s, None, -1.0); + let layout = RichText::compute_layout(&spans, &s, 1920.0, 1080.0, None, -1.0); assert_eq!(layout.lines.len(), 1); assert_eq!(layout.max_width, 0.0); } diff --git a/crates/rustmotion-components/src/table.rs b/crates/rustmotion-components/src/table.rs index 0c0bfab..666dae6 100644 --- a/crates/rustmotion-components/src/table.rs +++ b/crates/rustmotion-components/src/table.rs @@ -77,15 +77,17 @@ rustmotion_core::impl_traits!(Table { }); impl Table { - fn font_size(&self) -> f32 { - self.style.font_size_px_or(14.0) + /// `font_size` is resolved once by the caller (`paint`, against a real + /// `LengthContext`) and passed in — this used to be a zero-argument + /// method independently re-deriving the value via the context-free + /// `font_size_px_or` at every call site (`row_height`, `make_font`, + /// `paint` itself), which is exactly the kind of duplicate computation + /// that let a relative unit silently diverge (lot B, wave S). + fn row_height(&self, font_size: f32) -> f32 { + font_size * 2.5 } - fn row_height(&self) -> f32 { - self.font_size() * 2.5 - } - - fn make_font(&self, bold: bool) -> Option { + fn make_font(&self, bold: bool, font_size: f32) -> Option { let font_style = if bold { skia_safe::FontStyle::bold() } else { @@ -94,7 +96,7 @@ impl Table { let family = self.style.font_family.as_deref().unwrap_or("Inter"); let typeface = typeface_with_fallback(family, font_style).ok()?; - Some(skia_safe::Font::from_typeface(typeface, self.font_size())) + Some(skia_safe::Font::from_typeface(typeface, font_size)) } /// Resolve column widths: use explicit widths if provided, else equal distribution. @@ -135,11 +137,23 @@ impl Table { } impl Table { - fn paint(&self, canvas: &Canvas, layout_w: f32, layout_h: f32) { + fn paint(&self, canvas: &Canvas, layout_w: f32, layout_h: f32, ctx: &PaintCtx) { let w = layout_w; + // Resolved once, against the real per-frame viewport (`rem`/`vw`/ + // `vh` on `font-size` now resolve instead of silently dropping to + // 0px — lot B, wave S) and threaded through every call below that + // used to independently re-derive it via `font_size_px_or`. + let font_size = self.style.font_size_px_ctx( + &crate::intrinsic::font_size_ctx( + ctx.video_width as f32, + ctx.video_height as f32, + w.max(0.0), + ), + 14.0, + ); let col_count = self.headers.len().max(1); let col_widths = self.resolve_column_widths(w); - let row_h = self.row_height(); + let row_h = self.row_height(font_size); let header_color = self.header_color.as_deref().unwrap_or("#374151"); let border_color = self.border_color.as_deref().unwrap_or("#4B5563"); @@ -157,10 +171,10 @@ impl Table { // Resolve fonts before the optional clip below so an early return on // font failure keeps canvas save/restore balanced. - let Some(header_font) = self.make_font(true) else { + let Some(header_font) = self.make_font(true, font_size) else { return; }; - let Some(body_font) = self.make_font(false) else { + let Some(body_font) = self.make_font(false, font_size) else { return; }; @@ -175,7 +189,6 @@ impl Table { } // Header row - let font_size = self.font_size(); let emoji_font = emoji_typeface().map(|tf| skia_safe::Font::from_typeface(tf, font_size)); let (_, header_metrics) = header_font.metrics(); let header_ascent = -header_metrics.ascent; @@ -192,7 +205,7 @@ impl Table { let cw = col_widths.get(i).copied().unwrap_or(0.0); let text_w = measure_text_with_fallback(header, &header_font, &emoji_font, 0.0); let x = self.align_text_x(i, col_x, cw, text_w); - let y = (row_h - self.font_size()) / 2.0 + header_ascent; + let y = (row_h - font_size) / 2.0 + header_ascent; draw_text_with_fallback( canvas, @@ -233,7 +246,7 @@ impl Table { let cw = col_widths.get(col_idx).copied().unwrap_or(0.0); let text_w = measure_text_with_fallback(cell, &body_font, &emoji_font, 0.0); let x = self.align_text_x(col_idx, cx, cw, text_w); - let y = y_base + (row_h - self.font_size()) / 2.0 + body_ascent; + let y = y_base + (row_h - font_size) / 2.0 + body_ascent; draw_text_with_fallback( canvas, @@ -285,8 +298,84 @@ impl Painter for Table { canvas: &Canvas, layout: &BoxLayout, _props: &AnimatedProperties, - _ctx: &PaintCtx, + ctx: &PaintCtx, ) { - self.paint(canvas, layout.width, layout.height); + self.paint(canvas, layout.width, layout.height, ctx); + } +} + +#[cfg(test)] +mod tests { + use super::*; + use rustmotion_core::css::Length; + + // ─── Lot B, wave S: relative `font-size` units ───────────────────────── + + #[test] + fn rem_font_size_paints_visible_ink() { + // Reproduction: `font-size: "2rem"` used to resolve to 0px via the + // context-free `font_size_px_or`. + let table = Table { + headers: vec!["A".to_string(), "B".to_string()], + rows: vec![vec!["1".to_string(), "2".to_string()]], + header_color: None, + row_colors: None, + border_color: None, + header_text_color: None, + column_widths: None, + column_align: None, + cell_padding: DEFAULT_CELL_PADDING, + show_borders: true, + timing: Default::default(), + style: CssStyle { + font_size: Some(Length::String("2rem".into())), + ..Default::default() + }, + timeline: Vec::new(), + stagger: None, + }; + const W: i32 = 400; + const H: i32 = 200; + let mut surface = skia_safe::surfaces::raster_n32_premul((W, H)).expect("raster surface"); + let ctx = PaintCtx { + time: 0.0, + scene_duration: 1.0, + frame_index: 0, + fps: 30, + video_width: 400, + video_height: 200, + stagger_offset: 0.0, + }; + { + let canvas = surface.canvas(); + table.paint(canvas, W as f32, H as f32, &ctx); + } + let snapshot = surface.image_snapshot(); + let info = skia_safe::ImageInfo::new( + (W, H), + skia_safe::ColorType::RGBA8888, + skia_safe::AlphaType::Premul, + None, + ); + let mut buf = vec![0u8; (W * H * 4) as usize]; + let ok = snapshot.read_pixels( + &info, + &mut buf, + (W * 4) as usize, + skia_safe::IPoint::new(0, 0), + skia_safe::image::CachingHint::Disallow, + ); + assert!(ok, "pixel read should succeed"); + // Header text is white (#FFFFFF) on a #374151 header background — + // probe specifically for near-white text ink rather than any lit + // pixel (the header/row backgrounds paint regardless of font-size). + let text_ink = buf + .chunks_exact(4) + .filter(|p| p[3] > 0 && p[0] > 200 && p[1] > 200 && p[2] > 200) + .count(); + assert!( + text_ink > 10, + "table at font-size: 2rem must paint visible header/cell text, got {text_ink} pixels" + ); } } diff --git a/crates/rustmotion-components/src/terminal.rs b/crates/rustmotion-components/src/terminal.rs index 0e1d8ce..8d08d7e 100644 --- a/crates/rustmotion-components/src/terminal.rs +++ b/crates/rustmotion-components/src/terminal.rs @@ -147,14 +147,18 @@ pub(crate) fn resolve_typeface(style: &CssStyle) -> Option } impl Terminal { - fn make_font(&self) -> Option { + /// `font_size` is resolved once by the caller (`paint`, against a real + /// `LengthContext`) and threaded through here and [`Self::line_height`] + /// instead of each independently re-deriving it via the context-free + /// `font_size_px_or` — four separate call sites used to do exactly that, + /// which is how a relative unit could silently diverge between them + /// (lot B, wave S). + fn make_font(&self, font_size: f32) -> Option { let typeface = resolve_typeface(&self.style)?; - let size = self.style.font_size_px_or(FONT_SIZE); - Some(skia_safe::Font::from_typeface(typeface, size)) + Some(skia_safe::Font::from_typeface(typeface, font_size)) } - fn line_height(&self) -> f32 { - let font_size = self.style.font_size_px_or(FONT_SIZE); + fn line_height(&self, font_size: f32) -> f32 { (font_size * LINE_HEIGHT / FONT_SIZE).ceil() } @@ -229,10 +233,20 @@ impl Terminal { } impl Terminal { - fn paint(&self, canvas: &Canvas, layout_w: f32, layout_h: f32, time: f64) { + fn paint(&self, canvas: &Canvas, layout_w: f32, layout_h: f32, ctx: &PaintCtx) { + let time = ctx.time; let w = layout_w; let h = layout_h; + // Resolved once, against the real per-frame viewport (`rem`/`vw`/ + // `vh` on `font-size` now resolve instead of silently dropping to + // 0px — lot B, wave S) and threaded through every call below that + // used to independently re-derive it. + let font_size = self.style.font_size_px_ctx( + &crate::intrinsic::font_size_ctx(ctx.video_width as f32, ctx.video_height as f32, 0.0), + FONT_SIZE, + ); + // Background let bg_rect = Rect::from_xywh(0.0, 0.0, w, h); let bg_rrect = RRect::new_rect_xy(bg_rect, CORNER_RADIUS, CORNER_RADIUS); @@ -243,7 +257,7 @@ impl Terminal { // Resolve the terminal font up front — bail before any canvas.save() // so save/restore stays balanced if no font is available. - let Some(font) = self.make_font() else { + let Some(font) = self.make_font(font_size) else { return; }; @@ -278,7 +292,6 @@ impl Terminal { // Title if let Some(title) = &self.title { - let font_size = self.style.font_size_px_or(FONT_SIZE); let emoji_font = emoji_typeface().map(|tf| skia_safe::Font::from_typeface(tf, font_size)); let mut title_paint = paint_from_hex(self.theme.title_color()); @@ -302,7 +315,6 @@ impl Terminal { let (visible_lines, partial_chars, last_line_opacity) = self.compute_reveal(time); // Lines - let font_size = self.style.font_size_px_or(FONT_SIZE); let emoji_font = emoji_typeface().map(|tf| skia_safe::Font::from_typeface(tf, font_size)); let (_, metrics) = font.metrics(); let ascent = -metrics.ascent; @@ -321,7 +333,7 @@ impl Terminal { true, ); if self.auto_scroll { - let line_h = self.line_height(); + let line_h = self.line_height(font_size); let content_h = visible_lines as f32 * line_h + PADDING * 2.0 + chrome_h; let overflow = content_h - h; if overflow > 0.0 { @@ -437,7 +449,7 @@ impl Terminal { } } - y_offset += self.line_height(); + y_offset += self.line_height(font_size); } canvas.restore(); // close inner content clip @@ -453,6 +465,82 @@ impl Painter for Terminal { _props: &AnimatedProperties, ctx: &PaintCtx, ) { - self.paint(canvas, layout.width, layout.height, ctx.time); + self.paint(canvas, layout.width, layout.height, ctx); + } +} + +#[cfg(test)] +mod tests { + use super::*; + + // ─── Lot B, wave S: relative `font-size` units ───────────────────────── + + #[test] + fn rem_font_size_paints_visible_ink() { + // Reproduction: `font-size: "2rem"` used to resolve to 0px via the + // context-free `font_size_px_or`, at all four call sites that used + // to independently re-derive it in `paint`. + let terminal = Terminal { + lines: vec![TerminalLine { + text: "hello world".to_string(), + line_type: TerminalLineType::Output, + color: None, + }], + theme: TerminalTheme::default(), + title: None, + show_chrome: false, + reveal: None, + auto_scroll: true, + timing: Default::default(), + style: CssStyle { + font_size: Some(rustmotion_core::css::Length::String("2rem".into())), + ..Default::default() + }, + timeline: Vec::new(), + stagger: None, + }; + const W: i32 = 400; + const H: i32 = 200; + let mut surface = skia_safe::surfaces::raster_n32_premul((W, H)).expect("raster surface"); + let ctx = PaintCtx { + time: 0.0, + scene_duration: 1.0, + frame_index: 0, + fps: 30, + video_width: 400, + video_height: 200, + stagger_offset: 0.0, + }; + { + let canvas = surface.canvas(); + terminal.paint(canvas, W as f32, H as f32, &ctx); + } + let snapshot = surface.image_snapshot(); + let info = skia_safe::ImageInfo::new( + (W, H), + skia_safe::ColorType::RGBA8888, + skia_safe::AlphaType::Premul, + None, + ); + let mut buf = vec![0u8; (W * H * 4) as usize]; + let ok = snapshot.read_pixels( + &info, + &mut buf, + (W * 4) as usize, + skia_safe::IPoint::new(0, 0), + skia_safe::image::CachingHint::Disallow, + ); + assert!(ok, "pixel read should succeed"); + // Background always paints (opaque bg_rrect), so probe for text ink + // specifically: pixels that are not the dark theme background color + // (#1E1E1E) and not fully transparent. + let text_ink = buf + .chunks_exact(4) + .filter(|p| p[3] > 0 && !(p[0] < 40 && p[1] < 40 && p[2] < 40)) + .count(); + assert!( + text_ink > 20, + "terminal at font-size: 2rem must paint visible text ink, got {text_ink} pixels" + ); } } diff --git a/crates/rustmotion-components/src/text.rs b/crates/rustmotion-components/src/text.rs index 504fa6a..3efc3a9 100644 --- a/crates/rustmotion-components/src/text.rs +++ b/crates/rustmotion-components/src/text.rs @@ -360,26 +360,38 @@ impl Text { props: &AnimatedProperties, ctx: &PaintCtx, ) -> Result<()> { - // `font-size` stays on the context-free accessor: resolving a - // relative unit here correctly (`em`/`%`) would need the parent's - // *actual computed* font-size, which `cascade.rs` doesn't provide - // today (it inherits `font-size` as a raw, unresolved `Length` — - // see the module note on `CssStyle::font_size_px_ctx`, issue #125 - // §2). That cascade fix stays out of scope here; `vw`/`vh`/`rem` - // font-size still fall back to 0px with a loud warning via `.px()`. - let font_size = self.style.font_size_px_or(48.0); - // `letter-spacing` and `line-height`'s `em`/`%` are relative to this - // element's *own* (just-resolved) font-size, which has no cascade - // dependency — a real `LengthContext` is available here (real - // viewport dims from `ctx`, `font_size` just above), so these use - // the context-aware resolvers and correctly handle `vw`/`vh`/`rem`/ - // line-height-`%` (issue #125 §2). + // `font-size` itself, plus `letter-spacing`/`line-height`'s `em`/`%` + // (relative to this element's *own*, just-resolved font-size) are + // now all resolved together against a real `LengthContext` (real + // viewport dims from `ctx`) via `typography_px_ctx`, which re-derives + // the right base between the two steps (lot B, wave S — this used to + // stop at the context-free `font_size_px_or`, so `rem`/`vw`/`vh` + // font-size silently fell back to 0px with only a loud warning). + // + // `em`/`%` *on `font-size` itself* are the one case this still + // doesn't get right: per CSS they're relative to the *parent's* + // actual computed font-size, but `cascade.rs` inherits `font-size` + // down the tree as a raw, unresolved `Length`, not a resolved px + // value (see the module note on `CssStyle::font_size_px_ctx`) — no + // caller here can supply the real cascaded value, so `base_ctx` + // below uses the CSS root default (16px) as the best available + // stand-in. `rem` (always relative to a fixed root, not a per- + // ancestor chain) and `vw`/`vh` (relative to the real viewport, + // available here via `ctx`) do not have this problem. + let base_ctx = crate::intrinsic::font_size_ctx( + ctx.video_width as f32, + ctx.video_height as f32, + layout_width.max(0.0), + ); + let (font_size, letter_spacing, line_height_val) = + self.style.typography_px_ctx(&base_ctx, 48.0); + // This element's *own* resolved font-size as the `em`/`%` base — + // needed below for `text-shadow` (its blur/offset are relative to + // the shadow owner's own font-size, same rule as letter-spacing/ + // line-height, not the parent-proxy `base_ctx` above). let type_ctx = rustmotion_core::css::units::LengthContext { - viewport_width: ctx.video_width as f32, - viewport_height: ctx.video_height as f32, - parent_size: layout_width.max(0.0), font_size, - root_font_size: 16.0, + ..base_ctx }; // Animated color (timeline style-state transitions) overrides the // static style color. @@ -406,8 +418,6 @@ impl Text { Some(CssTextAlign::Right | CssTextAlign::End) => TextAlign::Right, _ => TextAlign::Left, }; - let line_height_val = self.style.line_height_for_ctx(font_size, &type_ctx); - let letter_spacing = self.style.letter_spacing_px_ctx(&type_ctx); let slant = match font_style_type { FontStyleType::Normal => skia_safe::font_style::Slant::Upright, @@ -813,6 +823,84 @@ mod tests { ); } + // ─── Lot B, wave S: relative `font-size` units ───────────────────────── + + #[test] + fn rem_font_size_paints_visible_ink() { + // Reproduction: `font-size: "2rem"` used to resolve to 0px (the + // context-free `font_size_px_or` cannot resolve `rem`), so + // `TextIntrinsic` measured a 0-height box and `paint_pass`'s + // `height <= 0.0` guard skipped painting this node entirely — + // `validate` reported success with only a warning. + let text = Text { + content: "HELLO".into(), + max_width: None, + timing: Default::default(), + style: CssStyle { + font_size: Some(Length::String("2rem".into())), + color: Some(rustmotion_core::css::style::Color::String("#FFFFFF".into())), + ..Default::default() + }, + timeline: Vec::new(), + stagger: None, + text_shadow: None, + stroke: None, + text_background: None, + }; + const W: i32 = 400; + const H: i32 = 200; + let mut surface = skia_safe::surfaces::raster_n32_premul((W, H)).expect("raster surface"); + let canvas = surface.canvas(); + let ctx = test_ctx(); + let props = AnimatedProperties::default(); + text.paint(canvas, 300.0, 0.0, &props, &ctx) + .expect("paint succeeds"); + let grid = alpha_grid(&mut surface, W, H); + + // 2rem against the 16px CSS root default = 32px — comfortably tall + // enough to show up in the first 60 rows. + assert!( + has_ink_in(&grid, W, 0, W, 0, 60), + "font-size: 2rem must paint visible ink (32px glyphs), got none" + ); + } + + #[test] + fn vh_font_size_paints_visible_ink_scaled_to_the_real_viewport() { + // `vh` needs the real per-frame viewport (`ctx.video_height`), not + // just a fixed root size — a different resolution path from `rem`. + // `test_ctx()` sets `video_height: 200`, so `20vh` = 40px. + let text = Text { + content: "HI".into(), + max_width: None, + timing: Default::default(), + style: CssStyle { + font_size: Some(Length::String("20vh".into())), + color: Some(rustmotion_core::css::style::Color::String("#FFFFFF".into())), + ..Default::default() + }, + timeline: Vec::new(), + stagger: None, + text_shadow: None, + stroke: None, + text_background: None, + }; + const W: i32 = 400; + const H: i32 = 200; + let mut surface = skia_safe::surfaces::raster_n32_premul((W, H)).expect("raster surface"); + let canvas = surface.canvas(); + let ctx = test_ctx(); + let props = AnimatedProperties::default(); + text.paint(canvas, 300.0, 0.0, &props, &ctx) + .expect("paint succeeds"); + let grid = alpha_grid(&mut surface, W, H); + + assert!( + has_ink_in(&grid, W, 0, W, 0, 70), + "font-size: 20vh (40px against a 200px-tall test viewport) must paint visible ink" + ); + } + // ─── char_blur_in ─────────────────────────────────────────────────── /// Fraction of "inked" pixels (alpha > 0) in the region that are diff --git a/crates/rustmotion-components/src/tooltip.rs b/crates/rustmotion-components/src/tooltip.rs index ded73d0..d60c325 100644 --- a/crates/rustmotion-components/src/tooltip.rs +++ b/crates/rustmotion-components/src/tooltip.rs @@ -73,8 +73,19 @@ rustmotion_core::impl_traits!(Tooltip { }); impl Tooltip { - fn make_font(&self) -> Option { - let fs = self.style.font_size_px_or(self.font_size); + /// Resolves `font-size` against a real per-frame viewport (`rem`/`vw`/ + /// `vh` now resolve instead of silently dropping to 0px — lot B, wave + /// S). `em`/`%` on `font-size` itself remain approximate — see + /// `crate::intrinsic::font_size_ctx`'s doc comment. + fn resolved_font_size(&self, ctx: &PaintCtx) -> f32 { + self.style.font_size_px_ctx( + &crate::intrinsic::font_size_ctx(ctx.video_width as f32, ctx.video_height as f32, 0.0), + self.font_size, + ) + } + + fn make_font(&self, ctx: &PaintCtx) -> Option { + let fs = self.resolved_font_size(ctx); let font_style = skia_safe::FontStyle::normal(); let family = self.style.font_family_or("Inter"); let typeface = typeface_with_fallback(family, font_style).ok()?; @@ -83,7 +94,7 @@ impl Tooltip { } impl Tooltip { - fn paint(&self, canvas: &Canvas, layout_w: f32, layout_h: f32) { + fn paint(&self, canvas: &Canvas, layout_w: f32, layout_h: f32, ctx: &PaintCtx) { let w = layout_w; let h = layout_h; let bg_color = self @@ -162,10 +173,10 @@ impl Tooltip { } // Text centered in body - let Some(font) = self.make_font() else { + let Some(font) = self.make_font(ctx) else { return; }; - let fs = self.style.font_size_px_or(self.font_size); + let fs = self.resolved_font_size(ctx); let emoji_font = emoji_typeface().map(|tf| skia_safe::Font::from_typeface(tf, fs)); let text_color = self.style.color_str().unwrap_or(&self.text_color); @@ -196,8 +207,84 @@ impl Painter for Tooltip { canvas: &Canvas, layout: &BoxLayout, _props: &AnimatedProperties, - _ctx: &PaintCtx, + ctx: &PaintCtx, ) { - self.paint(canvas, layout.width, layout.height); + self.paint(canvas, layout.width, layout.height, ctx); + } +} + +#[cfg(test)] +mod tests { + use super::*; + use rustmotion_core::css::CssStyle; + use rustmotion_core::css::Length; + + fn test_ctx() -> PaintCtx { + PaintCtx { + time: 0.0, + scene_duration: 1.0, + frame_index: 0, + fps: 30, + video_width: 400, + video_height: 200, + stagger_offset: 0.0, + } + } + + // ─── Lot B, wave S: relative `font-size` units ───────────────────────── + + #[test] + fn rem_font_size_paints_visible_ink() { + // Reproduction: `font-size: "2rem"` used to resolve to 0px via the + // context-free `font_size_px_or`. + let tooltip = Tooltip { + text: "hello".to_string(), + arrow: TooltipArrow::None, + font_size: default_font_size(), + background_color: default_bg_color(), + text_color: default_text_color(), + arrow_size: default_arrow_size(), + border_color: None, + timing: Default::default(), + style: CssStyle { + font_size: Some(Length::String("2rem".into())), + ..Default::default() + }, + timeline: Vec::new(), + stagger: None, + }; + const W: i32 = 200; + const H: i32 = 100; + let mut surface = skia_safe::surfaces::raster_n32_premul((W, H)).expect("raster surface"); + { + let canvas = surface.canvas(); + tooltip.paint(canvas, 150.0, 60.0, &test_ctx()); + } + let snapshot = surface.image_snapshot(); + let info = skia_safe::ImageInfo::new( + (W, H), + skia_safe::ColorType::RGBA8888, + skia_safe::AlphaType::Premul, + None, + ); + let mut buf = vec![0u8; (W * H * 4) as usize]; + let ok = snapshot.read_pixels( + &info, + &mut buf, + (W * 4) as usize, + skia_safe::IPoint::new(0, 0), + skia_safe::image::CachingHint::Disallow, + ); + assert!(ok, "pixel read should succeed"); + // Text is near-white (#E2E8F0 default) on a dark #1E293B body — + // probe for near-white ink specifically. + let text_ink = buf + .chunks_exact(4) + .filter(|p| p[3] > 0 && p[0] > 180 && p[1] > 180 && p[2] > 180) + .count(); + assert!( + text_ink > 5, + "tooltip at font-size: 2rem must paint visible text, got {text_ink} pixels" + ); } } diff --git a/crates/rustmotion-components/src/video.rs b/crates/rustmotion-components/src/video.rs index 2220f98..5e1f4a8 100644 --- a/crates/rustmotion-components/src/video.rs +++ b/crates/rustmotion-components/src/video.rs @@ -82,8 +82,26 @@ impl Painter for Video { } } - let Ok(frame_data) = extract_video_frame(&self.src, source_time, width, height) else { - return; + let frame_data = match extract_video_frame(&self.src, source_time, width, height) { + Ok(data) => data, + Err(e) => { + // Item 3 (issue #167): decoding failures (ffmpeg missing, or + // this specific frame failing) used to be a silent `return` + // — a video component would render entirely blank with no + // trace anywhere. `paint_content` runs once per frame, so + // the warning is deduplicated per `src` via `warn_once_for` + // (the same guard `lib.rs` already uses for exactly this + // per-frame-call-site problem) instead of printing the same + // line a thousand times over a render. + if crate::warn_once_for(&format!("video-frame:{}", self.src)) { + eprintln!( + "Warning: video '{}' could not be decoded: {e}. This component will \ + render nothing for the remainder of the video.", + self.src + ); + } + return; + } }; let skia_data = skia_safe::Data::new_copy(&frame_data); if let Some(img) = skia_safe::Image::from_encoded(skia_data) { @@ -93,3 +111,76 @@ impl Painter for Video { } } } + +#[cfg(test)] +mod tests { + use super::*; + use rustmotion_core::engine::animator::AnimatedProperties; + use rustmotion_core::engine::layout_pass::BoxLayout; + use rustmotion_core::traits::PaintCtx; + + fn base_ctx() -> PaintCtx { + PaintCtx { + time: 0.0, + scene_duration: 1.0, + frame_index: 0, + fps: 30, + video_width: 100, + video_height: 100, + stagger_offset: 0.0, + } + } + + /// A failed frame extraction (bad src, or no ffmpeg) must be reported, + /// not swallowed. Pre-fix, `paint_content` never calls `warn_once_for` + /// on this path at all, so the slot for this exact src stays unclaimed + /// ("first sighting" == true) forever — this is the observable half of + /// total silence we can assert on without capturing stderr. + #[test] + fn a_failed_frame_extraction_must_claim_its_warn_once_slot() { + let missing_src = std::env::temp_dir().join(format!( + "rustmotion-video-test-missing-{}-{}.mp4", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_nanos(), + )); + let _ = std::fs::remove_file(&missing_src); + let src_str = missing_src.to_str().unwrap().to_string(); + + let video = Video { + src: src_str.clone(), + trim_start: None, + trim_end: None, + playback_rate: None, + fit: Default::default(), + volume: 1.0, + loop_video: None, + timing: Default::default(), + style: CssStyle::default(), + timeline: Vec::new(), + stagger: None, + }; + let layout = BoxLayout { + width: 40.0, + height: 40.0, + ..Default::default() + }; + let ctx = base_ctx(); + let props = AnimatedProperties::default(); + let mut surface = skia_safe::surfaces::raster_n32_premul((40, 40)).unwrap(); + { + let canvas = surface.canvas(); + video.paint_content(canvas, &layout, &props, &ctx); + } + + let key = format!("video-frame:{}", src_str); + assert!( + !crate::warn_once_for(&key), + "paint_content must have claimed this warning slot on the failed extraction \ + path — it is still unclaimed (first sighting), meaning nothing warned about \ + the failure" + ); + } +} diff --git a/crates/rustmotion-components/tests/relative_font_size.rs b/crates/rustmotion-components/tests/relative_font_size.rs new file mode 100644 index 0000000..06127b2 --- /dev/null +++ b/crates/rustmotion-components/tests/relative_font_size.rs @@ -0,0 +1,120 @@ +//! Reproduction + regression test for relative `font-size` units (`rem`/ +//! `vw`/`vh`) resolving to 0px silently (Lot B, wave S). +//! +//! Routes a `text` component through the real pipeline (box_builder → +//! run_layout → paint_tree) so both the intrinsic measurement (the box taffy +//! reserves) and the painter (what actually gets drawn) are exercised +//! together — the geometry validator's overflow checks depend on the two +//! agreeing, so a fix that only touches one side is not a real fix (see +//! `crates/rustmotion-components/tests/caption_presets.rs` for the same +//! pipeline pattern). + +use rustmotion_components::box_builder::{build_scene_with_anim, BuildAnimationCtx}; +use rustmotion_components::legacy_dispatch::LegacyPaintDispatcher; +use rustmotion_components::{ChildComponent, Component, PositionMode}; +use rustmotion_core::css::taffy_bridge::ConversionContext; +use rustmotion_core::engine::layout_pass::run_layout; +use rustmotion_core::engine::paint_pass::{paint_tree, PaintFrame}; + +const W: u32 = 400; +const H: u32 = 300; + +fn render(json: serde_json::Value) -> Vec { + let component: Component = serde_json::from_value(json).expect("deserialize component"); + let child = ChildComponent { + component, + position: Some(PositionMode::Absolute { x: 20.0, y: 20.0 }), + x: None, + y: None, + z_index: None, + bleed: false, + }; + let children = vec![child]; + + let mut surface = + skia_safe::surfaces::raster_n32_premul((W as i32, H as i32)).expect("raster surface"); + let canvas = surface.canvas(); + canvas.clear(skia_safe::Color4f::new(0.0, 0.0, 0.0, 0.0)); + + let built = build_scene_with_anim( + &children, + (W as f32, H as f32), + BuildAnimationCtx { + time: 0.5, + scene_duration: 2.0, + fps: 30, + }, + ); + let layout = run_layout( + &built.root, + (W as f32, H as f32), + &ConversionContext::default(), + ); + let dispatcher = LegacyPaintDispatcher::for_scene(&built); + let frame = PaintFrame { + time: 0.5, + frame_index: 15, + fps: 30, + video_width: W, + video_height: H, + scene_duration: 2.0, + camera: None, + }; + paint_tree(canvas, &built.root, &layout, &frame, &dispatcher); + + let row_bytes = W as usize * 4; + let mut pixels = vec![0u8; row_bytes * H as usize]; + let info = skia_safe::ImageInfo::new( + (W as i32, H as i32), + skia_safe::ColorType::RGBA8888, + skia_safe::AlphaType::Premul, + None, + ); + surface.read_pixels(&info, &mut pixels, row_bytes, (0, 0)); + pixels +} + +fn lit_pixels(buf: &[u8]) -> usize { + buf.chunks_exact(4).filter(|p| p[3] > 0).count() +} + +#[test] +fn text_with_rem_font_size_paints_visible_pixels() { + // Reproduction: `rustmotion validate` on this exact style passes (exit + // 0, "Valid scenario") with only warnings — `2rem` silently resolves to + // 0px, so `TextIntrinsic` measures a 0-height box, `paint_pass`'s + // `height <= 0.0` guard skips painting the node entirely, and the + // rendered frame has no text at all. + let json = serde_json::json!({ + "type": "text", + "content": "HELLO", + "style": { "font-size": "2rem", "color": "#FFFFFF" } + }); + let buf = render(json); + let lit = lit_pixels(&buf); + assert!( + lit > 100, + "text at font-size: 2rem must paint visible pixels (2rem = 32px against the 16px CSS \ + root default), got {lit} lit pixels — 0 before the fix, since the relative unit \ + silently resolved to 0px" + ); +} + +#[test] +fn text_with_vh_font_size_paints_visible_pixels() { + // `vh` needs the real viewport (video_height), not just the root + // font-size — a separate resolution path from `rem` inside + // `LengthContext::resolve`. + let json = serde_json::json!({ + "type": "text", + "content": "HELLO", + // 10vh of a 300px-tall frame = 30px. + "style": { "font-size": "10vh", "color": "#FFFFFF" } + }); + let buf = render(json); + let lit = lit_pixels(&buf); + assert!( + lit > 100, + "text at font-size: 10vh must paint visible pixels, got {lit} lit pixels" + ); +} diff --git a/crates/rustmotion-core/src/engine/animator.rs b/crates/rustmotion-core/src/engine/animator.rs index fb07e20..3fbf118 100644 --- a/crates/rustmotion-core/src/engine/animator.rs +++ b/crates/rustmotion-core/src/engine/animator.rs @@ -337,6 +337,26 @@ fn ease_in_out_cubic(t: f64) -> f64 { // ─── Spring solver ────────────────────────────────────────────────────────── +/// Default `rest_threshold` (fraction of the 0→1 travel) used by +/// `spring_rest_time`/the `duration` remap in `spring_value` when a +/// `SpringConfig` does not set one explicitly. 0.5% is tight enough that +/// "at rest" reads as visually still, without demanding the numeric search +/// chase an asymptote that (for a critically- or over-damped spring) is +/// never reached exactly. +pub const DEFAULT_SPRING_REST_THRESHOLD: f64 = 0.005; + +/// Hard cap, in seconds, on how far into the future `spring_settle_time` +/// searches for a rest point. A very lightly damped spring can take an +/// arbitrarily long time to decay under `rest_threshold` — in the limit +/// (`damping == 0`) it never does, oscillating forever at constant +/// amplitude — so the search needs a bound or it would not terminate. When +/// the cap is hit, the spring is reported as resting at the cap itself: a +/// defined, tested "has not settled by then" answer (see +/// `spring_duration_tests::undamped_spring_is_capped_not_infinite` and +/// `spring_duration_tests::very_lightly_damped_spring_is_also_capped_when_beyond_the_bound`) +/// rather than an unbounded loop. +pub const MAX_SPRING_SEARCH_SECONDS: f64 = 30.0; + /// Solve spring animation at time t (seconds). /// Returns a value between 0.0 and 1.0 representing progress. /// @@ -351,11 +371,51 @@ fn ease_in_out_cubic(t: f64) -> f64 { /// combinations as errors (belt), and this floor keeps the solver itself /// finite and bounded even if an out-of-band caller skips validation /// (suspenders) — see `spring_robustness_tests` below. +/// +/// `duration` (issue #167 lot E, `SpringConfig::duration`): when set, `t` is +/// linearly rescaled before it reaches the physics below — not the physical +/// parameters themselves — so that `spring_rest_time` on the *unscaled* +/// spring lands exactly on `duration`. The spring's shape (oscillation +/// count, overshoot amplitude) is entirely a function of +/// `damping`/`stiffness`/`mass`, so rescaling only the time axis preserves +/// it; see `spring_duration_tests::duration_remap_preserves_shape`. This +/// does *not* resize whatever keyframe segment `spring_value` is being +/// evaluated within — see the `duration` field's doc comment on +/// `SpringConfig` for why that is a separate, author-owned concern. pub fn spring_value(t: f64, config: &SpringConfig) -> f64 { let damping = config.damping.max(0.0); let stiffness = config.stiffness.max(1e-6); let mass = config.mass.max(1e-6); + match config.duration { + Some(duration) if duration > 0.0 => { + let threshold = spring_rest_threshold(config); + let natural_rest = spring_settle_time( + damping, + stiffness, + mass, + threshold, + MAX_SPRING_SEARCH_SECONDS, + ); + if natural_rest < 1e-9 { + // Degenerate: the spring starts at distance 1.0 from its + // target, so in practice `natural_rest` is never this + // small — fall back to unscaled rather than divide by ~0. + spring_value_raw(t, damping, stiffness, mass) + } else { + let time_scale = natural_rest / duration; + spring_value_raw(t * time_scale, damping, stiffness, mass) + } + } + _ => spring_value_raw(t, damping, stiffness, mass), + } +} + +/// The physics solver itself, unscaled by any `duration` remap. Takes +/// already-floored parameters (see `spring_value`'s constat #6 doc comment) +/// so `spring_settle_time`'s search can call it directly without redoing +/// the floor on every sample. +fn spring_value_raw(t: f64, damping: f64, stiffness: f64, mass: f64) -> f64 { let omega = (stiffness / mass).sqrt(); let zeta = damping / (2.0 * (stiffness * mass).sqrt()); @@ -379,6 +439,125 @@ pub fn spring_value(t: f64, config: &SpringConfig) -> f64 { } } +/// Lower bound on the number of samples `spring_settle_time` takes across +/// `[0, max_t]` — enough to resolve slow (critically-/over-damped) decays +/// even when the natural oscillation period doesn't drive the sample count +/// up on its own. +const SPRING_SETTLE_MIN_SAMPLES: usize = 2_000; +/// Upper bound on samples, regardless of how short the oscillation period +/// is — keeps `spring_settle_time` (called on every `spring_value` sample +/// when `duration` is set) bounded-cost for very stiff/fast springs. +const SPRING_SETTLE_MAX_SAMPLES: usize = 20_000; +/// Target sample density within one oscillation period, chosen empirically +/// (see the workstream report) to keep the coarse-then-bisect search within +/// ~0.1% of a brute-force reference across a broad random sweep of +/// damping/stiffness/mass. Shallow, near-tangential graze-and-return +/// excursions across the threshold band (a spring that dips back below the +/// line by a razor-thin margin on a secondary oscillation) can still be +/// missed — `spring_rest_time`/`spring_settle_time` are a documented +/// numeric approximation, not an exact guarantee. +const SPRING_SETTLE_SAMPLES_PER_PERIOD: f64 = 48.0; + +/// First `t >= 0` from which `spring_value_raw` stays within `threshold` of +/// its target (1.0) forever after. Implements the "mesure du repos" from +/// issue #167 lot E: `spring_value_raw` is closed-form, so a coarse scan to +/// bracket the last exceedance, refined by bisection, is enough — no need +/// to integrate anything. +/// +/// Two regimes get explicit handling (both required by the workstream +/// brief, both exercised in `spring_duration_tests`): +/// - an overdamped (or critically damped) spring never touches its target +/// exactly, only approaches it asymptotically — the scan terminates via +/// `threshold`, never via an exact equality check; +/// - a very lightly damped spring can take arbitrarily long to settle (an +/// undamped spring, `damping == 0`, never does — it oscillates forever at +/// constant amplitude). `max_t` bounds the search; if the last sample is +/// still outside `threshold`, `max_t` itself is returned — defined, +/// tested behaviour instead of an unbounded search. +fn spring_settle_time(damping: f64, stiffness: f64, mass: f64, threshold: f64, max_t: f64) -> f64 { + let threshold = threshold.max(1e-9); + let omega = (stiffness / mass).sqrt(); + let period = if omega > 1e-9 { + std::f64::consts::TAU / omega + } else { + max_t + }; + let desired_steps = (max_t / (period / SPRING_SETTLE_SAMPLES_PER_PERIOD)).ceil() as usize; + let steps = desired_steps.clamp(SPRING_SETTLE_MIN_SAMPLES, SPRING_SETTLE_MAX_SAMPLES); + let dt = max_t / steps as f64; + + let mut last_exceed_idx: usize = 0; + for i in 0..=steps { + let t = i as f64 * dt; + if (spring_value_raw(t, damping, stiffness, mass) - 1.0).abs() > threshold { + last_exceed_idx = i; + } + } + + if last_exceed_idx >= steps { + // Still exceeding at (or past) max_t: capped, "not settled". + return max_t; + } + + // Refine within (last_exceed, last_exceed + dt]: the coarse scan found + // this as the last sample outside the threshold band, so bisect for the + // point within this bracket where it steps inside for good. + let mut lo = last_exceed_idx as f64 * dt; + let mut hi = (lo + dt).min(max_t); + for _ in 0..40 { + let mid = 0.5 * (lo + hi); + if (spring_value_raw(mid, damping, stiffness, mass) - 1.0).abs() > threshold { + lo = mid; + } else { + hi = mid; + } + } + hi +} + +/// The `rest_threshold` a `SpringConfig` resolves to: the author's value if +/// set, else `DEFAULT_SPRING_REST_THRESHOLD`, floored so `spring_settle_time` +/// always has a well-defined (nonzero) target — the same belt-and-suspenders +/// pattern `spring_value` already applies to `damping`/`stiffness`/`mass`. +/// `rustmotion-cli`'s `check_spring_config` rejects non-positive or absurd +/// (`>= 1.0`) values at the author-facing layer; this floor is the +/// solver-side backstop. +fn spring_rest_threshold(config: &SpringConfig) -> f64 { + config + .rest_threshold + .unwrap_or(DEFAULT_SPRING_REST_THRESHOLD) + .max(1e-9) +} + +/// Public "measure du repos" (issue #167 lot E): the instant, in seconds, +/// at which this spring settles within `rest_threshold` of its target and +/// stays there — what `rustmotion info` surfaces so an author can size a +/// scene/preset duration around a spring instead of discovering it by +/// trial and error. +/// +/// When `config.duration` is set, this *is* that duration, exactly — that +/// is the point of the time remap `spring_value` performs (see its doc +/// comment). Otherwise it is the natural settle time computed from +/// `damping`/`stiffness`/`mass` alone via `spring_settle_time`. +pub fn spring_rest_time(config: &SpringConfig) -> f64 { + match config.duration { + Some(d) if d > 0.0 => d, + _ => { + let damping = config.damping.max(0.0); + let stiffness = config.stiffness.max(1e-6); + let mass = config.mass.max(1e-6); + let threshold = spring_rest_threshold(config); + spring_settle_time( + damping, + stiffness, + mass, + threshold, + MAX_SPRING_SEARCH_SECONDS, + ) + } + } +} + // ─── Animation resolver ───────────────────────────────────────────────────── /// Resolved animated properties for a single layer at a specific frame @@ -980,6 +1159,21 @@ fn is_motion_property(property: &str) -> bool { /// - more than 2 keyframes with identical endpoints (continuous oscillators: /// pulse, shake, float): untouched — a spring toward the same value is a /// no-op and would freeze the effect. +/// +/// `spring.duration` (issue #167 lot E) is *not* consulted here to resize +/// the keyframe pair's own span: the pair's `[delay, end]` still comes from +/// `AnimationTiming::delay`/`duration` (the same preset-level timing every +/// other easing uses), untouched by whatever `SpringConfig::duration` says. +/// `spring_value` — not this function — is where `duration` acts, by +/// rescaling the *physics* time axis it is fed. Consequently, if the +/// preset's own `duration` is shorter than `spring.duration`, the segment +/// still ends (and the property still snaps to its final keyframe value) at +/// the preset's `end`, before the spring has visually settled — exactly the +/// pre-existing behaviour for any other easing curve given too short a +/// segment. Pin `AnimationTiming::duration` (or the `keyframes` effect's own +/// keyframe span, for the other call site in `resolve_animation_value_full`) +/// to at least `spring_rest_time` to avoid that cutoff; `rustmotion info` +/// reports `spring_rest_time` for exactly this purpose. fn apply_spring_to_motion(animations: &mut [Animation], spring: &SpringConfig) { for anim in animations.iter_mut() { if !is_motion_property(&anim.property) || anim.keyframes.len() < 2 { @@ -1569,6 +1763,7 @@ fn kf_anim_spring(property: &str, t0: f64, v0: f64, t1: f64, v1: f64) -> Animati damping: 12.0, stiffness: 100.0, mass: 1.0, + ..Default::default() }), } } @@ -1582,6 +1777,7 @@ fn kf_anim_spring_underdamped(property: &str, t0: f64, v0: f64, t1: f64, v1: f64 damping: 6.0, stiffness: 120.0, mass: 1.0, + ..Default::default() }), } } @@ -1655,6 +1851,7 @@ mod spring_preset_tests { damping: 8.0, stiffness: 120.0, mass: 1.0, + ..Default::default() } } @@ -1738,6 +1935,7 @@ mod spring_preset_tests { damping: 40.0, stiffness: 100.0, mass: 1.0, + ..Default::default() }; let d = scale_at(None, 0.3); let c = scale_at(Some(overdamped), 0.3); @@ -2217,6 +2415,7 @@ mod spring_robustness_tests { damping: 10.0, stiffness: 100.0, mass: 0.0, + ..Default::default() }; for i in 0..=20 { let t = i as f64 * 0.25; @@ -2234,6 +2433,7 @@ mod spring_robustness_tests { damping: 10.0, stiffness: 0.0, mass: 1.0, + ..Default::default() }; for i in 0..=20 { let t = i as f64 * 0.25; @@ -2251,6 +2451,7 @@ mod spring_robustness_tests { damping: -20.0, stiffness: 100.0, mass: 1.0, + ..Default::default() }; let v_at_5s = spring_value(5.0, &config); assert!( @@ -2260,3 +2461,317 @@ mod spring_robustness_tests { ); } } + +#[cfg(test)] +mod spring_duration_tests { + //! Issue #167 lot E: `SpringConfig::duration` forces a spring to settle + //! (see `rest_threshold`) at exactly that many seconds by rescaling the + //! time axis fed to the physics solver; `spring_rest_time` is the + //! public "measure du repos" `rustmotion info` surfaces. + use super::*; + + /// Reference settle time via a fine linear scan of the actual + /// implemented formula (`spring_value_raw`), independent of + /// `spring_settle_time`'s coarse-then-bisect implementation — these + /// tests check the algorithm against ground truth, not against itself. + fn brute_force_settle_time( + damping: f64, + stiffness: f64, + mass: f64, + threshold: f64, + max_t: f64, + steps: usize, + ) -> f64 { + let dt = max_t / steps as f64; + let mut last_exceed = 0.0; + for i in 0..=steps { + let t = i as f64 * dt; + if (spring_value_raw(t, damping, stiffness, mass) - 1.0).abs() > threshold { + last_exceed = t; + } + } + last_exceed + } + + #[test] + fn red_phase_duration_is_ignored_by_the_raw_physical_solver() { + // Captured red-phase numbers (issue #167 lot E, before `duration` + // existed on `SpringConfig`): a spring's settle time was purely + // emergent from damping/stiffness/mass. `spring_value_raw` is + // exactly that pre-existing, unscaled solver — by construction it + // does not know about `duration`. + // + // damping=6, stiffness=120, mass=1 (the same "underdamped" preset + // this file already uses for elastic_in / kf_anim_spring_underdamped) + // at t=0.8s: spring_value_raw(0.8, 6, 120, 1) ~= 1.043467 — 4.35% + // past the target, an order of magnitude outside any reasonable + // rest_threshold (default 0.5%). An author asking this spring to + // "finish at 0.8s" got a value nowhere near rest. + let v = spring_value_raw(0.8, 6.0, 120.0, 1.0); + assert!( + (v - 1.043467).abs() < 1e-5, + "captured red-phase reference value drifted: got {v}, expected ~1.043467" + ); + assert!( + (v - 1.0).abs() > 0.04, + "red-phase claim: at t=duration the unscaled spring must still be far from rest \ + (got diff {:.6}, expected > 0.04)", + (v - 1.0).abs() + ); + } + + #[test] + fn duration_makes_the_spring_settle_exactly_there() { + let config = SpringConfig { + damping: 6.0, + stiffness: 120.0, + mass: 1.0, + duration: Some(0.8), + rest_threshold: None, + }; + let threshold = DEFAULT_SPRING_REST_THRESHOLD; + + // Green phase: the same (damping, stiffness, mass) that the + // red-phase test above showed is 4.35% off at t=0.8s without a + // `duration` must now be within `threshold` of rest at t=0.8s. + let v_at_duration = spring_value(0.8, &config); + assert!( + (v_at_duration - 1.0).abs() <= threshold, + "spring_value(0.8, ..) with duration=Some(0.8) must be within {threshold} of rest, \ + got {v_at_duration} (diff {})", + (v_at_duration - 1.0).abs() + ); + + // And it must not already be at rest well before `duration` — + // this is a genuine rescale, not "duration happens to be late + // enough not to matter". + let v_at_half = spring_value(0.4, &config); + assert!( + (v_at_half - 1.0).abs() > threshold, + "sanity: spring must not already be at rest at half of duration, got diff {}", + (v_at_half - 1.0).abs() + ); + } + + #[test] + fn spring_rest_time_returns_duration_verbatim_when_set() { + let config = SpringConfig { + damping: 6.0, + stiffness: 120.0, + mass: 1.0, + duration: Some(0.8), + rest_threshold: None, + }; + assert_eq!(spring_rest_time(&config), 0.8); + } + + #[test] + fn spring_rest_time_matches_a_brute_force_reference_without_duration() { + let cases: [(f64, f64, f64, &str); 5] = [ + (15.0, 100.0, 1.0, "default"), + (12.0, 100.0, 1.0, "kf_anim_spring"), + (6.0, 120.0, 1.0, "underdamped elastic_in-like"), + ( + 20.0, + 100.0, + 1.0, + "critically damped (damping = 2*sqrt(stiffness*mass))", + ), + (60.0, 100.0, 1.0, "overdamped"), + ]; + for (damping, stiffness, mass, label) in cases { + let config = SpringConfig { + damping, + stiffness, + mass, + duration: None, + rest_threshold: None, + }; + let threshold = DEFAULT_SPRING_REST_THRESHOLD; + let got = spring_rest_time(&config); + let reference = brute_force_settle_time( + damping, + stiffness, + mass, + threshold, + MAX_SPRING_SEARCH_SECONDS, + 400_000, + ); + let abs_err = (got - reference).abs(); + assert!( + abs_err < 0.05, + "{label}: spring_rest_time={got:.5}s vs brute-force reference={reference:.5}s \ + (|err|={abs_err:.5}s, expected < 0.05s)" + ); + } + } + + #[test] + fn overdamped_spring_never_reaches_target_exactly_but_settle_time_is_found() { + // Pitfall called out in the brief: an overdamped spring approaches + // its target asymptotically and never touches it. The search must + // terminate via `rest_threshold`, not by looking for an exact hit. + let config = SpringConfig { + damping: 200.0, + stiffness: 100.0, + mass: 1.0, + duration: None, + rest_threshold: None, + }; + let t = spring_rest_time(&config); + assert!( + t > 0.0 && t < MAX_SPRING_SEARCH_SECONDS, + "expected a finite, non-degenerate settle time, got {t}" + ); + + // Confirm it genuinely never hits exactly 1.0 — the asymptotic + // property `rest_threshold` exists to work around. + for i in 1..=200 { + let sample_t = t + i as f64 * 0.1; + let v = spring_value_raw(sample_t, 200.0, 100.0, 1.0); + assert_ne!( + v, 1.0, + "an overdamped spring must never hit its target exactly (t={sample_t})" + ); + } + } + + #[test] + fn undamped_spring_is_capped_not_infinite() { + // Pitfall: damping=0 means the spring oscillates forever at + // constant amplitude — it never settles. The search must return the + // defined cap (`MAX_SPRING_SEARCH_SECONDS`), not loop forever. + let config = SpringConfig { + damping: 0.0, + stiffness: 100.0, + mass: 1.0, + duration: None, + rest_threshold: None, + }; + let t = spring_rest_time(&config); + assert_eq!( + t, MAX_SPRING_SEARCH_SECONDS, + "an undamped spring must be reported as capped at the search bound, got {t}" + ); + } + + #[test] + fn very_lightly_damped_spring_is_also_capped_when_beyond_the_bound() { + // Not literally undamped, but damped so lightly it does not reach a + // 0.5% rest threshold within the search bound — same defined-cap + // behaviour as the fully undamped case, exercised with nonzero + // damping so the `zeta == 0` special case isn't the only path + // that's actually bounded. + let config = SpringConfig { + damping: 0.05, + stiffness: 100.0, + mass: 1.0, + duration: None, + rest_threshold: None, + }; + let t = spring_rest_time(&config); + assert_eq!( + t, MAX_SPRING_SEARCH_SECONDS, + "expected the search to hit its cap, got {t}" + ); + } + + #[test] + fn duration_remap_preserves_shape() { + // The whole point of a spring's `duration` is to keep its shape — + // oscillation count, overshoot amplitude — and only rescale how + // fast it plays back. Compare the natural (no-duration) curve to a + // duration-remapped curve of the *same* underlying spring, sampled + // at matching fractions of each one's own settle time: if the remap + // were instead clipping the tail (shortening, not rescaling), these + // would diverge. + let damping = 6.0; + let stiffness = 120.0; + let mass = 1.0; + let natural = SpringConfig { + damping, + stiffness, + mass, + duration: None, + rest_threshold: None, + }; + let natural_rest = spring_rest_time(&natural); + + let pinned_duration = 2.5; // deliberately different from natural_rest + let pinned = SpringConfig { + damping, + stiffness, + mass, + duration: Some(pinned_duration), + rest_threshold: None, + }; + + let mut natural_overshoots = 0; + let mut pinned_overshoots = 0; + let mut max_natural_overshoot = 0.0_f64; + let mut max_pinned_overshoot = 0.0_f64; + let mut prev_natural_over = false; + let mut prev_pinned_over = false; + + for i in 0..=1000 { + let frac = i as f64 / 1000.0; + let v_natural = spring_value(frac * natural_rest, &natural); + let v_pinned = spring_value(frac * pinned_duration, &pinned); + + // Same fraction of each spring's own settle time must produce + // the same progress value — that is the shape being preserved, + // only the clock speed differs. + assert!( + (v_natural - v_pinned).abs() < 1e-9, + "shape mismatch at fraction {frac}: natural={v_natural} pinned={v_pinned}" + ); + + let natural_over = v_natural > 1.0; + if natural_over && !prev_natural_over { + natural_overshoots += 1; + } + prev_natural_over = natural_over; + max_natural_overshoot = max_natural_overshoot.max(v_natural - 1.0); + + let pinned_over = v_pinned > 1.0; + if pinned_over && !prev_pinned_over { + pinned_overshoots += 1; + } + prev_pinned_over = pinned_over; + max_pinned_overshoot = max_pinned_overshoot.max(v_pinned - 1.0); + } + + assert!( + natural_overshoots > 0, + "expected this underdamped spring to overshoot at least once" + ); + assert_eq!( + natural_overshoots, pinned_overshoots, + "oscillation count must be identical with/without duration" + ); + assert!( + (max_natural_overshoot - max_pinned_overshoot).abs() < 1e-9, + "overshoot amplitude must be identical with/without duration: natural={max_natural_overshoot} pinned={max_pinned_overshoot}" + ); + } + + #[test] + fn duration_does_not_change_delay_semantics() { + // `spring_value`'s `t` argument is already local to the enclosing + // segment (time since the segment/keyframe start — `delay` is + // baked into where that segment begins, upstream of this call). + // `duration` must not reinterpret that: t=0 must still be the + // spring's own start regardless of `duration`. + let config = SpringConfig { + damping: 6.0, + stiffness: 120.0, + mass: 1.0, + duration: Some(0.8), + rest_threshold: None, + }; + assert_eq!( + spring_value(0.0, &config), + spring_value_raw(0.0, 6.0, 120.0, 1.0) + ); + } +} diff --git a/crates/rustmotion-core/src/engine/renderer/assets.rs b/crates/rustmotion-core/src/engine/renderer/assets.rs index 18ab631..f6052a1 100644 --- a/crates/rustmotion-core/src/engine/renderer/assets.rs +++ b/crates/rustmotion-core/src/engine/renderer/assets.rs @@ -1,3 +1,4 @@ +use std::path::{Path, PathBuf}; use std::sync::{Arc, OnceLock}; use dashmap::DashMap; @@ -36,7 +37,87 @@ pub fn gif_cache() -> &'static GifCacheMap { // ─── Icon fetching ────────────────────────────────────────────────────────── +/// How much larger than the *target* (layout) size icons are rasterized, so +/// Skia's downscale keeps edges crisp under sub-pixel positioning and minor +/// scale animations. +/// +/// This lives here — not as a local `const` inside the painter — because it +/// must feed the exact same computation [`icon_cache_key`] uses. See that +/// function's doc for why (issue #166). +pub const ICON_OVERSAMPLE: u32 = 2; + +/// Single source of truth for both the oversampled rasterization size and +/// the [`asset_cache`] key used for a given icon at a given *target* +/// (layout) size. Returns `(render_width, render_height, cache_key)`. +/// +/// # Issue #166 +/// +/// Before this function existed, `icon.rs`'s painter and `preload.rs`'s +/// prefetcher each built the cache key from their own inlined `format!`. +/// The painter multiplied the target size by [`ICON_OVERSAMPLE`] before +/// building the key; the preloader did not. For a 40×40 icon the painter +/// looked up `"icon:...:80x80"` while the preloader could only ever have +/// written `"icon:...:40x40"` — the two keys could never collide, so +/// `prefetch_icons` never once avoided a duplicate network fetch, and (had +/// its rasterization size matched the key by coincidence) would have cached +/// a bitmap at half the resolution the painter actually samples. +/// +/// Both call sites now go through this one function, so they cannot drift +/// apart again — fixing the key without also fixing the raster size (or +/// vice versa) is no longer expressible. +pub fn icon_cache_key(icon: &str, color: &str, target_w: u32, target_h: u32) -> (u32, u32, String) { + let render_w = target_w.max(1) * ICON_OVERSAMPLE; + let render_h = target_h.max(1) * ICON_OVERSAMPLE; + let cache_key = format!("icon:{icon}:{color}:{render_w}x{render_h}"); + (render_w, render_h, cache_key) +} + +/// Returns the icon disk-cache directory: `~/.cache/rustmotion/icons`. +/// +/// Mirrors [`google_fonts::font_cache_dir`](super::google_fonts::font_cache_dir), +/// which does the same thing for downloaded font files — see that module +/// for the disk-cache-before-network shape this was lifted from. +pub fn icon_cache_dir() -> PathBuf { + #[cfg(target_os = "windows")] + let base = std::env::var_os("LOCALAPPDATA") + .map(PathBuf::from) + .unwrap_or_else(|| PathBuf::from(".")); + + #[cfg(not(target_os = "windows"))] + let base = std::env::var_os("HOME") + .map(|h| PathBuf::from(h).join(".cache")) + .unwrap_or_else(|| PathBuf::from(".cache")); + + base.join("rustmotion").join("icons") +} + +/// Deterministic on-disk file name for a given (icon, color, size). Icon ids +/// contain `:` (`"lucide:home"`); replaced so the id survives as a legible +/// file name instead of being hashed away. +fn icon_cache_file(cache_dir: &Path, icon: &str, color: &str, width: u32, height: u32) -> PathBuf { + let slug = icon.replace(':', "_"); + let hex_color = color.trim_start_matches('#').to_lowercase(); + cache_dir.join(format!("{slug}-{hex_color}-{width}x{height}.svg")) +} + +/// Fetch an icon's SVG bytes, checking the on-disk cache first and falling +/// back to the Iconify API on a miss. Same public signature as before this +/// fix — every existing caller (icon.rs, preload.rs, badge.rs, +/// notification.rs, list.rs, stat.rs) gets the disk cache for free. pub fn fetch_icon_svg(icon: &str, color: &str, width: u32, height: u32) -> Result> { + fetch_icon_svg_in(icon, color, width, height, &icon_cache_dir()) +} + +/// Core of [`fetch_icon_svg`], with the cache directory injectable so tests +/// can exercise the cache-hit path without touching `$HOME` or the network — +/// mirrors `google_fonts::resolve_google_font`'s `cache_dir` parameter. +pub fn fetch_icon_svg_in( + icon: &str, + color: &str, + width: u32, + height: u32, + cache_dir: &Path, +) -> Result> { let (prefix, name) = icon.split_once(':') .ok_or_else(|| RustmotionError::InvalidIconFormat { @@ -45,6 +126,14 @@ pub fn fetch_icon_svg(icon: &str, color: &str, width: u32, height: u32) -> Resul let hex_color = color.trim_start_matches('#'); let width = width.max(1); let height = height.max(1); + + let cache_file = icon_cache_file(cache_dir, icon, color, width, height); + if let Ok(data) = std::fs::read(&cache_file) { + if !data.is_empty() { + return Ok(data); + } + } + let url = format!( "https://api.iconify.design/{}/{}.svg?color=%23{}&width={}&height={}", prefix, name, hex_color, width, height @@ -62,6 +151,14 @@ pub fn fetch_icon_svg(icon: &str, color: &str, width: u32, height: u32) -> Resul icon: icon.to_string(), reason: e.to_string(), })?; + + // Best-effort disk-cache write: failing to persist must not fail a + // fetch that already succeeded (matches the in-memory `asset_cache`'s + // existing tolerance for a cache that just doesn't get populated). + if std::fs::create_dir_all(cache_dir).is_ok() { + let _ = std::fs::write(&cache_file, &body); + } + Ok(body) } @@ -97,6 +194,27 @@ pub fn find_closest_frame( Some((rgba, w, h)) } +/// Returns `true` if `ffmpeg` is on `PATH`. +/// +/// Single source of truth for "should we even attempt to shell out to +/// ffmpeg" — `extract_video_frame` below already surfaces a missing binary +/// as `RustmotionError::FfmpegSpawn` on first use, but callers that decode +/// many frames up front (`preload::preextract_video_frames`) want to check +/// once and print one clear warning instead of failing identically once per +/// frame. Mirrors the `ffmpeg_available` helper the `rustmotion` crate's +/// `encode::video_audio` module already uses for the embedded-audio +/// extraction path (PR #151) — same check, same reasoning, different asset +/// kind. +pub fn ffmpeg_available() -> bool { + std::process::Command::new("ffmpeg") + .args(["-version"]) + .stdout(std::process::Stdio::null()) + .stderr(std::process::Stdio::null()) + .status() + .map(|s| s.success()) + .unwrap_or(false) +} + pub fn extract_video_frame(src: &str, time: f64, width: u32, height: u32) -> Result> { let output = std::process::Command::new("ffmpeg") .args([ @@ -130,3 +248,132 @@ pub fn extract_video_frame(src: &str, time: f64, width: u32, height: u32) -> Res Ok(output.stdout) } + +#[cfg(test)] +mod tests { + use super::*; + + fn unique_temp_dir(name: &str) -> PathBuf { + let dir = std::env::temp_dir() + .join("rustmotion-test-icons") + .join(format!( + "{name}-{}-{}", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_nanos() + )); + std::fs::create_dir_all(&dir).expect("create test cache dir"); + dir + } + + // ── icon_cache_key: the fix for issue #166 ────────────────────────────── + + /// Regression for issue #166: `icon.rs`'s painter and `preload.rs`'s + /// prefetcher used to build the cache key independently and disagreed + /// (painter oversampled, preloader did not — see the RED-phase output + /// this test replaced: `"icon:lucide:home:#FFFFFF:80x80"` vs + /// `"icon:lucide:home:#FFFFFF:40x40"`). Both call sites now go through + /// this one function, so there is only one formula left to test. + #[test] + fn oversamples_the_target_size_and_keys_on_the_oversampled_size() { + let (render_w, render_h, key) = icon_cache_key("lucide:home", "#FFFFFF", 40, 40); + assert_eq!(render_w, 40 * ICON_OVERSAMPLE); + assert_eq!(render_h, 40 * ICON_OVERSAMPLE); + assert_eq!(key, "icon:lucide:home:#FFFFFF:80x80"); + } + + #[test] + fn zero_target_size_is_clamped_to_at_least_one_before_oversampling() { + let (render_w, render_h, _key) = icon_cache_key("lucide:home", "#FFFFFF", 0, 0); + assert_eq!(render_w, ICON_OVERSAMPLE); + assert_eq!(render_h, ICON_OVERSAMPLE); + } + + #[test] + fn distinct_icons_or_colors_never_collide() { + let (_, _, key_a) = icon_cache_key("lucide:home", "#FFFFFF", 40, 40); + let (_, _, key_b) = icon_cache_key("lucide:home", "#000000", 40, 40); + let (_, _, key_c) = icon_cache_key("lucide:settings", "#FFFFFF", 40, 40); + assert_ne!(key_a, key_b); + assert_ne!(key_a, key_c); + } + + // ── fetch_icon_svg_in: disk cache (issue #166 item 2) ──────────────────── + + #[test] + fn disk_cache_hit_returns_bytes_without_touching_the_network() { + let cache_dir = unique_temp_dir("cache-hit"); + let icon = "test-suite:offline-icon"; + let color = "#ABCDEF"; + let (w, h) = (48, 48); + let svg_bytes = b"fake cached icon for the test suite".to_vec(); + + let cache_file = icon_cache_file(&cache_dir, icon, color, w, h); + std::fs::write(&cache_file, &svg_bytes).unwrap(); + + // If this ever fell through to the network, either the test host is + // offline (fast, deterministic `IconFetch` error — `unwrap` panics + // clearly) or "test-suite:offline-icon" 404s upstream (same + // outcome). A silent pass here means the cache was genuinely hit. + let result = fetch_icon_svg_in(icon, color, w, h, &cache_dir).expect("cache hit"); + assert_eq!(result, svg_bytes); + } + + #[test] + fn disk_cache_is_keyed_by_icon_color_and_size() { + let cache_dir = unique_temp_dir("cache-keying"); + let a = icon_cache_file(&cache_dir, "lucide:home", "#FFFFFF", 80, 80); + let b = icon_cache_file(&cache_dir, "lucide:home", "#000000", 80, 80); + let c = icon_cache_file(&cache_dir, "lucide:home", "#FFFFFF", 40, 40); + assert_ne!(a, b, "different colors must not share a cache file"); + assert_ne!(a, c, "different sizes must not share a cache file"); + } + + #[test] + fn missing_colon_fails_fast_without_touching_disk_or_network() { + let cache_dir = unique_temp_dir("invalid-format"); + let result = fetch_icon_svg_in("not-a-valid-icon-id", "#FFFFFF", 40, 40, &cache_dir); + assert!(matches!( + result, + Err(RustmotionError::InvalidIconFormat { .. }) + )); + } + + // Live network test — mirrors `google_fonts`'s `live_fetch_inter_400`: + // excluded from normal runs, exercised manually when touching this path. + #[test] + #[ignore = "requires network access"] + fn live_fetch_writes_through_to_the_disk_cache() { + let cache_dir = unique_temp_dir("live-fetch"); + let icon = "lucide:home"; + let color = "#FFFFFF"; + let (w, h) = (32, 32); + + let first = fetch_icon_svg_in(icon, color, w, h, &cache_dir).expect("live fetch"); + assert!(!first.is_empty()); + + let cache_file = icon_cache_file(&cache_dir, icon, color, w, h); + assert!( + cache_file.exists(), + "a successful live fetch must be persisted to disk" + ); + + // A second call must be served from disk. `disk_cache_hit_returns_ + // bytes_without_touching_the_network` already proves the mechanism + // in isolation; this just confirms the live-written file round-trips. + let second = fetch_icon_svg_in(icon, color, w, h, &cache_dir).expect("cache hit"); + assert_eq!(first, second); + } + + // ── ffmpeg_available ────────────────────────────────────────────────── + + #[test] + fn ffmpeg_available_does_not_panic_either_way() { + // Not asserting the actual bool: whether ffmpeg is installed depends + // on the host. This just proves the probe itself cannot panic or + // hang the preload path that depends on it (item 3). + let _ = ffmpeg_available(); + } +} diff --git a/crates/rustmotion-core/src/schema/animation.rs b/crates/rustmotion-core/src/schema/animation.rs index ebd90d9..9d8cf02 100644 --- a/crates/rustmotion-core/src/schema/animation.rs +++ b/crates/rustmotion-core/src/schema/animation.rs @@ -86,6 +86,36 @@ pub struct SpringConfig { pub stiffness: f64, #[serde(default = "default_mass")] pub mass: f64, + /// Force the spring to *visually* settle at exactly this many seconds + /// (issue #167 lot E), instead of leaving the settle time as an + /// emergent, hard-to-predict consequence of `damping`/`stiffness`/ + /// `mass`. Implemented as a linear rescale of the time axis fed to the + /// physics solver (`engine::animator::spring_value`): the spring's + /// *shape* — number of oscillations, overshoot amplitude — is a + /// function of `damping`/`stiffness`/`mass` alone and is unchanged; + /// only how fast that shape plays back changes. `None` (default) + /// leaves the natural, emergent settle time in place. + /// + /// This does not resize the enclosing keyframe segment (the + /// `delay`/`duration` on the surrounding `AnimationTiming`, or the + /// author's own keyframe times on a `keyframes` effect): those still + /// decide when the segment starts and how long it spans. Set that + /// enclosing span to at least `duration` (`rustmotion info` reports the + /// computed settle time so you don't have to guess), or the segment's + /// own end will still cut the spring's motion short. + #[serde(default)] + pub duration: Option, + /// How close to the target counts as "at rest", as a fraction of the + /// total 0→1 travel (e.g. `0.01` = 1%). Defaults to + /// `engine::animator::DEFAULT_SPRING_REST_THRESHOLD` (0.5%) when unset. + /// Read by `engine::animator::spring_rest_time` — the "how long until + /// this spring settles" measurement `rustmotion info` surfaces — and, + /// when `duration` is set, by the remap above to know what "settled" + /// means. A critically- or over-damped spring approaches its target + /// asymptotically and never reaches it exactly, which is precisely why + /// this threshold exists. + #[serde(default)] + pub rest_threshold: Option, } impl Default for SpringConfig { @@ -94,6 +124,8 @@ impl Default for SpringConfig { damping: 15.0, stiffness: 100.0, mass: 1.0, + duration: None, + rest_threshold: None, } } } diff --git a/crates/rustmotion-core/tests/spring_duration.rs b/crates/rustmotion-core/tests/spring_duration.rs new file mode 100644 index 0000000..90e80e9 --- /dev/null +++ b/crates/rustmotion-core/tests/spring_duration.rs @@ -0,0 +1,81 @@ +//! Issue #167 lot E: `SpringConfig::duration`/`rest_threshold` and +//! `engine::animator::spring_rest_time` are meant to be used from outside +//! `rustmotion-core` (by `rustmotion-cli`'s `info` command, in particular). +//! This is a black-box check that the public surface actually works end to +//! end — the white-box coverage (settle-time correctness against a +//! brute-force reference, over/under-damped edge cases, shape preservation) +//! lives in `crates/rustmotion-core/src/engine/animator.rs`'s +//! `spring_duration_tests` module, since it needs access to private solver +//! internals that this crate does not expose. + +use rustmotion_core::engine::animator::{spring_rest_time, spring_value}; +use rustmotion_core::schema::SpringConfig; + +fn spring(damping: f64, stiffness: f64, mass: f64, duration: Option) -> SpringConfig { + SpringConfig { + damping, + stiffness, + mass, + duration, + rest_threshold: None, + } +} + +#[test] +fn spring_config_duration_and_rest_threshold_default_to_none() { + let config = SpringConfig::default(); + assert!(config.duration.is_none()); + assert!(config.rest_threshold.is_none()); +} + +#[test] +fn spring_config_duration_round_trips_through_json() { + let json = serde_json::json!({ + "damping": 8.0, + "stiffness": 120.0, + "mass": 1.0, + "duration": 0.8, + "rest_threshold": 0.01 + }); + let config: SpringConfig = serde_json::from_value(json).expect("valid SpringConfig"); + assert_eq!(config.duration, Some(0.8)); + assert_eq!(config.rest_threshold, Some(0.01)); +} + +#[test] +fn a_spring_without_duration_settles_on_its_own_schedule() { + // damping=6, stiffness=120, mass=1: the same underdamped spring used + // throughout the crate's internal tests (elastic_in / kf_anim_spring's + // sibling). Its natural settle time is a couple seconds, not 0.8s. + let config = spring(6.0, 120.0, 1.0, None); + let natural_rest = spring_rest_time(&config); + assert!( + natural_rest > 1.0, + "expected a natural settle time well past 0.8s for this lightly damped spring, got {natural_rest}" + ); +} + +#[test] +fn pinning_duration_moves_the_settle_point_there() { + let config = spring(6.0, 120.0, 1.0, Some(0.8)); + assert_eq!(spring_rest_time(&config), 0.8); + + let v = spring_value(0.8, &config); + assert!( + (v - 1.0).abs() <= 0.005, + "expected the pinned spring to be at rest (within the default 0.5% threshold) at \ + t=duration, got value {v}" + ); +} + +#[test] +fn two_different_pinned_durations_both_settle_exactly_where_asked() { + for duration in [0.3, 0.8, 1.5, 3.0] { + let config = spring(6.0, 120.0, 1.0, Some(duration)); + let v = spring_value(duration, &config); + assert!( + (v - 1.0).abs() <= 0.005, + "duration={duration}: expected value at t=duration to be within threshold of rest, got {v}" + ); + } +} diff --git a/crates/rustmotion/src/encode/video/ffmpeg.rs b/crates/rustmotion/src/encode/video/ffmpeg.rs index 2e33d19..f3e211e 100644 --- a/crates/rustmotion/src/encode/video/ffmpeg.rs +++ b/crates/rustmotion/src/encode/video/ffmpeg.rs @@ -1,4 +1,5 @@ use rayon::prelude::*; +use std::collections::HashSet; use std::io::Write; use std::sync::atomic::{AtomicU32, Ordering}; @@ -10,6 +11,153 @@ use crate::schema::ResolvedScenario as Scenario; use super::tasks::{build_frame_tasks, render_frame_task}; use super::EncodeProgress; +/// A hardware encoder family ffmpeg can drive, in probe priority order. +/// Deliberately not gated by `cfg(target_os)`: the machine that compiled +/// rustmotion is not necessarily the machine that will run it, a macOS box +/// can have a VideoToolbox-less ffmpeg build, and a Linux box can have an +/// nvenc-capable ffmpeg without a working NVIDIA driver. `probe_ffmpeg_encoders` +/// asks the actual binary instead of guessing from the target triple. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum HwEncoderFamily { + VideoToolbox, + Nvenc, + Qsv, + Amf, +} + +impl HwEncoderFamily { + const ALL: [HwEncoderFamily; 4] = [ + HwEncoderFamily::VideoToolbox, + HwEncoderFamily::Nvenc, + HwEncoderFamily::Qsv, + HwEncoderFamily::Amf, + ]; + + /// The concrete ffmpeg encoder name for this family + base codec, or + /// `None` when this family has no hardware path for that codec. vp9 and + /// prores stay software-only here: their hardware paths are far less + /// standard across ffmpeg builds than h264/h265's, and getting one + /// wrong means a confusing ffmpeg failure instead of a clean fallback. + fn encoder_name(self, codec: &str) -> Option<&'static str> { + use HwEncoderFamily::*; + match (self, codec) { + (VideoToolbox, "h264") => Some("h264_videotoolbox"), + (VideoToolbox, "h265" | "hevc") => Some("hevc_videotoolbox"), + (Nvenc, "h264") => Some("h264_nvenc"), + (Nvenc, "h265" | "hevc") => Some("hevc_nvenc"), + (Qsv, "h264") => Some("h264_qsv"), + (Qsv, "h265" | "hevc") => Some("hevc_qsv"), + (Amf, "h264") => Some("h264_amf"), + (Amf, "h265" | "hevc") => Some("hevc_amf"), + _ => None, + } + } +} + +/// What `ffmpeg_args` should do about hardware acceleration, decided once +/// up front and passed in as a plain value. Kept separate from the probe +/// (machine-dependent I/O, see `probe_ffmpeg_encoders`) so the *decision* +/// — which family to pick, and why not when none applies — is a pure +/// function (`select_hardware_encoder`) testable with a fake availability +/// set, no ffmpeg binary required. +#[derive(Debug, Clone, PartialEq, Eq)] +enum HardwareSelection { + /// Use this concrete ffmpeg encoder (e.g. "h264_videotoolbox"). + Use(String), + /// `--hardware-acceleration` was not requested. + NotRequested, + /// Requested, but this codec/transparency combination has no hardware + /// path at all — no known hardware encoder here produces an alpha + /// channel, and vp9/prores have no hardware family wired in. + Unsupported { reason: String }, + /// Requested, and the codec supports it in principle, but this + /// machine's `ffmpeg -encoders` didn't list any of the candidates. + Unavailable { tried: Vec }, +} + +/// Decide which hardware encoder (if any) to use. Pure: everything +/// machine-dependent comes in through `is_available`, so this is exercised +/// in tests with a fake set instead of a real probe. +fn select_hardware_encoder( + requested: bool, + codec: &str, + transparent: bool, + is_available: impl Fn(&str) -> bool, +) -> HardwareSelection { + if !requested { + return HardwareSelection::NotRequested; + } + if transparent { + return HardwareSelection::Unsupported { + reason: "no supported hardware encoder produces an alpha channel".to_string(), + }; + } + let mut tried = Vec::new(); + let mut any_family_supports_codec = false; + for family in HwEncoderFamily::ALL { + if let Some(name) = family.encoder_name(codec) { + any_family_supports_codec = true; + if is_available(name) { + return HardwareSelection::Use(name.to_string()); + } + tried.push(name.to_string()); + } + } + if !any_family_supports_codec { + return HardwareSelection::Unsupported { + reason: format!("no known hardware encoder exists for codec '{codec}'"), + }; + } + HardwareSelection::Unavailable { tried } +} + +/// Parse the encoder names out of `ffmpeg -encoders` output. Pure — the +/// real probe (`probe_ffmpeg_encoders`) is the only caller that touches a +/// process; this half is exercised with a captured sample of real ffmpeg +/// output, no binary required. +/// +/// Each encoder line looks like ` V..... h264_videotoolbox VideoToolbox +/// H.264 Encoder` (a flags column, the name, then a free-text description); +/// the legend above it looks like ` V..... = Video`, which has the same +/// flags shape but a bare `=` where a name would be — filtered out +/// explicitly rather than relied on to fail some other check. +fn parse_encoder_names(text: &str) -> HashSet { + text.lines() + .filter_map(|line| { + // `split_whitespace` already skips leading whitespace. + let mut parts = line.split_whitespace(); + let flags = parts.next()?; + if flags.len() < 2 || !flags.chars().all(|c| c == '.' || c.is_ascii_uppercase()) { + return None; + } + let name = parts.next()?; + if name.is_empty() || !name.chars().all(|c| c.is_ascii_alphanumeric() || c == '_') { + return None; + } + Some(name.to_string()) + }) + .collect() +} + +/// Ask this machine's actual ffmpeg what it offers, rather than assuming +/// from the compiled target platform (see `HwEncoderFamily`'s doc comment +/// for why that assumption is unsafe). Returns an empty set — never an +/// error — when ffmpeg can't be run or produces unexpected output: an +/// empty set makes `select_hardware_encoder` report `Unavailable`, which +/// falls back to software. Probing must never be the reason an encode that +/// would otherwise have worked in software fails outright. +fn probe_ffmpeg_encoders() -> HashSet { + let output = std::process::Command::new("ffmpeg") + .args(["-hide_banner", "-encoders"]) + .output(); + match output { + Ok(out) if out.status.success() => { + parse_encoder_names(&String::from_utf8_lossy(&out.stdout)) + } + _ => HashSet::new(), + } +} + /// Assemble FFmpeg's argument vector. /// /// The order is load-bearing. FFmpeg parses argv positionally: an option applies @@ -30,6 +178,7 @@ fn ffmpeg_args( codec: &str, crf_val: u8, transparent: bool, + hw_encoder: Option<&str>, audio_input: Option<&str>, output_path: &str, ) -> Vec { @@ -68,44 +217,68 @@ fn ffmpeg_args( without } }; - match codec { - "h265" | "hevc" => { - push( - &["-c:v", "libx265", "-crf", &crf, "-preset", "medium"], - &mut args, - ); - push(&["-pix_fmt", alpha_fmt("yuva420p", "yuv420p")], &mut args); - } - "vp9" => { - push( - &["-c:v", "libvpx-vp9", "-crf", &crf, "-b:v", "0"], - &mut args, - ); - push(&["-pix_fmt", alpha_fmt("yuva420p", "yuv420p")], &mut args); - } - "prores" => { - push(&["-c:v", "prores_ks", "-profile:v", "4"], &mut args); - push( - &["-pix_fmt", alpha_fmt("yuva444p10le", "yuv422p10le")], - &mut args, - ); - } - _ => { - push( - &[ - "-c:v", - "libx264", - "-crf", - &crf, - "-preset", - "medium", - "-profile:v", - "high10", - "-pix_fmt", - "yuv420p10le", - ], - &mut args, - ); + if let Some(hw_name) = hw_encoder { + // Hardware encoders are quality/bitrate-driven, not CRF-driven — + // VideoToolbox reasons in `-q:v`, NVENC in `-cq`/`-b:v`, QSV/AMF in + // `-global_quality`/`-b:v` — and the mapping between "CRF 23" and + // each of those is not a clean, verifiable translation. Emitting + // none of them and letting the encoder use its own default rate + // control is more honest than inventing one; `check_crf` tells the + // caller up front that `--crf` has no effect on this path. + // + // Likewise `-preset`/`-profile:v` are libx264/libx265 AVOptions — + // several hardware encoders (VideoToolbox in particular) reject an + // unrecognized option outright and abort, so the software knobs are + // not reused here at all, not even as a best-effort translation. + // + // None of the families wired into `HwEncoderFamily` support an + // alpha channel, so this branch always targets a fixed opaque + // `yuv420p` — `select_hardware_encoder` already refuses to select a + // hardware encoder when `transparent` is set, forcing the software + // branch below instead, so `transparent` is never silently dropped + // here. + push(&["-c:v", hw_name], &mut args); + push(&["-pix_fmt", "yuv420p"], &mut args); + } else { + match codec { + "h265" | "hevc" => { + push( + &["-c:v", "libx265", "-crf", &crf, "-preset", "medium"], + &mut args, + ); + push(&["-pix_fmt", alpha_fmt("yuva420p", "yuv420p")], &mut args); + } + "vp9" => { + push( + &["-c:v", "libvpx-vp9", "-crf", &crf, "-b:v", "0"], + &mut args, + ); + push(&["-pix_fmt", alpha_fmt("yuva420p", "yuv420p")], &mut args); + } + "prores" => { + push(&["-c:v", "prores_ks", "-profile:v", "4"], &mut args); + push( + &["-pix_fmt", alpha_fmt("yuva444p10le", "yuv422p10le")], + &mut args, + ); + } + _ => { + push( + &[ + "-c:v", + "libx264", + "-crf", + &crf, + "-preset", + "medium", + "-profile:v", + "high10", + "-pix_fmt", + "yuv420p10le", + ], + &mut args, + ); + } } } @@ -117,7 +290,11 @@ fn ffmpeg_args( args } -/// Encode using FFmpeg subprocess (for h265, vp9, prores, webm, mov, transparency) +/// Encode using FFmpeg subprocess (for h265, vp9, prores, webm, mov, transparency). +/// +/// Software-only. Kept with its original signature so existing callers +/// (the studio's exporter among them) are unaffected by hardware +/// acceleration support; see [`encode_with_ffmpeg_hw`] for the switch. pub fn encode_with_ffmpeg( scenario: &Scenario, output_path: &str, @@ -125,6 +302,36 @@ pub fn encode_with_ffmpeg( codec: &str, crf: Option, transparent: bool, + on_progress: Option<&mut dyn FnMut(EncodeProgress)>, +) -> Result<()> { + encode_with_ffmpeg_hw( + scenario, + output_path, + quiet, + codec, + crf, + transparent, + false, + on_progress, + ) +} + +/// Same as [`encode_with_ffmpeg`], with an opt-in `hardware_acceleration` +/// switch. When set, probes this machine's ffmpeg for a matching hardware +/// encoder (see `select_hardware_encoder` / `probe_ffmpeg_encoders`) and +/// uses it if found; otherwise — or when the codec/transparency combination +/// has no hardware path at all — falls back to the software encoder and +/// says so on stderr unless `quiet`. Never fails just because hardware +/// acceleration was requested but unavailable. +#[allow(clippy::too_many_arguments)] +pub fn encode_with_ffmpeg_hw( + scenario: &Scenario, + output_path: &str, + quiet: bool, + codec: &str, + crf: Option, + transparent: bool, + hardware_acceleration: bool, mut on_progress: Option<&mut dyn FnMut(EncodeProgress)>, ) -> Result<()> { let config = &scenario.video; @@ -189,6 +396,49 @@ pub fn encode_with_ffmpeg( // Build FFmpeg command let crf_val = crf.unwrap_or(23); + // Probing shells out to `ffmpeg -encoders`, so it only runs when + // hardware acceleration was actually requested — an unconditional probe + // would pay that cost on every encode for nothing. + let available_encoders = if hardware_acceleration { + Some(probe_ffmpeg_encoders()) + } else { + None + }; + let hw_selection = select_hardware_encoder(hardware_acceleration, codec, transparent, |name| { + available_encoders + .as_ref() + .is_some_and(|set| set.contains(name)) + }); + let hw_encoder = match hw_selection { + HardwareSelection::Use(name) => { + if !quiet { + eprintln!("Hardware acceleration: using {name}"); + } + Some(name) + } + HardwareSelection::NotRequested => None, + HardwareSelection::Unsupported { reason } => { + if !quiet { + eprintln!( + "Hardware acceleration requested but not applicable here ({reason}); \ + continuing with the software encoder." + ); + } + None + } + HardwareSelection::Unavailable { tried } => { + if !quiet { + eprintln!( + "Hardware acceleration requested but this machine's ffmpeg does not offer \ + any of the candidate encoders (tried: {}); continuing with the software \ + encoder.", + tried.join(", ") + ); + } + None + } + }; + let mut cmd = std::process::Command::new("ffmpeg"); cmd.args(ffmpeg_args( width, @@ -197,6 +447,7 @@ pub fn encode_with_ffmpeg( codec, crf_val, transparent, + hw_encoder.as_deref(), audio_input.as_deref(), output_path, )); @@ -339,7 +590,7 @@ pub fn encode_with_ffmpeg( #[cfg(test)] mod tests { - use super::ffmpeg_args; + use super::{ffmpeg_args, parse_encoder_names, select_hardware_encoder, HardwareSelection}; /// Every option that describes the *output* has to sit after the last `-i`. /// Put one before it and ffmpeg attaches it to the following input instead, @@ -357,7 +608,17 @@ mod tests { #[test] fn the_audio_input_is_declared_before_every_output_option() { for codec in ["h264", "h265", "vp9", "prores"] { - let args = ffmpeg_args(320, 240, 30, codec, 23, false, Some("/tmp/a.raw"), "o.mp4"); + let args = ffmpeg_args( + 320, + 240, + 30, + codec, + 23, + false, + None, + Some("/tmp/a.raw"), + "o.mp4", + ); let inputs = input_positions(&args); assert_eq!( inputs.len(), @@ -399,7 +660,7 @@ mod tests { #[test] fn a_silent_scenario_declares_a_single_input_and_no_audio_codec() { - let args = ffmpeg_args(320, 240, 30, "h264", 23, false, None, "o.mp4"); + let args = ffmpeg_args(320, 240, 30, "h264", 23, false, None, None, "o.mp4"); assert_eq!(input_positions(&args).len(), 1); assert!(!args.iter().any(|s| s == "-c:a" || s == "-b:a")); assert_eq!(args.last().unwrap(), "o.mp4"); @@ -413,7 +674,7 @@ mod tests { ("prores", "yuv422p10le", "yuva444p10le"), ] { let pix = |t: bool| { - let a = ffmpeg_args(320, 240, 30, codec, 23, t, None, "o.mov"); + let a = ffmpeg_args(320, 240, 30, codec, 23, t, None, None, "o.mov"); let i = a.iter().position(|s| s == "-pix_fmt").unwrap(); a[i + 1].clone() }; @@ -422,6 +683,182 @@ mod tests { } } + // ── Hardware acceleration: pure argument construction ─────────────────── + // No ffmpeg binary involved — `hw_encoder` is a plain `Option<&str>` the + // caller already resolved, exactly like `select_hardware_encoder`'s + // tests below resolve it without a real probe. + + #[test] + fn a_hardware_encoder_replaces_the_software_codec_and_its_rate_control() { + let args = ffmpeg_args( + 320, + 240, + 30, + "h264", + 23, + false, + Some("h264_videotoolbox"), + None, + "o.mp4", + ); + let cv_pos = args + .iter() + .position(|s| s == "-c:v") + .expect("-c:v must be present"); + assert_eq!(args[cv_pos + 1], "h264_videotoolbox"); + + // Software-only AVOptions: several hardware encoders (VideoToolbox + // among them) reject an unrecognized option outright and abort, so + // none of these may be reused as-is on the hardware path. + for absent in ["-crf", "-preset", "-profile:v"] { + assert!( + !args.iter().any(|s| s == absent), + "hardware path must not emit {absent}: {args:?}" + ); + } + assert_eq!(args.last().unwrap(), "o.mp4"); + } + + #[test] + fn a_hardware_encoder_still_sits_after_the_audio_input() { + // Same load-bearing ordering invariant as the software path: an + // output option before the last `-i` gets attached to that input by + // ffmpeg and aborts the process. + let args = ffmpeg_args( + 320, + 240, + 30, + "h264", + 23, + false, + Some("h264_nvenc"), + Some("/tmp/a.raw"), + "o.mp4", + ); + let audio_i = input_positions(&args)[1]; + let cv_pos = args.iter().position(|s| s == "-c:v").unwrap(); + assert!( + cv_pos > audio_i, + "-c:v (hardware) at {cv_pos} must come after the audio -i at {audio_i}" + ); + } + + #[test] + fn no_hardware_encoder_falls_back_to_the_existing_software_branch() { + // `hw_encoder: None` must reproduce byte-for-byte what the pre-hardware + // code emitted — the software branch is untouched, only wrapped. + let with_none = ffmpeg_args(320, 240, 30, "h264", 23, false, None, None, "o.mp4"); + assert!(with_none.iter().any(|s| s == "libx264")); + assert!(with_none.iter().any(|s| s == "-crf")); + } + + // ── Hardware acceleration: encoder selection (pure, no probe) ─────────── + + #[test] + fn selection_is_a_noop_when_not_requested() { + assert_eq!( + select_hardware_encoder(false, "h264", false, |_| true), + HardwareSelection::NotRequested + ); + } + + #[test] + fn selection_refuses_transparent_even_when_every_encoder_is_available() { + let selection = select_hardware_encoder(true, "h264", true, |_| true); + assert!( + matches!(selection, HardwareSelection::Unsupported { .. }), + "got {selection:?}" + ); + } + + #[test] + fn selection_refuses_codecs_with_no_hardware_family() { + for codec in ["vp9", "prores"] { + let selection = select_hardware_encoder(true, codec, false, |_| true); + assert!( + matches!(selection, HardwareSelection::Unsupported { .. }), + "{codec}: got {selection:?}" + ); + } + } + + #[test] + fn selection_picks_the_first_available_family_in_priority_order() { + // Only nvenc and amf "available" — videotoolbox and qsv are tried + // first (per `HwEncoderFamily::ALL`) but rejected, so nvenc wins. + let selection = select_hardware_encoder(true, "h264", false, |name| { + matches!(name, "h264_nvenc" | "h264_amf") + }); + assert_eq!(selection, HardwareSelection::Use("h264_nvenc".to_string())); + } + + #[test] + fn selection_reports_unavailable_with_every_candidate_tried_when_none_match() { + let selection = select_hardware_encoder(true, "h264", false, |_| false); + match selection { + HardwareSelection::Unavailable { tried } => { + assert_eq!( + tried, + vec!["h264_videotoolbox", "h264_nvenc", "h264_qsv", "h264_amf"] + ); + } + other => panic!("expected Unavailable, got {other:?}"), + } + } + + #[test] + fn selection_covers_h265_and_hevc_as_the_same_codec() { + for codec in ["h265", "hevc"] { + let selection = + select_hardware_encoder(true, codec, false, |name| name == "hevc_videotoolbox"); + assert_eq!( + selection, + HardwareSelection::Use("hevc_videotoolbox".to_string()), + "{codec}" + ); + } + } + + // ── Hardware acceleration: `ffmpeg -encoders` parsing (pure) ──────────── + + #[test] + fn parses_encoder_names_out_of_realistic_ffmpeg_encoders_output() { + // A trimmed, representative capture of `ffmpeg -hide_banner -encoders`: + // a legend (flags-shaped but no real name, just "="), a separator + // line, and a handful of real entries including the hardware ones + // this module knows about. + let sample = "\ +Encoders: + V..... = Video + A..... = Audio + S..... = Subtitle + ------ + V..... a64_multi Multicolor charset for Commodore 64 (codec a64_multi) + V....S alias_pix Alias/Wavefront PIX image + V..... libx264 libx264 H.264 / AVC / MPEG-4 AVC (codec h264) + V..... h264_videotoolbox VideoToolbox H.264 Encoder + V..... hevc_videotoolbox VideoToolbox H.265 Encoder + V..... h264_nvenc NVIDIA NVENC H.264 encoder (codec h264) + A..... aac AAC (Advanced Audio Coding) +"; + let names = parse_encoder_names(sample); + for expect in [ + "libx264", + "h264_videotoolbox", + "hevc_videotoolbox", + "h264_nvenc", + "aac", + "a64_multi", + "alias_pix", + ] { + assert!(names.contains(expect), "missing {expect}: {names:?}"); + } + // The legend's bare "=" and the "Encoders:"/"------" scaffolding + // must never be mistaken for encoder names. + assert!(!names.contains("=")); + assert!(names.iter().all(|n| n != "Video" && n != "Audio")); + } + // ── Integration test (gated on ffmpeg + ffprobe) ──────────────────────── // // Ties constat #1 (audio input declared before every output option — a @@ -570,4 +1007,85 @@ mod tests { let _ = std::fs::remove_file(&wav_path); let _ = std::fs::remove_file(&out); } + + // ── Hardware acceleration: machine-dependent, gated on ffmpeg ─────────── + // + // These exercise the real probe (`probe_ffmpeg_encoders`) and the real + // spawn against whatever this machine's ffmpeg actually offers — unlike + // the pure tests above, their outcome legitimately varies by machine, so + // neither asserts a specific encoder was picked. What they do assert + // (the probe doesn't panic; the encode succeeds and produces a file + // either way) holds on any machine, hardware-capable or not, which is + // what makes them safe to run in CI even though CI has no GPU. + + #[test] + fn hardware_probe_reports_what_this_machine_actually_offers() { + if !ffmpeg_on_path() { + eprintln!( + "hardware_probe_reports_what_this_machine_actually_offers: ffmpeg not found — skipping" + ); + return; + } + let available = super::probe_ffmpeg_encoders(); + let selection = + super::select_hardware_encoder(true, "h264", false, |name| available.contains(name)); + match selection { + HardwareSelection::Use(name) => { + eprintln!("this machine's ffmpeg offers hardware encoder: {name}"); + } + other => { + eprintln!( + "this machine's ffmpeg offers no known h264 hardware encoder ({other:?}); \ + the fallback path is covered by the pure tests above" + ); + } + } + } + + #[test] + fn encode_with_ffmpeg_hw_succeeds_whether_or_not_this_machine_has_a_hardware_encoder() { + if !ffmpeg_on_path() { + eprintln!( + "encode_with_ffmpeg_hw_succeeds_whether_or_not_this_machine_has_a_hardware_encoder: \ + ffmpeg not found — skipping" + ); + return; + } + let json = r#"{"video": {"width": 32, "height": 32, "fps": 10}, + "scenes": [{"duration": 0.5, "children": []}]}"#; + let scenario = crate::loader::load_scenario_from_source(None, Some(json)).expect("load"); + + let out = std::env::temp_dir().join(format!( + "rm_ffmpeg_hw_it_out_{}_{}.mp4", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_nanos() + )); + let _ = std::fs::remove_file(&out); + + super::encode_with_ffmpeg_hw( + &scenario, + out.to_str().unwrap(), + true, + "h264", + None, + false, + true, + None, + ) + .expect( + "hardware_acceleration=true must never fail the encode outright — available or \ + not, it must fall back to software rather than abort", + ); + + assert!(out.exists(), "output MP4 must exist"); + assert!( + std::fs::metadata(&out).unwrap().len() > 0, + "output MP4 must not be empty" + ); + + let _ = std::fs::remove_file(&out); + } } diff --git a/crates/rustmotion/src/engine/preload.rs b/crates/rustmotion/src/engine/preload.rs index 474a047..8f19c87 100644 --- a/crates/rustmotion/src/engine/preload.rs +++ b/crates/rustmotion/src/engine/preload.rs @@ -2,7 +2,10 @@ use std::sync::Arc; use crate::components::{ChildComponent, Component}; use crate::schema::Scene; -use rustmotion_core::engine::renderer::{asset_cache, fetch_icon_svg, video_frame_cache}; +use rustmotion_core::engine::renderer::{ + asset_cache, fetch_icon_svg, ffmpeg_available, icon_cache_dir, icon_cache_key, + video_frame_cache, +}; use rustmotion_core::traits::{Styled, Timed}; /// Pre-fetch and cache all icon components before rendering. @@ -77,19 +80,28 @@ pub fn prefetch_icons(scenes: &[Scene]) { } let cache = asset_cache(); + // Issue #166: icons that genuinely cannot be resolved (checked both the + // disk cache and the network, inside `fetch_icon_svg`) are collected + // instead of merely logged — a scene that silently renders without an + // icon is exactly the "valid but wrong" outcome this project treats as + // worse than a hard failure. Parse/rasterize errors (a malformed SVG + // response, not a missing icon) stay warnings: they are not what "icon + // remains unresolvable" means here, and are rare enough downstream + // provider bugs that they don't warrant aborting the whole render. + let mut unresolved: Vec = Vec::new(); for (icon, color, w, h) in &seen { - let cache_key = format!("icon:{}:{}:{}x{}", icon, color, w, h); + // Same formula the painter (`icon.rs`) uses at paint time — see + // `icon_cache_key`'s doc for why these used to disagree (issue #166). + let (render_w, render_h, cache_key) = icon_cache_key(icon, color, *w, *h); if cache.contains_key(&cache_key) { continue; } - match fetch_icon_svg(icon, color, *w, *h) { + match fetch_icon_svg(icon, color, render_w, render_h) { Ok(svg_data) => { let opt = usvg::Options::default(); match usvg::Tree::from_data(&svg_data, &opt) { Ok(tree) => { let svg_size = tree.size(); - let render_w = (*w).max(1); - let render_h = (*h).max(1); if let Some(mut pixmap) = tiny_skia::Pixmap::new(render_w, render_h) { let scale_x = render_w as f32 / svg_size.width(); let scale_y = render_h as f32 / svg_size.height(); @@ -117,15 +129,43 @@ pub fn prefetch_icons(scenes: &[Scene]) { } } Err(e) => { - eprintln!("Warning: failed to fetch icon '{}': {}", icon, e); + unresolved.push(format!("'{icon}' (color {color}, target {w}x{h}px): {e}")); } } } + + if !unresolved.is_empty() { + panic!( + "rustmotion: {} icon(s) could not be preloaded — checked the disk cache at \ + {} and the network, both failed:\n - {}\n\ + A render must not silently omit an icon: fix the identifier(s), or connect to \ + the network so they can be downloaded once and cached for offline use.", + unresolved.len(), + icon_cache_dir().display(), + unresolved.join("\n - ") + ); + } } /// Pre-extract all needed frames from video sources in a single ffmpeg pass. /// Called before the render loop to populate the video frame cache. +/// +/// Item 3 (issue #167): this used to fail in total silence — `ffmpeg` +/// missing, or a single extraction failing, both fell into `_ => {}` with no +/// trace anywhere, leaving affected `video` components entirely blank. +/// Replicates the `ffmpeg_available()` + one-time-warning discipline PR #151 +/// already established for embedded-video *audio* extraction +/// (`encode::video_audio::collect_video_audio_tracks`), which this frame +/// path never inherited. pub fn preextract_video_frames(scenes: &[Scene], fps: u32) { + if !ffmpeg_available() { + eprintln!( + "rustmotion: ffmpeg not found — video components will render blank frames. \ + Install ffmpeg to decode embedded video sources." + ); + return; + } + fn collect_videos(child: &ChildComponent, scene_frames: u32, fps: u32) { if let Component::Video(video) = &child.component { use rustmotion_core::css::style::Size as CSize; @@ -209,7 +249,21 @@ pub fn preextract_video_frames(scenes: &[Scene], fps: u32) { cache.insert(cache_key, Arc::new(frames)); } - _ => {} + Ok(output) => { + eprintln!( + "rustmotion: video frame preextraction: ffmpeg failed to decode \ + frames from '{}' (exit status: {}). This video will render blank \ + for the affected frames.", + video.src, output.status + ); + } + Err(e) => { + eprintln!( + "rustmotion: video frame preextraction: could not spawn ffmpeg for \ + '{}': {}. This video will render blank for the affected frames.", + video.src, e + ); + } } } @@ -240,3 +294,34 @@ pub fn preextract_video_frames(scenes: &[Scene], fps: u32) { } } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn an_unresolvable_icon_must_fail_the_preload_not_be_swallowed() { + // `fetch_icon_svg` fails deterministically (no network needed) for + // an icon id with no ':' — `InvalidIconFormat`. Pre-fix, + // `prefetch_icons` catches this in its `Err(e) => eprintln!(...)` + // arm and returns normally: the render proceeds as if nothing were + // wrong, and the icon silently never paints. + let scene: Scene = serde_json::from_value(serde_json::json!({ + "duration": 1.0, + "children": [ + {"type": "icon", "icon": "not-a-valid-icon-id-no-colon"} + ] + })) + .expect("scene must deserialize"); + + let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + prefetch_icons(std::slice::from_ref(&scene)); + })); + + assert!( + result.is_err(), + "prefetch_icons must panic (or otherwise hard-fail) when an icon cannot be \ + resolved via disk cache or network, instead of silently continuing" + ); + } +}