Skip to content

fix(IfcImporter): add vertex key to geometry dedup hash - #238

Open
ShaMan123 wants to merge 11 commits into
ThatOpen:mainfrom
ShaMan123:fix/geometry-vertex-dedup-hash#237
Open

fix(IfcImporter): add vertex key to geometry dedup hash#238
ShaMan123 wants to merge 11 commits into
ThatOpen:mainfrom
ShaMan123:fix/geometry-vertex-dedup-hash#237

Conversation

@ShaMan123

@ShaMan123 ShaMan123 commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Description

closes #237 by adding a vertex hash (representing a mesh's holes).

  • Added xxhash-wasm as a dependency (~2 KB gzipped).
  • Fixes IfcApi init: assigns to class after calling init

Additional context


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following:

  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Follow the Conventional Commits v1.0.0 standard for PR naming (e.g. feat(examples): add hello-world example).
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@ShaMan123

Copy link
Copy Markdown
Contributor Author

The issue references an concise ifc repro fixture that can be used as a test case (once vitest is in)

@ShaMan123 ShaMan123 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The vertex key construction is Claude's suggestion.

@ShaMan123

Copy link
Copy Markdown
Contributor Author

Actually I am not sure this is a correct fix.
This doesn't cover cases that are geometrically the same with differently ordered vertices.

@ShaMan123 ShaMan123 closed this Jul 7, 2026
@ShaMan123 ShaMan123 reopened this Jul 7, 2026
@ShaMan123
ShaMan123 marked this pull request as draft July 7, 2026 10:40
@agviegas

Copy link
Copy Markdown
Contributor

Great repro, it makes the problem obvious: both plates share outline, area, volume and centroid, and the only difference is where the holes sit, which is exactly what the old key never looked at.

Direction is right. One thing to fix first: the vertexKey sum can cancel. JS % keeps the sign of the dividend, so h goes negative for negative coordinates and stays signed through the mixing. Summed commutatively, two vertices whose hashes are exact negatives cancel to zero and drop out of the key. The final if (vertexKey < 0) only fixes the sign of the result, not the cancellation. Normalize each h first:

let h = Math.round(position[i] * p) % MODULUS;
if (h < 0) h += MODULUS;

And a question: does the key need to be order-invariant at all? The commutative sum buys a match on a permuted buffer but costs collision resistance, where an ordered polynomial hash is stronger and simpler. Do we actually see web-ifc emit permuted duplicates? If not, I'd take the ordered one.

Minor: the AABB doesn't separate your two plates (same outline, same box), so vertexKey is carrying the fix alone. Fine to keep the corners, just noting what's load-bearing.

The #237 repro would make a good regression test. Still a draft, what's left on your side?

@ShaMan123

Copy link
Copy Markdown
Contributor Author

I am not an expert of hashing so I do not feel comfortable suggesting the correct solution. I do understand and agree that the current is lacking due to sign cancellation (part of why the PR is a draft).
That being said:

And a question: does the key need to be order-invariant at all? The commutative sum buys a match on a permuted buffer but costs collision resistance, where an ordered polynomial hash is stronger and simpler. Do we actually see web-ifc emit permuted duplicates? If not, I'd take the ordered one.

I am not sure how web-ifc handles it. I am very cautious due to the last month (working on Tekla garbage ifc dedup). I would argue that we could add a simple sort on the holes before hashing and that will cover all cases.

Minor: the AABB doesn't separate your two plates (same outline, same box), so vertexKey is carrying the fix alone. Fine to keep the corners, just noting what's load-bearing.

You mean that the AABB isn't needed any longer since the vertex key handles it? Is there no other case that relies on the AABB?

@agviegas

Copy link
Copy Markdown
Contributor

Ran it through web-ifc 0.0.77 so we stop guessing. Three cases, reading the exact vertex buffer the dedup key sees:

  1. Two independent facesets, identical authoring: identical vertex order. Same geometry in, same buffer out.
  2. Same box, point list reversed and faces reindexed to match (same face order): still identical order. Reindexing points alone doesn't permute the output.
  3. Same box, same points, face list in a different order: same vertex set, permuted order.

So web-ifc never spontaneously permutes, but it hands back a permuted buffer when the source lists the same triangles in a different order (case 3). It emits in face-iteration order, so triangle order in the IFC is what leaks through.

What that means for the key. The common duplicate case (an exporter repeating a product, or a shared representation map / mapped items) is case 1, byte-identical, so an ordered polynomial hash over the buffer dedups it, and it drops the sign-cancellation problem for free since you never sum commutatively. The only thing ordered misses is case 3: geometrically identical but triangle-order-permuted, which is what your commutative sum buys today. Dropping to ordered would stop deduping those, a memory miss, not wrong geometry.

So the real decision is whether case 3 is worth chasing. If yes, don't keep the commutative sum (weak collisions plus the cancellation trap). Do it order-invariant the robust way: hash each vertex to a canonical value, sort those hashes, then fold them with an ordered polynomial. That is where a sort belongs, over all per-vertex hashes, not over holes, since case 3 shows the permutation is whole-buffer triangle order with no separable holes list to sort.

My read: case 3 needs two products that are the same shape but authored with different triangle order, which a single exporter rarely does within one file. I would ship the ordered hash, which fixes #237 and is simpler and stronger, and add the sorted order-invariant variant only if a real file actually shows triangle-order-permuted duplicates. Happy to go straight to the sorted version if your Tekla files make you want belt and braces, it is just the extra O(n log n) sort.

