From 929ea1dbf60cd0669de04a3366702949697b75bc Mon Sep 17 00:00:00 2001 From: Fini Date: Mon, 13 Jul 2026 21:41:31 +0800 Subject: [PATCH] fix(agent): one image per slot, and a photo band that shows the photo Five destination cards rendered as five identical blue bands. The photos were fine and all different - the cards were the problem. Each card's image slot carried TWO images (an absolutely-positioned image-filled plate AND the real image node) inside a band authored 56px tall against a 130px photo, so all that survived the clip was each photo's sky. The duplicate plate is dropped (the image node wins - it is what the image pipeline fills and re-searches), the band grows to the photo's own declared height when it would hide more than half of it, and the starved rail-card repair now reads fixed-width DESCENDANTS and assigns the demand as a definite width: an out-of-flow photo plate contributes nothing to a hug, so fit_content re-starved the very cards it was meant to rescue (126px against a 200px plate). Replaying the measured document through the repair loop: 4 geometry diagnostics to 0, cards 79px to 200px. --- .../src/geometry_validation.rs | 146 ++++++++++++++++-- .../src/geometry_validation_tests.rs | 130 +++++++++++++++- 2 files changed, 260 insertions(+), 16 deletions(-) diff --git a/crates/op-orchestrator/src/geometry_validation.rs b/crates/op-orchestrator/src/geometry_validation.rs index f0c962b6b..3f1c93984 100644 --- a/crates/op-orchestrator/src/geometry_validation.rs +++ b/crates/op-orchestrator/src/geometry_validation.rs @@ -188,6 +188,7 @@ pub fn geometry_validate_and_fix(sink: &mut dyn DocSink, root_id: &str) -> usize collect_frame_overflow_fixes(&v, &rects, &mut cmds); collect_oversized_image_fixes(&v, &mut cmds); collect_absolute_fill_image_fixes(&v, &rects, &mut cmds); + collect_image_slot_fixes(&v, &mut cmds); collect_grow_to_fit_fixes(&v, &rects, &mut cmds); collect_starved_rail_card_fixes(&v, &rects, &mut cmds); collect_row_gap_fixes(&v, &rects, &mut cmds); @@ -1126,13 +1127,27 @@ fn collect_grow_to_fit_fixes( const RAIL_STARVE_EPS: f64 = 12.0; const RAIL_MIN_CARDS: usize = 3; -/// The widest fixed-width DIRECT child plus the card's own side padding — the -/// width this card provably needs to show its authored content. +/// The widest fixed-width DESCENDANT plus the card's own side padding — the +/// width this card provably needs to show its authored content. Descendants, +/// not just direct children: a destination card's photo is often an +/// absolutely-positioned 200px plate two levels down, and that number is +/// still the card's authored width intent (measured test0711-1-glm, where a +/// direct-children-only scan saw nothing and left the cards at 79px). fn fixed_content_demand(card: &Value) -> f64 { - let widest = children(card) - .iter() - .filter_map(fixed_width) - .fold(0.0_f64, f64::max); + fn widest_fixed(v: &Value, depth: usize) -> f64 { + if depth == 0 { + return 0.0; + } + children(v) + .iter() + .map(|c| { + fixed_width(c) + .unwrap_or(0.0) + .max(widest_fixed(c, depth - 1)) + }) + .fold(0.0_f64, f64::max) + } + let widest = widest_fixed(card, 3); if widest == 0.0 { 0.0 } else { @@ -1166,10 +1181,20 @@ fn collect_starved_rail_card_fixes( if all_starved { for c in &cards { if let Some(id) = c.get("id").and_then(Value::as_str) { - cmds.push(EditorCommand::SetNodeLayoutProp { + // The card takes its DEMAND as a definite width, not + // fit_content: an absolutely-positioned photo plate is + // out of flow, so hugging would size the card to its + // text alone and starve the photo all over again + // (measured: hug gave 126px against a 200px plate). + cmds.push(EditorCommand::UpdateNode { node_id: NodeId::new(id.to_string()), - property: "width".to_string(), - value: LayoutPropValue::Keyword("fit_content".to_string()), + x: None, + y: None, + width: Some(fixed_content_demand(c).round() as i32), + height: None, + name: None, + fill_hex: None, + page_id: None, }); } } @@ -1245,6 +1270,109 @@ fn collect_absolute_fill_image_fixes( } } +/// TWO images in one slot, and a photo band cropped to a sliver. +/// +/// Measured (test0711-1-glm): a destination card's image wrapper was authored +/// `height: 56` and held BOTH an absolutely-positioned 200x130 frame whose +/// FILL is the photo AND a sibling image NODE carrying the same slot's photo - +/// two images stacked in one box, of which only the top 56px survived the +/// wrapper's clip. Every card's surviving 56px was that photo's sky, so five +/// different photos rendered as five identical blue bands (user report: "最近 +/// 怎么经常有这种情况"). +/// +/// Two repairs, both provable from the authored tree: +/// 1. **One image per slot** - when a wrapper holds an image-filled frame AND +/// an image node, the image NODE wins (it is what the image pipeline fills +/// and re-searches); the duplicate fill-frame is deleted. +/// 2. **The band keeps the photo's height** - the photo the model authored for +/// the slot declares a definite height; a wrapper shorter than half of it is +/// an authoring slip, not art direction, so the wrapper grows to the photo's +/// height. (A wrapper that already fits, or crops only mildly, is left +/// alone - intentional letterboxing stays intentional.) +const IMAGE_BAND_MIN_VISIBLE_FRACTION: f64 = 0.5; + +fn is_image_filled_frame(v: &Value) -> bool { + matches!( + v.get("type").and_then(Value::as_str), + Some("frame" | "group" | "rectangle") + ) && v + .get("fill") + .and_then(Value::as_array) + .is_some_and(|fills| { + fills.iter().any(|f| { + f.get("type").and_then(Value::as_str) == Some("image") + || f.get("url").is_some() + || f.get("src").is_some() + }) + }) +} + +fn fixed_height(v: &Value) -> Option { + match v.get("height") { + Some(Value::Number(n)) => n.as_f64(), + Some(Value::String(s)) => s.parse::().ok(), + _ => None, + } +} + +fn collect_image_slot_fixes(v: &Value, cmds: &mut Vec) { + let kids = children(v); + let image_nodes: Vec<&Value> = kids + .iter() + .filter(|c| c.get("type").and_then(Value::as_str) == Some("image")) + .collect(); + let filled_frames: Vec<&Value> = kids.iter().filter(|c| is_image_filled_frame(c)).collect(); + + // (1) One image per slot: the node wins, the duplicate fill-frame goes. + // Only a CHILDLESS plate qualifies — a filled frame that carries content + // (a hero with a headline on it) is a real container, not a stray twin. + if !image_nodes.is_empty() { + for frame in filled_frames.iter().filter(|f| children(f).is_empty()) { + if let Some(id) = frame.get("id").and_then(Value::as_str) { + cmds.push(EditorCommand::DeleteNode { + node_id: NodeId::new(id.to_string()), + page_id: None, + }); + } + } + } + + // (2) The band keeps the photo's height. The photo's own declared height + // is the model's intent for the slot; take it from whichever carrier is + // surviving repair (1). + if let Some(band) = fixed_height(v) { + let photo_h = if image_nodes.is_empty() { + filled_frames + .iter() + .filter_map(|f| fixed_height(f)) + .fold(0.0, f64::max) + } else { + image_nodes + .iter() + .chain(filled_frames.iter()) + .filter_map(|f| fixed_height(f)) + .fold(0.0, f64::max) + }; + if photo_h > 0.0 && band < photo_h * IMAGE_BAND_MIN_VISIBLE_FRACTION { + if let Some(id) = v.get("id").and_then(Value::as_str) { + cmds.push(EditorCommand::UpdateNode { + node_id: NodeId::new(id.to_string()), + x: None, + y: None, + width: None, + height: Some(photo_h.round() as i32), + name: None, + fill_hex: None, + page_id: None, + }); + } + } + } + for c in kids { + collect_image_slot_fixes(c, cmds); + } +} + /// A frame declaring a NUMERIC height but resolving MUCH taller — an /// oversized child inflated it (jian grows the parent instead of letting the /// child spill, so no edge ever crosses another and the width-overflow echo diff --git a/crates/op-orchestrator/src/geometry_validation_tests.rs b/crates/op-orchestrator/src/geometry_validation_tests.rs index 3a967c221..14f6c3bd7 100644 --- a/crates/op-orchestrator/src/geometry_validation_tests.rs +++ b/crates/op-orchestrator/src/geometry_validation_tests.rs @@ -2118,21 +2118,29 @@ fn starved_rail_rects() -> std::collections::HashMap { } #[test] -fn starved_rail_cards_hug_and_rail_becomes_scroller() { +fn starved_rail_cards_take_their_demand_and_the_rail_becomes_a_scroller() { let mut cmds = Vec::new(); collect_starved_rail_card_fixes(&starved_rail(), &starved_rail_rects(), &mut cmds); - let hugged: Vec<&str> = cmds + let sized: Vec<(&str, i32)> = cmds .iter() .filter_map(|c| match c { - EditorCommand::SetNodeLayoutProp { + EditorCommand::UpdateNode { node_id, - property, - value: LayoutPropValue::Keyword(k), - } if property == "width" && k == "fit_content" => Some(node_id.as_str()), + width: Some(w), + .. + } => Some((node_id.as_str(), *w)), _ => None, }) .collect(); - assert_eq!(hugged.len(), 5, "all five cards hug: {cmds:?}"); + assert_eq!( + sized.len(), + 5, + "all five cards take a definite width: {cmds:?}" + ); + assert!( + sized.iter().all(|(_, w)| *w == 188), + "each card takes its widest fixed content as its width: {sized:?}" + ); let rail_clipped = cmds.iter().any(|c| { matches!(c, EditorCommand::SetNodeLayoutProp { node_id, property, value: LayoutPropValue::Bool(true) } @@ -2209,3 +2217,111 @@ fn rail_with_one_flexible_card_is_not_forced_to_hug() { collect_starved_rail_card_fixes(&rail, &starved_rail_rects(), &mut cmds); assert!(cmds.is_empty(), "mixed rail left to the echo: {cmds:?}"); } + +// ── one image per slot + un-cropped photo band ── + +/// The measured test0711-1-glm card: a 56px image band holding BOTH a +/// 200x130 image-filled plate and the real image node. +fn double_image_slot() -> serde_json::Value { + json!({ + "type": "frame", "id": "band", "name": "Dest Image Bali", + "width": "fill_container", "height": 56, "layout": "horizontal", + "clipContent": true, + "children": [ + { "type": "frame", "id": "plate", "name": "Dest Image Fill Bali", + "width": 200, "height": 130, "x": 0, "y": 0, + "fill": [{ "type": "image", "url": "data:image/jpeg;base64,AAAA" }] }, + { "type": "icon_font", "id": "heart", "width": 22, "height": 22, "x": 165, "y": 10 }, + { "type": "image", "id": "photo", "name": "Bali temple rice terraces", + "width": "fill_container", "height": "fill_container", + "src": "data:image/jpeg;base64,BBBB" } + ] + }) +} + +#[test] +fn duplicate_image_plate_is_dropped_and_the_band_keeps_the_photo_height() { + let mut cmds = Vec::new(); + collect_image_slot_fixes(&double_image_slot(), &mut cmds); + + assert!( + cmds.iter().any(|c| matches!(c, + EditorCommand::DeleteNode { node_id, .. } if node_id.as_str() == "plate")), + "the childless image plate is the duplicate — the image node wins: {cmds:?}" + ); + assert!( + !cmds.iter().any(|c| matches!(c, + EditorCommand::DeleteNode { node_id, .. } if node_id.as_str() == "photo")), + "the real image node survives" + ); + let grown = cmds.iter().find_map(|c| match c { + EditorCommand::UpdateNode { + node_id, height, .. + } if node_id.as_str() == "band" => *height, + _ => None, + }); + assert_eq!( + grown, + Some(130), + "a 56px band cropped a 130px photo to its sky — the band takes the photo's height" + ); +} + +#[test] +fn a_lone_image_filled_plate_is_never_deleted() { + // No image node in the slot — the plate IS the image. Untouched. + let slot = json!({ + "type": "frame", "id": "band", "width": "fill_container", "height": 130, + "children": [ + { "type": "frame", "id": "plate", "width": 200, "height": 130, + "fill": [{ "type": "image", "url": "data:image/jpeg;base64,AAAA" }] } + ] + }); + let mut cmds = Vec::new(); + collect_image_slot_fixes(&slot, &mut cmds); + assert!( + cmds.is_empty(), + "the only image in the slot stays: {cmds:?}" + ); +} + +#[test] +fn an_image_filled_hero_with_content_on_it_is_not_a_duplicate_plate() { + // A filled frame that CARRIES content is a real container (hero with a + // headline), not a stray twin — even beside an image node. + let slot = json!({ + "type": "frame", "id": "band", "width": "fill_container", "height": 200, + "children": [ + { "type": "frame", "id": "hero", "width": 200, "height": 200, + "fill": [{ "type": "image", "url": "data:image/jpeg;base64,AAAA" }], + "children": [{ "type": "text", "id": "headline", "content": "Explore" }] }, + { "type": "image", "id": "photo", "width": 80, "height": 80, + "src": "data:image/jpeg;base64,BBBB" } + ] + }); + let mut cmds = Vec::new(); + collect_image_slot_fixes(&slot, &mut cmds); + assert!( + !cmds + .iter() + .any(|c| matches!(c, EditorCommand::DeleteNode { .. })), + "a hero container is not deleted: {cmds:?}" + ); +} + +#[test] +fn a_mild_crop_stays_intentional() { + // 130px photo in a 100px band — letterboxing the model may well have + // meant. Only a band that hides MORE than half the photo is repaired. + let slot = json!({ + "type": "frame", "id": "band", "width": "fill_container", "height": 100, + "clipContent": true, + "children": [ + { "type": "image", "id": "photo", "width": 200, "height": 130, + "src": "data:image/jpeg;base64,BBBB" } + ] + }); + let mut cmds = Vec::new(); + collect_image_slot_fixes(&slot, &mut cmds); + assert!(cmds.is_empty(), "a mild crop is left alone: {cmds:?}"); +}