Skip to content

Commit f94bcad

Browse files
committed
fix(geometry): resolve vw/vh against the scenario viewport
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
1 parent 3a156c2 commit f94bcad

2 files changed

Lines changed: 203 additions & 2 deletions

File tree

‎crates/rustmotion/src/cli/commands/geometry.rs‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -177,7 +177,11 @@ pub fn validate_geometry(scenario: &ResolvedScenario) -> Vec<GeometryViolation>
177177

178178
let root_css = render::root_style(scene.layout.as_ref(), view.view_type.clone());
179179
let built = build_scene_from_refs(children.iter(), viewport_f, root_css, None);
180-
let layouts = run_layout(&built.root, viewport_f, &ConversionContext::default());
180+
let layouts = run_layout(
181+
&built.root,
182+
viewport_f,
183+
&ConversionContext::for_viewport(viewport_f.0, viewport_f.1),
184+
);
181185

182186
let camera = scene
183187
.camera
@@ -1344,7 +1348,11 @@ pub fn validate_geometry_animated(scenario: &ResolvedScenario) -> Vec<GeometryVi
13441348
fps,
13451349
});
13461350
let built = build_scene_from_refs(children.iter(), viewport_f, root_css, anim);
1347-
let layouts = run_layout(&built.root, viewport_f, &ConversionContext::default());
1351+
let layouts = run_layout(
1352+
&built.root,
1353+
viewport_f,
1354+
&ConversionContext::for_viewport(viewport_f.0, viewport_f.1),
1355+
);
13481356

