From 0d8937e896b43d5287bbe8f7b51169487d360280 Mon Sep 17 00:00:00 2001 From: Quaternions Date: Wed, 4 Jun 2025 14:38:16 -0700 Subject: [PATCH 1/6] refactor SpawnID --- validation/src/check.rs | 42 +++++++++++++++++++++++++---------------- 1 file changed, 26 insertions(+), 16 deletions(-) diff --git a/validation/src/check.rs b/validation/src/check.rs index 5ba1c64..c037cb7 100644 --- a/validation/src/check.rs +++ b/validation/src/check.rs @@ -87,27 +87,37 @@ impl std::str::FromStr for Zone{ } #[derive(Clone,Copy,Debug,Hash,Eq,PartialEq)] -struct SpawnID(u64); -impl SpawnID{ +struct StageID(u64); +impl StageID{ const FIRST:Self=Self(1); } -enum SpawnTeleport{ - Teleport(SpawnID), - Spawn(SpawnID), +enum StageElementBehaviour{ + Teleport, + Spawn, +} +struct StageElement{ + stage_id:StageID, + behaviour:StageElementBehaviour, } // Parse a SpawnTeleport from a part name -impl std::str::FromStr for SpawnTeleport{ +impl std::str::FromStr for StageElement{ type Err=IDParseError; fn from_str(s:&str)->Result{ // Trigger ForceTrigger Teleport ForceTeleport SpawnAt ForceSpawnAt let bonus_start_pattern=lazy_regex::lazy_regex!(r"^(?:Force)?(Teleport|SpawnAt|Trigger)(\d+)$"); if let Some(captures)=bonus_start_pattern.captures(s){ - return Ok(Self::Teleport(SpawnID(captures[1].parse().map_err(IDParseError::ParseInt)?))); + return Ok(StageElement{ + behaviour:StageElementBehaviour::Teleport, + stage_id:StageID(captures[1].parse().map_err(IDParseError::ParseInt)?), + }); } // Spawn let bonus_finish_pattern=lazy_regex::lazy_regex!(r"^Spawn(\d+)$"); if let Some(captures)=bonus_finish_pattern.captures(s){ - return Ok(Self::Spawn(SpawnID(captures[1].parse().map_err(IDParseError::ParseInt)?))); + return Ok(StageElement{ + behaviour:StageElementBehaviour::Spawn, + stage_id:StageID(captures[1].parse().map_err(IDParseError::ParseInt)?), + }); } Err(IDParseError::NoCaptures) } @@ -141,8 +151,8 @@ struct Counts<'a>{ mode_start_counts:HashMap>, mode_finish_counts:HashMap>, mode_anticheat_counts:HashMap>, - teleport_counts:HashMap>, - spawn_counts:HashMap, + teleport_counts:HashMap>, + spawn_counts:HashMap, wormhole_in_counts:HashMap, wormhole_out_counts:HashMap, } @@ -171,8 +181,8 @@ pub fn get_model_info<'a>(dom:&'a rbx_dom_weak::WeakDom,model_instance:&'a rbx_d } // Spawns & Teleports match instance.name.parse(){ - Ok(SpawnTeleport::Teleport(spawn_id))=>counts.teleport_counts.entry(spawn_id).or_default().push(instance.name.as_str()), - Ok(SpawnTeleport::Spawn(spawn_id))=>*counts.spawn_counts.entry(spawn_id).or_insert(0)+=1, + Ok(StageElement{behaviour:StageElementBehaviour::Teleport,stage_id})=>counts.teleport_counts.entry(stage_id).or_default().push(instance.name.as_str()), + Ok(StageElement{behaviour:StageElementBehaviour::Spawn,stage_id})=>*counts.spawn_counts.entry(stage_id).or_insert(0)+=1, Err(_)=>(), } // Wormholes @@ -345,9 +355,9 @@ struct MapCheck<'a>{ // Spawn1 must exist spawn1:Result, // Check for dangling Teleport# (no associated Spawn#) - teleport_counts:SetDifferenceCheck>>, + teleport_counts:SetDifferenceCheck>>, // No duplicate Spawn# - spawn_counts:DuplicateCheck, + spawn_counts:DuplicateCheck, // Check for dangling WormholeIn# (no associated WormholeOut#) wormhole_in_counts:SetDifferenceCheck>, // No duplicate WormholeOut# (duplicate WormholeIn# ok) @@ -391,7 +401,7 @@ impl<'a> ModelInfo<'a>{ }; // Spawn1 must exist - let spawn1=if self.counts.spawn_counts.contains_key(&SpawnID::FIRST){ + let spawn1=if self.counts.spawn_counts.contains_key(&StageID::FIRST){ Ok(Exists) }else{ Err(Absent) @@ -584,7 +594,7 @@ impl std::fmt::Display for MapCheck<'_>{ } if let DuplicateCheck(Err(DuplicateCheckContext(context)))=&self.spawn_counts{ write!(f,"Duplicate Spawn: ")?; - write_comma_separated(f,context.iter(),|f,(SpawnID(spawn_id),count)| + write_comma_separated(f,context.iter(),|f,(StageID(spawn_id),count)| write!(f,"Spawn{spawn_id} ({count} duplicates)") )?; writeln!(f)?; From 845f8e69d94354e983a66bc1816646b49ac3d21f Mon Sep 17 00:00:00 2001 From: Quaternions Date: Wed, 4 Jun 2025 14:46:27 -0700 Subject: [PATCH 2/6] refactor ModeID --- validation/src/check.rs | 47 ++++++++++++++++++++++++++--------------- 1 file changed, 30 insertions(+), 17 deletions(-) diff --git a/validation/src/check.rs b/validation/src/check.rs index c037cb7..1cac473 100644 --- a/validation/src/check.rs +++ b/validation/src/check.rs @@ -47,38 +47,51 @@ impl ModeID{ const BONUS:Self=Self(1); } enum Zone{ - Start(ModeID), - Finish(ModeID), - Anticheat(ModeID), + Start, + Finish, + Anticheat, +} +struct ModeElement{ + zone:Zone, + mode_id:ModeID, } #[allow(dead_code)] pub enum IDParseError{ NoCaptures, - ParseInt(core::num::ParseIntError) + ParseInt(core::num::ParseIntError), } // Parse a Zone from a part name -impl std::str::FromStr for Zone{ +impl std::str::FromStr for ModeElement{ type Err=IDParseError; fn from_str(s:&str)->Result{ match s{ - "MapStart"=>Ok(Self::Start(ModeID::MAIN)), - "MapFinish"=>Ok(Self::Finish(ModeID::MAIN)), - "MapAnticheat"=>Ok(Self::Anticheat(ModeID::MAIN)), - "BonusStart"=>Ok(Self::Start(ModeID::BONUS)), - "BonusFinish"=>Ok(Self::Finish(ModeID::BONUS)), - "BonusAnticheat"=>Ok(Self::Anticheat(ModeID::BONUS)), + "MapStart"=>Ok(Self{zone:Zone::Start,mode_id:ModeID::MAIN}), + "MapFinish"=>Ok(Self{zone:Zone::Finish,mode_id:ModeID::MAIN}), + "MapAnticheat"=>Ok(Self{zone:Zone::Anticheat,mode_id:ModeID::MAIN}), + "BonusStart"=>Ok(Self{zone:Zone::Start,mode_id:ModeID::BONUS}), + "BonusFinish"=>Ok(Self{zone:Zone::Finish,mode_id:ModeID::BONUS}), + "BonusAnticheat"=>Ok(Self{zone:Zone::Anticheat,mode_id:ModeID::BONUS}), other=>{ let bonus_start_pattern=lazy_regex::lazy_regex!(r"^Bonus(\d+)Start$|^BonusStart(\d+)$"); if let Some(captures)=bonus_start_pattern.captures(other){ - return Ok(Self::Start(ModeID(captures[1].parse().map_err(IDParseError::ParseInt)?))); + return Ok(Self{ + zone:Zone::Start, + mode_id:ModeID(captures[1].parse().map_err(IDParseError::ParseInt)?), + }); } let bonus_finish_pattern=lazy_regex::lazy_regex!(r"^Bonus(\d+)Finish$|^BonusFinish(\d+)$"); if let Some(captures)=bonus_finish_pattern.captures(other){ - return Ok(Self::Finish(ModeID(captures[1].parse().map_err(IDParseError::ParseInt)?))); + return Ok(Self{ + zone:Zone::Finish, + mode_id:ModeID(captures[1].parse().map_err(IDParseError::ParseInt)?), + }); } let bonus_finish_pattern=lazy_regex::lazy_regex!(r"^Bonus(\d+)Anticheat$|^BonusAnticheat(\d+)$"); if let Some(captures)=bonus_finish_pattern.captures(other){ - return Ok(Self::Anticheat(ModeID(captures[1].parse().map_err(IDParseError::ParseInt)?))); + return Ok(Self{ + zone:Zone::Anticheat, + mode_id:ModeID(captures[1].parse().map_err(IDParseError::ParseInt)?), + }); } Err(IDParseError::NoCaptures) } @@ -174,9 +187,9 @@ pub fn get_model_info<'a>(dom:&'a rbx_dom_weak::WeakDom,model_instance:&'a rbx_d if class_is_a(instance.class.as_str(),"BasePart"){ // Zones match instance.name.parse(){ - Ok(Zone::Start(mode_id))=>counts.mode_start_counts.entry(mode_id).or_default().push(instance.name.as_str()), - Ok(Zone::Finish(mode_id))=>counts.mode_finish_counts.entry(mode_id).or_default().push(instance.name.as_str()), - Ok(Zone::Anticheat(mode_id))=>counts.mode_anticheat_counts.entry(mode_id).or_default().push(instance.name.as_str()), + Ok(ModeElement{zone:Zone::Start,mode_id})=>counts.mode_start_counts.entry(mode_id).or_default().push(instance.name.as_str()), + Ok(ModeElement{zone:Zone::Finish,mode_id})=>counts.mode_finish_counts.entry(mode_id).or_default().push(instance.name.as_str()), + Ok(ModeElement{zone:Zone::Anticheat,mode_id})=>counts.mode_anticheat_counts.entry(mode_id).or_default().push(instance.name.as_str()), Err(_)=>(), } // Spawns & Teleports From b0829bc1fc5342a2730e123a53e98235ef006f33 Mon Sep 17 00:00:00 2001 From: Quaternions Date: Wed, 4 Jun 2025 14:49:33 -0700 Subject: [PATCH 3/6] refactor WormholeID --- validation/src/check.rs | 26 ++++++++++++++++++-------- 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/validation/src/check.rs b/validation/src/check.rs index 1cac473..6d8c028 100644 --- a/validation/src/check.rs +++ b/validation/src/check.rs @@ -138,21 +138,31 @@ impl std::str::FromStr for StageElement{ #[derive(Clone,Copy,Debug,Hash,Eq,PartialEq)] struct WormholeID(u64); -enum Wormhole{ - In(WormholeID), - Out(WormholeID), +enum WormholeBehaviour{ + In, + Out, +} +struct WormholeElement{ + behaviour:WormholeBehaviour, + wormhole_id:WormholeID, } // Parse a Wormhole from a part name -impl std::str::FromStr for Wormhole{ +impl std::str::FromStr for WormholeElement{ type Err=IDParseError; fn from_str(s:&str)->Result{ let bonus_start_pattern=lazy_regex::lazy_regex!(r"^WormholeIn(\d+)$"); if let Some(captures)=bonus_start_pattern.captures(s){ - return Ok(Self::In(WormholeID(captures[1].parse().map_err(IDParseError::ParseInt)?))); + return Ok(Self{ + behaviour:WormholeBehaviour::In, + wormhole_id:WormholeID(captures[1].parse().map_err(IDParseError::ParseInt)?), + }); } let bonus_finish_pattern=lazy_regex::lazy_regex!(r"^WormholeOut(\d+)$"); if let Some(captures)=bonus_finish_pattern.captures(s){ - return Ok(Self::Out(WormholeID(captures[1].parse().map_err(IDParseError::ParseInt)?))); + return Ok(Self{ + behaviour:WormholeBehaviour::Out, + wormhole_id:WormholeID(captures[1].parse().map_err(IDParseError::ParseInt)?), + }); } Err(IDParseError::NoCaptures) } @@ -200,8 +210,8 @@ pub fn get_model_info<'a>(dom:&'a rbx_dom_weak::WeakDom,model_instance:&'a rbx_d } // Wormholes match instance.name.parse(){ - Ok(Wormhole::In(wormhole_id))=>*counts.wormhole_in_counts.entry(wormhole_id).or_insert(0)+=1, - Ok(Wormhole::Out(wormhole_id))=>*counts.wormhole_out_counts.entry(wormhole_id).or_insert(0)+=1, + Ok(WormholeElement{behaviour:WormholeBehaviour::In,wormhole_id})=>*counts.wormhole_in_counts.entry(wormhole_id).or_insert(0)+=1, + Ok(WormholeElement{behaviour:WormholeBehaviour::Out,wormhole_id})=>*counts.wormhole_out_counts.entry(wormhole_id).or_insert(0)+=1, Err(_)=>(), } } From fdc024069859454e4d851b5d79cef2822385d9d3 Mon Sep 17 00:00:00 2001 From: Quaternions Date: Tue, 3 Jun 2025 18:26:45 -0700 Subject: [PATCH 4/6] MapCheckSummary --- validation/src/check.rs | 412 ++++++++++++++++++++--------- validation/src/check_mapfix.rs | 2 +- validation/src/check_submission.rs | 2 +- 3 files changed, 284 insertions(+), 132 deletions(-) diff --git a/validation/src/check.rs b/validation/src/check.rs index 6d8c028..c963364 100644 --- a/validation/src/check.rs +++ b/validation/src/check.rs @@ -12,6 +12,7 @@ pub enum Error{ Download(crate::download::Error), ModelFileDecode(ReadDomError), GetRootInstance(GetRootInstanceError), + ToJsonValue(serde_json::Error), } impl std::fmt::Display for Error{ fn fmt(&self,f:&mut std::fmt::Formatter<'_>)->std::fmt::Result{ @@ -98,6 +99,21 @@ impl std::str::FromStr for ModeElement{ } } } +impl std::fmt::Display for ModeElement{ + fn fmt(&self,f:&mut std::fmt::Formatter<'_>)->std::fmt::Result{ + match self{ + ModeElement{zone:Zone::Start,mode_id:ModeID::MAIN}=>write!(f,"MapStart"), + ModeElement{zone:Zone::Start,mode_id:ModeID::BONUS}=>write!(f,"BonusStart"), + ModeElement{zone:Zone::Start,mode_id:ModeID(mode_id)}=>write!(f,"Bonus{mode_id}Start"), + ModeElement{zone:Zone::Finish,mode_id:ModeID::MAIN}=>write!(f,"MapFinish"), + ModeElement{zone:Zone::Finish,mode_id:ModeID::BONUS}=>write!(f,"BonusFinish"), + ModeElement{zone:Zone::Finish,mode_id:ModeID(mode_id)}=>write!(f,"Bonus{mode_id}Finish"), + ModeElement{zone:Zone::Anticheat,mode_id:ModeID::MAIN}=>write!(f,"MapAnticheat"), + ModeElement{zone:Zone::Anticheat,mode_id:ModeID::BONUS}=>write!(f,"BonusAnticheat"), + ModeElement{zone:Zone::Anticheat,mode_id:ModeID(mode_id)}=>write!(f,"Bonus{mode_id}Anticheat"), + } + } +} #[derive(Clone,Copy,Debug,Hash,Eq,PartialEq)] struct StageID(u64); @@ -135,6 +151,14 @@ impl std::str::FromStr for StageElement{ Err(IDParseError::NoCaptures) } } +impl std::fmt::Display for StageElement{ + fn fmt(&self,f:&mut std::fmt::Formatter<'_>)->std::fmt::Result{ + match self{ + StageElement{behaviour:StageElementBehaviour::Spawn,stage_id:StageID(stage_id)}=>write!(f,"Spawn{stage_id}"), + StageElement{behaviour:StageElementBehaviour::Teleport,stage_id:StageID(stage_id)}=>write!(f,"Teleport{stage_id}"), + } + } +} #[derive(Clone,Copy,Debug,Hash,Eq,PartialEq)] struct WormholeID(u64); @@ -167,6 +191,14 @@ impl std::str::FromStr for WormholeElement{ Err(IDParseError::NoCaptures) } } +impl std::fmt::Display for WormholeElement{ + fn fmt(&self,f:&mut std::fmt::Formatter<'_>)->std::fmt::Result{ + match self{ + WormholeElement{behaviour:WormholeBehaviour::In,wormhole_id:WormholeID(wormhole_id)}=>write!(f,"WormholeIn{wormhole_id}"), + WormholeElement{behaviour:WormholeBehaviour::Out,wormhole_id:WormholeID(wormhole_id)}=>write!(f,"WormholeOut{wormhole_id}"), + } + } +} /// Count various map elements #[derive(Default)] @@ -479,7 +511,7 @@ impl<'a> ModelInfo<'a>{ } impl MapCheck<'_>{ - fn result(self)->Result{ + fn result(self)->Result>{ match self{ MapCheck{ model_class:StringCheck(Ok(())), @@ -503,156 +535,269 @@ impl MapCheck<'_>{ game_id, }) }, - other=>Err(other.to_string()), + other=>Err(other.itemize()), } } } -fn write_comma_separated( - f:&mut std::fmt::Formatter<'_>, - mut it:impl Iterator, - custom_write:impl Fn(&mut std::fmt::Formatter<'_>,T)->std::fmt::Result -)->std::fmt::Result{ - if let Some(t)=it.next(){ - custom_write(f,t)?; - for t in it{ - write!(f,", ")?; - custom_write(f,t)?; - } +struct Separated{ + iter:I, + separator:&'static str, +} +impl Separated{ + fn new(separator:&'static str,iter:I)->Self{ + Self{separator,iter} + } +} +impl std::fmt::Display for Separated + where + D:std::fmt::Display, + I:Clone, + I:Iterator, +{ + fn fmt(&self,f:&mut std::fmt::Formatter<'_>)->std::fmt::Result{ + let mut it=self.iter.clone(); + if let Some(first)=it.next(){ + write!(f,"{first}")?; + for item in it{ + write!(f,"{}{item}",self.separator)?; + } + } + Ok(()) } - Ok(()) } -/// Write a zone string such as BonusStart -macro_rules! write_zone{ - ($f:expr,$mode:expr,$zone:expr)=>{ - match $mode{ - ModeID(0)=>write!($f,concat!("Map",$zone)), - ModeID(1)=>write!($f,concat!("Bonus",$zone)), - ModeID(other)=>write!($f,concat!("Bonus{}",$zone),other), +struct Duplicates{ + display:D, + duplicates:usize, +} +impl Duplicates{ + fn new(display:D,duplicates:usize)->Self{ + Self{ + display, + duplicates, + } + } +} +impl std::fmt::Display for Duplicates{ + fn fmt(&self,f:&mut std::fmt::Formatter<'_>)->std::fmt::Result{ + write!(f,"{} ({} duplicates)",self.display,self.duplicates) + } +} + +#[derive(serde::Serialize)] +struct CheckSummary{ + name:&'static str, + summary:String, + passed:bool, + details:serde_json::Value, +} +impl CheckSummary{ + const fn passed(name:&'static str)->Self{ + Self{ + name, + summary:String::new(), + passed:true, + details:serde_json::Value::Null, + } + } +} +macro_rules! summary{ + ($name:literal,$summary:expr,$details:expr)=>{ + CheckSummary{ + name:$name, + summary:$summary, + passed:false, + details:serde_json::to_value($details)?, + } + }; +} +macro_rules! summary_format{ + ($name:literal,$fmt:literal,$details:expr)=>{ + CheckSummary{ + name:$name, + summary:format!($fmt), + passed:false, + details:serde_json::to_value($details)?, } }; } // Generate an error message for each observed issue separated by newlines. // This defines MapCheck.to_string() which is used in MapCheck.result() -impl std::fmt::Display for MapCheck<'_>{ - fn fmt(&self,f:&mut std::fmt::Formatter<'_>)->std::fmt::Result{ - if let StringCheck(Err(context))=&self.model_class{ - writeln!(f,"Invalid model class: {context}")?; - } - if let StringCheck(Err(context))=&self.model_name{ - writeln!(f,"Model name must have snake_case: {context}")?; - } - match &self.display_name{ - Ok(Ok(StringCheck(Ok(_))))=>(), - Ok(Ok(StringCheck(Err(context))))=>writeln!(f,"DisplayName must have Title Case: {context}")?, - Ok(Err(context))=>writeln!(f,"Invalid DisplayName: {context}")?, - Err(StringValueError::ObjectNotFound)=>writeln!(f,"Missing DisplayName StringValue")?, - Err(StringValueError::ValueNotSet)=>writeln!(f,"DisplayName Value not set")?, - Err(StringValueError::NonStringValue)=>writeln!(f,"DisplayName Value is not a String")?, - } - match &self.creator{ - Ok(Ok(_))=>(), - Ok(Err(context))=>writeln!(f,"Invalid Creator: {context}")?, - Err(StringValueError::ObjectNotFound)=>writeln!(f,"Missing Creator StringValue")?, - Err(StringValueError::ValueNotSet)=>writeln!(f,"Creator Value not set")?, - Err(StringValueError::NonStringValue)=>writeln!(f,"Creator Value is not a String")?, - } - if let Err(_parse_game_id_error)=&self.game_id{ - writeln!(f,"Model name must be prefixed with bhop_ surf_ or flytrials_")?; - } - if let Err(Absent)=&self.mapstart{ - writeln!(f,"Model has no MapStart")?; - } - if let DuplicateCheck(Err(DuplicateCheckContext(context)))=&self.mode_start_counts{ - write!(f,"Duplicate start zones: ")?; - write_comma_separated(f,context.iter(),|f,(mode_id,names)|{ - write_zone!(f,mode_id,"Start")?; - write!(f," ({} duplicates)",names.len())?; - Ok(()) - })?; - writeln!(f)?; - } - if let SetDifferenceCheck(Err(context))=&self.mode_finish_counts{ - if !context.extra.is_empty(){ - let plural=if context.extra.len()==1{"zone"}else{"zones"}; - write!(f,"No matching start zone for finish {plural}: ")?; - write_comma_separated(f,context.extra.iter(),|f,(mode_id,_names)| - write_zone!(f,mode_id,"Finish") - )?; - writeln!(f)?; +impl MapCheck<'_>{ + fn itemize(&self)->Result{ + let model_class=match &self.model_class{ + StringCheck(Ok(()))=>CheckSummary::passed("ModelClass"), + StringCheck(Err(context))=>summary_format!("ModelClass","Invalid model class: {context}",()), + }; + let model_name=match &self.model_name{ + StringCheck(Ok(()))=>CheckSummary::passed("ModelName"), + StringCheck(Err(context))=>summary_format!("ModelName","Model name must have snake_case: {context}",()), + }; + let display_name=match &self.display_name{ + Ok(Ok(StringCheck(Ok(_))))=>CheckSummary::passed("DisplayName"), + Ok(Ok(StringCheck(Err(context))))=>summary_format!("DisplayName","DisplayName must have Title Case: {context}",()), + Ok(Err(context))=>summary_format!("DisplayName","Invalid DisplayName: {context}",()), + Err(StringValueError::ObjectNotFound)=>summary!("DisplayName","Missing DisplayName StringValue".to_owned(),()), + Err(StringValueError::ValueNotSet)=>summary!("DisplayName","DisplayName Value not set".to_owned(),()), + Err(StringValueError::NonStringValue)=>summary!("DisplayName","DisplayName Value is not a String".to_owned(),()), + }; + let creator=match &self.creator{ + Ok(Ok(_))=>CheckSummary::passed("Creator"), + Ok(Err(context))=>summary_format!("Creator","Invalid Creator: {context}",()), + Err(StringValueError::ObjectNotFound)=>summary!("Creator","Missing Creator StringValue".to_owned(),()), + Err(StringValueError::ValueNotSet)=>summary!("Creator","Creator Value not set".to_owned(),()), + Err(StringValueError::NonStringValue)=>summary!("Creator","Creator Value is not a String".to_owned(),()), + }; + let game_id=match &self.game_id{ + Ok(_)=>CheckSummary::passed("GameID"), + Err(ParseGameIDError)=>summary!("GameID","Model name must be prefixed with bhop_ surf_ or flytrials_".to_owned(),()), + }; + let mapstart=match &self.mapstart{ + Ok(Exists)=>CheckSummary::passed("MapStart"), + Err(Absent)=>summary_format!("MapStart","Model has no MapStart",()), + }; + let duplicate_start=match &self.mode_start_counts{ + DuplicateCheck(Ok(()))=>CheckSummary::passed("DuplicateStart"), + DuplicateCheck(Err(DuplicateCheckContext(context)))=>{ + let context=Separated::new(", ",context.iter().map(|(&mode_id,names)| + Duplicates::new(ModeElement{zone:Zone::Start,mode_id},names.len()) + )); + summary_format!("DuplicateStart","Duplicate start zones: {context}",()) } - if !context.missing.is_empty(){ - let plural=if context.missing.len()==1{"zone"}else{"zones"}; - write!(f,"Missing finish {plural}: ")?; - write_comma_separated(f,context.missing.iter(),|f,mode_id| - write_zone!(f,mode_id,"Finish") - )?; - writeln!(f)?; + }; + let (extra_finish,missing_finish)=match &self.mode_finish_counts{ + SetDifferenceCheck(Ok(()))=>(CheckSummary::passed("ExtraFinish"),CheckSummary::passed("MissingFinish")), + SetDifferenceCheck(Err(context))=>( + if context.extra.is_empty(){ + CheckSummary::passed("ExtraFinish") + }else{ + let plural=if context.extra.len()==1{"zone"}else{"zones"}; + let context=Separated::new(", ",context.extra.iter().map(|(&mode_id,_names)| + ModeElement{zone:Zone::Finish,mode_id} + )); + summary_format!("ExtraFinish","No matching start zone for finish {plural}: {context}",()) + }, + if context.missing.is_empty(){ + CheckSummary::passed("MissingFinish") + }else{ + let plural=if context.missing.len()==1{"zone"}else{"zones"}; + let context=Separated::new(", ",context.missing.iter().map(|&mode_id| + ModeElement{zone:Zone::Finish,mode_id} + )); + summary_format!("MissingFinish","Missing finish {plural}: {context}",()) + } + ), + }; + let dangling_anticheat=match &self.mode_anticheat_counts{ + SetDifferenceCheck(Ok(()))=>CheckSummary::passed("DanglingAnticheat"), + SetDifferenceCheck(Err(context))=>{ + if context.extra.is_empty(){ + CheckSummary::passed("DanglingAnticheat") + }else{ + let plural=if context.extra.len()==1{"zone"}else{"zones"}; + let context=Separated::new(", ",context.extra.iter().map(|(&mode_id,_names)| + ModeElement{zone:Zone::Anticheat,mode_id} + )); + summary_format!("DanglingAnticheat","No matching start zone for anticheat {plural}: {context}",()) + } } - } - if let SetDifferenceCheck(Err(context))=&self.mode_anticheat_counts{ - if !context.extra.is_empty(){ - let plural=if context.extra.len()==1{"zone"}else{"zones"}; - write!(f,"No matching start zone for anticheat {plural}: ")?; - write_comma_separated(f,context.extra.iter(),|f,(mode_id,_names)| - write_zone!(f,mode_id,"Anticheat") - )?; - writeln!(f)?; + }; + let spawn1=match &self.spawn1{ + Ok(Exists)=>CheckSummary::passed("Spawn1"), + Err(Absent)=>summary_format!("Spawn1","Model has no Spawn1",()), + }; + let dangling_teleport=match &self.teleport_counts{ + SetDifferenceCheck(Ok(()))=>CheckSummary::passed("DanglingTeleport"), + SetDifferenceCheck(Err(context))=>{ + let unique_names:HashSet<_>=context.extra.values().flat_map(|names|names.iter().copied()).collect(); + let plural=if unique_names.len()==1{"object"}else{"objects"}; + let context=Separated::new(", ",unique_names.iter()); + summary_format!("DanglingTeleport","No matching Spawn for {plural}: {context}",()) } - } - if let Err(Absent)=&self.spawn1{ - writeln!(f,"Model has no Spawn1")?; - } - if let SetDifferenceCheck(Err(context))=&self.teleport_counts{ - for names in context.extra.values(){ - let plural=if names.len()==1{"object"}else{"objects"}; - write!(f,"No matching Spawn for {plural}: ")?; - write_comma_separated(f,names.iter(),|f,&name|{ - write!(f,"{name}") - })?; - writeln!(f)?; + }; + let duplicate_spawns=match &self.spawn_counts{ + DuplicateCheck(Ok(()))=>CheckSummary::passed("DuplicateSpawn"), + DuplicateCheck(Err(DuplicateCheckContext(context)))=>{ + let context=Separated::new(", ",context.iter().map(|(&stage_id,&names)| + Duplicates::new(StageElement{behaviour:StageElementBehaviour::Spawn,stage_id},names as usize) + )); + summary_format!("DuplicateSpawn","Duplicate Spawn: {context}",()) } - } - if let DuplicateCheck(Err(DuplicateCheckContext(context)))=&self.spawn_counts{ - write!(f,"Duplicate Spawn: ")?; - write_comma_separated(f,context.iter(),|f,(StageID(spawn_id),count)| - write!(f,"Spawn{spawn_id} ({count} duplicates)") - )?; - writeln!(f)?; - } - if let SetDifferenceCheck(Err(context))=&self.wormhole_in_counts{ - if !context.extra.is_empty(){ - write!(f,"WormholeIn with no matching WormholeOut: ")?; - write_comma_separated(f,context.extra.iter(),|f,(WormholeID(wormhole_id),_count)| - write!(f,"WormholeIn{wormhole_id}") - )?; - writeln!(f)?; + }; + let (extra_wormhole_in,missing_wormhole_in)=match &self.wormhole_in_counts{ + SetDifferenceCheck(Ok(()))=>(CheckSummary::passed("ExtraWormholeIn"),CheckSummary::passed("MissingWormholeIn")), + SetDifferenceCheck(Err(context))=>( + if context.extra.is_empty(){ + CheckSummary::passed("ExtraWormholeIn") + }else{ + let context=Separated::new(", ",context.extra.iter().map(|(&wormhole_id,_names)| + WormholeElement{behaviour:WormholeBehaviour::In,wormhole_id} + )); + summary_format!("ExtraWormholeIn","WormholeIn with no matching WormholeOut: {context}",()) + }, + if context.missing.is_empty(){ + CheckSummary::passed("MissingWormholeIn") + }else{ + // This counts WormholeIn objects, but + // flipped logic is easier to understand + let context=Separated::new(", ",context.missing.iter().map(|&wormhole_id| + WormholeElement{behaviour:WormholeBehaviour::Out,wormhole_id} + )); + summary_format!("MissingWormholeIn","WormholeOut with no matching WormholeIn: {context}",()) + } + ) + }; + let duplicate_wormhole_out=match &self.wormhole_out_counts{ + DuplicateCheck(Ok(()))=>CheckSummary::passed("DuplicateWormholeOut"), + DuplicateCheck(Err(DuplicateCheckContext(context)))=>{ + let context=Separated::new(", ",context.iter().map(|(&wormhole_id,&names)| + Duplicates::new(WormholeElement{behaviour:WormholeBehaviour::Out,wormhole_id},names as usize) + )); + summary_format!("DuplicateWormholeOut","Duplicate WormholeOut: {context}",()) } - if !context.missing.is_empty(){ - // This counts WormholeIn objects, but - // flipped logic is easier to understand - write!(f,"WormholeOut with no matching WormholeIn: ")?; - write_comma_separated(f,context.missing.iter(),|f,WormholeID(wormhole_id)| - write!(f,"WormholeOut{wormhole_id}") - )?; - writeln!(f)?; - } - } - if let DuplicateCheck(Err(DuplicateCheckContext(context)))=&self.wormhole_out_counts{ - write!(f,"Duplicate WormholeOut: ")?; - write_comma_separated(f,context.iter(),|f,(WormholeID(wormhole_id),count)| - write!(f,"WormholeOut{wormhole_id} ({count} duplicates)") - )?; - writeln!(f)?; - } - Ok(()) + }; + Ok(MapCheckList{checks:[ + model_class, + model_name, + display_name, + creator, + game_id, + mapstart, + duplicate_start, + extra_finish, + missing_finish, + dangling_anticheat, + spawn1, + dangling_teleport, + duplicate_spawns, + extra_wormhole_in, + missing_wormhole_in, + duplicate_wormhole_out, + ]}) } } +#[derive(serde::Serialize)] +struct MapCheckList{ + checks:[CheckSummary;16], +} +impl MapCheckList{ + fn summary(&self)->String{ + Separated::new("; ",self.checks.iter().filter_map(|check| + (!check.passed).then_some(check.summary.as_str()) + )).to_string() + } +} + +pub struct Summary{ + pub summary:String, + pub json:serde_json::Value, +} + pub struct CheckReportAndVersion{ - pub status:Result, + pub status:Result, pub version:u64, } @@ -689,7 +834,14 @@ impl crate::message_handler::MessageHandler{ let map_check=model_info.check(); // check the report, generate an error message if it fails the check - let status=map_check.result(); + let status=match map_check.result(){ + Ok(map_info)=>Ok(map_info), + Err(Ok(summary))=>Err(Summary{ + summary:summary.summary(), + json:serde_json::to_value(&summary).map_err(Error::ToJsonValue)?, + }), + Err(Err(e))=>return Err(Error::ToJsonValue(e)), + }; Ok(CheckReportAndVersion{status,version}) } diff --git a/validation/src/check_mapfix.rs b/validation/src/check_mapfix.rs index 20513b5..d8ed2fd 100644 --- a/validation/src/check_mapfix.rs +++ b/validation/src/check_mapfix.rs @@ -34,7 +34,7 @@ impl crate::message_handler::MessageHandler{ Ok(CheckReportAndVersion{status:Err(report),..})=>self.api.action_mapfix_request_changes( submissions_api::types::ActionMapfixRequestChangesRequest{ MapfixID:mapfix_id, - ErrorMessage:report, + ErrorMessage:report.summary, } ).await.map_err(Error::ApiActionMapfixCheck)?, // update the mapfix model status to request changes diff --git a/validation/src/check_submission.rs b/validation/src/check_submission.rs index 4e0842f..7e6463a 100644 --- a/validation/src/check_submission.rs +++ b/validation/src/check_submission.rs @@ -35,7 +35,7 @@ impl crate::message_handler::MessageHandler{ Ok(CheckReportAndVersion{status:Err(report),..})=>self.api.action_submission_request_changes( submissions_api::types::ActionSubmissionRequestChangesRequest{ SubmissionID:submission_id, - ErrorMessage:report, + ErrorMessage:report.summary, } ).await.map_err(Error::ApiActionSubmissionCheck)?, // update the submission model status to request changes From 534598ba7080e8126fd11d5dfbf670eb8cd48115 Mon Sep 17 00:00:00 2001 From: Quaternions Date: Wed, 4 Jun 2025 17:13:24 -0700 Subject: [PATCH 5/6] box list to appease clippy --- validation/src/check.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/validation/src/check.rs b/validation/src/check.rs index c963364..88fcb16 100644 --- a/validation/src/check.rs +++ b/validation/src/check.rs @@ -758,7 +758,7 @@ impl MapCheck<'_>{ summary_format!("DuplicateWormholeOut","Duplicate WormholeOut: {context}",()) } }; - Ok(MapCheckList{checks:[ + Ok(MapCheckList{checks:Box::new([ model_class, model_name, display_name, @@ -775,13 +775,13 @@ impl MapCheck<'_>{ extra_wormhole_in, missing_wormhole_in, duplicate_wormhole_out, - ]}) + ])}) } } #[derive(serde::Serialize)] struct MapCheckList{ - checks:[CheckSummary;16], + checks:Box<[CheckSummary;16]>, } impl MapCheckList{ fn summary(&self)->String{ From 90d13d28ae94f28bfaa366a7440e1dd6f5e66ede Mon Sep 17 00:00:00 2001 From: Quaternions Date: Wed, 4 Jun 2025 18:18:18 -0700 Subject: [PATCH 6/6] use closure instead of iterator --- validation/src/check.rs | 38 +++++++++++++++++++------------------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/validation/src/check.rs b/validation/src/check.rs index 88fcb16..2a6569d 100644 --- a/validation/src/check.rs +++ b/validation/src/check.rs @@ -540,23 +540,23 @@ impl MapCheck<'_>{ } } -struct Separated{ - iter:I, +struct Separated{ + f:F, separator:&'static str, } -impl Separated{ - fn new(separator:&'static str,iter:I)->Self{ - Self{separator,iter} +impl Separated{ + fn new(separator:&'static str,f:F)->Self{ + Self{separator,f} } } -impl std::fmt::Display for Separated +impl std::fmt::Display for Separated where D:std::fmt::Display, - I:Clone, - I:Iterator, + I:IntoIterator, + F:Fn()->I, { fn fmt(&self,f:&mut std::fmt::Formatter<'_>)->std::fmt::Result{ - let mut it=self.iter.clone(); + let mut it=(self.f)().into_iter(); if let Some(first)=it.next(){ write!(f,"{first}")?; for item in it{ @@ -661,7 +661,7 @@ impl MapCheck<'_>{ let duplicate_start=match &self.mode_start_counts{ DuplicateCheck(Ok(()))=>CheckSummary::passed("DuplicateStart"), DuplicateCheck(Err(DuplicateCheckContext(context)))=>{ - let context=Separated::new(", ",context.iter().map(|(&mode_id,names)| + let context=Separated::new(", ",||context.iter().map(|(&mode_id,names)| Duplicates::new(ModeElement{zone:Zone::Start,mode_id},names.len()) )); summary_format!("DuplicateStart","Duplicate start zones: {context}",()) @@ -674,7 +674,7 @@ impl MapCheck<'_>{ CheckSummary::passed("ExtraFinish") }else{ let plural=if context.extra.len()==1{"zone"}else{"zones"}; - let context=Separated::new(", ",context.extra.iter().map(|(&mode_id,_names)| + let context=Separated::new(", ",||context.extra.iter().map(|(&mode_id,_names)| ModeElement{zone:Zone::Finish,mode_id} )); summary_format!("ExtraFinish","No matching start zone for finish {plural}: {context}",()) @@ -683,7 +683,7 @@ impl MapCheck<'_>{ CheckSummary::passed("MissingFinish") }else{ let plural=if context.missing.len()==1{"zone"}else{"zones"}; - let context=Separated::new(", ",context.missing.iter().map(|&mode_id| + let context=Separated::new(", ",||context.missing.iter().map(|&mode_id| ModeElement{zone:Zone::Finish,mode_id} )); summary_format!("MissingFinish","Missing finish {plural}: {context}",()) @@ -697,7 +697,7 @@ impl MapCheck<'_>{ CheckSummary::passed("DanglingAnticheat") }else{ let plural=if context.extra.len()==1{"zone"}else{"zones"}; - let context=Separated::new(", ",context.extra.iter().map(|(&mode_id,_names)| + let context=Separated::new(", ",||context.extra.iter().map(|(&mode_id,_names)| ModeElement{zone:Zone::Anticheat,mode_id} )); summary_format!("DanglingAnticheat","No matching start zone for anticheat {plural}: {context}",()) @@ -713,14 +713,14 @@ impl MapCheck<'_>{ SetDifferenceCheck(Err(context))=>{ let unique_names:HashSet<_>=context.extra.values().flat_map(|names|names.iter().copied()).collect(); let plural=if unique_names.len()==1{"object"}else{"objects"}; - let context=Separated::new(", ",unique_names.iter()); + let context=Separated::new(", ",||&unique_names); summary_format!("DanglingTeleport","No matching Spawn for {plural}: {context}",()) } }; let duplicate_spawns=match &self.spawn_counts{ DuplicateCheck(Ok(()))=>CheckSummary::passed("DuplicateSpawn"), DuplicateCheck(Err(DuplicateCheckContext(context)))=>{ - let context=Separated::new(", ",context.iter().map(|(&stage_id,&names)| + let context=Separated::new(", ",||context.iter().map(|(&stage_id,&names)| Duplicates::new(StageElement{behaviour:StageElementBehaviour::Spawn,stage_id},names as usize) )); summary_format!("DuplicateSpawn","Duplicate Spawn: {context}",()) @@ -732,7 +732,7 @@ impl MapCheck<'_>{ if context.extra.is_empty(){ CheckSummary::passed("ExtraWormholeIn") }else{ - let context=Separated::new(", ",context.extra.iter().map(|(&wormhole_id,_names)| + let context=Separated::new(", ",||context.extra.iter().map(|(&wormhole_id,_names)| WormholeElement{behaviour:WormholeBehaviour::In,wormhole_id} )); summary_format!("ExtraWormholeIn","WormholeIn with no matching WormholeOut: {context}",()) @@ -742,7 +742,7 @@ impl MapCheck<'_>{ }else{ // This counts WormholeIn objects, but // flipped logic is easier to understand - let context=Separated::new(", ",context.missing.iter().map(|&wormhole_id| + let context=Separated::new(", ",||context.missing.iter().map(|&wormhole_id| WormholeElement{behaviour:WormholeBehaviour::Out,wormhole_id} )); summary_format!("MissingWormholeIn","WormholeOut with no matching WormholeIn: {context}",()) @@ -752,7 +752,7 @@ impl MapCheck<'_>{ let duplicate_wormhole_out=match &self.wormhole_out_counts{ DuplicateCheck(Ok(()))=>CheckSummary::passed("DuplicateWormholeOut"), DuplicateCheck(Err(DuplicateCheckContext(context)))=>{ - let context=Separated::new(", ",context.iter().map(|(&wormhole_id,&names)| + let context=Separated::new(", ",||context.iter().map(|(&wormhole_id,&names)| Duplicates::new(WormholeElement{behaviour:WormholeBehaviour::Out,wormhole_id},names as usize) )); summary_format!("DuplicateWormholeOut","Duplicate WormholeOut: {context}",()) @@ -785,7 +785,7 @@ struct MapCheckList{ } impl MapCheckList{ fn summary(&self)->String{ - Separated::new("; ",self.checks.iter().filter_map(|check| + Separated::new("; ",||self.checks.iter().filter_map(|check| (!check.passed).then_some(check.summary.as_str()) )).to_string() }