From 7f4f2ff122e4212ef2ac7714babb5a66343dde13 Mon Sep 17 00:00:00 2001 From: "@permadeath.com" Date: Wed, 19 Aug 2026 15:16:16 -0400 Subject: [PATCH] fix(unit-rules): a rewrite edits the document, it does not rebuild one Elements beside the units were dropped entirely: , which is how a player organised their lances, and a crew that got out. Only entities were kept, and everything the root held went with the root. The document is now kept whole, with each entity held once and its place marked, and locations are updated where they stand rather than stripped and re-appended - which reordered anything a file carried after them. Conformance compares the whole document across all 30 shipped files rather than the model, since the model only covers the parts helm understands. Change-Id: I9079bd943a5c125ca085559523bae25db870414f --- TODO.md | 9 + crates/helm-bv/tests/conformance.rs | 26 ++- crates/helm-unitfile/src/lib.rs | 2 +- crates/helm-unitfile/src/mul.rs | 272 +++++++++++++++++++++++----- 4 files changed, 250 insertions(+), 59 deletions(-) diff --git a/TODO.md b/TODO.md index 5a8f747..bdd98e0 100644 --- a/TODO.md +++ b/TODO.md @@ -346,6 +346,15 @@ not obvious from any one of them. `0.51.00`, and rewriting one as the other destroyed the only record of where a force came from. + A rewrite is an *edit* of the document that arrived, never a rebuild of + one from the model. Four separate losses came from getting that wrong - + dropped attributes, dropped `` elements, a rewritten version + string, reordered locations - and each was silent. The conformance test + now compares the whole document element for element and attribute for + attribute across every `.mul` the install ships, because comparing the + *model* only proves the parts helm understands survive, which is the + easy half. + What the metadata has to carry, so a stored figure can be told from one computed against something else: `helm_bv::CONFORMED_TO_MEGAMEK`, the MegaMek these rules agree with, and `helm_bv::RULES_VERSION`, the build diff --git a/crates/helm-bv/tests/conformance.rs b/crates/helm-bv/tests/conformance.rs index f007c58..748cacf 100644 --- a/crates/helm-bv/tests/conformance.rs +++ b/crates/helm-bv/tests/conformance.rs @@ -433,22 +433,18 @@ fn every_mul_megamek_ships_survives_a_round_trip() { continue; } }; - // What the force *is*, not the element tree it came in: the tree is - // allowed to tidy - a plate this crate does not record is not written - // back - while the units, their crews and their damage may not move. - let same = once.units.len() == twice.units.len() - && once.units.iter().zip(&twice.units).all(|(a, b)| { - (&a.chassis, &a.model, a.gunnery, a.piloting, a.pilot_hits) - == (&b.chassis, &b.model, b.gunnery, b.piloting, b.pilot_hits) - && a.armor == b.armor - && a.structure == b.structure - && a.destroyed == b.destroyed - && a.empty_ammo == b.empty_ammo - }); - if !same { + // The whole document, element for element and attribute for + // attribute - not just the parts this crate understands. A `.mul` + // carries ``, quirks, bomb loads and camouflage that helm has + // no rules about, and those are exactly the things a rewrite drops + // without anything noticing. + if once.root != twice.root { + let (a, b) = (&once.root, &twice.root); wrong.push(format!( - "{}: the force changed on the way round", - path.display() + "{}: the document changed on the way round ({} children became {})", + path.display(), + a.children.len(), + b.children.len() )); continue; } diff --git a/crates/helm-unitfile/src/lib.rs b/crates/helm-unitfile/src/lib.rs index 9ea3abe..123809d 100644 --- a/crates/helm-unitfile/src/lib.rs +++ b/crates/helm-unitfile/src/lib.rs @@ -27,7 +27,7 @@ mod mul; pub use blk::parse_blk; pub use mtf::{armor_location_order, parse_mtf, split_pipe_list, split_system_field}; -pub use mul::{MUL_VERSION, Mul, MulUnit, parse_mul, write_mul}; +pub use mul::{Kept, MUL_VERSION, Mul, MulUnit, parse_mul, write_mul}; #[cfg(feature = "library")] mod library; diff --git a/crates/helm-unitfile/src/mul.rs b/crates/helm-unitfile/src/mul.rs index 28749d5..407cb0c 100644 --- a/crates/helm-unitfile/src/mul.rs +++ b/crates/helm-unitfile/src/mul.rs @@ -112,9 +112,15 @@ pub const MUL_VERSION: &str = "0.51.00"; pub struct Mul { /// Every entity in the file, crews that got out included. pub units: Vec, - /// What the file said it was written by, if it said. Kept so it can be - /// written back, and so a reader can tell MegaMek's files from another - /// tool's. + /// The document itself, entities included, so that everything beside them + /// survives being written back. + /// + /// A `.mul` holds more than units: `` records how they are + /// organised into lances, and a crew that got out of one unit and into + /// another is its own element. Keeping only the entities loses all of it, + /// and loses it silently. + pub root: Kept, + /// What the file said it was written by, if it said. pub version: Option, } @@ -243,13 +249,7 @@ pub fn parse_mul(text: &str) -> Result { loop { match reader.read_event().map_err(|e| Error(e.to_string()))? { Event::Eof => break, - Event::Start(tag) => { - let kept = element(&tag)?; - if kept.name == "unit" && mul.version.is_none() { - mul.version = kept.get("version").map(str::to_string); - } - open.push(kept); - } + Event::Start(tag) => open.push(element(&tag)?), Event::Empty(tag) => { let done = element(&tag)?; close(&mut open, done, &mut mul); @@ -262,16 +262,36 @@ pub fn parse_mul(text: &str) -> Result { _ => {} } } + // A file that ends without closing its tags still has whatever it said. + while let Some(done) = open.pop() { + close(&mut open, done, &mut mul); + } + mul.version = mul.root.get("version").map(str::to_string); Ok(mul) } /// Attach a finished element to whatever contains it - or, if it is an entity, /// read it into the model. fn close(open: &mut [Kept], done: Kept, mul: &mut Mul) { - if done.name == "entity" { + // An entity is read into the model, and the document keeps an empty one in + // its place. The position is what the document needs - a `.mul` can carry + // `` and a crew that got out beside its units, and writing them + // back in the wrong order is still a change to somebody's file - while the + // entity itself is held once, by the unit that speaks for it. + let done = if done.name == "entity" { + let name = done.name.clone(); mul.units.push(read_entity(done)); - } else if let Some(parent) = open.last_mut() { - parent.children.push(done); + Kept { + name, + ..Default::default() + } + } else { + done + }; + match open.last_mut() { + Some(parent) => parent.children.push(done), + // Nothing contains it, so it is the document. + None => mul.root = done, } } @@ -421,14 +441,51 @@ fn read_slot(unit: &mut MulUnit, name: &str, slot_tag: &Kept) { /// numbered from one, and a location with nothing to say is not written at all /// rather than written as zero. pub fn write_mul(mul: &Mul) -> String { - let mut out = String::from("\n\n\n"); - for unit in &mul.units { - write_element(&mut out, &entity_of(unit), 1); + let mut out = String::from("\n\n"); + + // Written from the document that arrived, so that everything beside the + // units - how they are organised into lances, a crew that got out - is + // still there afterwards. Each entity is replaced by its updated self as + // it is reached, which keeps the order of the file. + let mut root = if mul.root.name == "unit" { + mul.root.clone() + } else { + Kept { + name: "unit".to_string(), + attributes: vec![( + "version".to_string(), + mul.version + .clone() + .unwrap_or_else(|| MUL_VERSION.to_string()), + )], + children: mul + .units + .iter() + .map(|_| Kept { + name: "entity".to_string(), + ..Default::default() + }) + .collect(), + } + }; + if let Some(version) = &mul.version { + root.set("version", version); } - out.push_str("\n"); + + let mut units = mul.units.iter(); + for child in &mut root.children { + if child.name == "entity" + && let Some(unit) = units.next() + { + *child = entity_of(unit); + } + } + // A unit added since the file was read has nowhere to go yet. + for unit in units { + root.children.push(entity_of(unit)); + } + + write_element(&mut out, &root, 0); out } @@ -475,11 +532,12 @@ fn entity_of(unit: &MulUnit) -> Kept { pilot.set("hits", unit.pilot_hits.to_string()); } - // Locations are updated rather than rebuilt: a slot says which equipment - // is in it and a destroyed location says `Destroyed` rather than nought, - // and neither is this crate's to invent or to throw away. + // Locations are updated where they stand, not stripped and re-appended: + // a `.mul` may carry elements after them that this crate knows nothing + // about, and moving the locations past those reorders somebody else's + // document for no reason. let shape = locations_for(unit.unit_type.as_deref()); - let mut rebuilt: Vec = Vec::new(); + let mut fresh: Vec<(usize, Kept)> = Vec::new(); for (index, (code, name)) in shape.iter().enumerate() { let rear = format!("RT{}", code.chars().next().unwrap_or('C')); let front = unit.armor.get(*code); @@ -487,32 +545,29 @@ fn entity_of(unit: &MulUnit) -> Kept { let internal = unit.structure.get(*code); let gone = unit.destroyed.get(*name); let empty = unit.empty_ammo.get(*name); - - let existing = entity - .children - .iter() - .find(|c| c.name == "location" && c.number("index") == Some(index as i64)) - .cloned(); if front.is_none() && behind.is_none() && internal.is_none() && gone.is_none() && empty.is_none() { - // Nothing this crate has to say, so keep whatever the file did. - if let Some(kept) = existing { - rebuilt.push(kept); - } + // Nothing this crate has to say, so whatever the file said stands. continue; } - let mut location = existing.unwrap_or(Kept { - name: "location".to_string(), - attributes: vec![("index".to_string(), index.to_string())], - children: Vec::new(), - }); + let existing = entity + .children + .iter() + .position(|c| c.name == "location" && c.number("index") == Some(index as i64)); + let mut location = match existing { + Some(at) => entity.children[at].clone(), + None => Kept { + name: "location".to_string(), + attributes: vec![("index".to_string(), index.to_string())], + children: Vec::new(), + }, + }; location.set("index", index.to_string()); - for (points, kind) in [ (front, None), (behind, Some("Rear")), @@ -521,10 +576,15 @@ fn entity_of(unit: &MulUnit) -> Kept { set_armor(&mut location, points, kind); } set_slots(&mut location, gone, empty); - rebuilt.push(location); + + match existing { + Some(at) => entity.children[at] = location, + None => fresh.push((index, location)), + } } - entity.children.retain(|c| c.name != "location"); - entity.children.extend(rebuilt); + // A location the file did not mention goes after the ones it did. + fresh.sort_by_key(|(index, _)| *index); + entity.children.extend(fresh.into_iter().map(|(_, l)| l)); entity } @@ -1047,6 +1107,132 @@ mod tests { assert!(write_mul(&ours).contains(&format!(r#""#))); } + /// The document as a string that ignores whitespace and formatting but + /// nothing else: every element, in order, with every attribute and value. + /// + /// This is what a round trip has to preserve. Comparing the *model* only + /// proves the parts this crate understands survive, which is the easy + /// half - ``, a crew that got out, a quirk, a bomb load and a + /// camouflage are all things it does not understand and must not lose. + fn canonical(kept: &Kept) -> String { + let mut out = String::new(); + fn walk(k: &Kept, depth: usize, out: &mut String) { + out.push_str(&" ".repeat(depth)); + out.push_str(&k.name); + // Sorted, because attribute order is not meaning; presence and + // value are. + let mut attrs: Vec<_> = k.attributes.iter().collect(); + attrs.sort(); + for (name, value) in attrs { + out.push(' '); + out.push_str(name); + out.push('='); + out.push_str(value); + } + out.push('\n'); + for child in &k.children { + walk(child, depth + 1, out); + } + } + walk(kept, 0, &mut out); + out + } + + /// Read, write, read: the second document must be the first, element for + /// element and attribute for attribute. + fn survives_structurally(xml: &str) -> Result<(), String> { + let once = parse_mul(xml).map_err(|e| e.to_string())?; + let twice = parse_mul(&write_mul(&once)).map_err(|e| e.to_string())?; + let (a, b) = (canonical(&once.root), canonical(&twice.root)); + if a == b { + return Ok(()); + } + let differ = a + .lines() + .zip(b.lines()) + .find(|(x, y)| x != y) + .map(|(x, y)| format!("\n was: {x}\n now: {y}")) + .unwrap_or_else(|| { + format!( + "\n {} lines became {}", + a.lines().count(), + b.lines().count() + ) + }); + Err(differ) + } + + #[test] + fn the_whole_document_survives_a_round_trip() { + survives_structurally(AFTER_A_FIGHT).unwrap(); + } + + // The things this crate has no rules about are the ones a round trip + // quietly drops, because nothing else notices they are gone. `` + // says how a player organised their lances. + #[test] + fn elements_this_crate_knows_nothing_about_survive() { + const EXOTIC: &str = r#" + + + + + + + + Center Torso + + + + + + +"#; + survives_structurally(EXOTIC).unwrap(); + + // And specifically the ones that live beside the units rather than + // inside them, which is where a reader that keeps only entities loses + // everything. + let written = write_mul(&parse_mul(EXOTIC).unwrap()); + for kept in [ + " = a.lines().zip(b.lines()).filter(|(x, y)| x != y).collect(); + assert_eq!(changed.len(), 1, "more than the camo changed: {changed:?}"); + assert!(changed[0].1.contains("camoCategory=Clans/Ghost Bear")); + } + // `type` must not match `crewType`, and `armor` must not match // `armorDivisor`: an attribute name only counts at a word boundary. #[test] -- 2.51.2