From e796260fe47e0e48572dc0bb4585ee5d2690a80d Mon Sep 17 00:00:00 2001 From: Orual Date: Mon, 10 Aug 2026 12:52:20 -0400 Subject: [PATCH] PM-84: eliminate dead ModelSummary fields, fix validation crash Epic: PM-86 Task: PM-84 --- crates/polymodel-ldraw-core/src/lib.rs | 73 ++++++--- crates/polymodel-ldraw-core/src/model.rs | 2 - crates/polymodel-ldraw-core/src/traversal.rs | 9 -- .../polymodel-renderer-protocol/src/scene.rs | 14 +- .../src/bin/ldraw_preview.rs | 4 +- crates/polymodel-renderer-worker/src/lib.rs | 70 +++----- crates/polymodel-renderer-worker/src/tests.rs | 149 +++++++----------- 7 files changed, 139 insertions(+), 182 deletions(-) diff --git a/crates/polymodel-ldraw-core/src/lib.rs b/crates/polymodel-ldraw-core/src/lib.rs index 6dc8627..85a3ad1 100644 --- a/crates/polymodel-ldraw-core/src/lib.rs +++ b/crates/polymodel-ldraw-core/src/lib.rs @@ -125,7 +125,7 @@ mod tests { &[], ) .expect("compatibility profile should continue after invalid colour"); - assert_eq!(result.models[0].geometry.len(), 1); + assert_eq!(event_geometry(&result, &result.models[0]).len(), 1); assert_eq!(result.models[0].triangles, 1); assert_eq!(result.models[0].quads, 0); } @@ -217,7 +217,8 @@ mod tests { let result = LdrawParser .parse_bytes(b"5 16 0 0 0 1 0 0 0 1 0 2 0 0\n", options(), &[]) .unwrap(); - let geometry = &result.models[0].geometry[0]; + let model_geometry = event_geometry(&result, &result.models[0]); + let geometry = &model_geometry[0]; let (endpoints, controls) = geometry .conditional_endpoints() .expect("type-5 geometry should expose its point roles"); @@ -860,6 +861,37 @@ mod tests { } } + fn event_geometry(result: &ParseResult<'_>, model: &ModelSummary) -> Vec { + result + .events + .iter() + .filter_map(|record| match &record.event { + SemanticEvent::GeometryEmitted { + primitive_id, + source, + span, + line_type, + colour, + vertices, + bfc, + texmap, + inverted, + } if source == &model.cache_key => Some(GeometryRecord { + primitive_id: *primitive_id, + source: source.clone(), + span: *span, + line_type: *line_type, + colour: *colour, + vertices: vertices.clone(), + bfc: *bfc, + inverted: *inverted, + texmap: texmap.clone(), + }), + _ => None, + }) + .collect() + } + fn assert_scene_counts( result: &ParseResult<'_>, triangles: u64, @@ -882,8 +914,9 @@ mod tests { .iter() .find(|model| model.path == "sticker.ldr") .expect("sticker sub-model"); + let sticker_geometry = event_geometry(result, sticker); assert_geometry( - &sticker.geometry, + &sticker_geometry, &[( 4, 16, @@ -898,7 +931,7 @@ mod tests { for model in &result.models { if model.path != "sticker.ldr" { assert!( - model.geometry.is_empty(), + event_geometry(result, model).is_empty(), "non-sticker model {} unexpectedly contains geometry", model.path ); @@ -927,7 +960,7 @@ mod tests { assert_scene_counts(&result, 2, 0, 0, 0); assert_geometry( - &result.models[0].geometry, + &event_geometry(&result, &result.models[0]), &[ ( 3, @@ -1025,7 +1058,7 @@ mod tests { assert_eq!(start.reference.as_deref(), Some("sticker.png")); assert_scene_counts(&result, 2, 1, 0, 0); assert_geometry( - &result.models[0].geometry, + &event_geometry(&result, &result.models[0]), &[ ( 3, @@ -1075,8 +1108,9 @@ mod tests { let result = LdrawParser .parse_bytes(bytes.as_bytes(), options(), &[]) .expect("valid bang-colon geometry should parse"); + let model_geometry = event_geometry(&result, &result.models[0]); assert_eq!( - result.models[0].geometry.len(), + model_geometry.len(), usize::from(expected_type != LineType::One) ); if expected_type == LineType::One { @@ -1089,7 +1123,7 @@ mod tests { .any(|diagnostic| diagnostic.code == DiagnosticCode::UnresolvedReference) ); } else { - let geometry = result.models[0].geometry.last().unwrap(); + let geometry = model_geometry.last().unwrap(); assert_eq!(geometry.line_type, expected_type); assert_eq!( geometry.texmap.as_ref().map(|value| value.pngfile.as_str()), @@ -1106,13 +1140,8 @@ mod tests { &[], ) .expect("fallback bang-colon geometry should parse"); - assert!( - result.models[0].geometry[0] - .texmap - .as_ref() - .unwrap() - .fallback - ); + let model_geometry = event_geometry(&result, &result.models[0]); + assert!(model_geometry[0].texmap.as_ref().unwrap().fallback); } #[test] @@ -1141,7 +1170,7 @@ mod tests { .iter() .any(|diagnostic| diagnostic.code == code) ); - assert!(result.models[0].geometry.is_empty()); + assert!(event_geometry(&result, &result.models[0]).is_empty()); } } @@ -1212,8 +1241,9 @@ mod tests { result.semantic.texmap_events[0].reference.as_deref(), Some("next.png") ); + let model_geometry = event_geometry(&result, &result.models[0]); assert_eq!( - result.models[0].geometry[0] + model_geometry[0] .texmap .as_ref() .and_then(|value| value.glossmap.as_deref()), @@ -1242,9 +1272,10 @@ mod tests { .iter() .any(|diagnostic| diagnostic.code == DiagnosticCode::InvalidColour) ); - assert_eq!(result.models[0].geometry.len(), 1); + let model_geometry = event_geometry(&result, &result.models[0]); + assert_eq!(model_geometry.len(), 1); assert_eq!( - result.models[0].geometry[0] + model_geometry[0] .texmap .as_ref() .map(|value| value.pngfile.as_str()), @@ -1277,7 +1308,7 @@ mod tests { ); assert_scene_counts(&result, 1, 1, 1, 1); assert_geometry( - &result.models[0].geometry, + &event_geometry(&result, &result.models[0]), &[ ( 2, @@ -1458,7 +1489,7 @@ mod tests { assert_eq!(result.semantic.colour_state, crate::ColourCode(301)); assert_scene_counts(&result, 2, 0, 0, 0); assert_geometry( - &result.models[0].geometry, + &event_geometry(&result, &result.models[0]), &[ ( 3, diff --git a/crates/polymodel-ldraw-core/src/model.rs b/crates/polymodel-ldraw-core/src/model.rs index a181504..d9c922c 100644 --- a/crates/polymodel-ldraw-core/src/model.rs +++ b/crates/polymodel-ldraw-core/src/model.rs @@ -879,8 +879,6 @@ pub struct ModelSummary { pub conditional_lines: u64, /// LDParse-authoritative direct-mesh index offsets for this model's steps. pub step_ends: Vec, - pub semantic: LdMeshSemantic, - pub geometry: Vec, pub bfc: BfcState, } impl<'src> ParseResult<'src> { diff --git a/crates/polymodel-ldraw-core/src/traversal.rs b/crates/polymodel-ldraw-core/src/traversal.rs index 618b31c..c54539a 100644 --- a/crates/polymodel-ldraw-core/src/traversal.rs +++ b/crates/polymodel-ldraw-core/src/traversal.rs @@ -311,13 +311,6 @@ pub(crate) fn traverse( ), ); } - let semantic = crate::model::LdMeshSemantic { - positions: Vec::new(), - normals: None, - indices: Vec::new(), - bf_indices: Vec::new(), - step_offsets: Vec::new(), - }; let step_ends = model .step_ends .iter() @@ -343,8 +336,6 @@ pub(crate) fn traverse( lines: model.lines, conditional_lines: model.conditional_lines, step_ends, - semantic, - 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 11a648d..c9084b3 100644 --- a/crates/polymodel-renderer-protocol/src/scene.rs +++ b/crates/polymodel-renderer-protocol/src/scene.rs @@ -383,16 +383,22 @@ impl Scene { finite(value, "scene bounds")?; } } - const BOUNDS_EPSILON: f64 = 1.0e-9; + let max_extent = bounds + .min + .iter() + .zip(bounds.max) + .map(|(min, max)| (max - min).abs()) + .fold(0.0, f64::max); + let bounds_epsilon = (max_extent * 1.0e-6).max(1.0e-6); if bounds .min .iter() .zip(bounds.max) - .any(|(min, max)| *min > max + BOUNDS_EPSILON) + .any(|(min, max)| *min > max + bounds_epsilon) || all_points.iter().any(|point| { point.iter().enumerate().any(|(index, value)| { - *value < bounds.min[index] - BOUNDS_EPSILON - || *value > bounds.max[index] + BOUNDS_EPSILON + *value < bounds.min[index] - bounds_epsilon + || *value > bounds.max[index] + bounds_epsilon }) }) { diff --git a/crates/polymodel-renderer-worker/src/bin/ldraw_preview.rs b/crates/polymodel-renderer-worker/src/bin/ldraw_preview.rs index 83e60b3..66e3c9c 100644 --- a/crates/polymodel-renderer-worker/src/bin/ldraw_preview.rs +++ b/crates/polymodel-renderer-worker/src/bin/ldraw_preview.rs @@ -496,7 +496,7 @@ fn run_visible_preview( } frame_event_capacity = Some(event_capacity); if frame_number == 0 { - eprintln!( + tracing::debug!( "[ldraw-preview] viewport requested={}x{} actual={}x{}", width, height, frame.viewport.width, frame.viewport.height ); @@ -857,7 +857,7 @@ fn run_visible_preview( loop_trace.finish(NativeTerminal::Failure(format!("swap buffers: {error}"))); *control_flow = ControlFlow::Exit; } else { - eprintln!( + tracing::debug!( "[ldraw-metrics] phase=frame frame={} latency_us={} event_vec_stability={} event_capacity={}", frame_number, frame_started.elapsed().as_micros(), diff --git a/crates/polymodel-renderer-worker/src/lib.rs b/crates/polymodel-renderer-worker/src/lib.rs index 350bb69..8eda14d 100644 --- a/crates/polymodel-renderer-worker/src/lib.rs +++ b/crates/polymodel-renderer-worker/src/lib.rs @@ -248,7 +248,13 @@ pub fn adapt_ldraw_parse_with_resources( if models.is_empty() { return Err(RendererError::parse("parse emitted no root model")); } - let scene = adapt_ldraw_assembly(models, Some(&result.result.scene), Some(result), resources)?; + let scene = adapt_ldraw_assembly( + models, + Some(&result.result.scene), + Some(result), + None, + resources, + )?; if std::env::var_os("POLYMODEL_LDRAW_TIMING").is_some() { eprintln!( "[ldraw-metrics] phase=adapt-ldraw elapsed_ms={} geometries={} materials={} textures={} occurrences={} points={} triangles={} lines={} conditional_lines={}", @@ -289,6 +295,7 @@ fn append_boxed(slot: &mut Box<[T]>, items: impl IntoIterator) { *slot = values.into_boxed_slice(); } +#[derive(Clone)] struct EventPrimitive { line_type: polymodel_ldraw_core::LineType, colour: polymodel_ldraw_core::ColourCode, @@ -491,15 +498,17 @@ fn adapt_ldraw_assembly( models: &[ModelSummary], parsed_scene: Option<&polymodel_ldraw_core::SceneRecord>, parsed: Option<&OwnedParseResult>, + fixture_event_primitives: Option< + &std::collections::HashMap>, + >, resources: Option<&[polymodel_renderer_protocol::ModelResource]>, ) -> Result { use polymodel_ldraw_core::LineType; use std::collections::HashMap; // Semantic events are the adapter's geometry authority. Build a borrowed - // per-definition view for batching; never re-materialize GeometryRecord or - // copy it back into the compatibility summaries. - let event_primitives = parsed.map(|parsed| { + // per-definition view for batching. + let parsed_event_primitives = parsed.map(|parsed| { let mut by_key = HashMap::>::new(); for record in &parsed.result.events { if let SemanticEvent::GeometryEmitted { @@ -528,34 +537,9 @@ fn adapt_ldraw_assembly( } by_key }); - // Unit fixtures can call this private assembly adapter with summaries only. - // Real parsed requests always take the event stream above; this compatibility - // projection keeps those synthetic callers useful without making the worker's - // production path depend on ModelSummary.geometry. - let event_primitives = event_primitives.or_else(|| { - Some( - models - .iter() - .map(|model| { - ( - model.cache_key.clone(), - model - .geometry - .iter() - .map(|geometry| EventPrimitive { - line_type: geometry.line_type, - colour: geometry.colour, - vertices: geometry.vertices.clone(), - texmap: geometry.texmap.clone(), - inverted: geometry.inverted, - bfc: geometry.bfc, - }) - .collect(), - ) - }) - .collect(), - ) - }); + let event_primitives = parsed_event_primitives + .as_ref() + .or(fixture_event_primitives); let event_models = models; let diagnostics_enabled = std::env::var_os("POLYMODEL_LDRAW_ADAPTER_DIAGNOSTICS").is_some() @@ -993,24 +977,10 @@ fn adapt_ldraw_assembly( effective_colour, primitive.texmap.as_ref(), )?), - bfc_state: smol_str::format_smolstr!( - "{:?}", - event_include - .map(|(_, _, _, bfc, _)| bfc.state) - .unwrap_or(primitive.bfc_state) - ), - bfc_cull: event_include - .map(|(_, _, _, bfc, _)| bfc.clipping) - .unwrap_or(primitive.bfc_cull), - bfc_ccw: event_include - .map(|(_, _, _, bfc, _)| { - matches!(bfc.winding, polymodel_ldraw_core::Winding::Ccw) - }) - .unwrap_or(primitive.bfc_ccw), - bfc_inverted: event_include - .map(|(_, _, _, bfc, _)| bfc.invert_next) - .unwrap_or(occurrence.inverted) - ^ primitive.reverse_winding, + bfc_state: smol_str::format_smolstr!("{:?}", primitive.bfc_state), + bfc_cull: primitive.bfc_cull, + bfc_ccw: primitive.bfc_ccw, + bfc_inverted: occurrence.inverted ^ primitive.reverse_winding, transform, }); } diff --git a/crates/polymodel-renderer-worker/src/tests.rs b/crates/polymodel-renderer-worker/src/tests.rs index f6f474a..25e0db9 100644 --- a/crates/polymodel-renderer-worker/src/tests.rs +++ b/crates/polymodel-renderer-worker/src/tests.rs @@ -1,5 +1,4 @@ use super::*; -use polymodel_ldraw_core::LdMeshSemantic; #[cfg(not(target_arch = "wasm32"))] fn encoded_png( @@ -2253,7 +2252,7 @@ fn finite_zero_volume_bounds_have_explicit_policy() { #[test] fn ldraw_occurrence_transform_is_conjugated_and_bounds_use_instances() { - use polymodel_ldraw_core::{CacheKey, GeometryRecord, LineType, ModelSummary, NormalizedPath}; + use polymodel_ldraw_core::{CacheKey, LineType, ModelSummary, NormalizedPath}; let root_path = NormalizedPath::new("root.ldr").unwrap(); let child_path = NormalizedPath::new("parts/child.dat").unwrap(); @@ -2263,6 +2262,23 @@ fn ldraw_occurrence_transform_is_conjugated_and_bounds_use_instances() { polymodel_ldraw_core::RootId::OfficialLibrary, b"child-a", ); + let child_geometry = EventPrimitive { + line_type: LineType::Three, + colour: polymodel_ldraw_core::ColourCode(4), + vertices: vec![ + polymodel_ldraw_core::Point3::new(0.0, 0.0, 0.0), + polymodel_ldraw_core::Point3::new(1.0, 0.0, 0.0), + polymodel_ldraw_core::Point3::new(0.0, 1.0, 0.0), + ], + texmap: None, + inverted: false, + bfc: polymodel_ldraw_core::BfcFrame { + state: polymodel_ldraw_core::BfcState::Certified, + clipping: true, + winding: polymodel_ldraw_core::Winding::Ccw, + invert_next: false, + }, + }; let mut child = ModelSummary { model: polymodel_ldraw_core::ModelId(579), id: "child".into(), @@ -2276,38 +2292,6 @@ fn ldraw_occurrence_transform_is_conjugated_and_bounds_use_instances() { lines: 0, conditional_lines: 0, step_ends: Vec::new(), - semantic: LdMeshSemantic { - positions: vec![[0.0, 0.0, 0.0], [1.0, 0.0, 0.0], [0.0, 1.0, 0.0]], - normals: None, - indices: vec![0, 1, 2], - bf_indices: vec![2, 1, 0], - step_offsets: Vec::new(), - }, - geometry: vec![GeometryRecord { - primitive_id: 0, - source: child_key.clone(), - span: polymodel_ldraw_core::Span { - start: 0, - end: 31, - line: 1, - column: 1, - }, - line_type: LineType::Three, - colour: polymodel_ldraw_core::ColourCode(4), - vertices: vec![ - polymodel_ldraw_core::Point3::new(0.0, 0.0, 0.0), - polymodel_ldraw_core::Point3::new(1.0, 0.0, 0.0), - polymodel_ldraw_core::Point3::new(0.0, 1.0, 0.0), - ], - bfc: polymodel_ldraw_core::BfcFrame { - state: polymodel_ldraw_core::BfcState::Certified, - clipping: true, - winding: polymodel_ldraw_core::Winding::Ccw, - invert_next: false, - }, - inverted: false, - texmap: None, - }], bfc: polymodel_ldraw_core::BfcState::Certified, }; let root = ModelSummary { @@ -2323,29 +2307,20 @@ fn ldraw_occurrence_transform_is_conjugated_and_bounds_use_instances() { lines: 0, conditional_lines: 0, step_ends: Vec::new(), - semantic: LdMeshSemantic { - positions: Vec::new(), - normals: None, - indices: Vec::new(), - bf_indices: Vec::new(), - step_offsets: Vec::new(), - }, - geometry: Vec::new(), bfc: polymodel_ldraw_core::BfcState::Unknown, }; - let mut clockwise = child.geometry[0].clone(); - clockwise.primitive_id = 1; + let mut clockwise = child_geometry.clone(); clockwise.bfc.clipping = false; clockwise.bfc.winding = polymodel_ldraw_core::Winding::Cw; clockwise.inverted = true; - let mut edge = child.geometry[0].clone(); - edge.primitive_id = 2; + let mut edge = child_geometry.clone(); edge.line_type = LineType::Two; edge.colour = polymodel_ldraw_core::ColourCode(24); edge.vertices.truncate(2); - child.geometry.extend([clockwise, edge]); child.triangles = 2; child.lines = 1; + let mut fixture_events = std::collections::HashMap::new(); + fixture_events.insert(child_key.clone(), vec![child_geometry, clockwise, edge]); let transform = polymodel_ldraw_core::Transform { translation: polymodel_ldraw_core::Point3::new(10.0, 20.0, 30.0), matrix: polymodel_ldraw_core::Matrix3 { @@ -2381,7 +2356,14 @@ fn ldraw_occurrence_transform_is_conjugated_and_bounds_use_instances() { bounds: None, reflection: false, }; - let scene = adapt_ldraw_assembly(&[root, child], Some(&parsed_scene), None, None).unwrap(); + let scene = adapt_ldraw_assembly( + &[root, child], + Some(&parsed_scene), + None, + Some(&fixture_events), + None, + ) + .unwrap(); let child_geometry = scene .geometries .iter() @@ -2433,7 +2415,7 @@ fn ldraw_occurrence_transform_is_conjugated_and_bounds_use_instances() { #[test] fn ldraw_repeated_same_path_instances_keep_exact_definition_keys() { - use polymodel_ldraw_core::{CacheKey, GeometryRecord, LineType, ModelSummary, NormalizedPath}; + use polymodel_ldraw_core::{CacheKey, LineType, ModelSummary, NormalizedPath}; let root_key = CacheKey::new( NormalizedPath::new("root.ldr").unwrap(), @@ -2466,38 +2448,6 @@ fn ldraw_repeated_same_path_instances_keep_exact_definition_keys() { lines: 0, conditional_lines: 0, step_ends: Vec::new(), - semantic: LdMeshSemantic { - positions: vec![[0.0, 0.0, 0.0], [1.0, 0.0, 0.0], [0.0, 1.0, 0.0]], - normals: None, - indices: vec![0, 1, 2], - bf_indices: vec![2, 1, 0], - step_offsets: Vec::new(), - }, - geometry: vec![GeometryRecord { - primitive_id: 0, - source: key, - span: polymodel_ldraw_core::Span { - start: 0, - end: 31, - line: 1, - column: 1, - }, - line_type: LineType::Three, - colour: polymodel_ldraw_core::ColourCode(4), - vertices: vec![ - polymodel_ldraw_core::Point3::new(0.0, 0.0, 0.0), - polymodel_ldraw_core::Point3::new(1.0, 0.0, 0.0), - polymodel_ldraw_core::Point3::new(0.0, 1.0, 0.0), - ], - bfc: polymodel_ldraw_core::BfcFrame { - state: polymodel_ldraw_core::BfcState::Certified, - clipping: true, - winding: polymodel_ldraw_core::Winding::Ccw, - invert_next: false, - }, - inverted: false, - texmap: None, - }], bfc: polymodel_ldraw_core::BfcState::Certified, }; let root = ModelSummary { @@ -2513,14 +2463,6 @@ fn ldraw_repeated_same_path_instances_keep_exact_definition_keys() { lines: 0, conditional_lines: 0, step_ends: Vec::new(), - semantic: LdMeshSemantic { - positions: Vec::new(), - normals: None, - indices: Vec::new(), - bf_indices: Vec::new(), - step_offsets: Vec::new(), - }, - geometry: Vec::new(), bfc: polymodel_ldraw_core::BfcState::Unknown, }; let transform = polymodel_ldraw_core::Transform::default(); @@ -2544,6 +2486,23 @@ fn ldraw_repeated_same_path_instances_keep_exact_definition_keys() { texmap: None, } }; + let fixture_primitive = || EventPrimitive { + line_type: LineType::Three, + colour: polymodel_ldraw_core::ColourCode(4), + vertices: vec![ + polymodel_ldraw_core::Point3::new(0.0, 0.0, 0.0), + polymodel_ldraw_core::Point3::new(1.0, 0.0, 0.0), + polymodel_ldraw_core::Point3::new(0.0, 1.0, 0.0), + ], + texmap: None, + inverted: false, + bfc: polymodel_ldraw_core::BfcFrame { + state: polymodel_ldraw_core::BfcState::Certified, + clipping: true, + winding: polymodel_ldraw_core::Winding::Ccw, + invert_next: false, + }, + }; let parsed_scene = polymodel_ldraw_core::SceneRecord { model_id: "root".into(), instance_ids: vec!["a".into(), "b".into()], @@ -2558,14 +2517,16 @@ fn ldraw_repeated_same_path_instances_keep_exact_definition_keys() { bounds: None, reflection: false, }; + let child_a = child(polymodel_ldraw_core::ModelId(2), "a", child_a_key.clone()); + let child_b = child(polymodel_ldraw_core::ModelId(3), "b", child_b_key.clone()); + let mut fixture_events = std::collections::HashMap::new(); + fixture_events.insert(child_a.cache_key.clone(), vec![fixture_primitive()]); + fixture_events.insert(child_b.cache_key.clone(), vec![fixture_primitive()]); let scene = adapt_ldraw_assembly( - &[ - root, - child(polymodel_ldraw_core::ModelId(2), "a", child_a_key), - child(polymodel_ldraw_core::ModelId(3), "b", child_b_key), - ], + &[root, child_a, child_b], Some(&parsed_scene), None, + Some(&fixture_events), None, ) .unwrap(); -- 2.51.2