From 32ef90f488a9a938647289ddcbaf6cdb2659ebf7 Mon Sep 17 00:00:00 2001 From: Orual Date: Mon, 10 Aug 2026 12:05:05 -0400 Subject: [PATCH] PM-84: resolve Round 6 review findings Epic: PM-86 Task: PM-84 --- crates/polymodel-ldraw-core/src/mpd.rs | 14 +- crates/polymodel-ldraw-core/src/parser.rs | 67 ++++++ crates/polymodel-ldraw-core/src/semantic.rs | 30 ++- crates/polymodel-ldraw-core/src/traversal.rs | 111 ++++----- crates/polymodel-ldraw-testkit/src/gates.rs | 44 +++- .../polymodel-renderer-protocol/src/scene.rs | 182 ++++++++++++++- .../src/bin/ldraw_preview.rs | 6 +- crates/polymodel-renderer-worker/src/lib.rs | 214 ++++++++++-------- crates/polymodel-renderer-worker/src/tests.rs | 2 +- 9 files changed, 504 insertions(+), 166 deletions(-) diff --git a/crates/polymodel-ldraw-core/src/mpd.rs b/crates/polymodel-ldraw-core/src/mpd.rs index 7ff357b..85d3613 100644 --- a/crates/polymodel-ldraw-core/src/mpd.rs +++ b/crates/polymodel-ldraw-core/src/mpd.rs @@ -308,6 +308,8 @@ pub enum ResourceClosureError { Sha256Mismatch, #[error("required resource is missing from the closure")] MissingRequired, + #[error("duplicate resolved resource identity")] + DuplicateIdentity, } impl ResolvedResource { @@ -324,7 +326,11 @@ impl ResolvedResource { impl ResourceClosure { pub fn validate(&self) -> Result<(), ResourceClosureError> { + let mut identities = std::collections::HashSet::with_capacity(self.resolved.len()); for resource in &self.resolved { + if !identities.insert((resource.root, resource.path.clone())) { + return Err(ResourceClosureError::DuplicateIdentity); + } resource.validate()?; } for dependency in self @@ -344,8 +350,12 @@ impl ResourceClosure { pub fn resolved_texture(&self, path: &NormalizedPath) -> Option<&ResolvedResource> { let authored = path.as_str(); - let prefixed = format!("textures/{authored}"); - let stripped = authored.strip_prefix("textures/").unwrap_or(authored); + let stripped = authored + .split_once('/') + .filter(|(prefix, _)| prefix.eq_ignore_ascii_case("textures")) + .map(|(_, rest)| rest) + .unwrap_or(authored); + let prefixed = format!("textures/{stripped}"); [authored, prefixed.as_str(), stripped] .into_iter() .find_map(|candidate| { diff --git a/crates/polymodel-ldraw-core/src/parser.rs b/crates/polymodel-ldraw-core/src/parser.rs index 4f3a395..c48c983 100644 --- a/crates/polymodel-ldraw-core/src/parser.rs +++ b/crates/polymodel-ldraw-core/src/parser.rs @@ -280,6 +280,20 @@ impl LdrawParser { metrics.report(&started, "parsed-materializations"); project_ldparse_step_ends(&mut models, source_model_count); let mut event_collector = EventCollector::default(); + for diagnostic in &diagnostics { + event_collector.on_event( + &SemanticEvent::Diagnostic(diagnostic.clone()), + &TraversalContext::for_model( + crate::Transform::default(), + DEFAULT_COLOUR, + BfcFrame::default(), + TexmapState::Inactive, + None, + 0, + diagnostic.span, + ), + ); + } for model in &models { if let Some(colour) = colours.get(model.colour) { event_collector.on_event( @@ -298,6 +312,59 @@ impl LdrawParser { ), ); } + for texmap in &model.texmap_events { + let descriptor = texmap.mode.map(|mode| TextureDescriptor { + mode, + parameters: texmap + .parameters + .iter() + .map(|value| Cow::Owned(value.to_string())) + .collect(), + pngfile: texmap + .reference + .as_ref() + .map(|value| Cow::Owned(value.to_string())) + .unwrap_or_else(|| Cow::Borrowed("")), + glossmap: None, + }); + event_collector.on_event( + &SemanticEvent::TexmapTransition { + kind: match texmap.kind { + crate::TexmapKind::Start => crate::TexmapKind::Start, + crate::TexmapKind::Next => crate::TexmapKind::Next, + crate::TexmapKind::Fallback => crate::TexmapKind::Fallback, + crate::TexmapKind::End => crate::TexmapKind::End, + }, + descriptor, + }, + &TraversalContext::for_model( + crate::Transform::default(), + model.colour, + model.bfc, + texmap.state, + model.texmap_descriptor.as_ref(), + model.file_id, + Some(texmap.span), + ), + ); + } + for payload in &model.data_payloads { + event_collector.on_event( + &SemanticEvent::DataPayload { + filename: payload.filename.as_str().into(), + bytes: payload.bytes.clone(), + }, + &TraversalContext::for_model( + crate::Transform::default(), + model.colour, + model.bfc, + model.texmap_state, + model.texmap_descriptor.as_ref(), + model.file_id, + None, + ), + ); + } } let (scene, summaries) = traverse( &models, diff --git a/crates/polymodel-ldraw-core/src/semantic.rs b/crates/polymodel-ldraw-core/src/semantic.rs index 6271a67..5089fe8 100644 --- a/crates/polymodel-ldraw-core/src/semantic.rs +++ b/crates/polymodel-ldraw-core/src/semantic.rs @@ -4,8 +4,16 @@ use crate::geom::{Point3, Transform}; use crate::model::ColourData; use crate::scanner::LineType; use crate::texmap::{TexmapAssociation, TexmapState, TextureDescriptor}; -use crate::types::Span; +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 /// is walking occurrences, before retained compatibility records are assembled. @@ -17,7 +25,7 @@ pub trait SemanticSink { #[derive(Clone, Debug, PartialEq)] pub struct TraversalContext { pub composed_transform: Transform, - pub colour_scope: crate::ColourCode, + pub colour_scope: ColourScopeSnapshot, pub bfc_frame: BfcFrame, pub texmap_state: TexmapState, pub texmap_descriptor: Option>, @@ -37,7 +45,10 @@ impl TraversalContext { ) -> Self { Self { composed_transform: transform, - colour_scope: colour, + colour_scope: ColourScopeSnapshot { + effective: colour, + local_mapping: None, + }, bfc_frame, texmap_state, texmap_descriptor: texmap_descriptor.map(|descriptor| TextureDescriptor { @@ -67,9 +78,13 @@ pub enum SemanticEvent { root: CacheKey, }, GeometryEmitted { + primitive_id: u32, + source: CacheKey, + span: Span, line_type: LineType, colour: crate::ColourCode, vertices: Vec, + bfc: BfcFrame, texmap: Option, inverted: bool, }, @@ -85,6 +100,15 @@ pub enum SemanticEvent { code: crate::ColourCode, data: ColourData, }, + TexmapTransition { + kind: crate::TexmapKind, + descriptor: Option>, + }, + DataPayload { + filename: SmolStr, + bytes: Vec, + }, + Diagnostic(Diagnostic), } #[derive(Clone, Debug, PartialEq)] diff --git a/crates/polymodel-ldraw-core/src/traversal.rs b/crates/polymodel-ldraw-core/src/traversal.rs index 32e152a..f84c6ba 100644 --- a/crates/polymodel-ldraw-core/src/traversal.rs +++ b/crates/polymodel-ldraw-core/src/traversal.rs @@ -1,6 +1,7 @@ use crate::cache::CacheKey; +use crate::geom::GeometryRecord; use crate::geom::{Bounds3, Transform}; -use crate::model::{InstanceRecord, LdMeshSemantic, ModelData, ModelId, ModelSummary, SceneRecord}; +use crate::model::{InstanceRecord, ModelData, ModelId, ModelSummary, SceneRecord}; use crate::semantic::{SemanticEvent, SemanticSink, TraversalContext}; use crate::types::{ Diagnostic, DiagnosticCode, LimitCounters, LimitKind, ParseError, ParseOptions, add_diag, @@ -8,57 +9,6 @@ use crate::types::{ }; use std::collections::{HashMap, HashSet, VecDeque}; -pub(super) fn semantic_export(model: &ModelData<'_>) -> Result { - let mut positions = Vec::new(); - let mut indices = Vec::new(); - let mut bf_indices = Vec::new(); - for geometry in &model.geometry { - match geometry.line_type { - crate::scanner::LineType::Three if geometry.vertices.len() == 3 => { - let base = u32::try_from(positions.len()) - .map_err(|_| ParseError::Overflow("semantic position index"))?; - positions.extend( - geometry - .vertices - .iter() - .map(|point| [point.x, point.y, point.z]), - ); - let forward = [base, base + 1, base + 2]; - indices.extend(forward); - bf_indices.extend([base + 2, base + 1, base]); - } - crate::scanner::LineType::Four if geometry.vertices.len() == 4 => { - let base = u32::try_from(positions.len()) - .map_err(|_| ParseError::Overflow("semantic position index"))?; - positions.extend( - geometry - .vertices - .iter() - .map(|point| [point.x, point.y, point.z]), - ); - indices.extend([base, base + 1, base + 2, base, base + 2, base + 3]); - bf_indices.extend([base + 2, base + 1, base, base + 3, base + 2, base]); - } - _ => {} - } - } - let semantic = LdMeshSemantic { - positions, - normals: None, - indices, - bf_indices, - step_offsets: model - .step_ends - .iter() - .map(|offset| { - usize::try_from(*offset).map_err(|_| ParseError::Overflow("step end offset")) - }) - .collect::, _>>()?, - }; - semantic.validate_finite().map_err(ParseError::Overflow)?; - Ok(semantic) -} - fn target_index( models: &[ModelData<'_>], source: &ModelData<'_>, @@ -323,15 +273,21 @@ 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, + source: geometry.source.clone(), + span: geometry.span, + line_type: geometry.line_type, + colour: geometry.colour, + vertices: geometry.vertices.clone(), + bfc: geometry.bfc, + texmap: geometry.texmap.clone(), + inverted: geometry.inverted, + }; sink.on_event( - &SemanticEvent::GeometryEmitted { - line_type: geometry.line_type, - colour: geometry.colour, - vertices: geometry.vertices.clone(), - texmap: geometry.texmap.clone(), - inverted: geometry.inverted, - }, + &event, &TraversalContext::for_model( transform, colour, @@ -342,6 +298,30 @@ 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( @@ -357,7 +337,16 @@ pub(crate) fn traverse( ), ); } - let semantic = semantic_export(model)?; + // 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, + indices: Vec::new(), + bf_indices: Vec::new(), + step_offsets: Vec::new(), + }; let step_ends = model .step_ends .iter() @@ -384,7 +373,7 @@ pub(crate) fn traverse( conditional_lines: model.conditional_lines, step_ends, semantic, - geometry: model.geometry.clone(), + geometry: compatibility_geometry, bfc: model.bfc.state, }); } diff --git a/crates/polymodel-ldraw-testkit/src/gates.rs b/crates/polymodel-ldraw-testkit/src/gates.rs index b11acc2..a0c43e0 100644 --- a/crates/polymodel-ldraw-testkit/src/gates.rs +++ b/crates/polymodel-ldraw-testkit/src/gates.rs @@ -371,10 +371,27 @@ impl CorpusGate for NamedGate { for field in definition.required_fields { let present = match *field { "candidate.syntax" => !actual.syntax.is_empty(), - "candidate.diagnostics" => true, - "candidate.scene" => true, + "candidate.diagnostics" => actual + .diagnostics + .iter() + .all(|diagnostic| !diagnostic.message.is_empty()), + "candidate.scene" => { + actual.scene.instances.len() == actual.scene.instance_ids.len() + && actual.scene.bounds.is_none_or(|bounds| { + bounds.min.x <= bounds.max.x + && bounds.min.y <= bounds.max.y + && bounds.min.z <= bounds.max.z + }) + } "candidate.semantic.limits" => !actual.semantic.limits.is_empty(), - "candidate.semantic.texmap_events" => true, + "candidate.semantic.texmap_events" => { + actual.semantic.texmap_events.iter().all(|event| { + event + .reference + .as_ref() + .is_none_or(|reference| !reference.is_empty()) + }) + } "candidate.semantic.canonical_path" => { actual.semantic.canonical_path.is_some() } @@ -447,7 +464,26 @@ impl CorpusGate for NamedGate { Ok(()) } "graph_identity_and_instances" => { - if actual.scene.instances.len() != actual.scene.instance_ids.len() { + let ids = actual + .scene + .instances + .iter() + .map(|instance| instance.instance_id.as_str()) + .collect::>(); + if ids.len() != actual.scene.instance_ids.len() + || ids + .iter() + .zip(&actual.scene.instance_ids) + .any(|(actual, expected)| *actual != expected) + || actual + .scene + .instances + .windows(2) + .any(|instances| instances[0].instance_id == instances[1].instance_id) + || actual.scene.instances.iter().any(|instance| { + instance.source_model.is_empty() || instance.target_model.is_empty() + }) + { return fail("candidate.scene.instances"); } Ok(()) diff --git a/crates/polymodel-renderer-protocol/src/scene.rs b/crates/polymodel-renderer-protocol/src/scene.rs index 69e0c1d..22f068c 100644 --- a/crates/polymodel-renderer-protocol/src/scene.rs +++ b/crates/polymodel-renderer-protocol/src/scene.rs @@ -213,19 +213,195 @@ pub enum SceneValidationError { DuplicateGeometryId, #[error("occurrence references unknown geometry")] UnknownGeometry, + #[error("duplicate {0} id")] + DuplicateId(&'static str), + #[error("non-finite {0}")] + NonFinite(&'static str), + #[error("{0} index is out of range")] + IndexOutOfRange(&'static str), + #[error("geometry UV cardinality does not match points")] + UvCardinality, + #[error("scene references an unknown material")] + UnknownMaterial, + #[error("material references an unknown texture")] + UnknownTexture, + #[error("primitive range exceeds geometry points")] + PrimitiveRange, + #[error("scene bounds are inconsistent with geometry points")] + BoundsInconsistent, #[error("{0} count overflow")] CountOverflow(&'static str), } impl Scene { + pub fn validate(&self) -> Result<(), SceneValidationError> { + use std::collections::HashSet; + + let geometry_ids = self + .geometries + .iter() + .map(|geometry| geometry.id.0) + .collect::>(); + if geometry_ids.len() != self.geometries.len() { + return Err(SceneValidationError::DuplicateGeometryId); + } + let material_ids = self + .materials + .iter() + .map(|material| material.id.0) + .collect::>(); + if material_ids.len() != self.materials.len() { + return Err(SceneValidationError::DuplicateId("material")); + } + let texture_ids = self + .textures + .iter() + .map(|texture| texture.id.0) + .collect::>(); + if texture_ids.len() != self.textures.len() { + return Err(SceneValidationError::DuplicateId("texture")); + } + let finite = |value: f64, name: &'static str| { + value + .is_finite() + .then_some(()) + .ok_or(SceneValidationError::NonFinite(name)) + }; + let mut all_points = Vec::new(); + for geometry in &self.geometries { + for point in geometry.points.iter() { + for value in point { + finite(*value, "geometry point")?; + } + } + if !geometry.uvs.is_empty() && geometry.uvs.len() != geometry.points.len() { + return Err(SceneValidationError::UvCardinality); + } + let check = |index: u32, kind: &'static str| { + (usize::try_from(index) + .ok() + .is_some_and(|index| index < geometry.points.len())) + .then_some(()) + .ok_or(SceneValidationError::IndexOutOfRange(kind)) + }; + for triangle in &geometry.triangles { + for index in triangle { + check(*index, "triangle")?; + } + } + for quad in &geometry.quads { + for index in quad { + check(*index, "quad")?; + } + } + for line in &geometry.lines { + for index in line { + check(*index, "line")?; + } + } + for line in &geometry.conditional_lines { + for index in line { + check(*index, "conditional-line")?; + } + } + for primitive in &geometry.primitives { + let end = usize::try_from(primitive.point_start) + .ok() + .and_then(|start| { + usize::try_from(primitive.point_count) + .ok() + .and_then(|count| start.checked_add(count)) + }); + if end.is_none_or(|end| end > geometry.points.len()) { + return Err(SceneValidationError::PrimitiveRange); + } + } + } + for material in &self.materials { + if material + .albedo_texture + .is_some_and(|id| !texture_ids.contains(&id.0)) + || material + .metallic_roughness_texture + .is_some_and(|id| !texture_ids.contains(&id.0)) + { + return Err(SceneValidationError::UnknownTexture); + } + } + for occurrence in &self.occurrences { + for row in &occurrence.transform { + for value in row { + finite(*value, "occurrence transform")?; + } + } + if !geometry_ids.contains(&occurrence.node.0) { + return Err(SceneValidationError::UnknownGeometry); + } + if occurrence + .material + .is_some_and(|id| !material_ids.contains(&id.0)) + { + return Err(SceneValidationError::UnknownMaterial); + } + if occurrence.uvs.as_ref().is_some_and(|uvs| { + self.geometries + .iter() + .find(|geometry| geometry.id.0 == occurrence.node.0) + .is_some_and(|geometry| uvs.len() != geometry.points.len()) + }) { + return Err(SceneValidationError::UvCardinality); + } + if let Some(geometry) = self + .geometries + .iter() + .find(|geometry| geometry.id.0 == occurrence.node.0) + { + for point in &geometry.points { + let values = [point[0], point[1], point[2], 1.0]; + let transformed = (0..3) + .map(|row| { + (0..4) + .map(|column| occurrence.transform[column][row] * values[column]) + .sum::() + }) + .collect::>(); + for value in &transformed { + finite(*value, "transformed geometry point")?; + } + all_points.push([transformed[0], transformed[1], transformed[2]]); + } + } + } + if let Some(bounds) = self.bounds { + for point in [bounds.min, bounds.max] { + for value in point { + finite(value, "scene bounds")?; + } + } + if bounds + .min + .iter() + .zip(bounds.max) + .any(|(min, max)| *min > max) + || all_points.iter().any(|point| { + point.iter().enumerate().any(|(index, value)| { + *value < bounds.min[index] || *value > bounds.max[index] + }) + }) + { + return Err(SceneValidationError::BoundsInconsistent); + } + } + Ok(()) + } + pub fn recompute_stats(&mut self) -> Result<(), SceneValidationError> { use std::collections::HashMap; + self.validate()?; let mut geometries = HashMap::with_capacity(self.geometries.len()); for geometry in &self.geometries { - if geometries.insert(geometry.id.0, geometry).is_some() { - return Err(SceneValidationError::DuplicateGeometryId); - } + geometries.insert(geometry.id.0, geometry); } let mut stats = SceneStats { occurrences: u64::try_from(self.occurrences.len()) diff --git a/crates/polymodel-renderer-worker/src/bin/ldraw_preview.rs b/crates/polymodel-renderer-worker/src/bin/ldraw_preview.rs index 2ea1dac..d887aa5 100644 --- a/crates/polymodel-renderer-worker/src/bin/ldraw_preview.rs +++ b/crates/polymodel-renderer-worker/src/bin/ldraw_preview.rs @@ -372,7 +372,7 @@ fn run_visible_preview( let mut redraw_requested = false; let mut frame_number = 0_u32; let mut frame_event_capacity = None; - let mut frame_allocations = 0_u64; + let mut event_vec_stability = 0_u64; let mut base_conditional_generation = 0_u64; let runtime_diagnostics = env::var_os("POLYMODEL_LDRAW_RUNTIME_DIAGNOSTICS").is_some(); let mut resized = false; @@ -492,7 +492,7 @@ fn run_visible_preview( let mut frame = frame_generator.generate(&context); let event_capacity = frame.events.capacity(); if frame_event_capacity.is_some_and(|capacity| event_capacity > capacity) { - frame_allocations += 1; + event_vec_stability += 1; } frame_event_capacity = Some(event_capacity); if frame_number == 0 { @@ -861,7 +861,7 @@ fn run_visible_preview( "[ldraw-metrics] phase=frame frame={} latency_us={} allocations={} event_capacity={}", frame_number, frame_started.elapsed().as_micros(), - frame_allocations, + event_vec_stability, event_capacity, ); if rendered && frame_number == 1 { diff --git a/crates/polymodel-renderer-worker/src/lib.rs b/crates/polymodel-renderer-worker/src/lib.rs index 07bcf5d..28f4ea2 100644 --- a/crates/polymodel-renderer-worker/src/lib.rs +++ b/crates/polymodel-renderer-worker/src/lib.rs @@ -224,8 +224,12 @@ fn texture_resource_for<'a>( RootId::OfficialLibrary => matches!(label, "official-parts" | "official-p"), }; let authored = path.as_str(); - let prefixed = format!("textures/{authored}"); - let stripped = authored.strip_prefix("textures/").unwrap_or(authored); + let stripped = authored + .split_once('/') + .filter(|(prefix, _)| prefix.eq_ignore_ascii_case("textures")) + .map(|(_, rest)| rest) + .unwrap_or(authored); + let prefixed = format!("textures/{stripped}"); for candidate in [authored, prefixed.as_str(), stripped] { if let Some(resource) = resources .iter() @@ -287,8 +291,18 @@ fn append_boxed(slot: &mut Box<[T]>, items: impl IntoIterator) { *slot = values.into_boxed_slice(); } +struct EventPrimitive { + line_type: polymodel_ldraw_core::LineType, + colour: polymodel_ldraw_core::ColourCode, + vertices: Vec, + texmap: Option, + inverted: bool, + bfc: polymodel_ldraw_core::BfcFrame, +} + fn build_primitive_batches( model: &ModelSummary, + primitives: &[EventPrimitive], provenance_id: ProvenanceId, geometry_offset: usize, texmap_available: F, @@ -301,7 +315,7 @@ where let mut batches = Vec::<(PrimitiveBatchKey, SceneGeometry)>::new(); let mut batch_indices = HashMap::>::new(); - for primitive in &model.geometry { + for primitive in primitives { let texmap = primitive.texmap.as_ref().and_then(|texmap| { let mode = match texmap.mode { polymodel_ldraw_core::TexmapProjectionMode::Planar => { @@ -484,64 +498,67 @@ fn adapt_ldraw_assembly( use polymodel_ldraw_core::LineType; use std::collections::HashMap; - // The semantic stream is the primary geometry/occurrence input. Retained - // summaries remain available for IDs and debugging while consumers migrate. - let event_models = parsed.map(|parsed| { - let mut current_key = None; - let mut next_primitive_id = 0_u32; - let mut geometry_by_key = HashMap::>::new(); + // 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| { + let mut by_key = HashMap::>::new(); for record in &parsed.result.events { - match &record.event { - SemanticEvent::ModelBoundary { root, .. } => { - current_key = Some(root.clone()); - next_primitive_id = 0; - } - SemanticEvent::GeometryEmitted { - line_type, - colour, - vertices, - texmap, - inverted, - } => { - if let Some(key) = current_key.as_ref() { - next_primitive_id = next_primitive_id.saturating_add(1); - geometry_by_key.entry(key.clone()).or_default().push( - polymodel_ldraw_core::GeometryRecord { - primitive_id: next_primitive_id, - source: key.clone(), - span: record.context.source_span.unwrap_or( - polymodel_ldraw_core::Span { - start: 0, - end: 0, - line: 0, - column: 0, - }, - ), - line_type: *line_type, - colour: *colour, - vertices: vertices.clone(), - bfc: record.context.bfc_frame, - inverted: *inverted, - texmap: texmap.clone(), - }, - ); - } - } - _ => {} + if let SemanticEvent::GeometryEmitted { + source, + line_type, + colour, + vertices, + bfc, + texmap, + inverted, + .. + } = &record.event + { + by_key + .entry(source.clone()) + .or_default() + .push(EventPrimitive { + line_type: *line_type, + colour: *colour, + vertices: vertices.clone(), + texmap: texmap.clone(), + inverted: *inverted, + bfc: *bfc, + }); } } - models - .iter() - .cloned() - .map(|mut model| { - if let Some(geometry) = geometry_by_key.get(&model.cache_key) { - model.geometry = geometry.clone(); - } - model - }) - .collect::>() + 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_models = event_models.as_deref().unwrap_or(models); + let event_models = models; let diagnostics_enabled = std::env::var_os("POLYMODEL_LDRAW_ADAPTER_DIAGNOSTICS").is_some() || std::env::var_os("POLYMODEL_LDRAW_SCENE_DIAGNOSTICS").is_some(); @@ -553,19 +570,20 @@ fn adapt_ldraw_assembly( .map(|model| (model.model, Arc::::from(model.path.as_str()))) .collect::>(); if std::env::var_os("POLYMODEL_LDRAW_ADAPTER_DIAGNOSTICS").is_some() { - let definitions_with_semantic_triangles = event_models - .iter() - .filter(|model| !model.semantic.indices.is_empty()) - .count(); - let definitions_with_geometry_triangles = event_models - .iter() - .filter(|model| { - model - .geometry - .iter() - .any(|geometry| matches!(geometry.line_type, LineType::Three | LineType::Four)) + let definitions_with_semantic_triangles = event_primitives + .as_ref() + .map(|by_key| { + by_key + .values() + .filter(|primitives| { + primitives.iter().any(|primitive| { + matches!(primitive.line_type, LineType::Three | LineType::Four) + }) + }) + .count() }) - .count(); + .unwrap_or(0); + let definitions_with_geometry_triangles = definitions_with_semantic_triangles; tracing::debug!( "[ldraw-adapter] definitions={} semantic_triangle_defs={} geometry_triangle_defs={} root_key={:?}", event_models.len(), @@ -578,16 +596,28 @@ fn adapt_ldraw_assembly( "[ldraw-adapter] def[{index}] path={} key={:?} semantic_triangles={} geometry_records={} geometry_triangles={}", model.path, model.cache_key, - model.semantic.indices.len() / 3, - model.geometry.len(), - model - .geometry - .iter() - .filter(|geometry| matches!( - geometry.line_type, - LineType::Three | LineType::Four - )) - .count(), + event_primitives + .as_ref() + .and_then(|by_key| by_key.get(&model.cache_key)) + .map(|primitives| primitives.len()) + .unwrap_or(0), + event_primitives + .as_ref() + .and_then(|by_key| by_key.get(&model.cache_key)) + .map(|primitives| primitives.len()) + .unwrap_or(0), + event_primitives + .as_ref() + .and_then(|by_key| by_key.get(&model.cache_key)) + .map(|primitives| { + primitives + .iter() + .filter(|primitive| { + matches!(primitive.line_type, LineType::Three | LineType::Four) + }) + .count() + }) + .unwrap_or(0), ); } } @@ -638,11 +668,6 @@ fn adapt_ldraw_assembly( resource.validate().ok().map(|_| ()) }; valid(texmap.pngfile.as_str()).is_some() - && texmap - .glossmap - .as_deref() - .map(|reference| valid(reference).is_some()) - .unwrap_or(true) }; let mut material_ids = HashMap::< ( @@ -662,8 +687,18 @@ fn adapt_ldraw_assembly( source: model.path.clone(), span: None, }); - let batches = - build_primitive_batches(model, provenance_id, geometries.len(), &texmap_available)?; + let primitives = event_primitives + .as_ref() + .and_then(|by_key| by_key.get(&model.cache_key)) + .map(Vec::as_slice) + .unwrap_or(&[]); + let batches = build_primitive_batches( + model, + primitives, + provenance_id, + geometries.len(), + &texmap_available, + )?; let nodes = batches .into_iter() .map(|(key, geometry)| { @@ -717,6 +752,7 @@ fn adapt_ldraw_assembly( .is_some() .then_some(texmap) .flatten() + .filter(|texmap| texmap_available(root, texmap)) .map(|texmap| textures(root, texmap.pngfile.as_str(), "albedo")) .transpose()?; let roughness = resources @@ -1175,9 +1211,9 @@ fn adapt_ldraw_assembly( bounds, stats: SceneStats::default(), }; - scene - .recompute_stats() - .map_err(|_| RendererError::ParseAdaptation("scene statistics are invalid"))?; + scene.recompute_stats().map_err(|error| { + RendererError::ParseMessage(format!("scene statistics are invalid: {error}")) + })?; Ok(scene) } diff --git a/crates/polymodel-renderer-worker/src/tests.rs b/crates/polymodel-renderer-worker/src/tests.rs index bec078b..5903e20 100644 --- a/crates/polymodel-renderer-worker/src/tests.rs +++ b/crates/polymodel-renderer-worker/src/tests.rs @@ -2016,7 +2016,7 @@ fn ldraw_policy_matches_three_d_direction_and_material_contract() { } #[test] -fn ldraw_policy_lights_top_and_bottom_faces_differently() { +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]| { -- knotmirror 0.0.1