From fc20b4c80be6f943a63c7e4ee8f18c7a1d765d6e Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Tue, 22 Sep 2026 10:43:16 +0200 Subject: [PATCH] fix(geometry): resolve vw/vh against the scenario viewport MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ConversionContext::default() carries LengthContext { viewport_width: 1920.0, viewport_height: 1080.0, .. } (rustmotion-core/src/css/units.rs:66-74), and taffy_bridge::size_to_dim/lp_to_lp/lp_to_lp_auto resolve every vw/vh/rem/em against it (taffy_bridge.rs:335-338). The renderer does NOT: render_with_new_pipeline_iter (engine/render/scene.rs:554-558) and render_scene_hits (scene.rs:792) both pass viewport_conversion_context(vw, vh), whose own doc comment calls the 1920×1080 default a 78% error on a 1080×1920 vertical video. So on any non-1920×1080 scenario the mandatory rustmotion validate gate checks a layout the renderer never produces: width: "50vw" is measured as 960px by the validator and painted as 540px, 50vh as 540px vs 960px. The file header promises the opposite ("so the geometry it checks matches what the renderer will actually paint"). The same file builds a correct LengthContext with the real viewport at line 477 for its transform-origin math, so the omission is confined to the layout pass. Both the resting walk (:180) and the --strict-anim walk (:1347) are affected. Fix: Make viewport_conversion_context public (or move it next to run_layout in rustmotion-core) and call it from both geometry.rs sites: run_layout(&built.root, viewport_f, &viewport_conversion_context(viewport_f.0, viewport_f.1)). Better: stop exposing ConversionContext at the run_layout signature at all — derive it from the viewport argument inside run_layout, which makes the wrong context unrepresentable. Refs #220 --- .../rustmotion/src/cli/commands/geometry.rs | 12 +- crates/rustmotion/tests/audit_ws_b.rs | 196 ++++++++++++++++++ 2 files changed, 206 insertions(+), 2 deletions(-) create mode 100644 crates/rustmotion/tests/audit_ws_b.rs diff --git a/crates/rustmotion/src/cli/commands/geometry.rs b/crates/rustmotion/src/cli/commands/geometry.rs index 929f592d..0fd7d069 100644 --- a/crates/rustmotion/src/cli/commands/geometry.rs +++ b/crates/rustmotion/src/cli/commands/geometry.rs @@ -177,7 +177,11 @@ pub fn validate_geometry(scenario: &ResolvedScenario) -> Vec let root_css = render::root_style(scene.layout.as_ref(), view.view_type.clone()); let built = build_scene_from_refs(children.iter(), viewport_f, root_css, None); - let layouts = run_layout(&built.root, viewport_f, &ConversionContext::default()); + let layouts = run_layout( + &built.root, + viewport_f, + &ConversionContext::for_viewport(viewport_f.0, viewport_f.1), + ); let camera = scene .camera @@ -1344,7 +1348,11 @@ pub fn validate_geometry_animated(scenario: &ResolvedScenario) -> Vec`, +//! whose JSON is `commands::geometry::GeometryViolation`'s public `Serialize` +//! output — is the only externally-observable contract for what the +//! validator decided (mirrors `motion_path_strict_anim.rs`'s reasoning). +//! +//! One section per finding, in briefing order: viewport units, transform +//! lengths, path rewriting, the three box-model checks, and helper reuse. + +use std::path::{Path, PathBuf}; +use std::process::{Command, Output}; + +/// Minimal RAII scratch file — mirrors `motion_path_strict_anim.rs`'s +/// `ScratchFile`. +struct ScratchFile(PathBuf); + +impl ScratchFile { + fn new(label: &str) -> Self { + let unique = format!( + "rustmotion-audit-ws-b-{label}-{}-{}", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .expect("system clock before UNIX epoch") + .as_nanos() + ); + Self(std::env::temp_dir().join(unique)) + } +} + +impl Drop for ScratchFile { + fn drop(&mut self) { + let _ = std::fs::remove_file(&self.0); + } +} + +/// Minimal RAII scratch directory — mirrors `skill_files_match_disk.rs`'s +/// `ScratchDir`. Only the path-rewriting case needs a directory (a file plus a +/// sibling asset file). +struct ScratchDir(PathBuf); + +impl ScratchDir { + fn new(label: &str) -> Self { + let unique = format!( + "rustmotion-audit-ws-b-{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); + std::fs::create_dir_all(&path).expect("create scratch dir"); + Self(path) + } +} + +impl Drop for ScratchDir { + fn drop(&mut self) { + let _ = std::fs::remove_dir_all(&self.0); + } +} + +fn run_validate( + scenario_path: &Path, + report_path: Option<&Path>, + fix: bool, + strict_anim: bool, +) -> Output { + let mut cmd = Command::new(env!("CARGO_BIN_EXE_rustmotion")); + cmd.arg("validate").arg("--file").arg(scenario_path); + if let Some(report_path) = report_path { + cmd.arg("--report").arg(report_path); + } + if fix { + cmd.arg("--fix"); + } + if strict_anim { + cmd.arg("--strict-anim"); + } + cmd.output().expect("failed to spawn `rustmotion validate`") +} + +fn read_report(path: &Path) -> serde_json::Value { + let text = std::fs::read_to_string(path).expect("read report"); + serde_json::from_str(&text).expect("report is valid JSON") +} + +fn violations(report: &serde_json::Value) -> &Vec { + report["geometry_violations"] + .as_array() + .expect("geometry_violations is an array") +} + +fn count_kind(report: &serde_json::Value, kind: &str) -> usize { + violations(report) + .iter() + .filter(|v| v["kind"] == kind) + .count() +} + +fn find_kind<'a>(report: &'a serde_json::Value, kind: &str) -> Option<&'a serde_json::Value> { + violations(report).iter().find(|v| v["kind"] == kind) +} + +// ─── vw/vh must resolve against the scenario's real viewport ─────── + +/// A `width: "90vw"` shape on a 1080×1920 scenario, positioned so its right +/// edge crosses the viewport edge at EITHER candidate width — 972px (90% of +/// the real 1080px-wide viewport) or 1728px (90% of the hardcoded +/// `ConversionContext::default()` 1920px fallback). The violation fires +/// either way; only the reported `bbox.w` distinguishes a correct +/// measurement from the buggy one. +fn vw_shape_scenario(x: f32) -> String { + format!( + r##"{{ + "video": {{ "width": 1080, "height": 1920 }}, + "scenes": [{{ + "duration": 1.0, + "children": [{{ + "type": "shape", + "shape": "rect", + "position": "absolute", + "x": {x}, "y": 100, + "style": {{ "width": "90vw", "height": "50px" }}, + "fill": "#ff0000" + }}] + }}] + }}"## + ) +} + +#[test] +fn resting_layout_measures_vw_against_the_real_viewport_width() { + let scenario = ScratchFile::new("rm05-resting-scenario"); + let report = ScratchFile::new("rm05-resting-report"); + std::fs::write(&scenario.0, vw_shape_scenario(200.0)).expect("write scenario"); + + let output = run_validate(&scenario.0, Some(&report.0), false, false); + assert!( + !output.status.success(), + "a shape whose right edge is past the viewport at either candidate width must block; \ + stdout={} stderr={}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + + let report_json = read_report(&report.0); + let violation = find_kind(&report_json, "viewport_overflow") + .expect("expected a viewport_overflow violation"); + let width = violation["bbox"]["w"].as_f64().expect("bbox.w is a number"); + assert!( + (width - 972.0).abs() < 2.0, + "90vw on a 1080px-wide viewport must resolve to ~972px (the real viewport), \ + not 1728px (0.9 x the hardcoded 1920 default); report: {report_json}" + ); +} + +/// Same shape, repositioned so it overflows ONLY under the buggy +/// 1920x1080 default (right edge 1778px vs an 1080px-wide viewport) and +/// stays clean at the correct 972px width (right edge 1022px). Isolates +/// the `--strict-anim` call site (`validate_geometry_animated`, geometry.rs +/// ~1347) from the resting one above (~180): each builds its own box tree +/// through `ConversionContext::default()` independently, so fixing only +/// one would still leave this failing. +#[test] +fn strict_anim_also_measures_vw_against_the_real_viewport_width() { + let scenario = ScratchFile::new("rm05-strict-anim-scenario"); + let report = ScratchFile::new("rm05-strict-anim-report"); + std::fs::write(&scenario.0, vw_shape_scenario(50.0)).expect("write scenario"); + + let output = run_validate(&scenario.0, Some(&report.0), false, true); + let report_json = read_report(&report.0); + assert!( + output.status.success(), + "the real 1080px-wide viewport keeps this shape on-screen at every sample; \ + stdout={} stderr={} report={report_json}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + assert_eq!( + count_kind(&report_json, "viewport_overflow"), + 0, + "resting pass (line ~180) must be clean: {report_json}" + ); + assert_eq!( + count_kind(&report_json, "animated_text_overflow"), + 0, + "--strict-anim pass (line ~1347) must also be clean: {report_json}" + ); +}