fix(IfcImporter): add vertex key to geometry dedup hash - #238
Conversation
|
The issue references an concise ifc repro fixture that can be used as a test case (once vitest is in) |
ShaMan123
left a comment
There was a problem hiding this comment.
The vertex key construction is Claude's suggestion.
|
Actually I am not sure this is a correct fix. |
|
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 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 The #237 repro would make a good regression test. Still a draft, what's left on your side? |
|
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).
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.
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? |
|
Ran it through web-ifc 0.0.77 so we stop guessing. Three cases, reading the exact vertex buffer the dedup key sees:
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 On performance: no meaningful cost either way. The vertex-key loop runs once per distinct geometry, not per instance (the On the AABB: keep it, I wasn't calling it redundant. In your repro the two plates share the same box, so there the |
|
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. |
d07d74d to
58dfb09
Compare
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>
7e88688 to
2e2674a
Compare
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>
|
The comment is quite daunting.
I prefer delegation instead of hand written hard to reason about logic. |
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>
23ee56c to
56d62c4
Compare
There was a problem hiding this comment.
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.
Description
closes #237 by adding a vertex hash (representing a mesh's holes).
xxhash-wasmas a dependency (~2 KB gzipped).Additional context
What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following:
feat(examples): add hello-world example).fixes #123).