Skip to content

Commit 731dc06

Browse files
authored
fix(viewer): apply the else-by-type quantity fallback to area and weight, not just volume (#2777)
* fix(viewer): apply the else-by-type quantity fallback to area and weight, not just volume MaterialTotalsPanel's pickQuantity docstring promised "pick a quantity value by candidate names (case-insensitive), else by type," but only the volume total call site implemented that fallback. An element whose only area (or weight) quantity carried a vendor-specific name outside the IFC-standard candidate list contributed zero to that material's total and its row was hidden, while the identical situation for volume was counted. Present since the file's first commit (#978). Extracts the per-element map-building + pick logic (previously three call sites built from the same *ByName maps) into aggregateQuantitiesFromQsets, and makes pickQuantity's else-by-type fallback pick the alphabetically-first named quantity of that type — deterministic, rather than depending on qset scan order as the previous volume-only fallback did. * fix(viewer): exclude CrossSectionArea from the material area fallback louistrue's PR #2777 review proved on the app's own shipped infra-bridge.ifc sample that the alphabetical by-type fallback picks a beam's CrossSectionArea — a section property, not a surface extent — as its Area, since no candidate name matched and crosssectionarea sorts first among the beam's (non-candidate) area names. AREA_CANDIDATES now recognises the standard surface-area names (netsurfacearea/grosssurfacearea/outersurfacearea) by name, so standard beams/columns/members resolve without reaching the fallback at all. The fallback itself gains a deny set, currently just crosssectionarea, so a section property can never be selected even as a last resort — it degrades to "no value" instead of a wrong one. Also replaces the aggregation tests review 1 showed could be deleted (the whole candidate-matching loop, or the toLowerCase() call) with 10/10 green: every new fixture holds two quantities chosen so the candidate/case-insensitive path and the (deny-filtered) alphabetical fallback disagree, so removing either mutation now fails the specific tests exercising it.
1 parent 969cff9 commit 731dc06

3 files changed

Lines changed: 343 additions & 17 deletions

File tree

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
---
2+
'@ifc-lite/viewer': patch
3+
---
4+
5+
Fix the material totals panel dropping area/weight for vendor-named quantities.
6+
7+
`MaterialTotalsPanel`'s `pickQuantity` docstring promised "pick a quantity
8+
value by candidate names (case-insensitive), else by type," but only the
9+
volume total implemented the else-by-type fallback. An element whose only
10+
area (or weight) quantity used a name outside the IFC-standard candidate
11+
list — a vendor-specific `PerimeterArea` or `TopArea`, say — contributed
12+
zero to that material's area/weight total and its row stayed hidden, while
13+
the identical situation for volume was counted correctly.
14+
15+
`pickQuantity` now applies the else-by-type fallback uniformly to volume,
16+
area and weight, picking the alphabetically-first named quantity of that
17+
type when nothing matches a candidate name — a deterministic tiebreak,
18+
rather than depending on the qset scan order the previous volume-only
19+
fallback relied on. The per-element map-building + pick logic that all
20+
three totals shared is now a single extracted function instead of three
21+
call sites that could (and did) drift apart.
22+
23+
Follow-up fix: the alphabetical fallback could select `CrossSectionArea`
24+
a beam/column/member's section (profile) property, not a surface extent —
25+
as the element's Area, because no candidate name matched it and it sorts
26+
before every real surface-area name those elements carry
27+
(`GrossSurfaceArea`, `NetSurfaceArea`, `OuterSurfaceArea`). Proven on the
28+
app's own shipped `infra-bridge.ifc` sample, this reported a bridge beam's
29+
0.12 m² cross-section as its material area instead of leaving the total
30+
unset. `AREA_CANDIDATES` now recognises the standard surface-area names by
31+
name (so standard beams/columns resolve without reaching the fallback at
32+
all), and the fallback itself excludes `crosssectionarea` so it can never
33+
be picked even as a last resort — degrading to "no value" rather than a
34+
wrong one when it's the only area quantity present.
Lines changed: 222 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,222 @@
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+
/**
6+
* `pickQuantity`'s docstring on `MaterialTotalsPanel` promises: "Pick a
7+
* quantity value by candidate names (case-insensitive), else by type." The
8+
* volume call site implemented that fallback; the area and weight call
9+
* sites — built from the same `*ByName` maps in the same `qset.quantities`
10+
* loop — did not. An element whose only area (or weight) quantity carries a
11+
* vendor-specific name outside the IFC-standard candidate list (e.g.
12+
* "PerimeterArea", "TopArea") contributed zero to that material's total and
13+
* its row was hidden, while the identical situation for volume was counted.
14+
* This has been present since the file's first commit (#978).
15+
*
16+
* These tests drive `aggregateQuantitiesFromQsets` — the function extracted
17+
* from the three call sites so the "match a candidate name, else by type"
18+
* rule is applied identically to volume, area and weight instead of three
19+
* copies that can silently drift apart.
20+
*
21+
* PR #2777 review 1 (louistrue): proved on the app's own shipped sample
22+
* (`apps/viewer/public/samples/infra-bridge.ifc`) that the alphabetical
23+
* by-type fallback picks a beam's `CrossSectionArea` — a section property,
24+
* not a surface extent — as its Area, because no candidate name matched and
25+
* `crosssectionarea` sorts before the beam's other (non-candidate) area
26+
* names. Review 2: the fix is `AREA_CANDIDATES` recognising the standard
27+
* surface-area names by NAME (so beams/columns/members resolve without ever
28+
* reaching the fallback) plus `AREA_TYPE_FALLBACK_DENY` excluding
29+
* `crosssectionarea` from the fallback itself, so it can never be selected
30+
* even as a last resort.
31+
*
32+
* Review 1's MAJOR finding: every fixture in the original suite held exactly
33+
* one quantity of the kind under test, so the candidate-match path and the
34+
* type-fallback path returned the same value — deleting the candidate
35+
* matching entirely (`for (const c of candidates) ...` -> `void candidates;`)
36+
* left all 10 tests green. The "candidate wins over fallback" and
37+
* "case-insensitive match" tests below use TWO quantities per fixture, one
38+
* that only the candidate-priority / case-insensitive path picks correctly
39+
* and one that the (deny-filtered) alphabetical fallback would pick instead
40+
* if matching were skipped — see the mutation notes on each test.
41+
*/
42+
43+
import { describe, it } from 'node:test';
44+
import assert from 'node:assert/strict';
45+
import { QuantityType } from '@ifc-lite/data';
46+
import { aggregateQuantitiesFromQsets } from './MaterialTotalsPanel.js';
47+
48+
type Qty = { name: string; type: number; value: number };
49+
type Qset = { quantities: readonly Qty[] };
50+
51+
function qsets(quantities: Qty[]): Qset[] {
52+
return [{ quantities }];
53+
}
54+
55+
describe('aggregateQuantitiesFromQsets', () => {
56+
describe('volume', () => {
57+
it('picks a candidate-named quantity (existing behaviour)', () => {
58+
const r = aggregateQuantitiesFromQsets(
59+
qsets([{ name: 'NetVolume', type: QuantityType.Volume, value: 12 }]),
60+
);
61+
assert.equal(r.volume, 12);
62+
});
63+
64+
it('falls back to a non-candidate-named quantity of the same type (the fix)', () => {
65+
const r = aggregateQuantitiesFromQsets(
66+
qsets([{ name: 'CustomVendorVolume', type: QuantityType.Volume, value: 7 }]),
67+
);
68+
assert.equal(r.volume, 7);
69+
});
70+
71+
it('is undefined when no volume quantity exists', () => {
72+
const r = aggregateQuantitiesFromQsets(
73+
qsets([{ name: 'NetArea', type: QuantityType.Area, value: 5 }]),
74+
);
75+
assert.equal(r.volume, undefined);
76+
});
77+
});
78+
79+
describe('area', () => {
80+
it('picks a candidate-named quantity (existing behaviour)', () => {
81+
const r = aggregateQuantitiesFromQsets(
82+
qsets([{ name: 'NetArea', type: QuantityType.Area, value: 30 }]),
83+
);
84+
assert.equal(r.area, 30);
85+
});
86+
87+
it('falls back to a non-candidate-named quantity of the same type (the fix)', () => {
88+
// RED against pre-fix code: the area call site had no else-by-type
89+
// fallback, so this returned undefined and the total stayed 0.
90+
const r = aggregateQuantitiesFromQsets(
91+
qsets([{ name: 'PerimeterArea', type: QuantityType.Area, value: 18 }]),
92+
);
93+
assert.equal(r.area, 18);
94+
});
95+
96+
it('is undefined when no area quantity exists', () => {
97+
const r = aggregateQuantitiesFromQsets(
98+
qsets([{ name: 'NetVolume', type: QuantityType.Volume, value: 9 }]),
99+
);
100+
assert.equal(r.area, undefined);
101+
});
102+
});
103+
104+
describe('weight', () => {
105+
it('picks a candidate-named quantity (existing behaviour)', () => {
106+
const r = aggregateQuantitiesFromQsets(
107+
qsets([{ name: 'NetWeight', type: QuantityType.Weight, value: 500 }]),
108+
);
109+
assert.equal(r.weight, 500);
110+
});
111+
112+
it('falls back to a non-candidate-named quantity of the same type (the fix)', () => {
113+
// RED against pre-fix code: the weight call site had no else-by-type
114+
// fallback, so this returned undefined and the total stayed 0.
115+
const r = aggregateQuantitiesFromQsets(
116+
qsets([{ name: 'ShippingWeight', type: QuantityType.Weight, value: 250 }]),
117+
);
118+
assert.equal(r.weight, 250);
119+
});
120+
121+
it('is undefined when no weight quantity exists', () => {
122+
const r = aggregateQuantitiesFromQsets(
123+
qsets([{ name: 'NetArea', type: QuantityType.Area, value: 4 }]),
124+
);
125+
assert.equal(r.weight, undefined);
126+
});
127+
});
128+
129+
it('breaks ties among several non-candidate names deterministically (alphabetical)', () => {
130+
const r = aggregateQuantitiesFromQsets(
131+
qsets([
132+
{ name: 'ZTopArea', type: QuantityType.Area, value: 99 },
133+
{ name: 'APerimeterArea', type: QuantityType.Area, value: 11 },
134+
]),
135+
);
136+
assert.equal(r.area, 11);
137+
});
138+
139+
describe('CrossSectionArea must never be reported as a surface Area (PR #2777 review 1)', () => {
140+
it('is undefined when the only area quantity is CrossSectionArea (infra-bridge.ifc beam shape)', () => {
141+
// Real shape of apps/viewer/public/samples/infra-bridge.ifc's
142+
// Qto_BeamBaseQuantities (#385/#442/#493): NetVolume, Length,
143+
// CrossSectionArea — no surface-area quantity at all. Before this
144+
// fix, the alphabetical fallback had exactly one Area-typed name to
145+
// choose from and picked it, reporting 0.12 m2 (the beam's profile
146+
// area) as the material's surface area. Absence is the honest result.
147+
const r = aggregateQuantitiesFromQsets(
148+
qsets([
149+
{ name: 'NetVolume', type: QuantityType.Volume, value: 0.48 },
150+
{ name: 'Length', type: QuantityType.Length, value: 4000 },
151+
{ name: 'CrossSectionArea', type: QuantityType.Area, value: 0.12 },
152+
]),
153+
);
154+
assert.equal(r.area, undefined);
155+
});
156+
157+
it('picks a real surface-area quantity over CrossSectionArea by name (the requested beam fixture)', () => {
158+
// The exact fixture louistrue's second review asked for: "a beam
159+
// carrying CrossSectionArea AND OuterSurfaceArea, asserting the
160+
// surface area is chosen." outersurfacearea is now in
161+
// AREA_CANDIDATES, so this resolves by name and never reaches the
162+
// fallback at all.
163+
const r = aggregateQuantitiesFromQsets(
164+
qsets([
165+
{ name: 'CrossSectionArea', type: QuantityType.Area, value: 0.06 },
166+
{ name: 'OuterSurfaceArea', type: QuantityType.Area, value: 11.9 },
167+
]),
168+
);
169+
assert.equal(r.area, 11.9);
170+
});
171+
172+
it('excludes CrossSectionArea from the by-type fallback even when a non-candidate name is also present', () => {
173+
// Neither name matches AREA_CANDIDATES, so this exercises the
174+
// fallback path (not name matching). MUTATION: delete
175+
// AREA_TYPE_FALLBACK_DENY (or stop passing it to pickQuantity) and
176+
// this goes RED — alphabetical sort puts 'crosssectionarea' before
177+
// 'vendorsurfacearea', returning 0.06 instead of 11.9.
178+
const r = aggregateQuantitiesFromQsets(
179+
qsets([
180+
{ name: 'CrossSectionArea', type: QuantityType.Area, value: 0.06 },
181+
{ name: 'VendorSurfaceArea', type: QuantityType.Area, value: 11.9 },
182+
]),
183+
);
184+
assert.equal(r.area, 11.9);
185+
});
186+
});
187+
188+
describe('candidate priority is load-bearing, not incidental (PR #2777 review 1 MAJOR finding)', () => {
189+
it('prefers Net over Gross even though Gross sorts first alphabetically', () => {
190+
// MUTATION: delete the candidate-matching loop in pickQuantity
191+
// (`for (const c of candidates) ...` -> `void candidates;`) and this
192+
// goes RED — with matching skipped, both fixtures fall straight to
193+
// the alphabetical fallback, which prefers 'grossarea' (13) over
194+
// 'netarea' (7) purely by spelling. Every fixture in the original
195+
// suite held one quantity per kind, so this priority was unguarded:
196+
// the candidate path and the fallback path always agreed.
197+
const r = aggregateQuantitiesFromQsets(
198+
qsets([
199+
{ name: 'GrossArea', type: QuantityType.Area, value: 13 },
200+
{ name: 'NetArea', type: QuantityType.Area, value: 7 },
201+
]),
202+
);
203+
assert.equal(r.area, 7);
204+
});
205+
206+
it('matches candidate names case-insensitively even when a differently-cased fallback name would sort first', () => {
207+
// MUTATION: change `q.name.toLowerCase()` to `q.name` in
208+
// aggregateQuantitiesFromQsets and this goes RED — the map key stays
209+
// 'NETAREA' (uppercase), which no longer matches the lowercase
210+
// candidate 'netarea', so it falls to the alphabetical fallback.
211+
// Case-sensitive sort puts 'AVendorArea' (5000) before 'NETAREA'
212+
// (10), returning the wrong value.
213+
const r = aggregateQuantitiesFromQsets(
214+
qsets([
215+
{ name: 'NETAREA', type: QuantityType.Area, value: 10 },
216+
{ name: 'AVendorArea', type: QuantityType.Area, value: 5000 },
217+
]),
218+
);
219+
assert.equal(r.area, 10);
220+
});
221+
});
222+
});

apps/viewer/src/components/viewer/properties/MaterialTotalsPanel.tsx

Lines changed: 87 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -50,16 +50,100 @@ interface MaterialTotals {
5050
byClass: Array<{ ifcClass: string; count: number }>;
5151
}
5252

53-
/** Pick a quantity value by candidate names (case-insensitive), else by type. */
53+
/**
54+
* Pick a quantity value by candidate names (case-insensitive), else by type:
55+
* when nothing matches a candidate but the map (minus `deny`) is non-empty,
56+
* fall back to the alphabetically-first remaining name. That fallback exists
57+
* for vendor-specific quantity names (e.g. "PerimeterArea", "TopArea") that
58+
* carry a real value of the right type but never match the IFC-standard
59+
* candidate list — without it, such an element contributes nothing to the
60+
* total and its row is dropped from the panel even though the quantity was
61+
* there all along.
62+
*
63+
* The tiebreak is alphabetical rather than "whichever the map saw first":
64+
* map insertion order mirrors the order qsets/quantities were encountered
65+
* while scanning the element (itself downstream of on-demand-parse vs.
66+
* type-fallback ordering, see the qsets selection above) — a happenstance of
67+
* traversal, not a meaningful choice. Sorting by name at least makes the
68+
* fallback reproducible independent of that traversal order.
69+
*
70+
* `deny` excludes names that are the right IFC *type* (e.g. `IfcQuantityArea`)
71+
* but not the right *kind* of value — `CrossSectionArea` is a section
72+
* property (profile area), not a surface extent, yet it type-checks as an
73+
* area and sorts alphabetically before every real surface-area name a beam
74+
* or column carries (`GrossSurfaceArea`, `NetSurfaceArea`, `OuterSurfaceArea`).
75+
* A name-blind alphabetical fallback would silently select it and report a
76+
* value up to two orders of magnitude too small. Skipping a denied name
77+
* still degrades to "no value" rather than a wrong one when it is the only
78+
* candidate present — the honest outcome PR #2777 traded away.
79+
*/
5480
function pickQuantity(
5581
byName: Map<string, number>,
5682
candidates: string[],
83+
deny: ReadonlySet<string> = EMPTY_DENY,
5784
): number | undefined {
5885
for (const c of candidates) {
5986
const v = byName.get(c);
6087
if (v !== undefined) return v;
6188
}
62-
return undefined;
89+
const fallbackNames = [...byName.keys()].filter((n) => !deny.has(n)).sort();
90+
if (fallbackNames.length === 0) return undefined;
91+
return byName.get(fallbackNames[0]);
92+
}
93+
94+
const EMPTY_DENY: ReadonlySet<string> = new Set();
95+
96+
const VOLUME_CANDIDATES = ['netvolume', 'grossvolume', 'volume'];
97+
// Most-specific first: the true extents (net/gross *surface* area) are tried
98+
// before the generic "area" name, so a beam/column/member carrying a
99+
// standard `NetSurfaceArea`/`OuterSurfaceArea` quantity resolves BY NAME and
100+
// never reaches the by-type fallback at all (louistrue, PR #2777 review 2).
101+
const AREA_CANDIDATES = [
102+
'netarea',
103+
'grossarea',
104+
'netsidearea',
105+
'grosssidearea',
106+
'netfloorarea',
107+
'grossfloorarea',
108+
'netsurfacearea',
109+
'grosssurfacearea',
110+
'outersurfacearea',
111+
'area',
112+
];
113+
// `CrossSectionArea` (IfcBeam/IfcColumn/IfcMember/IfcFooting/IfcPile/duct
114+
// and pipe segments' Qto_*BaseQuantities) is an `IfcQuantityArea` but a
115+
// section/profile property, not an extent — the by-type fallback must never
116+
// select it. See `pickQuantity`'s docstring.
117+
const AREA_TYPE_FALLBACK_DENY: ReadonlySet<string> = new Set(['crosssectionarea']);
118+
const WEIGHT_CANDIDATES = ['netweight', 'grossweight', 'weight'];
119+
120+
/**
121+
* Build per-name quantity maps from an element's (or its type's) qsets and
122+
* pick one value of each quantity kind — the single place all three call
123+
* sites (volume/area/weight) share, so the "match a candidate name, else by
124+
* type" rule from `pickQuantity`'s docstring is applied identically to all
125+
* three instead of three copies that can silently drift apart (as they did:
126+
* only the volume call site implemented the fallback).
127+
*/
128+
export function aggregateQuantitiesFromQsets(
129+
qsets: ReadonlyArray<{ quantities: ReadonlyArray<{ name: string; type: number; value: number }> }>,
130+
): { volume?: number; area?: number; weight?: number } {
131+
const volByName = new Map<string, number>();
132+
const areaByName = new Map<string, number>();
133+
const weightByName = new Map<string, number>();
134+
for (const qset of qsets) {
135+
for (const q of qset.quantities) {
136+
const key = q.name.toLowerCase();
137+
if (q.type === QuantityType.Volume) volByName.set(key, q.value);
138+
else if (q.type === QuantityType.Area) areaByName.set(key, q.value);
139+
else if (q.type === QuantityType.Weight) weightByName.set(key, q.value);
140+
}
141+
}
142+
return {
143+
volume: pickQuantity(volByName, VOLUME_CANDIDATES),
144+
area: pickQuantity(areaByName, AREA_CANDIDATES, AREA_TYPE_FALLBACK_DENY),
145+
weight: pickQuantity(weightByName, WEIGHT_CANDIDATES),
146+
};
63147
}
64148

65149
/** Format an aggregated quantity with magnitude-appropriate precision. */
@@ -191,33 +275,19 @@ export function MaterialTotalsPanel({ materialId, modelId }: { materialId: numbe
191275
return own.some((qset) => qset.quantities.length > 0) ? own : typeQsetsFor(entityId);
192276
})();
193277
if (qsets.length === 0) continue;
194-
const volByName = new Map<string, number>();
195-
const areaByName = new Map<string, number>();
196-
const weightByName = new Map<string, number>();
197-
for (const qset of qsets) {
198-
for (const q of qset.quantities) {
199-
const key = q.name.toLowerCase();
200-
if (q.type === QuantityType.Volume) volByName.set(key, q.value);
201-
else if (q.type === QuantityType.Area) areaByName.set(key, q.value);
202-
else if (q.type === QuantityType.Weight) weightByName.set(key, q.value);
203-
}
204-
}
278+
const { volume: vol, area, weight: wt } = aggregateQuantitiesFromQsets(qsets);
205279

206-
const vol = pickQuantity(volByName, ['netvolume', 'grossvolume', 'volume'])
207-
?? (volByName.size > 0 ? [...volByName.values()][0] : undefined);
208280
if (vol !== undefined) {
209281
result.volume += vol * weight;
210282
result.hasVolume = true;
211283
result.elementsWithVolume += 1;
212284
}
213285

214-
const area = pickQuantity(areaByName, ['netarea', 'grossarea', 'netsidearea', 'grosssidearea', 'netfloorarea', 'grossfloorarea', 'area']);
215286
if (area !== undefined) {
216287
result.area += area * weight;
217288
result.hasArea = true;
218289
}
219290

220-
const wt = pickQuantity(weightByName, ['netweight', 'grossweight', 'weight']);
221291
if (wt !== undefined) {
222292
result.weight += wt * weight;
223293
result.hasWeight = true;

0 commit comments

Comments
 (0)