13491357
walk_anim(
13501358
&children,
Lines changed: 193 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,193 @@
1+
//! Regression tests — audit round chantier/audit-2026-09, workstream B
2+
//! (geometry validator).
3+
//!
4+
//! Every test here drives the actual compiled `rustmotion` binary rather
5+
//! than calling `commands::geometry`/`commands::validate` in-process:
6+
//! `rustmotion::cli::commands` is a private module (`mod commands;` in
7+
//! `src/cli/mod.rs`), so the CLI subprocess — `validate --report <path>`,
8+
//! whose JSON is `commands::geometry::GeometryViolation`'s public `Serialize`
9+
//! output — is the only externally-observable contract for what the
10+
//! validator decided (mirrors `motion_path_strict_anim.rs`'s reasoning).
11+
//!
12+
//! One section per finding, in briefing order: viewport units, transform
13+
//! lengths, path rewriting, the three box-model checks, and helper reuse.
14+
15+
use std::path::{Path, PathBuf};
16+
use std::process::{Command, Output};
17+
18+
/// Minimal RAII scratch file — mirrors `motion_path_strict_anim.rs`'s
19+
/// `ScratchFile`.
20+
struct ScratchFile(PathBuf);
21+
22+
impl ScratchFile {
23+
fn new(label: &str) -> Self {
24+
let unique = format!(
25+
"rustmotion-audit-ws-b-{label}-{}-{}",
26+
std::process::id(),
27+
std::time::SystemTime::now()
28+
.duration_since(std::time::UNIX_EPOCH)
29+
.expect("system clock before UNIX epoch")
30+
.as_nanos()
31+
);
32+
Self(std::env::temp_dir().join(unique))
33+
}
34+
}
35+
36+
impl Drop for ScratchFile {
37+
fn drop(&mut self) {
38+
let _ = std::fs::remove_file(&self.0);
39+
}
40+
}
41+
42+
/// Minimal RAII scratch directory — mirrors `skill_files_match_disk.rs`'s
43+
/// `ScratchDir`. Only the path-rewriting case needs a directory (a file plus a
44+
/// sibling asset file).
45+
struct ScratchDir(PathBuf);
46+
47+
impl ScratchDir {
48+
fn new(label: &str) -> Self {
49+
let unique = format!(
50+
"rustmotion-audit-ws-b-{label}-{}-{}",
51+
std::process::id(),
52+
std::time::SystemTime::now()
53+
.duration_since(std::time::UNIX_EPOCH)
54+
.expect("system clock before UNIX epoch")
55+
.as_nanos()
56+
);
57+
let path = std::env::temp_dir().join(unique);
58+
std::fs::create_dir_all(&path).expect("create scratch dir");
59+
Self(path)
60+
}
61+
}
62+
63+
impl Drop for ScratchDir {
64+
fn drop(&mut self) {
65+
let _ = std::fs::remove_dir_all(&self.0);
66+
}
67+
}
68+
69+
fn run_validate(
70+
scenario_path: &Path,
71+
report_path: Option<&Path>,
72+
fix: bool,
73+
strict_anim: bool,
74+
) -> Output {
75+
let mut cmd = Command::new(env!("CARGO_BIN_EXE_rustmotion"));
76+
cmd.arg("validate").arg("--file").arg(scenario_path);
77+
if let Some(report_path) = report_path {
78+
cmd.arg("--report").arg(report_path);
79+
}
80+
if fix {
81+
cmd.arg("--fix");
82+
}
83+
if strict_anim {
84+
cmd.arg("--strict-anim");
85+
}
86+
cmd.output().expect("failed to spawn `rustmotion validate`")
87+
}
88+
89+
fn read_report(path: &Path) -> serde_json::Value {
90+
let text = std::fs::read_to_string(path).expect("read report");
91+
serde_json::from_str(&text).expect("report is valid JSON")
92+
}
93+
94+
fn violations(report: &serde_json::Value) -> &Vec<serde_json::Value> {
95+
report["geometry_violations"]
96+
.as_array()
97+
.expect("geometry_violations is an array")
98+
}
99+
100+
fn count_kind(report: &serde_json::Value, kind: &str) -> usize {
101+
violations(report).iter().filter(|v| v["kind"] == kind).count()
102+
}
103+
104+
fn find_kind<'a>(report: &'a serde_json::Value, kind: &str) -> Option<&'a serde_json::Value> {
105+
violations(report).iter().find(|v| v["kind"] == kind)
106+
}
107+
108+
// ─── vw/vh must resolve against the scenario's real viewport ───────
109+
110+
/// A `width: "90vw"` shape on a 1080×1920 scenario, positioned so its right
111+
/// edge crosses the viewport edge at EITHER candidate width — 972px (90% of
112+
/// the real 1080px-wide viewport) or 1728px (90% of the hardcoded
113+
/// `ConversionContext::default()` 1920px fallback). The violation fires
114+
/// either way; only the reported `bbox.w` distinguishes a correct
115+
/// measurement from the buggy one.
116+
fn vw_shape_scenario(x: f32) -> String {
117+
format!(
118+
r##"{{
119+
"video": {{ "width": 1080, "height": 1920 }},
120+
"scenes": [{{
121+
"duration": 1.0,
122+
"children": [{{
123+
"type": "shape",
124+
"shape": "rect",
125+
"position": "absolute",
126+
"x": {x}, "y": 100,
127+
"style": {{ "width": "90vw", "height": "50px" }},
128+
"fill": "#ff0000"
129+
}}]
130+
}}]
131+
}}"##
132+
)
133+
}
134+
135+
#[test]
136+
fn resting_layout_measures_vw_against_the_real_viewport_width() {
137+
let scenario = ScratchFile::new("rm05-resting-scenario");
138+
let report = ScratchFile::new("rm05-resting-report");
139+
std::fs::write(&scenario.0, vw_shape_scenario(200.0)).expect("write scenario");
140+
141+
let output = run_validate(&scenario.0, Some(&report.0), false, false);
142+
assert!(
143+
!output.status.success(),
144+
"a shape whose right edge is past the viewport at either candidate width must block; \
145+
stdout={} stderr={}",
146+
String::from_utf8_lossy(&output.stdout),
147+
String::from_utf8_lossy(&output.stderr)
148+
);
149+
150+
let report_json = read_report(&report.0);
151+
let violation =
152+
find_kind(&report_json, "viewport_overflow").expect("expected a viewport_overflow violation");
153+
let width = violation["bbox"]["w"].as_f64().expect("bbox.w is a number");
154+
assert!(
155+
(width - 972.0).abs() < 2.0,
156+
"90vw on a 1080px-wide viewport must resolve to ~972px (the real viewport), \
157+
not 1728px (0.9 x the hardcoded 1920 default); report: {report_json}"
158+
);
159+
}
160+
161+
/// Same shape, repositioned so it overflows ONLY under the buggy
162+
/// 1920x1080 default (right edge 1778px vs an 1080px-wide viewport) and
163+
/// stays clean at the correct 972px width (right edge 1022px). Isolates
164+
/// the `--strict-anim` call site (`validate_geometry_animated`, geometry.rs
165+
/// ~1347) from the resting one above (~180): each builds its own box tree
166+
/// through `ConversionContext::default()` independently, so fixing only
167+
/// one would still leave this failing.
168+
#[test]
169+
fn strict_anim_also_measures_vw_against_the_real_viewport_width() {
170+
let scenario = ScratchFile::new("rm05-strict-anim-scenario");
171+
let report = ScratchFile::new("rm05-strict-anim-report");
172+
std::fs::write(&scenario.0, vw_shape_scenario(50.0)).expect("write scenario");
173+
174+
let output = run_validate(&scenario.0, Some(&report.0), false, true);
175+
let report_json = read_report(&report.0);
176+
assert!(
177+
output.status.success(),
178+
"the real 1080px-wide viewport keeps this shape on-screen at every sample; \
179+
stdout={} stderr={} report={report_json}",
180+
String::from_utf8_lossy(&output.stdout),
181+
String::from_utf8_lossy(&output.stderr)
182+
);
183+
assert_eq!(
184+
count_kind(&report_json, "viewport_overflow"),
185+
0,
186+
"resting pass (line ~180) must be clean: {report_json}"
187+
);
188+
assert_eq!(
189+
count_kind(&report_json, "animated_text_overflow"),
190+
0,
191+
"--strict-anim pass (line ~1347) must also be clean: {report_json}"
192+
);
193+
}

0 commit comments

Comments
 (0)