Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(functions): Remove lossy weld #1357

Merged
merged 2 commits into from
Apr 14, 2024
Merged

Conversation

donmccurdy
Copy link
Owner

  • BREAKING CHANGE: weld is now lossless, and does not accept tolerance options.

Avoids having to maintain two versions of weld(), the lossless version is vastly faster. Future updates to Meshoptimizer will include simplifyWithAttributes (#1335), which I hope will provide a better approach for joining vertices despite differences in vertex attributes, and allowing weld() to remain a faster, more focused, and more maintainable method.

I spent some time trying other approaches — like #1356, trying to consolidate the two methods. But that got complicated quickly. If I were trying that again, I think I'd take an approach similar to #1356 but have the QuantizedVertexStream class apply rounding when reading each vertex, rather than iterating the vertex attributes up front. Then we need to rebuild the 'writeMap' after removing degenerate triangles, write a temporary index to the geometry without those triangles, then run it through compactPrimitive... it's just messier.

But I'm hoping a lossless weld is enough — that's all gltfpack appears to do for example. And when #1335 is available, that will be a (likely better) alternative for merging vertices that are not identical.

- BREAKING CHANGE: weld is now lossless, and does not accept tolerance options.
- Avoids having to maintain two versions of weld(), the lossless version is vastly faster
- Future updates to Meshoptimizer will include simplifyWithAttributes, which I hope will
  provide a better approach for joining vertices despite differences in vertex attributes,
  and alowing weld() to remain a faster, more focused, and more maintainable method.
@donmccurdy donmccurdy merged commit cf92b37 into main Apr 14, 2024
6 checks passed
@donmccurdy donmccurdy deleted the chore/remove-lossy-weld branch April 14, 2024 20:49
@zeux
Copy link

zeux commented Apr 15, 2024

fwiw currently simplifyWithAttributes will still treat topological discontinuities the same as simplify; attributes are currently used to reduce attribute+geometric error during simplification but don't affect any collapse restrictions. I have some long term plans to improve this but just noting that it's not going to be a drop-in replacement to "weld with tolerance + simplify" for some time. (is it a problem for gltf-transform? not sure! gltfpack doesn't do tolerance-based welding either)

@donmccurdy
Copy link
Owner Author

donmccurdy commented Apr 16, 2024

Thanks @zeux! My impression so far is that welding based on bitwise-equality before simplification works well, both here and in gltfpack, so I'm hopeful that glTF Transform's previous lossy welding was just unnecessary.

Another possible workaround — if anyone does want a more aggressive weld — would be to quantize before welding, like:

import { quantize, weld, simplify } from '@gltf-transform/functions';
import { MeshoptSimplifier } from 'meshoptimizer';

await MeshoptSimplifier.ready;

await document.transform(
  quantize({
    quantizePosition: 14,
    quantizeNormal: 8,
    quantizeColor: 8
  }),
  weld(),
  simplify({
    simplifier: MeshoptSimplifier,
    error: 0.001
  })
);

That would provide more control than the previous tolerance-based weld, which didn't expose as much as QuantizeOptions. I'll add something about that to the weld() documentation as well.

@donmccurdy
Copy link
Owner Author

donmccurdy commented Apr 17, 2024

Unfortunately, a simple counter-example to my idea:

  1. create the default 'monkey' mesh in blender
  2. subdivide it 5-6 times
  3. export the result to GLB

Here's that mesh, compressed to meet GitHub's size restrictions:

monkey_500k_vertices+draco.glb.zip

A lossless weld won't do anything to this model, and as a result simplification does almost nothing either – it works only where the vertices are mostly coplanar. Applying quantization first, as I suggested above, allows simplification to proceed successfully only if I reduce quantizationNormal bits to 4 (normally limited to the 8–16 range, made local changes to test).

I suspect 4-bit normals will cause quality issues, so perhaps this quantization does need to be included as a condition in welding, rather than as a pre-processing step.

@marwie
Copy link
Contributor

marwie commented Jun 6, 2024

Hello @donmccurdy I've just encountered this issue as well when running our mesh LOD generation - for testing I've been using this model

Previously I was able to increase the threshold for lower-res LOD level which produced decent simplifcation (with some errors but that was mostly OK since those meshes would only be displayed at a distance) - see image below

image

After updating to 4.x all our LOD meshes are discarded unfortunately since weld+simplify does never exceed our simplification threshold of 5%

Using gltf-transform 3 I can specify the tolerance and increase it to 0.001 for example where simplify is reducing a decent amount of vertices.

I will look into adding a quantize op now and will reply here again :)

-- Update

Unfortunately using quantize isn't an option for me right now because of a non exported quantizePrimitive method - I need to be able to quantize a single primitive

@donmccurdy
Copy link
Owner Author

Probably we should try to export quantizePrimitive regardless of this issue. I think the API just needs a bit of cleanup first, to something like...

import { quantizePrimitive } from '@gltf-transform/functions';

quantizePrimitive(prim, transform /* mat4 */, options /* QuantizeOptions | undefined */);

But, I don't think that will be a complete fix for more aggressive welding and simplification. The previous lossy weld implementation was very slow, and a completely different code path from the newer one (inspired by Meshoptimizer), so I'd prefer not to bring that back as-is. Instead I think the best approach would be to extend the VertexStream implementation here...

equal(a: number, b: number): boolean {
for (const { u8, byteStride } of this.attributes) {
for (let j = 0; j < byteStride; j++) {
if (u8[a * byteStride + j] !== u8[b * byteStride + j]) {
return false;
}
}
}
return true;
}

... with a lossyEquals function that compares attributes based on the configured per-attribute precision, or perhaps some different precision criteria. It will be slower than the lossless mode, but hopefully not as slow as the old code.

After that, we'll need a bit of extra work to clean up degenerate triangles and remove the primitive entirely if it collapses, which can't happen in the lossless mode. I don't think weldPrimitive needs to handle that cleanup, but weld() probably should.

kzhsw added a commit to kzhsw/glTF-Transform that referenced this pull request Jul 11, 2024
Docs for calling `weld` updated, weld tolerance option is no longer available since donmccurdy#1357
donmccurdy pushed a commit that referenced this pull request Jul 12, 2024
Docs for calling `weld` updated, weld tolerance option is no longer available since #1357
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants