From 2ab980a7f20ce6f8aca052d854fd250ea54e9be3 Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Mon, 10 Aug 2026 17:00:20 +0200 Subject: [PATCH 1/5] fix(skills): distribute every rule instead of the ones someone remembered MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `rustmotion skills install` is the only channel through which a generating agent receives the rules. `SKILL_FILES` embedded 30 of the 47 on disk. The 17 absentees were not a random sample. They included `geometry-safety.md`, `world-view.md` and `audio-reactive.md` — the three CLAUDE.md cites by name — and `html-css-mental-model.md`, which PR #156 had just corrected because it taught JSON forms that delete the component from the video. That file had never been in the list, before or after the fix: the content was repaired and the pipe was not. The list is now generated by a build script that walks the skills tree, so "every rule is embedded" holds by construction rather than by memory. Adding 17 entries by hand would have left the next rule just as invisible. Two details the generation had to preserve: the table is still ordered with SKILL.md first, and `show()` no longer depends on that — it locates SKILL.md by path, where it previously indexed position 0. Verified by running the installed binary into an empty directory: 49 files written, 47 of them rules. A test now compares what is embedded against the directory and fails naming any file that drifts out. --- crates/rustmotion-cli/build.rs | 104 ++++++++++ crates/rustmotion-cli/src/skills.rs | 150 ++------------- .../tests/skill_files_match_disk.rs | 177 ++++++++++++++++++ 3 files changed, 299 insertions(+), 132 deletions(-) create mode 100644 crates/rustmotion-cli/build.rs create mode 100644 crates/rustmotion-cli/tests/skill_files_match_disk.rs 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/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 + ); +} From 4620a543f0b3df7341b6b24e21f61628b9a683cd Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Mon, 10 Aug 2026 17:00:34 +0200 Subject: [PATCH 2/5] feat(text): resolve font-size in relative units instead of silently zeroing it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A `font-size` in a relative unit resolved to 0px without failing. `validate` returned exit 0 and "Valid scenario" on a `text` with `"font-size": "2rem"`, warning only that it was "resolving to 0px" — and the render produced a frame with no text in it. The contextual accessor already existed and was tested; nothing called it. Every component reached for `font_size_px_or`, the context-free one. The resolution pattern was already written and proven in `text.rs` for `letter-spacing` and `line-height`: build a length context locally from the frame dimensions. This extends it to `font-size` itself and rolls it out across the components that render text, through one shared `intrinsic::font_size_ctx` rather than fifteen copies — copy-paste of a derived value is what produced several of the defects this chantier is fixing. `PaintCtx` is deliberately untouched: threading the context through it would have rippled into all 57 painters. Each component has a test that paints and asserts non-transparent pixels, not merely that a number came back non-zero. Measurement and paint were checked together, since the geometry validator reads the measured box: a component that measured 0 while painting 32px would have moved the bug rather than fixed it. Known limit, unchanged: `em` and `%` on `font-size` depend on the parent's resolved size, and the cascade still copies the parent's raw unresolved `Length`. `rem`, `vw` and `vh` are correct now; `em` and `%` remain approximate. --- crates/rustmotion-components/src/badge.rs | 86 +++++++++-- crates/rustmotion-components/src/callout.rs | 94 ++++++++++++- crates/rustmotion-components/src/caption.rs | 42 +++++- .../src/codeblock/dimensions.rs | 9 +- .../src/codeblock/render.rs | 66 ++++++++- crates/rustmotion-components/src/counter.rs | 82 +++++++++-- .../src/gradient_text.rs | 65 +++++++-- crates/rustmotion-components/src/intrinsic.rs | 133 +++++++++++++----- crates/rustmotion-components/src/kbd.rs | 95 +++++++++++-- crates/rustmotion-components/src/list.rs | 101 +++++++++++-- crates/rustmotion-components/src/marquee.rs | 92 +++++++++++- .../rustmotion-components/src/notification.rs | 117 +++++++++++++-- crates/rustmotion-components/src/pill_nav.rs | 106 ++++++++++++-- crates/rustmotion-components/src/rich_text.rs | 77 +++++++--- crates/rustmotion-components/src/table.rs | 123 +++++++++++++--- crates/rustmotion-components/src/terminal.rs | 112 +++++++++++++-- crates/rustmotion-components/src/text.rs | 128 ++++++++++++++--- crates/rustmotion-components/src/tooltip.rs | 101 ++++++++++++- .../tests/relative_font_size.rs | 120 ++++++++++++++++ 19 files changed, 1550 insertions(+), 199 deletions(-) create mode 100644 crates/rustmotion-components/tests/relative_font_size.rs 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/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/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" + ); +} From c07fd44f643906971a8b5970412f825f0a7c0235 Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Mon, 10 Aug 2026 17:00:44 +0200 Subject: [PATCH 3/5] fix(assets): make icon preloading work at all, and asset failures visible MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three defects that all end the same way: a hole in the video and no diagnostic. - Icon preloading could never work. The painter keyed its cache on the oversampled render size, the preloader on the target size — for a 40x40 icon one wrote `80x80` and the other read `40x40`, so the keys could not collide at any size. The mechanism meant to stop N render threads hitting the network in parallel added a wasted download and rasterization, then let every thread fetch anyway. The preloader also rasterized without the oversample, so fixing the key alone would have turned a useless preload into a harmful one: the painter would finally find a bitmap, at half the resolution. Both sites now go through one `icon_cache_key`, which applies the oversample and builds the key — the drift is no longer expressible. - Icons had no disk cache and failed silently at every level. There is now a cache on disk, following the pattern `google_fonts.rs` already uses in this crate, and an unresolvable icon fails the preload instead of leaving a gap. - Video frame extraction failed silently too: a missing or failing ffmpeg produced an entirely empty video with no warning. PR #151 had already hardened the *audio* extraction of embedded videos in this same crate, with an availability probe and a warning emitted once; that discipline is now extended to the frame path, which had been left behind. Warnings added here are deduplicated: `prepare_scene` runs per frame, so an unguarded one would print over a thousand times on a 1200-frame render. --- crates/rustmotion-components/src/icon.rs | 129 ++++++++- crates/rustmotion-components/src/video.rs | 95 ++++++- .../src/engine/renderer/assets.rs | 247 ++++++++++++++++++ crates/rustmotion/src/engine/preload.rs | 99 ++++++- 4 files changed, 552 insertions(+), 18 deletions(-) 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/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-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/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" + ); + } +} From b74cc391c721b98f8f6401c08957bfe42a4a8576 Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Mon, 10 Aug 2026 17:00:59 +0200 Subject: [PATCH 4/5] feat(encode): offer hardware encoding, and say so when it is not available MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ffmpeg_args` knew only software encoders. On machines that have VideoToolbox or NVENC, the encode step was leaving an order of magnitude on the table. `--hardware-acceleration` now probes `ffmpeg -encoders` for what the machine actually offers rather than inferring it from the compile target: a macOS box without a usable VideoToolbox exists, and so does an ffmpeg built without NVENC. When acceleration is asked for and unavailable, it says so and continues in software — it neither aborts nor switches silently. CRF is meaningless on most hardware encoders, which think in bitrate, so `check_crf` now warns explicitly when `--crf` is passed alongside an acceleration that ignores it, instead of letting the setting look honoured. PR #143 had extracted `ffmpeg_args` into a pure function after an argument ordering bug put an input after the output options. That property is preserved and is why this was cheap: the probe (machine-dependent) and the argument construction (pure) are separate, and the tests assert the presence *and position* of the hardware arguments without needing a machine that has one — CI does not. `cmd_render` grew a parameter, so `cmd_batch`'s call site passes `false`: `batch` has no flag of its own, and wiring one is a separate change. --- crates/rustmotion-cli/src/commands/batch.rs | 4 + crates/rustmotion-cli/src/commands/render.rs | 33 +- .../rustmotion-cli/src/commands/validation.rs | 59 +- crates/rustmotion-cli/src/lib.rs | 25 +- crates/rustmotion/src/encode/video/ffmpeg.rs | 604 ++++++++++++++++-- 5 files changed, 674 insertions(+), 51 deletions(-) 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/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/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/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); + } } From 1a24baac982bcfed750361e04013f36d7fea27b3 Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Mon, 10 Aug 2026 17:01:15 +0200 Subject: [PATCH 5/5] feat(animation): let a spring be given a duration, and report when it settles MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A spring's duration used to be an emergent consequence of `damping`, `stiffness` and `mass`. An author who wanted "this spring, finishing in 0.8s" had to guess at three coupled numbers, and nothing exposed the instant a given spring actually comes to rest — the one figure you need to time the rest of a scene against it. `SpringConfig` now takes an optional `duration` and `rest_threshold`. When `duration` is set, time is rescaled linearly so the settle point lands exactly there. The spring's shape is preserved by construction — the solver is called with a scaled `t`, not with altered physics — and a test asserts it: sampled at matching fractions of each one's own settle time, the natural and pinned curves agree to 1e-9, with identical oscillation counts and overshoot amplitudes. Without that test nothing would distinguish a real remap from a crude shortening that flattens the spring. The settle search handles the regimes that break naive implementations: an overdamped spring never reaches its target exactly, which is what `rest_threshold` is for; a very lightly damped one can take arbitrarily long, so the search is bounded and the bound is tested rather than left to spin. `rustmotion info` now reports each spring's rest time, so a scene can be timed against it instead of by trial and error, and `validate` rejects a non-positive `duration` or a negative `rest_threshold` alongside the checks PR #158 already added. --- crates/rustmotion-cli/src/commands/info.rs | 195 ++++++- .../src/commands/validate_schema.rs | 132 +++++ crates/rustmotion-core/src/engine/animator.rs | 515 ++++++++++++++++++ .../rustmotion-core/src/schema/animation.rs | 32 ++ .../rustmotion-core/tests/spring_duration.rs | 81 +++ 5 files changed, 954 insertions(+), 1 deletion(-) create mode 100644 crates/rustmotion-core/tests/spring_duration.rs 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/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-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/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}" + ); + } +}