From 4d25fb1e5511bd33e4b2afaf8e4e55ad5c944091 Mon Sep 17 00:00:00 2001 From: Orual Date: Mon, 10 Aug 2026 17:48:50 -0400 Subject: [PATCH] PM-84: fix limit charging, derive Hash, fix test quality findings Epic: PM-86 Task: PM-84 --- crates/polymodel-ldraw-core/src/bfc.rs | 2 +- crates/polymodel-ldraw-core/src/lib.rs | 33 ++++++++++ crates/polymodel-ldraw-core/src/traversal.rs | 28 ++++++++- .../polymodel-renderer-protocol/src/scene.rs | 4 +- crates/polymodel-renderer-worker/src/lib.rs | 31 ++-------- crates/polymodel-renderer-worker/src/tests.rs | 62 ++++++++++++------- .../tests/ldraw_preview.rs | 5 +- 7 files changed, 107 insertions(+), 58 deletions(-) diff --git a/crates/polymodel-ldraw-core/src/bfc.rs b/crates/polymodel-ldraw-core/src/bfc.rs index 7557b88..ea87031 100644 --- a/crates/polymodel-ldraw-core/src/bfc.rs +++ b/crates/polymodel-ldraw-core/src/bfc.rs @@ -6,7 +6,7 @@ use crate::types::{ }; use serde::{Deserialize, Serialize}; -#[derive(Clone, Copy, Debug, Eq, PartialEq, Serialize, Deserialize)] +#[derive(Clone, Copy, Debug, Eq, PartialEq, Hash, Serialize, Deserialize)] #[serde(rename_all = "snake_case")] pub enum BfcState { Unknown, diff --git a/crates/polymodel-ldraw-core/src/lib.rs b/crates/polymodel-ldraw-core/src/lib.rs index 9663085..b0a359c 100644 --- a/crates/polymodel-ldraw-core/src/lib.rs +++ b/crates/polymodel-ldraw-core/src/lib.rs @@ -383,6 +383,39 @@ mod tests { )); } + #[test] + fn quad_geometry_counts_as_two_triangles_for_limits() { + let mut parse_options = options(); + parse_options.limits.triangles = 1; + let error = LdrawParser + .parse_bytes(b"4 16 0 0 0 1 0 0 0 1 0 0 0 1\n", parse_options, &[]) + .expect_err("a quad must consume two triangle-limit units"); + assert!(matches!( + error, + LdrawError::Diagnostic(Diagnostic { + code: DiagnosticCode::TrianglesLimit, + .. + }) + )); + } + + #[test] + fn repeated_instances_charge_aggregate_geometry_against_limits() { + let mut parse_options = options(); + parse_options.limits.triangles = 2; + let bytes = b"0 FILE root.ldr\n1 16 0 0 0 1 0 0 0 1 0 0 0 0 child.dat\n1 16 2 0 0 1 0 0 0 1 0 0 0 0 child.dat\n0 NOFILE\n0 FILE child.dat\n3 16 0 0 0 1 0 0 0 1 0\n0 NOFILE\n"; + let error = LdrawParser + .parse_bytes(bytes, parse_options, &[]) + .expect_err("repeated instances must charge each occurrence"); + assert!(matches!( + error, + LdrawError::Diagnostic(Diagnostic { + code: DiagnosticCode::TrianglesLimit, + .. + }) + )); + } + #[test] fn colour_scope_commit_rollback_and_edge_lookup_are_transactional() { let mut scope = ColourScope::new(); diff --git a/crates/polymodel-ldraw-core/src/traversal.rs b/crates/polymodel-ldraw-core/src/traversal.rs index 66f5747..b31ef9a 100644 --- a/crates/polymodel-ldraw-core/src/traversal.rs +++ b/crates/polymodel-ldraw-core/src/traversal.rs @@ -306,11 +306,20 @@ pub(crate) fn traverse( if !summarized.insert(definition_id) { continue; } + let definition_triangles = model + .quads + .checked_mul(2) + .and_then(|quads| model.triangles.checked_add(quads)) + .ok_or(ParseError::Overflow("definition triangles"))?; counters - .add(LimitKind::Triangles, model.triangles, &options.limits) + .add(LimitKind::Triangles, definition_triangles, &options.limits) .map_err(|name| limit_error(name, None))?; + let definition_lines = model + .lines + .checked_add(model.conditional_lines) + .ok_or(ParseError::Overflow("definition lines"))?; counters - .add(LimitKind::Lines, model.lines, &options.limits) + .add(LimitKind::Lines, definition_lines, &options.limits) .map_err(|name| limit_error(name, None))?; metrics.definition_visits += 1; let model_id = if let Some(model_id) = ids.get(&definition_id) { @@ -420,6 +429,21 @@ pub(crate) fn traverse( if let Some(started) = bounds_started { metrics.traversal_bounds_ns += started.elapsed().as_nanos(); } + let occurrence_triangles = model + .quads + .checked_mul(2) + .and_then(|quads| model.triangles.checked_add(quads)) + .ok_or(ParseError::Overflow("occurrence triangles"))?; + counters + .add(LimitKind::Triangles, occurrence_triangles, &options.limits) + .map_err(|name| limit_error(name, None))?; + let occurrence_lines = model + .lines + .checked_add(model.conditional_lines) + .ok_or(ParseError::Overflow("occurrence lines"))?; + counters + .add(LimitKind::Lines, occurrence_lines, &options.limits) + .map_err(|name| limit_error(name, None))?; scene_triangles = scene_triangles .checked_add(model.triangles) .ok_or(ParseError::Overflow("scene triangles"))?; diff --git a/crates/polymodel-renderer-protocol/src/scene.rs b/crates/polymodel-renderer-protocol/src/scene.rs index 33439a7..b768475 100644 --- a/crates/polymodel-renderer-protocol/src/scene.rs +++ b/crates/polymodel-renderer-protocol/src/scene.rs @@ -65,7 +65,7 @@ pub struct SceneStats { pub occurrences: u64, } -#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Serialize, Deserialize)] #[serde(rename_all = "snake_case")] pub enum SceneTexmapProjectionMode { Planar, @@ -73,7 +73,7 @@ pub enum SceneTexmapProjectionMode { Spherical, } -#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize)] pub struct SceneTexmapAssociation { pub mode: SceneTexmapProjectionMode, pub parameters: Vec, diff --git a/crates/polymodel-renderer-worker/src/lib.rs b/crates/polymodel-renderer-worker/src/lib.rs index 2b6eb9e..49ba856 100644 --- a/crates/polymodel-renderer-worker/src/lib.rs +++ b/crates/polymodel-renderer-worker/src/lib.rs @@ -314,7 +314,7 @@ struct DefinitionMetadata { source_class: SmolStr, } -#[derive(Clone, PartialEq, Eq)] +#[derive(Clone, Hash, PartialEq, Eq)] struct PrimitiveBatchKey { colour: u32, bfc_state: polymodel_ldraw_core::BfcState, @@ -456,26 +456,7 @@ impl<'a> AdapterSink<'a> { texmap, }; let mut hasher = std::collections::hash_map::DefaultHasher::new(); - key.colour.hash(&mut hasher); - match key.bfc_state { - polymodel_ldraw_core::BfcState::Unknown => 0_u8.hash(&mut hasher), - polymodel_ldraw_core::BfcState::Certified => 1_u8.hash(&mut hasher), - polymodel_ldraw_core::BfcState::Uncertified => 2_u8.hash(&mut hasher), - } - key.bfc_cull.hash(&mut hasher); - key.bfc_ccw.hash(&mut hasher); - key.reverse_winding.hash(&mut hasher); - if let Some(texmap) = &key.texmap { - match texmap.mode { - SceneTexmapProjectionMode::Planar => 0_u8.hash(&mut hasher), - SceneTexmapProjectionMode::Cylindrical => 1_u8.hash(&mut hasher), - SceneTexmapProjectionMode::Spherical => 2_u8.hash(&mut hasher), - } - texmap.parameters.hash(&mut hasher); - texmap.pngfile.hash(&mut hasher); - texmap.glossmap.hash(&mut hasher); - texmap.fallback.hash(&mut hasher); - } + key.hash(&mut hasher); let key_hash = hasher.finish(); let batch_key = (provenance, key_hash); let batch_index = self.batch_indices.get(&batch_key).and_then(|indices| { @@ -2816,6 +2797,7 @@ pub fn scene_mesh_topology_for_view( #[derive(Clone, Copy)] enum SceneTopologyKind { Faces, + Edges, Complete, } @@ -2825,8 +2807,9 @@ fn scene_mesh_topology_for_view_kind( kind: SceneTopologyKind, ) -> Result { let include_faces = matches!(kind, SceneTopologyKind::Faces | SceneTopologyKind::Complete); - let include_explicit = matches!(kind, SceneTopologyKind::Complete); - let include_conditional = matches!(kind, SceneTopologyKind::Complete); + let include_explicit = matches!(kind, SceneTopologyKind::Edges | SceneTopologyKind::Complete); + let include_conditional = + matches!(kind, SceneTopologyKind::Edges | SceneTopologyKind::Complete); let source_positions = geometry .points .iter() @@ -3995,7 +3978,6 @@ fn scene_instance_batches_for_mode( bfc_cull: bool, reverse_winding: bool, reflection: bool, - _conditional_occurrence: Option, textured_occurrence: Option, } @@ -4064,7 +4046,6 @@ fn scene_instance_batches_for_mode( bfc_cull: occurrence.bfc_cull, reverse_winding, reflection, - _conditional_occurrence: None, textured_occurrence: occurrence.uvs.as_ref().map(|_| occurrence_index), }; let instance = SceneBatchInstance { diff --git a/crates/polymodel-renderer-worker/src/tests.rs b/crates/polymodel-renderer-worker/src/tests.rs index c46a20b..25c3ada 100644 --- a/crates/polymodel-renderer-worker/src/tests.rs +++ b/crates/polymodel-renderer-worker/src/tests.rs @@ -1448,16 +1448,23 @@ fn edge_topology_excludes_source_faces() { ); geometry.lines = vec![[0, 1]].into(); - let faces = - scene_mesh_topology_for_view_kind(&geometry, [0.0, 0.0, 1.0], SceneTopologyKind::Faces) + let edges = + scene_mesh_topology_for_view_kind(&geometry, [0.0, 0.0, 1.0], SceneTopologyKind::Edges) .unwrap(); assert_eq!( - faces.triangle_count(), - 1, - "source face remains in face topology" + edges.triangle_count(), + 2, + "authored line becomes an edge ribbon" + ); + assert_eq!(edges.positions.len(), 4); + assert!( + edges + .positions + .iter() + .all(|position| position[2].abs() < f32::EPSILON), + "edge topology contains authored line positions, not source face vertices" ); - assert_eq!(faces.positions.len(), 3); } #[cfg(not(target_arch = "wasm32"))] @@ -1827,27 +1834,19 @@ fn render_plan_owns_authoritative_edge_sources_without_changing_face_batches() { .iter() .all(|source| source.conditional.len() == 1) ); - let faces_plan = scene_render_plan(&scene).expect("faces plan"); - let edges_plan = scene_render_plan(&scene).expect("edges plan"); - assert_eq!(faces_plan.batches, edges_plan.batches); + assert_eq!(plan.batches.len(), 1); assert_eq!( - faces_plan.upload_batch_indices, - edges_plan.upload_batch_indices - ); - assert_eq!(faces_plan.planned_to_upload, edges_plan.planned_to_upload); - assert_eq!(faces_plan.occurrence_ranges, edges_plan.occurrence_ranges); - let ray = PickRay { - origin: [0.0, 0.0, 10.0], - direction: [0.0, 0.0, -1.0], - }; + plan.upload_batch_indices, + [0].into_iter().collect::>() + ); assert_eq!( - ScenePicker::new(&scene, faces_plan) - .expect("faces picker") - .pick(ray), - ScenePicker::new(&scene, edges_plan) - .expect("edges picker") - .pick(ray) + plan.planned_to_upload, + [Some(0)].into_iter().collect::]>>() ); + assert_eq!(plan.occurrence_ranges.len(), scene.occurrences.len()); + assert!(plan.occurrence_ranges.iter().all(Option::is_some)); + assert_eq!(plan.topologies.len(), scene.geometries.len()); + assert_eq!(plan.topologies[0].triangle_count(), 1); } #[test] @@ -2077,6 +2076,21 @@ fn seeded_ldconfig_red_is_not_the_grey_material_fallback() { .expect("seeded LDConfig colour should parse"); sink.take_error().expect("seeded geometry should adapt"); let scene = adapt_ldraw_parse_result(&parsed, None, sink).expect("seeded colour should adapt"); + assert_eq!(parsed.result.scene.triangles, 1); + assert_eq!(parsed.result.scene.quads, 0); + assert_eq!(parsed.result.scene.lines, 0); + assert_eq!(parsed.result.scene.conditional_lines, 0); + assert_eq!(scene.stats.triangles, 1); + assert_eq!(scene.stats.lines, 0); + assert_eq!(scene.stats.points, 3); + assert_eq!(scene.stats.occurrences, 1); + assert_eq!(scene.provenance.len(), 1); + assert_eq!(scene.provenance[0].id, ProvenanceId(1)); + assert_eq!(scene.provenance[0].source, "seeded-ldconfig-red"); + assert_eq!(scene.occurrences.len(), 1); + assert_eq!(scene.geometries[0].provenance, ProvenanceId(1)); + assert_eq!(scene.occurrences[0].source, "seeded-ldconfig-red".into()); + assert_eq!(scene.occurrences[0].target, "seeded-ldconfig-red".into()); let material = scene.materials.first().expect("seeded code-4 material"); assert_eq!(material.colour, 4); assert_eq!(material.colour_rgb, Some([201, 26, 9])); diff --git a/crates/polymodel-renderer-worker/tests/ldraw_preview.rs b/crates/polymodel-renderer-worker/tests/ldraw_preview.rs index 851719b..24a4446 100644 --- a/crates/polymodel-renderer-worker/tests/ldraw_preview.rs +++ b/crates/polymodel-renderer-worker/tests/ldraw_preview.rs @@ -1127,10 +1127,7 @@ fn ldraw_light_policy_preserves_three_d_direction_semantics() { } #[test] -fn ordinary_format_preview_parity() { - let fixture = Path::new(env!("CARGO_MANIFEST_DIR")) - .join("../polymodel-ldraw-testkit/corpus/omr-10030-1.mpd"); - assert!(fixture.exists()); +fn pinned_scene_smoke_test() { let scene = pinned_scene(); assert!(scene.stats.occurrences > 0); assert!(scene.stats.triangles > scene.geometries[0].triangles.len() as u64); -- knotmirror 0.0.1