Skip to content

Commit 754837b

Browse files
authored
fix(geometry): one MAX_PLACEMENT_DEPTH, and it is 100 not 32 (#2873) (#2969)
Two constants bounded the SAME `IfcLocalPlacement.PlacementRelTo` walk with different values. `router::transforms` (mesh path) capped at 32, `profile_extractor` (2D drawing path) at 100, and both exceed-branches return `Matrix4::identity()`. So an element on a 33-to-101-link chain was composed in full by one path and flattened by the other: the same element in two places, no error from either side. ## The decision: 100 survives, 32 does not The 32 carried the only stated rationale — "keep low for WASM, each frame uses ~2KB+ of stack with Matrix4<f64> locals". Checked rather than repeated: - The stack budget is 8 MiB. `-C link-arg=-zstack-size=8388608` in `.cargo/config.toml` and in both extra bundles in `scripts/build-wasm.sh`; geometry web workers each instantiate their own module, so none gets a smaller one. Native hosts give the geometry pool 256 MiB (`rust/ffi`, `rust/python`). - Frames measured, not assumed: a stack-address probe at each recursion level over a synthetic chain gives 944 B/frame for the router walk and 1008 B for the extractor's, unoptimised aarch64. 101 frames = 100,800 B — 1.2% of the wasm stack. Measured native; NOT measured on wasm32 (the shipped bundle has its name section stripped). - Fan-out is 1. `PlacementRelTo` is a single reference, so this walk costs O(depth), not the O(k^depth) that makes a depth cap the wrong instrument elsewhere (AGENTS.md). The two candidates are not symmetric, which is what decides it. Equalising UP lets the mesh path compose chains it currently flattens. Equalising DOWN would make the 2D path START flattening chains it composes correctly today — a new silent truncation, bought for 68 frames (~68 KB) of stack that is not scarce. Corpus, so the risk of either direction is quantified rather than asserted: over the 111 files in `tests/models` that contain placements, the deepest chain is 7 links (deepest: 7, `ara3d/ISSUE_021_Mini Project.ifc`). Zero files exceed 16. Neither cap binds on any fixture, so no fixture's geometry moves either way. ## What is NOT fixed here Both sites still fall back to a silent identity. On a chain past the cap the geometry is placed wrongly with nothing reported, which is the failure mode the #2866 family spent a week converting into catchable errors elsewhere. Making it loud is a larger behaviour change than #2873 asked for and is deliberately left out; this change makes the two paths wrong in the SAME way rather than in two different ways. ## Tests Following #2955: one `pub const` in `ifc_lite_core::limits`, and a test at EACH site asserting its constant IS the shared one — sharing alone does not stop a private copy shadowing the import. Mutation-verified, both directions: private `= 32` back in the router -> the_router_cap_is_the_shared_cap FAILS private `= 50` back in the extractor -> the_profile_extractor_cap_... FAILS plus a test that pins the divergence itself: a chain longer than the cap must give the same transform from both walks. RED before the fix, verbatim: assertion `left == right` failed: the mesh path and the 2D drawing path must place a 109-link chain identically; they differ by 68 links of translation, and neither reports an error left: 33.0 right: 101.0 The fixture translates +1 X per link so the composed X literally counts the placements each walk composed, and each walk gets its own decoder — the router memoises composed placements, and a shared decoder would hand walk one's answer to walk two. `ifc-lite-geometry` 932 passed / 0 failed / 27 ignored, `ifc-lite-core` 159 passed / 0 failed, module_size_ratchet 5 passed (profile_extractor.rs 757 lines, budget 758, unchanged), `cargo clippy --workspace --all-targets -D warnings` clean.
1 parent 00f6e79 commit 754837b

6 files changed

Lines changed: 200 additions & 8 deletions

File tree

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'@ifc-lite/geometry': patch
3+
---
4+
5+
Raise the mesh path's `IfcLocalPlacement.PlacementRelTo` depth cap from 32 to 100, matching the 2D drawing path, and make the two share one constant (`ifc_lite_core::limits::MAX_PLACEMENT_DEPTH`). The two walks follow the same attribute of the same entity and both return the IDENTITY on exceeding their cap, so an element on a 33-to-101-link placement chain was composed in full by the 2D profile extractor and flattened by the router — the same element drawn in two different places, with no error from either side. The cap's stated basis for 32 ("keep low for WASM — each frame uses ~2KB+ of stack") does not hold against the linked stack budget: every wasm bundle is built with `-zstack-size=8388608`, and the walk's frames measure ~1KB in an unoptimised native build, so the deeper cap's worst case is around 1% of the 8MiB stack; `PlacementRelTo` is a single reference, so the walk has fan-out 1 and costs O(depth). Chains beyond the cap are still truncated silently, exactly as before — only the depth at which that happens changes, and it now happens at the same depth on both paths. No file in the `tests/models` corpus has a chain deeper than 7 links, so no fixture's geometry moves. New tests pin each site's cap to the shared constant and require both walks to return the same transform for a chain past it.

rust/core/src/lib.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -89,7 +89,7 @@ pub use fast_parse::{
8989
};
9090
pub use generated::{IfcType, IFC_TYPES};
9191
pub use georef::{GeoRefExtractor, GeoRefSource, GeoReference, RtcOffset};
92-
pub use limits::MAX_MAPPED_ITEM_DEPTH;
92+
pub use limits::{MAX_MAPPED_ITEM_DEPTH, MAX_PLACEMENT_DEPTH};
9393
pub use legacy_entities::{
9494
get_legacy_entity_info, is_legacy_entity, map_legacy_to_base_type, LegacyEntityInfo,
9595
};

rust/core/src/limits.rs

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,41 @@
3636
/// sufficient (see the module docs above for which kind).
3737
pub const MAX_MAPPED_ITEM_DEPTH: u32 = 32;
3838

39+
/// Maximum `IfcLocalPlacement.PlacementRelTo` chain any walk in this workspace
40+
/// will follow. A chain longer than this composes only its first
41+
/// `MAX_PLACEMENT_DEPTH + 1` placements and the rest is dropped — silently, on
42+
/// every site that uses it.
43+
///
44+
/// Shared because two walks follow the SAME attribute of the SAME entity and
45+
/// their bounds must agree: `ifc_lite_geometry::router::transforms` (the mesh
46+
/// path) and `ifc_lite_geometry::profile_extractor` (the 2D drawing path).
47+
/// They disagreed — 32 against 100 — and because both exceed-branches return
48+
/// the IDENTITY rather than an error, an element on a 33-to-101-link chain was
49+
/// drawn in two different places by the two paths with nothing reported. #2873
50+
///
51+
/// ## Why 100 and not 32
52+
///
53+
/// The 32 carried the rationale "keep low for WASM — each frame uses ~2KB+ of
54+
/// stack". The wasm bundles are linked with `-zstack-size=8388608`
55+
/// (`.cargo/config.toml` and both extra bundles in `scripts/build-wasm.sh`), so
56+
/// the budget is 8 MiB and the cap's own worst case is ~0.1% of it; native
57+
/// hosts give the geometry pool 256 MiB (`rust/ffi`, `rust/python`). Measured
58+
/// rather than assumed: see the frame figures in #2873. `PlacementRelTo` is a
59+
/// single reference, so this walk has fan-out 1 and costs O(depth) — the
60+
/// breadth blow-up that makes a depth cap the wrong instrument elsewhere (see
61+
/// AGENTS.md) does not apply here.
62+
///
63+
/// The two candidates are not symmetric. Equalising UP lets the mesh path
64+
/// compose chains it currently flattens; equalising DOWN would make the 2D path
65+
/// start flattening chains it composes correctly today, silently. Across the
66+
/// 111 fixtures in `tests/models` that contain placements the deepest chain is
67+
/// 7 links, so neither value binds on any file in the corpus and the choice is
68+
/// decided entirely by which failure is worse on a file that does exceed it.
69+
///
70+
/// This bounds one path's LENGTH only. A walk over a chain that can revisit
71+
/// also needs a cycle guard; see the module docs above.
72+
pub const MAX_PLACEMENT_DEPTH: usize = 100;
73+
3974
#[cfg(test)]
4075
mod tests {
4176
use super::*;
@@ -47,4 +82,12 @@ mod tests {
4782
fn mapped_item_depth_is_the_documented_32() {
4883
assert_eq!(MAX_MAPPED_ITEM_DEPTH, 32);
4984
}
85+
86+
/// The documented contract, pinned separately from the two sites that
87+
/// import it — those assert they ARE this constant, which cannot also pin
88+
/// its value.
89+
#[test]
90+
fn placement_depth_is_the_documented_100() {
91+
assert_eq!(MAX_PLACEMENT_DEPTH, 100);
92+
}
5093
}

rust/geometry/src/profile_extractor.rs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
2020
use crate::profiles::ProfileProcessor;
2121
use crate::{Error, Point3, Result, TessellationQuality, Vector3};
22+
pub(crate) use ifc_lite_core::MAX_PLACEMENT_DEPTH;
2223
use ifc_lite_core::{
2324
build_entity_index, AttributeValue, DecodedEntity, EntityDecoder, EntityScanner, IfcSchema,
2425
IfcType,
@@ -519,9 +520,8 @@ fn get_placement_transform(
519520
}
520521
}
521522

522-
const MAX_PLACEMENT_DEPTH: usize = 100;
523-
524-
fn get_placement_recursive(
523+
/// `pub(crate)` so the #2873 divergence test can drive this walk and the mesh path's.
524+
pub(crate) fn get_placement_recursive(
525525
placement: &DecodedEntity,
526526
decoder: &mut EntityDecoder,
527527
depth: usize,

rust/geometry/src/router/transforms/mod.rs

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,10 @@ pub(crate) fn cartesian_point_at(
4343

4444
mod parsers;
4545

46+
#[cfg(test)]
47+
#[path = "placement_depth_tests.rs"]
48+
mod placement_depth_tests;
49+
4650
use super::GeometryRouter;
4751
use crate::{Mesh, Result, SubMeshCollection};
4852
use ifc_lite_core::{DecodedEntity, EntityDecoder, IfcType};
@@ -256,8 +260,8 @@ impl GeometryRouter {
256260

257261
/// Recursively resolve placement hierarchy
258262
///
259-
/// Uses a depth limit (100) to prevent stack overflow on malformed files
260-
/// with circular placement references or extremely deep hierarchies.
263+
/// Bounded by [`Self::MAX_PLACEMENT_DEPTH`] so a malformed file with a
264+
/// circular or absurdly deep placement hierarchy cannot overflow the stack.
261265
pub(super) fn get_placement_transform(
262266
&self,
263267
placement: &DecodedEntity,
@@ -267,8 +271,12 @@ impl GeometryRouter {
267271
}
268272

269273
/// Internal helper with depth tracking to prevent stack overflow.
270-
/// Keep low for WASM — each frame uses ~2KB+ of stack with Matrix4<f64> locals.
271-
const MAX_PLACEMENT_DEPTH: usize = 32;
274+
///
275+
/// The bound is [`ifc_lite_core::limits::MAX_PLACEMENT_DEPTH`], shared with
276+
/// `profile_extractor`'s walk over the same attribute. It was a private 32
277+
/// against that walk's 100, and since both exceed-branches return the
278+
/// identity the two paths drew a deep chain in two places, silently. #2873
279+
pub(super) const MAX_PLACEMENT_DEPTH: usize = ifc_lite_core::MAX_PLACEMENT_DEPTH;
272280

273281
pub(super) fn get_placement_transform_with_depth(
274282
&self,
Lines changed: 136 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,136 @@
1+
// This Source Code Form is subject to the terms of the Mozilla Public
2+
// License, v. 2.0. If a copy of the MPL was not distributed with this
3+
// file, You can obtain one at https://mozilla.org/MPL/2.0/.
4+
5+
//! The `IfcLocalPlacement.PlacementRelTo` walk is implemented twice — once in
6+
//! `router::transforms` (mesh path) and once in `profile_extractor` (2D drawing
7+
//! path) — and both bound it with a depth cap whose exceed-branch returns the
8+
//! IDENTITY. Two caps over one chain therefore do not disagree loudly: they
9+
//! disagree by putting the same element in two different places, with no error
10+
//! on either side. #2873
11+
//!
12+
//! These tests pin (a) each site's cap IS `ifc_lite_core::limits::MAX_PLACEMENT_DEPTH`
13+
//! and (b) the two walks agree on a chain deeper than that cap. (a) alone is not
14+
//! enough: a private `const MAX_PLACEMENT_DEPTH` shadows the import and nothing
15+
//! else notices, which is exactly how #2955's mapped-item family failed.
16+
17+
use super::*;
18+
use ifc_lite_core::limits::MAX_PLACEMENT_DEPTH;
19+
use ifc_lite_core::EntityDecoder;
20+
21+
/// `links` chained `IfcLocalPlacement`s, each translating +1.0 in X, so the
22+
/// composed world X of the deepest one equals the number of placements the walk
23+
/// actually composed. That makes the truncation directly readable off the
24+
/// result instead of inferred.
25+
///
26+
/// Ids: `#1` axis placement, `#2` its origin, `#10..=#(10 + links)` the chain
27+
/// (`#10` is the root, `#(10 + links)` the leaf).
28+
fn deep_placement_chain(links: usize) -> String {
29+
let mut s = String::from(
30+
"ISO-10303-21;\nHEADER;\nFILE_DESCRIPTION((''),'2;1');\n\
31+
FILE_NAME('t.ifc','2024-01-01T00:00:00',(''),(''),'','','');\n\
32+
FILE_SCHEMA(('IFC4'));\nENDSEC;\nDATA;\n\
33+
#1=IFCAXIS2PLACEMENT3D(#2,$,$);\n\
34+
#2=IFCCARTESIANPOINT((1.,0.,0.));\n\
35+
#10=IFCLOCALPLACEMENT($,#1);\n",
36+
);
37+
for i in 1..=links {
38+
s.push_str(&format!("#{}=IFCLOCALPLACEMENT(#{},#1);\n", 10 + i, 9 + i));
39+
}
40+
s.push_str("ENDSEC;\nEND-ISO-10303-21;\n");
41+
s
42+
}
43+
44+
/// World X of the leaf of a `links`-long chain, from each of the two walks.
45+
/// Each walk gets its OWN decoder: the router memoises composed placements on
46+
/// the decoder, and sharing one would let the first walk's answer be handed to
47+
/// the second, which is the one thing this test must not do.
48+
fn both_walks(links: usize) -> (f64, f64) {
49+
let content = deep_placement_chain(links);
50+
let leaf_id = (10 + links) as u32;
51+
52+
let mut router_decoder = EntityDecoder::new(&content);
53+
let leaf = router_decoder.decode_by_id(leaf_id).expect("leaf placement");
54+
let router = GeometryRouter::new();
55+
let router_x = router
56+
.get_placement_transform(&leaf, &mut router_decoder)
57+
.expect("router placement transform")
58+
.column(3)[0];
59+
60+
let mut extractor_decoder = EntityDecoder::new(&content);
61+
let leaf = extractor_decoder
62+
.decode_by_id(leaf_id)
63+
.expect("leaf placement");
64+
let extractor_x =
65+
crate::profile_extractor::get_placement_recursive(&leaf, &mut extractor_decoder, 0)
66+
.column(3)[0];
67+
68+
(router_x, extractor_x)
69+
}
70+
71+
/// A chain the walk composes in full: both must report every link, which also
72+
/// proves the fixture measures what it claims before the truncation cases use it.
73+
#[test]
74+
fn both_walks_compose_a_chain_inside_the_cap_identically() {
75+
let links = MAX_PLACEMENT_DEPTH / 2;
76+
let (router_x, extractor_x) = both_walks(links);
77+
let expected = (links + 1) as f64;
78+
assert_eq!(
79+
router_x, expected,
80+
"the router must compose all {} placements of an in-cap chain",
81+
links + 1
82+
);
83+
assert_eq!(
84+
extractor_x, expected,
85+
"the extractor must compose all {} placements of an in-cap chain",
86+
links + 1
87+
);
88+
}
89+
90+
/// THE divergence. A chain longer than the shared cap is truncated by both
91+
/// walks, and truncation is silent on both — so the only thing that can make
92+
/// the mesh path and the 2D path put an element in the same place is the two
93+
/// caps being equal. With the router at 32 and the extractor at 100 this
94+
/// returned 33.0 and 41.0 for a 40-link chain: same file, same element, two
95+
/// positions, no error.
96+
#[test]
97+
fn both_walks_truncate_a_chain_beyond_the_cap_at_the_same_link() {
98+
let links = MAX_PLACEMENT_DEPTH + 8;
99+
let (router_x, extractor_x) = both_walks(links);
100+
assert_eq!(
101+
router_x, extractor_x,
102+
"the mesh path and the 2D drawing path must place a {}-link chain \
103+
identically; they differ by {} links of translation, and neither \
104+
reports an error",
105+
links + 1,
106+
(router_x - extractor_x).abs()
107+
);
108+
assert_eq!(
109+
router_x,
110+
(MAX_PLACEMENT_DEPTH + 1) as f64,
111+
"the cap admits depths 0..=MAX_PLACEMENT_DEPTH, i.e. MAX_PLACEMENT_DEPTH + 1 placements"
112+
);
113+
}
114+
115+
/// Site 1 of 2. Sharing the constant removes today's drift; this stops a
116+
/// private copy from reintroducing it. #2955 proved the need by mutation: a
117+
/// local `const` shadowed the import and 800 tests stayed green.
118+
#[test]
119+
fn the_router_cap_is_the_shared_cap() {
120+
assert_eq!(
121+
GeometryRouter::MAX_PLACEMENT_DEPTH,
122+
MAX_PLACEMENT_DEPTH,
123+
"router::transforms must bound PlacementRelTo with ifc_lite_core::limits::MAX_PLACEMENT_DEPTH"
124+
);
125+
}
126+
127+
/// Site 2 of 2. See above.
128+
#[test]
129+
fn the_profile_extractor_cap_is_the_shared_cap() {
130+
assert_eq!(
131+
crate::profile_extractor::MAX_PLACEMENT_DEPTH,
132+
MAX_PLACEMENT_DEPTH,
133+
"profile_extractor must bound PlacementRelTo with ifc_lite_core::limits::MAX_PLACEMENT_DEPTH"
134+
);
135+
}
136+

0 commit comments

Comments
 (0)