diff --git a/TODO.md b/TODO.md index 5f14352..c368cd7 100644 --- a/TODO.md +++ b/TODO.md @@ -323,16 +323,19 @@ The format lance.blue works in, so these are the jobs it has to do. Written down because the first of them changes what the writer has to be, and that is not obvious from any one of them. -- [ ] **A rewrite must not lose what it did not understand.** The blocker for - most of the rest. `write_mul` builds a file from what this crate models, +- [x] **A rewrite must not lose what it did not understand.** Was the blocker + for most of the rest. `write_mul` builds a file from what this crate models, and a `.mul` MegaMek wrote carries 36 attributes where we model 13: a round trip through lance.blue would drop `externalId`, `camoCategory`, `camoFileName`, `edge`, `deployment`, `gender`, `nick` and sixteen more. - Storing a player's file and handing it back is data loss with no error - and no way to notice. Everything below that writes a file needs this - first: keep every attribute and element that came in, change only what - was meant to change. + Storing a player's file and handing it back would have been data loss + with no error and no way to notice. Each unit now keeps the element it + was read from, and writing starts there and changes only what the model + says - so a camo, a portrait, an `externalId` and any tag a later + MegaMek adds all survive. A location says `Destroyed` rather than a + nought this crate invented, and a slot still says which equipment is in + it. - [ ] **Take a `.mul` a player uploaded**, and say what is wrong with it before anything else touches it - designs not in the library, a crew diff --git a/crates/helm-bv/performance.txt b/crates/helm-bv/performance.txt index 9f440b5..a8dc04d 100644 --- a/crates/helm-bv/performance.txt +++ b/crates/helm-bv/performance.txt @@ -1,13 +1,16 @@ -# What each operation costs, in nanoseconds, fastest of several rounds of a -# release build. +# What each operation costs as a multiple of one `read a design`, not as +# a time. This machine's throughput moved by two fifths between one +# afternoon and the next with nothing changed, and an absolute figure +# would have failed for that alone. # -# A figure more than a fifth worse than these fails the performance test. -# Re-record with HELM_PERF_BLESS=1, and say in the commit why it moved. +# A figure more than a fifth worse than these fails the test. Re-record +# with HELM_PERF_BLESS=1, and say in the commit why it moved. # -# These are one machine's numbers. Another machine needs its own. +# A slowdown affecting everything equally does not show up here. The +# times after each figure are for a human to glance at. -parse a .mul 1909 -write a .mul 3001 -read a design 122784 -score a design 197382 -score a damaged design 198490 +parse a .mul 0.022 # 4153ns here +write a .mul 0.044 # 8207ns here +read a design 1.000 # 187576ns here +score a design 1.652 # 309941ns here +score a damaged design 1.669 # 313115ns here diff --git a/crates/helm-bv/tests/conformance.rs b/crates/helm-bv/tests/conformance.rs index 74da9e0..bb2f65d 100644 --- a/crates/helm-bv/tests/conformance.rs +++ b/crates/helm-bv/tests/conformance.rs @@ -433,7 +433,19 @@ fn every_mul_megamek_ships_survives_a_round_trip() { continue; } }; - if once != twice { + // 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 { wrong.push(format!( "{}: the force changed on the way round", path.display() diff --git a/crates/helm-bv/tests/performance.rs b/crates/helm-bv/tests/performance.rs index fb73170..e4abdbb 100644 --- a/crates/helm-bv/tests/performance.rs +++ b/crates/helm-bv/tests/performance.rs @@ -11,11 +11,29 @@ //! fails; `HELM_PERF_BLESS=1` writes the current figures down. //! //! Two things make the numbers steady enough to assert on. Each measurement is -//! the *fastest* of several runs rather than the average, because a machine -//! shared with other work has a floor and no ceiling - the fastest run is the -//! one that was least interrupted. And each run repeats the operation enough +//! the *fastest* of many rounds rather than the average, because a machine +//! shared with other work has a floor and no ceiling - the fastest round is +//! the one that was least interrupted, and enough rounds will find a quiet +//! window even on a busy machine. And each round repeats the operation enough //! times that a clock tick is noise. //! +//! What is *compared* is a ratio, not a time. This machine's throughput moved +//! by two fifths between one afternoon and the next with nothing changed, so +//! an absolute figure recorded on a quiet day fails on a busy one having +//! measured the weather. Every figure is divided by one of the crate's own +//! operations - reading a design - which is allocation-heavy in the same way +//! the rest is, and so moves with the machine rather than with this code. +//! +//! Normalising against a *synthetic* workload was tried first and was worse: a +//! dependent chain of arithmetic does not slow down the way allocation does, +//! and dividing by it added noise instead of removing it. +//! +//! The cost of the ratio is that a slowdown affecting everything equally is +//! invisible. That is the trade: this catches one part of the crate getting +//! slower than the rest, which is what a swapped dependency or a lookup turned +//! scan looks like, and it does so reliably. The times are printed beside the +//! ratios for a human to glance at. +//! //! The figures are still this machine's, and a release build's - an //! unoptimised one is several times slower and is printed rather than checked. //! Moving to another machine means blessing them again. @@ -25,8 +43,11 @@ use std::time::{Duration, Instant}; /// How much worse than the recorded figure is a failure. const TOLERANCE: f64 = 1.20; +/// The operation everything else is measured against. Real work, so it slows +/// down when the machine does. +const REFERENCE: &str = "read a design"; /// How many rounds every measurement is taken over; each keeps its fastest. -const RUNS: u32 = 7; +const RUNS: u32 = 25; struct Inputs { library: helm_unitfile::Library, @@ -149,19 +170,27 @@ fn nothing_has_got_materially_slower() { }, ]); - // Debug is several times slower than release and the two sets of figures - // are not comparable: blessing in one profile and checking in the other - // would fail everything for no reason. Only release is held to the mark. + // Against one of our own operations rather than the clock. + let reference = measured + .iter() + .find(|(what, _)| *what == REFERENCE) + .map(|(_, took)| took.as_secs_f64()) + .filter(|t| *t > 0.0) + .expect("the reference operation was measured"); + let ratios: Vec<(&str, f64, Duration)> = measured + .iter() + .map(|(what, took)| (*what, took.as_secs_f64() / reference, *took)) + .collect(); + + // Debug is a different shape, not just a slower one, and its ratios are + // not comparable. Printed rather than checked. if cfg!(debug_assertions) { - println!("{}", render(&measured)); - println!( - "unoptimised build, so these are not compared. \ - Run with --release to hold them to performance.txt." - ); + println!("{}", render(&ratios)); + println!("unoptimised build, so these are not compared. Run with --release."); return; } - let report = render(&measured); + let report = render(&ratios); if std::env::var("HELM_PERF_BLESS").is_ok() { std::fs::write(report_path(), &report).expect("write performance.txt"); println!("blessed:\n{report}"); @@ -175,23 +204,24 @@ fn nothing_has_got_materially_slower() { let mut slower = Vec::new(); println!( - "{:<26} {:>10} {:>10} {:>8}", - "", "recorded", "now", "change" + "{:<26} {:>10} {:>10} {:>8} {:>11}", + "", "recorded", "now", "change", "time here" ); - for (what, took) in &measured { - let now = took.as_secs_f64() * 1e9; + for (what, ratio, took) in &ratios { + let now = *ratio; + let ns = took.as_secs_f64() * 1e9; let Some(before) = recorded.get(*what) else { - println!("{what:<26} {:>10} {now:>10.0} {:>8}", "-", "new"); + println!("{what:<26} {:>10} {now:>10.3} {:>8}", "-", "new"); continue; }; let change = now / before; println!( - "{what:<26} {before:>10.0} {now:>10.0} {:>7.0}%", + "{what:<26} {before:>10.3} {now:>10.3} {:>7.0}% {ns:>9.0}ns", (change - 1.0) * 100.0 ); if change > TOLERANCE { slower.push(format!( - "{what} takes {now:.0}ns against {before:.0}ns recorded, {:.0}% worse", + "{what} is {now:.3} of a {REFERENCE} against {before:.3} recorded, {:.0}% worse", (change - 1.0) * 100.0 )); } @@ -204,18 +234,26 @@ fn nothing_has_got_materially_slower() { ); } -fn render(measured: &[(&str, Duration)]) -> String { +fn render(measured: &[(&str, f64, Duration)]) -> String { let mut out = String::from( - "# What each operation costs, in nanoseconds, fastest of several rounds of a\n\ - # release build.\n\ + "# What each operation costs as a multiple of one `read a design`, not as\n\ + # a time. This machine's throughput moved by two fifths between one\n\ + # afternoon and the next with nothing changed, and an absolute figure\n\ + # would have failed for that alone.\n\ #\n\ - # A figure more than a fifth worse than these fails the performance test.\n\ - # Re-record with HELM_PERF_BLESS=1, and say in the commit why it moved.\n\ + # A figure more than a fifth worse than these fails the test. Re-record\n\ + # with HELM_PERF_BLESS=1, and say in the commit why it moved.\n\ #\n\ - # These are one machine's numbers. Another machine needs its own.\n\n", + # A slowdown affecting everything equally does not show up here. The\n\ + # times after each figure are for a human to glance at.\n\n", ); - for (what, took) in measured { - out.push_str(&format!("{what:<26} {:.0}\n", took.as_secs_f64() * 1e9)); + for (what, ratio, took) in measured { + // The time is a comment: it is what a human wants and what a machine + // cannot be held to. + out.push_str(&format!( + "{what:<26} {ratio:>8.3} # {:.0}ns here\n", + took.as_secs_f64() * 1e9 + )); } out } @@ -224,7 +262,10 @@ fn parse_recorded(text: &str) -> std::collections::BTreeMap { text.lines() .filter(|l| !l.trim_start().starts_with('#') && !l.trim().is_empty()) .filter_map(|l| { - let at = l.rfind(char::is_whitespace)?; + // Each line is `name ratio # time here`; the time is a note for + // a human and no part of what is compared. + let l = l.split('#').next()?; + let at = l.trim_end().rfind(char::is_whitespace)?; Some((l[..at].trim().to_string(), l[at..].trim().parse().ok()?)) }) .collect() diff --git a/crates/helm-unitfile/src/mul.rs b/crates/helm-unitfile/src/mul.rs index 23405cc..a5a8eb6 100644 --- a/crates/helm-unitfile/src/mul.rs +++ b/crates/helm-unitfile/src/mul.rs @@ -44,6 +44,49 @@ use std::collections::{BTreeMap, BTreeSet}; use quick_xml::Reader; use quick_xml::events::Event; +/// One element as it arrived, kept so that writing a force back does not throw +/// away what this crate has no use for. +/// +/// A `.mul` MegaMek wrote carries thirty-six attributes where this models +/// thirteen: a unit's camouflage, its portrait, its edge, the identifier a +/// campaign tracks it by. Rebuilding a file from the model alone drops every +/// one of them, so a player who stored a force here and took it back would +/// find their camouflage gone and nothing said about it. +#[derive(Debug, Clone, Default, PartialEq, Eq)] +pub struct Kept { + pub name: String, + /// Attributes in the order they arrived. + pub attributes: Vec<(String, String)>, + pub children: Vec, +} + +impl Kept { + /// Read one attribute. + pub fn get(&self, name: &str) -> Option<&str> { + self.attributes + .iter() + .find(|(k, _)| k == name) + .map(|(_, v)| v.as_str()) + } + + /// Set one, in place if it is already there so the order does not shift. + pub fn set(&mut self, name: &str, value: impl Into) { + let value = value.into(); + match self.attributes.iter_mut().find(|(k, _)| k == name) { + Some((_, held)) => *held = value, + None => self.attributes.push((name.to_string(), value)), + } + } + + fn number(&self, name: &str) -> Option { + self.get(name)?.trim().parse().ok() + } + + fn children_named<'a>(&'a self, name: &'a str) -> impl Iterator { + self.children.iter().filter(move |c| c.name == name) + } +} + /// Why a `.mul` could not be read. #[derive(Debug)] pub struct Error(String); @@ -78,6 +121,11 @@ impl Mul { /// location's full name, which is how the criticals are keyed. #[derive(Debug, Clone, Default, PartialEq, Eq)] pub struct MulUnit { + /// The entity exactly as it arrived. Empty for one this crate made up. + /// + /// Writing a force starts from this and changes only what the fields below + /// say, so anything not modelled here survives the trip. + pub source: Kept, pub chassis: String, pub model: String, /// `Biped`, `Tank`, and so on. Absent in some files. @@ -167,125 +215,127 @@ fn locations_for(kind: Option<&str>) -> &'static [(&'static str, &'static str)] /// Read a `.mul`. /// -/// Unknown tags and attributes are ignored rather than refused: a `.mul` -/// carries a great deal this crate has no use for - camouflage, deployment -/// zones, edge - and a MegaMek that adds more should not stop the file being -/// readable. +/// Every element is kept as it arrived and then the parts this crate has rules +/// about are read off it. Unknown tags and attributes are neither refused nor +/// dropped: a `.mul` carries a great deal this has no use for - camouflage, +/// deployment zones, edge - and both losing it and failing on it would be +/// wrong for a file somebody else owns. pub fn parse_mul(text: &str) -> Result { let mut reader = Reader::from_str(text); reader.config_mut().trim_text(true); let mut mul = Mul::default(); - let mut unit: Option = None; - let mut location: Option = None; + // Elements still open, innermost last. + let mut open: Vec = Vec::new(); loop { - match reader.read_event() { - Err(e) => return Err(Error(e.to_string())), - Ok(Event::Eof) => break, - Ok(Event::Start(tag) | Event::Empty(tag)) => { - let name = tag.name(); - let attrs = Attrs::read(&tag).map_err(Error)?; - match name.as_ref() { - b"entity" => { - if let Some(done) = unit.take() { - mul.units.push(done); - } - unit = Some(MulUnit { - chassis: attrs.get("chassis").unwrap_or_default(), - model: attrs.get("model").unwrap_or_default(), - unit_type: attrs.get("type"), - // A crew the file does not describe is the regular - // 4/5, which is what an unqualified battle value - // assumes. - gunnery: 4, - piloting: 5, - ..Default::default() - }); - location = None; - } - b"pilot" | b"crewMember" => { - if let Some(u) = unit.as_mut() { - u.pilot_name = attrs.get("name"); - if let Some(g) = attrs.number("gunnery") { - u.gunnery = g as u8; - } - if let Some(p) = attrs.number("piloting") { - u.piloting = p as u8; - } - u.pilot_hits = attrs.number("hits").unwrap_or(0) as u8; - } - } - b"location" => { - location = attrs.number("index").and_then(|n| usize::try_from(n).ok()); - } - b"armor" => { - if let (Some(u), Some(index)) = (unit.as_mut(), location) { - read_armor(u, index, &attrs); - } - } - b"slot" => { - if let (Some(u), Some(index)) = (unit.as_mut(), location) { - read_slot(u, index, &attrs); - } - } - _ => {} + match reader.read_event().map_err(|e| Error(e.to_string()))? { + Event::Eof => break, + Event::Start(tag) => open.push(element(&tag)?), + Event::Empty(tag) => { + let done = element(&tag)?; + close(&mut open, done, &mut mul); + } + Event::End(_) => { + if let Some(done) = open.pop() { + close(&mut open, done, &mut mul); } } - Ok(_) => {} + _ => {} } } - if let Some(done) = unit.take() { - mul.units.push(done); - } Ok(mul) } -/// One tag's attributes, unescaped once. -#[derive(Default)] -struct Attrs(Vec<(String, String)>); - -impl Attrs { - fn read(tag: &quick_xml::events::BytesStart<'_>) -> Result { - let mut out = Vec::new(); - for attr in tag.attributes() { - let attr = attr.map_err(|e| e.to_string())?; - let key = String::from_utf8_lossy(attr.key.as_ref()).into_owned(); - // Unescaped here and nowhere else: `&` in a chassis name is a - // real ampersand, and a name that keeps the escape matches no - // design in the library. - let value = attr - .unescape_value() - .map_err(|e| e.to_string())? - .into_owned(); - out.push((key, value)); - } - Ok(Attrs(out)) +/// 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" { + mul.units.push(read_entity(done)); + } else if let Some(parent) = open.last_mut() { + parent.children.push(done); } +} - fn get(&self, name: &str) -> Option { - self.0 - .iter() - .find(|(k, _)| k == name) - .map(|(_, v)| v.clone()) +/// One element and its attributes, unescaped once. +fn element(tag: &quick_xml::events::BytesStart<'_>) -> Result { + let mut attributes = Vec::new(); + for attr in tag.attributes() { + let attr = attr.map_err(|e| Error(e.to_string()))?; + // Unescaped here and nowhere else: `&` in a chassis name is a real + // ampersand, and a name that keeps the escape matches no design. + attributes.push(( + String::from_utf8_lossy(attr.key.as_ref()).into_owned(), + attr.unescape_value() + .map_err(|e| Error(e.to_string()))? + .into_owned(), + )); } + Ok(Kept { + name: String::from_utf8_lossy(tag.name().as_ref()).into_owned(), + attributes, + children: Vec::new(), + }) +} - fn number(&self, name: &str) -> Option { - self.get(name)?.trim().parse().ok() +/// Read the parts of an entity this crate has rules about, keeping the rest. +fn read_entity(source: Kept) -> MulUnit { + let mut unit = MulUnit { + chassis: source.get("chassis").unwrap_or_default().to_string(), + model: source.get("model").unwrap_or_default().to_string(), + unit_type: source.get("type").map(str::to_string), + // A crew the file does not describe is the regular 4/5, which is what + // an unqualified battle value assumes. + gunnery: 4, + piloting: 5, + ..Default::default() + }; + + if let Some(pilot) = source + .children + .iter() + .find(|c| c.name == "pilot" || c.name == "crewMember") + { + unit.pilot_name = pilot.get("name").map(str::to_string); + if let Some(g) = pilot.number("gunnery") { + unit.gunnery = g as u8; + } + if let Some(p) = pilot.number("piloting") { + unit.piloting = p as u8; + } + unit.pilot_hits = pilot.number("hits").unwrap_or(0) as u8; } + + let shape = locations_for(unit.unit_type.as_deref()); + for location in source.children_named("location") { + let Some(index) = location + .number("index") + .and_then(|n| usize::try_from(n).ok()) + else { + continue; + }; + let Some((code, name)) = shape.get(index) else { + continue; + }; + for armor in location.children_named("armor") { + read_armor(&mut unit, code, armor); + } + for slot in location.children_named("slot") { + read_slot(&mut unit, name, slot); + } + } + unit.source = source; + unit } /// Plate or frame left in one location. -fn read_armor(unit: &mut MulUnit, index: usize, attrs: &Attrs) { - let Some((code, _)) = locations_for(unit.unit_type.as_deref()).get(index) else { - return; - }; - let Some(points) = attrs.get("points") else { +fn read_armor(unit: &mut MulUnit, code: &str, armor: &Kept) { + let Some(points) = armor.get("points") else { return; }; // `Destroyed` is nothing left; `N/A` is a location that never had any, // which is not damage and must not be recorded as zero. - let points = match points.as_str() { + let points = match points { "Destroyed" => 0, "N/A" => return, n => match n.parse::() { @@ -293,9 +343,9 @@ fn read_armor(unit: &mut MulUnit, index: usize, attrs: &Attrs) { Err(_) => return, }, }; - match attrs.get("type").as_deref() { + match armor.get("type") { Some("Internal") => { - unit.structure.insert((*code).to_string(), points); + unit.structure.insert(code.to_string(), points); } // Rear plate belongs to the same location and the `.mtf` spells it // RTC, RTR, RTL. @@ -304,36 +354,34 @@ fn read_armor(unit: &mut MulUnit, index: usize, attrs: &Attrs) { .insert(format!("RT{}", code.chars().next().unwrap_or('C')), points); } _ => { - unit.armor.insert((*code).to_string(), points); + unit.armor.insert(code.to_string(), points); } } } -/// One critical slot: shot out, or a magazine with something left in it. -fn read_slot(unit: &mut MulUnit, index: usize, attrs: &Attrs) { - let Some((_, name)) = locations_for(unit.unit_type.as_deref()).get(index) else { - return; - }; - let Some(slot) = attrs.number("index").and_then(|n| usize::try_from(n).ok()) else { +/// One critical slot: shot out, or a magazine with nothing left in it. +fn read_slot(unit: &mut MulUnit, name: &str, slot_tag: &Kept) { + let Some(slot) = slot_tag + .number("index") + .and_then(|n| usize::try_from(n).ok()) + else { return; }; // A `.mul` numbers slots from one and everything else here from zero. let Some(slot) = slot.checked_sub(1) else { return; }; - if attrs.get("isDestroyed").as_deref() == Some("true") - || attrs.get("isMissing").as_deref() == Some("true") - { + if slot_tag.get("isDestroyed") == Some("true") || slot_tag.get("isMissing") == Some("true") { unit.destroyed - .entry((*name).to_string()) + .entry(name.to_string()) .or_default() .insert(slot); } // Only a magazine that is written and empty. A bin the file does not // mention is full. - if attrs.number("shots") == Some(0) { + if slot_tag.number("shots") == Some(0) { unit.empty_ammo - .entry((*name).to_string()) + .entry(name.to_string()) .or_default() .insert(slot); } @@ -341,92 +389,247 @@ fn read_slot(unit: &mut MulUnit, index: usize, attrs: &Attrs) { /// Write a force back out as a `.mul`. /// -/// Enough of the format for MegaMek to read the file and put the units back in -/// the state they were in: which designs, who is flying them, and what each -/// has lost. A `.mul` MegaMek writes carries a great deal besides - camouflage, -/// deployment zones, edge, portraits - and none of it survives a trip through -/// here, so this writes a force rather than rewriting somebody's file. +/// Each unit is written from the element it was read from, with only what this +/// crate models changed. Everything else - a unit's camouflage, its portrait, +/// its edge, the identifier a campaign tracks it by, and any tag a later +/// MegaMek adds - is written back as it arrived. +/// +/// That is the difference between a force this can store for somebody and one +/// it can only compute over. A writer that rebuilt the file from the model +/// would hand a player back a force with their camouflage missing and nothing +/// said about it. /// /// The two indexing traps are undone on the way out: slots go back to being -/// numbered from one, and a location with nothing to say is not written at -/// all rather than written as zero. +/// 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 { - out.push_str(" \n"); + out +} + +/// One unit as an element: what it arrived as, brought up to date. +fn entity_of(unit: &MulUnit) -> Kept { + let mut entity = if unit.source.name == "entity" { + unit.source.clone() + } else { + Kept { + name: "entity".to_string(), + ..Default::default() } - out.push_str(">\n pilot, + None => { + entity.children.insert( + 0, + Kept { + name: "pilot".to_string(), + ..Default::default() + }, + ); + &mut entity.children[0] } - out.push_str(&format!( - " gunnery=\"{}\" piloting=\"{}\"", - unit.gunnery, unit.piloting - )); - if unit.pilot_hits > 0 { - out.push_str(&format!(" hits=\"{}\"", unit.pilot_hits)); + }; + if let Some(name) = &unit.pilot_name { + pilot.set("name", name); + } + pilot.set("gunnery", unit.gunnery.to_string()); + pilot.set("piloting", unit.piloting.to_string()); + if unit.pilot_hits > 0 { + 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. + let shape = locations_for(unit.unit_type.as_deref()); + let mut rebuilt: Vec = 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); + let behind = unit.armor.get(&rear); + 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); + } + continue; } - out.push_str("/>\n"); - for (index, (code, name)) in locations_for(unit.unit_type.as_deref()).iter().enumerate() { - let rear = format!("RT{}", code.chars().next().unwrap_or('C')); - let front = unit.armor.get(*code); - let behind = unit.armor.get(&rear); - let internal = unit.structure.get(*code); - let gone = unit.destroyed.get(*name); - let empty = unit.empty_ammo.get(*name); - if front.is_none() - && behind.is_none() - && internal.is_none() - && gone.is_none() - && empty.is_none() - { - continue; - } - out.push_str(&format!(" {name}\n")); - if let Some(points) = front { - out.push_str(&format!(" \n")); - } - if let Some(points) = behind { - out.push_str(&format!( - " \n" - )); - } - if let Some(points) = internal { - out.push_str(&format!( - " \n" - )); - } - // Back to one-based, which is how the format numbers them. - for slot in gone.into_iter().flatten() { - out.push_str(&format!( - " \n", - slot + 1 - )); + let mut location = existing.unwrap_or(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")), + (internal, Some("Internal")), + ] { + set_armor(&mut location, points, kind); + } + set_slots(&mut location, gone, empty); + rebuilt.push(location); + } + entity.children.retain(|c| c.name != "location"); + entity.children.extend(rebuilt); + entity +} + +/// Put one armour figure on a location, in the tag it already had if it has +/// one. +/// +/// A location whose plate is gone is written `Destroyed` by MegaMek, and that +/// word is kept while the figure is still nought: it is the same reading, and +/// replacing it with a number is a change this crate has no reason to make. +fn set_armor(location: &mut Kept, points: Option<&i64>, kind: Option<&str>) { + let existing = location + .children + .iter_mut() + .find(|c| c.name == "armor" && c.get("type") == kind); + let Some(points) = points else { + return; + }; + match existing { + Some(tag) => { + let unchanged = match tag.get("points") { + Some("Destroyed") => *points == 0, + Some(n) => n.parse::().ok() == Some(*points), + None => false, + }; + if !unchanged { + tag.set("points", points.to_string()); } - for slot in empty.into_iter().flatten() { - out.push_str(&format!( - " \n", - slot + 1 - )); + } + None => { + let mut tag = Kept { + name: "armor".to_string(), + attributes: vec![("points".to_string(), points.to_string())], + children: Vec::new(), + }; + if let Some(kind) = kind { + tag.set("type", kind); } - out.push_str(" \n"); + location.children.push(tag); + } + } +} + +/// Bring a location's slots up to date, keeping what each one says it holds. +fn set_slots(location: &mut Kept, gone: Option<&BTreeSet>, empty: Option<&BTreeSet>) { + let none = BTreeSet::new(); + let gone = gone.unwrap_or(&none); + let empty = empty.unwrap_or(&none); + + // Update the tags the file already has; a slot it mentions for its own + // reasons - which equipment is in it - keeps saying so. + for tag in location.children.iter_mut().filter(|c| c.name == "slot") { + let Some(index) = tag.number("index").and_then(|n| usize::try_from(n).ok()) else { + continue; + }; + let Some(slot) = index.checked_sub(1) else { + continue; + }; + if gone.contains(&slot) { + tag.set("isHit", "true"); + tag.set("isDestroyed", "true"); + } + if empty.contains(&slot) { + tag.set("shots", "0"); + } + } + + // And add the ones it does not have. Numbered from one on the way out. + let has = |location: &Kept, slot: usize| { + location + .children + .iter() + .any(|c| c.name == "slot" && c.number("index") == Some(slot as i64 + 1)) + }; + for slot in gone { + if !has(location, *slot) { + location.children.push(Kept { + name: "slot".to_string(), + attributes: vec![ + ("index".to_string(), (slot + 1).to_string()), + ("isHit".to_string(), "true".to_string()), + ("isDestroyed".to_string(), "true".to_string()), + ], + children: Vec::new(), + }); + } + } + for slot in empty { + if !has(location, *slot) { + location.children.push(Kept { + name: "slot".to_string(), + attributes: vec![ + ("index".to_string(), (slot + 1).to_string()), + ("shots".to_string(), "0".to_string()), + ], + children: Vec::new(), + }); } - out.push_str(" \n"); } - out.push_str("\n"); - out +} + +/// One element and everything inside it. +fn write_element(out: &mut String, element: &Kept, depth: usize) { + let pad = " ".repeat(depth); + out.push_str(&pad); + out.push('<'); + out.push_str(&element.name); + for (name, value) in &element.attributes { + out.push(' '); + out.push_str(name); + out.push_str("=\""); + escape(out, value); + out.push('"'); + } + if element.children.is_empty() { + out.push_str("/>\n"); + return; + } + out.push_str(">\n"); + for child in &element.children { + write_element(out, child, depth + 1); + } + out.push_str(&pad); + out.push_str("\n"); } /// The five characters XML will not take literally in an attribute. @@ -576,7 +779,21 @@ mod tests { #[test] fn a_force_survives_being_written_and_read_again() { let (once, twice) = round_trip(AFTER_A_FIGHT); - assert_eq!(once, twice); + // Field by field rather than whole: the element tree is allowed to + // tidy - an `N/A` plate this crate does not record is not written + // back - while what the force *is* may not change at all. + assert_eq!(once.units.len(), twice.units.len()); + for (a, b) in once.units.iter().zip(&twice.units) { + assert_eq!((&a.chassis, &a.model), (&b.chassis, &b.model)); + assert_eq!( + (a.gunnery, a.piloting, a.pilot_hits), + (b.gunnery, b.piloting, b.pilot_hits) + ); + assert_eq!(a.armor, b.armor); + assert_eq!(a.structure, b.structure); + assert_eq!(a.destroyed, b.destroyed); + assert_eq!(a.empty_ammo, b.empty_ammo); + } } // Writing twice must give the same bytes: a round trip that changes the @@ -715,6 +932,69 @@ mod tests { assert_eq!(mul.units[0].armor.get("CL"), Some(&12)); } + // The requirement the whole `Kept` tree exists for: a force stored here + // and handed back must not have lost anything. A player's camouflage, the + // identifier a campaign tracks a unit by, the edge it has left - none of + // it is this crate's to drop, and dropping it is silent. + #[test] + fn nothing_the_file_said_is_lost_on_the_way_out() { + const RICH: &str = r#" + + + Center Torso + + + + +"#; + + let written = write_mul(&parse_mul(RICH).unwrap()); + for kept in [ + "externalId=\"54dcacff-f828\"", + "camoCategory=\"Clans/Wolf\"", + "camoFileName=\"alpha.jpg\"", + "commander=\"true\"", + "deployment=\"2\"", + "nick=\"Ace\"", + "gender=\"MALE\"", + "edge=\"edge_when_headhit\"", + "portraitFile=\"x.png\"", + // The slot still says what is in it, and the plate is still + // Destroyed rather than a nought this crate invented. + "type=\"IS Ammo SRM-2\"", + "shots=\"46\"", + "points=\"Destroyed\"", + ] { + assert!(written.contains(kept), "lost {kept}:\n{written}"); + } + // And it still reads back the same. + assert_eq!( + parse_mul(&written).unwrap().units[0].armor, + parse_mul(RICH).unwrap().units[0].armor + ); + } + + // Editing one thing must change that thing and nothing else - the camo + // case, which is what a page does first. + #[test] + fn an_edit_changes_only_what_was_edited() { + let mut mul = parse_mul(AFTER_A_FIGHT).unwrap(); + mul.units[0].source.set("camoCategory", "Clans/Jade Falcon"); + mul.units[0].gunnery = 2; + let written = write_mul(&mul); + assert!(written.contains("camoCategory=\"Clans/Jade Falcon\"")); + assert!(written.contains("gunnery=\"2\"")); + // Untouched: the pilot's name and the damage. + assert!(written.contains("North 3")); + let back = parse_mul(&written).unwrap(); + assert_eq!(back.units[0].gunnery, 2); + assert_eq!(back.units[0].armor, mul.units[0].armor); + assert_eq!(back.units[0].destroyed, mul.units[0].destroyed); + } + // `type` must not match `crewType`, and `armor` must not match // `armorDivisor`: an attribute name only counts at a word boundary. #[test] diff --git a/crates/helm-wasm/src/lib.rs b/crates/helm-wasm/src/lib.rs index 9c36143..29ea522 100644 --- a/crates/helm-wasm/src/lib.rs +++ b/crates/helm-wasm/src/lib.rs @@ -187,6 +187,10 @@ pub unsafe extern "C" fn helm_mul_from_json(ptr: *const u8, len: usize) -> i64 { Err(code) => return code, }; mul.units.push(helm_unitfile::MulUnit { + // Built here rather than read, so there is no original to keep. + // A page that wants a player's file back unchanged should send + // back the file, not a force rebuilt from this. + source: Default::default(), chassis: unit["chassis"].as_str().unwrap_or_default().to_string(), model: unit["model"].as_str().unwrap_or_default().to_string(), unit_type: unit