On performance: no meaningful cost either way. The vertex-key loop runs once per distinct geometry, not per instance (the _previousGeometriesIDs check short-circuits repeated representations before any buffer work), and it can be fused into the unit-scaling loop that already walks every coordinate, so it is zero extra passes. web-ifc's tessellation dominates. The only variant that actually costs conversion time is the sorted one, because of the per-geometry sort, which is the other reason I would default to ordered.

On the AABB: keep it, I wasn't calling it redundant. In your repro the two plates share the same box, so there the vertexKey is what separates them and the box isn't doing the separating. In general the box is a cheap early discriminator that rejects differently sized geometry before the per-vertex loop runs, and it is independent signal from the fold. Worth keeping.

@agviegas

agviegas commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Thanks for pushing this forward! We are on board with the direction. To land it, let's do the changes discussed above: replace the commutative sum with the ordered polynomial hash so sign cancellations can't merge distinct geometries, keep the AABB term in the key, and add a regression test with the #237 repro so this can't come back. Once that's in and the PR is marked ready, we'll merge it. This one probably improves #259 and #260 too, so it's high value.

@ShaMan123
ShaMan123 force-pushed the fix/geometry-vertex-dedup-hash#237 branch from d07d74d to 58dfb09 Compare September 10, 2026 12:58
ShaMan123 and others added 2 commits September 10, 2026 16:03
The commutative sum could cancel: JS `%` keeps the sign of the dividend, so
per-vertex hashes stayed signed and two vertices hashing to exact negatives
dropped out of the key entirely. Fold the coordinates with an ordered
polynomial instead, normalizing each into [0, MODULUS) first.

web-ifc emits vertices in face-iteration order, so a repeated representation
comes back byte-identical and still dedups. The only case the ordered fold
gives up is the same shape authored with a different triangle order, which
costs memory rather than correctness, and it buys back collision resistance.

Adds the ThatOpen#237 repro as a regression test: two plates sharing outline, area,
volume, centroid and bounding box, differing only in where their bolt holes
sit. Before the fix both hashed alike and the second rendered with the first
one's holes.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Rewrite the comments around the dedup hash to teach the mechanism rather
than restate it: the fold is decimal place-value with a prime base, which
is what makes it order-sensitive. Drop the claim that avoiding bitwise
operators buys key width - MODULUS is just under 2 ** 32, so the key is
~32 bits either way.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@ShaMan123
ShaMan123 force-pushed the fix/geometry-vertex-dedup-hash#237 branch from 7e88688 to 2e2674a Compare September 10, 2026 13:23
The previous wording credited FACTOR being prime for the fold's
distribution and implied exceeding 2 ** 53 would break the hash. Neither
is quite right: MODULUS being prime is what makes multiplication
invertible, FACTOR only has to be large and not a multiple of it, and
past 2 ** 53 the fold stays deterministic but distributes worse. Also
notes that the base doesn't outrun coordinates at building scale, so the
digit-per-coordinate picture is a mental model rather than a literal one.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@ShaMan123

Copy link
Copy Markdown
Contributor Author

The comment is quite daunting.
I don't like it.
I would prefer one of the following:

  • extract to a hashing util (and decide whether to refactor to vertexKey = (Math.imul(vertexKey, 16777619) ^ Math.round(position[i] * p)) | 0; which is unreadable for non bitwise eyes)
  • delegate to an existing solution: Web Crypto subtle.digest is async, it doesn't seem a blocker to me since the caller is async (though callback itself is not).

I prefer delegation instead of hand written hard to reason about logic.

ShaMan123 and others added 4 commits September 10, 2026 16:56
Replaces the modular-arithmetic polynomial with MurmurHash3's 32-bit
mixer. Math.imul and the bitwise operators are defined on 32-bit two's
complement, so the sign normalization and the 2 ** 53 headroom argument
the previous fold needed both disappear - there is nothing to normalize
and nothing that can overflow.

Picked the mixer by measurement, not preference. Building coordinates are
highly structured, and over 68k plates differing only in hole position a
word-wise FNV-1a fold collided 13 times against a birthday expectation of
0.5; MurmurHash3 collided once. Byte-wise FNV-1a sat between the two at
2, and cost an allocation and four times the iterations.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Moves the coordinate fold out of loadShellGeometry into its own module
next to it, inlining the mixing steps rather than splitting them across
helpers. The eslint-disable for no-bitwise now covers a file that is
entirely 32-bit hash arithmetic instead of sitting mid-method.

Kept private: no barrel re-exports it, so it stays out of the package's
public API and off the rolled-up .d.ts.

Being standalone makes it directly testable, so this also adds unit
tests for the properties the dedup key depends on - determinism, order
sensitivity, opposite values not cancelling, one quantization step
separating, sub-step float noise collapsing, the ThatOpen#237 plates separating,
and collisions staying at chance over 90k structured boxes.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@ShaMan123
ShaMan123 marked this pull request as ready for review September 10, 2026 14:39
@ShaMan123
ShaMan123 force-pushed the fix/geometry-vertex-dedup-hash#237 branch from 23ee56c to 56d62c4 Compare September 10, 2026 14:43

@ShaMan123 ShaMan123 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Introduced xxhash-wasm npm module to handle hashing instead of the ghastly hand written function that was failing (over grouping) under 90K occurances (see the added test).
This solves my inhibitions regarding the code so I feel confident shipping it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

IfcFileReader#loadShellGeometry WRONG geometry deduplication

2 participants