From 76e4ca5cea1ba88f09d4a440461e0e3f89d565f4 Mon Sep 17 00:00:00 2001 From: Orual Date: Mon, 10 Aug 2026 12:40:17 -0400 Subject: [PATCH] PM-84: repair Round 7 validation findings Epic: PM-86 Task: PM-84 --- crates/polymodel-ldraw-core/src/mpd.rs | 25 ++++++++++ crates/polymodel-ldraw-core/src/semantic.rs | 9 +--- crates/polymodel-ldraw-core/src/traversal.rs | 31 +------------ .../polymodel-renderer-protocol/src/scene.rs | 40 +++++++++++++++- .../src/bin/ldraw_preview.rs | 2 +- crates/polymodel-renderer-worker/src/lib.rs | 14 +----- crates/polymodel-renderer-worker/src/tests.rs | 46 ++++++++++--------- 7 files changed, 93 insertions(+), 74 deletions(-) diff --git a/crates/polymodel-ldraw-core/src/mpd.rs b/crates/polymodel-ldraw-core/src/mpd.rs index 85d3613..a88e36f 100644 --- a/crates/polymodel-ldraw-core/src/mpd.rs +++ b/crates/polymodel-ldraw-core/src/mpd.rs @@ -840,6 +840,31 @@ mod tests { assert!(closure.needs_ldconfig); } + #[test] + fn duplicate_resource_identity_is_rejected_directly() { + let bytes = b"same resource".to_vec(); + let resource = ResolvedResource { + path: NormalizedPath::new("models/root.ldr").unwrap(), + root: RootId::UploadedLdraw, + sha256: Sha256Hash::digest(&bytes).as_bytes().to_vec(), + byte_length: bytes.len() as u64, + bytes: bytes.clone(), + mime_type: "text/plain", + required: false, + fallback_available: false, + }; + let duplicate = ResourceClosure { + include_references: Vec::new(), + dependencies: Vec::new(), + resolved: vec![resource.clone(), resource], + needs_ldconfig: false, + }; + assert_eq!( + duplicate.validate(), + Err(ResourceClosureError::DuplicateIdentity) + ); + } + #[test] fn malformed_type_one_and_missing_target_fail_closed() { let malformed = b"1 16 0 0 0 1 0 0 0 1 0 0 0 child.dat\n"; diff --git a/crates/polymodel-ldraw-core/src/semantic.rs b/crates/polymodel-ldraw-core/src/semantic.rs index 5089fe8..192a42a 100644 --- a/crates/polymodel-ldraw-core/src/semantic.rs +++ b/crates/polymodel-ldraw-core/src/semantic.rs @@ -6,13 +6,9 @@ use crate::scanner::LineType; use crate::texmap::{TexmapAssociation, TexmapState, TextureDescriptor}; use crate::types::{Diagnostic, Span}; use smol_str::SmolStr; -use std::sync::Arc; - #[derive(Clone, Debug, PartialEq)] pub struct ColourScopeSnapshot { pub effective: crate::ColourCode, - /// The local-scope binding that produced `effective`, when applicable. - pub local_mapping: Option>, } /// A semantic parser callback. Implementations receive events while traversal @@ -45,10 +41,7 @@ impl TraversalContext { ) -> Self { Self { composed_transform: transform, - colour_scope: ColourScopeSnapshot { - effective: colour, - local_mapping: None, - }, + colour_scope: ColourScopeSnapshot { effective: colour }, bfc_frame, texmap_state, texmap_descriptor: texmap_descriptor.map(|descriptor| TextureDescriptor { diff --git a/crates/polymodel-ldraw-core/src/traversal.rs b/crates/polymodel-ldraw-core/src/traversal.rs index f84c6ba..618b31c 100644 --- a/crates/polymodel-ldraw-core/src/traversal.rs +++ b/crates/polymodel-ldraw-core/src/traversal.rs @@ -1,5 +1,4 @@ use crate::cache::CacheKey; -use crate::geom::GeometryRecord; use crate::geom::{Bounds3, Transform}; use crate::model::{InstanceRecord, ModelData, ModelId, ModelSummary, SceneRecord}; use crate::semantic::{SemanticEvent, SemanticSink, TraversalContext}; @@ -273,7 +272,6 @@ pub(crate) fn traverse( None, ), ); - let mut compatibility_geometry = Vec::with_capacity(model.geometry.len()); for geometry in &model.geometry { let event = SemanticEvent::GeometryEmitted { primitive_id: geometry.primitive_id, @@ -298,30 +296,6 @@ pub(crate) fn traverse( Some(geometry.span), ), ); - if let SemanticEvent::GeometryEmitted { - primitive_id, - source, - span, - line_type, - colour, - vertices, - bfc, - texmap, - inverted, - } = event - { - compatibility_geometry.push(GeometryRecord { - primitive_id, - source, - span, - line_type, - colour, - vertices, - bfc, - inverted, - texmap, - }); - } } for _ in &model.step_ends { sink.on_event( @@ -337,9 +311,6 @@ pub(crate) fn traverse( ), ); } - // Geometry is owned by GeometryEmitted events. The legacy - // semantic field is intentionally empty until a consumer - // explicitly requests a compatibility projection. let semantic = crate::model::LdMeshSemantic { positions: Vec::new(), normals: None, @@ -373,7 +344,7 @@ pub(crate) fn traverse( conditional_lines: model.conditional_lines, step_ends, semantic, - geometry: compatibility_geometry, + geometry: model.geometry.clone(), bfc: model.bfc.state, }); } diff --git a/crates/polymodel-renderer-protocol/src/scene.rs b/crates/polymodel-renderer-protocol/src/scene.rs index 22f068c..11a648d 100644 --- a/crates/polymodel-renderer-protocol/src/scene.rs +++ b/crates/polymodel-renderer-protocol/src/scene.rs @@ -211,6 +211,8 @@ pub struct Scene { pub enum SceneValidationError { #[error("duplicate geometry id")] DuplicateGeometryId, + #[error("scene root references unknown geometry")] + UnknownRoot, #[error("occurrence references unknown geometry")] UnknownGeometry, #[error("duplicate {0} id")] @@ -245,6 +247,9 @@ impl Scene { if geometry_ids.len() != self.geometries.len() { return Err(SceneValidationError::DuplicateGeometryId); } + if !geometry_ids.contains(&self.root.0) { + return Err(SceneValidationError::UnknownRoot); + } let material_ids = self .materials .iter() @@ -378,14 +383,16 @@ impl Scene { finite(value, "scene bounds")?; } } + const BOUNDS_EPSILON: f64 = 1.0e-9; if bounds .min .iter() .zip(bounds.max) - .any(|(min, max)| *min > max) + .any(|(min, max)| *min > max + BOUNDS_EPSILON) || all_points.iter().any(|point| { point.iter().enumerate().any(|(index, value)| { - *value < bounds.min[index] || *value > bounds.max[index] + *value < bounds.min[index] - BOUNDS_EPSILON + || *value > bounds.max[index] + BOUNDS_EPSILON }) }) { @@ -514,5 +521,34 @@ mod tests { let bytes = postcard::to_allocvec(&scene).unwrap(); assert_eq!(postcard::from_bytes::(&bytes).unwrap(), scene); assert_eq!(scene.stats.triangles, 1); + + let mut invalid = scene.clone(); + invalid.geometries[0].triangles[0][2] = 99; + assert_eq!( + invalid.validate(), + Err(SceneValidationError::IndexOutOfRange("triangle")) + ); + + let mut invalid = scene.clone(); + invalid.geometries[0].uvs = Box::new([[0.0, 0.0]]); + assert_eq!(invalid.validate(), Err(SceneValidationError::UvCardinality)); + + let mut invalid = scene.clone(); + invalid.occurrences[0].material = Some(MaterialId(7)); + assert_eq!( + invalid.validate(), + Err(SceneValidationError::UnknownMaterial) + ); + + let mut invalid = scene.clone(); + invalid.root = NodeId(99); + assert_eq!(invalid.validate(), Err(SceneValidationError::UnknownRoot)); + + let mut invalid = scene; + invalid.occurrences[0].transform[0][0] = f64::NAN; + assert_eq!( + invalid.validate(), + Err(SceneValidationError::NonFinite("occurrence transform")) + ); } } diff --git a/crates/polymodel-renderer-worker/src/bin/ldraw_preview.rs b/crates/polymodel-renderer-worker/src/bin/ldraw_preview.rs index d887aa5..83e60b3 100644 --- a/crates/polymodel-renderer-worker/src/bin/ldraw_preview.rs +++ b/crates/polymodel-renderer-worker/src/bin/ldraw_preview.rs @@ -858,7 +858,7 @@ fn run_visible_preview( *control_flow = ControlFlow::Exit; } else { eprintln!( - "[ldraw-metrics] phase=frame frame={} latency_us={} allocations={} event_capacity={}", + "[ldraw-metrics] phase=frame frame={} latency_us={} event_vec_stability={} event_capacity={}", frame_number, frame_started.elapsed().as_micros(), event_vec_stability, diff --git a/crates/polymodel-renderer-worker/src/lib.rs b/crates/polymodel-renderer-worker/src/lib.rs index 28f4ea2..350bb69 100644 --- a/crates/polymodel-renderer-worker/src/lib.rs +++ b/crates/polymodel-renderer-worker/src/lib.rs @@ -2,9 +2,7 @@ #[cfg(not(target_arch = "wasm32"))] use polymodel_ldraw_core::Materialization; -use polymodel_ldraw_core::{ - LdMeshSemantic, ModelSummary, NormalizedPath, OwnedParseResult, RootId, SemanticEvent, -}; +use polymodel_ldraw_core::{ModelSummary, NormalizedPath, OwnedParseResult, RootId, SemanticEvent}; use polymodel_renderer_protocol::scene::{ GeometryId, MaterialId, NodeId, OccurrenceId, ProvenanceId, Scene, SceneBounds, SceneGeometry, SceneId, SceneMaterial, SceneOccurrence, ScenePrimitiveRange, SceneStats, @@ -760,8 +758,7 @@ fn adapt_ldraw_assembly( .then_some(texmap) .flatten() .and_then(|texmap| texmap.glossmap.as_deref()) - .map(|reference| textures(root, reference, "metallic_roughness")) - .transpose()?; + .and_then(|reference| textures(root, reference, "metallic_roughness").ok()); let key = (colour, colour_rgb, alpha, albedo, roughness); Ok::(*material_ids.entry(key).or_insert_with(|| { let id = MaterialId((materials.len() + 1) as u64); @@ -2029,13 +2026,6 @@ pub fn scene_radius(bounds: SceneBounds) -> Result { finite_f32(radius).map_err(|_| CameraDepthError::F32Conversion) } -pub fn semantic_counts(semantic: &LdMeshSemantic) -> (u64, u64) { - ( - semantic.positions.len() as u64, - (semantic.indices.len() / 3) as u64, - ) -} - #[cfg(not(target_arch = "wasm32"))] fn official_texture_resources( closure: &polymodel_ldraw_core::ResourceClosure, diff --git a/crates/polymodel-renderer-worker/src/tests.rs b/crates/polymodel-renderer-worker/src/tests.rs index 5903e20..f6f474a 100644 --- a/crates/polymodel-renderer-worker/src/tests.rs +++ b/crates/polymodel-renderer-worker/src/tests.rs @@ -1,4 +1,5 @@ use super::*; +use polymodel_ldraw_core::LdMeshSemantic; #[cfg(not(target_arch = "wasm32"))] fn encoded_png( @@ -146,6 +147,30 @@ fn texmap_cylindrical_and_spherical_use_degree_extents() { assert!((spherical[1][1] - 0.5).abs() < 1e-6); } +#[test] +fn texmap_spherical_clamps_rounding_outside_unit_interval() { + let parameters = [ + 0.0, + 0.0, + 0.0, + 1.0, + 9.208261502450901e-9, + -9.221431951215706e-9, + 0.0, + 1.0, + 0.0, + 360.0, + 180.0, + ]; + let uvs = texmap_uvs( + SceneTexmapProjectionMode::Spherical, + ¶meters, + &[[1.0, 5.4417490399506624e-9, -6.839130689711565e-9]], + ) + .expect("rounding at asin boundary should remain valid"); + assert!(uvs.iter().flatten().all(|value| value.is_finite())); +} + #[cfg(not(target_arch = "wasm32"))] #[test] fn albedo_decode_expands_rgb_and_preserves_alpha() { @@ -2015,27 +2040,6 @@ fn ldraw_policy_matches_three_d_direction_and_material_contract() { assert!(!policy.alpha_blend); } -#[test] -fn ldraw_policy_direction_contract_distinguishes_top_and_bottom_faces() { - let policy = LdrawLightingMaterialPolicy::ldraw(); - let directional_irradiance = |normal: [f32; 3]| { - let dot = |direction: [f32; 3]| { - (normal[0] * -direction[0] + normal[1] * -direction[1] + normal[2] * -direction[2]) - .max(0.0) - }; - policy.ambient_energy - + policy.key_energy * dot(policy.key_direction) - + policy.fill_energy * dot(policy.fill_direction) - }; - let top = directional_irradiance([0.0, 1.0, 0.0]); - let bottom = directional_irradiance([0.0, -1.0, 0.0]); - - assert!(top.is_finite() && bottom.is_finite()); - assert!(top > bottom, "top={top} bottom={bottom}"); - assert!((top - 1.88).abs() < 1.0e-6); - assert!((bottom - 0.73).abs() < 1.0e-6); -} - // This is a white-box policy assertion, not a visual regression test. #[test] fn seeded_ldconfig_red_is_not_the_grey_material_fallback() { -- 2.51.2