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]