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

BufferGeometryUtils.mergeVertices: Shift the hash bin to avoid missing neighbors #26746

Merged
merged 1 commit into from Sep 13, 2023

Conversation

gkjohnson
Copy link
Collaborator

@gkjohnson gkjohnson commented Sep 12, 2023

Fixed #24621

Description

Adjusts the hashing approach for mergeVertice to shift the hash bin boundary away from round numbers where vertices will more commonly be. This is not a perfect fix - I think that would require a significantly slower solution - but should improve the ability to merge in more common cases.

Here's version of the codesandbox from #24621 using the new function showing that it fixes the missed vertex in this case.

https://jsfiddle.net/zejt0Lrb/

Before / After

image image

cc @erasta @donmccurdy

Copy link
Collaborator

@donmccurdy donmccurdy left a comment

Choose a reason for hiding this comment

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

Thanks @gkjohnson! This sounds good to me.

The glTF Transform implementation has evolved since I shared it in the last thread, and now uses a spatial grid and customizable per-attribute tolerance, if anyone needs those features or wants to merge vertices offline.

https://github.com/donmccurdy/glTF-Transform/blob/68ce0f09cd96fe966a8bf2eda1e8dd5ea60137d9/packages/functions/src/weld.ts

That approach is, indeed, slower. 😄

@Mugen87 Mugen87 added this to the r157 milestone Sep 13, 2023
@Mugen87 Mugen87 merged commit e58a74e into mrdoob:dev Sep 13, 2023
18 checks passed
@gkjohnson gkjohnson deleted the merge-vertices-hash-fix branch September 13, 2023 09:06
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.

mergeVertices truncates coordinates which misses some neighbors
3 participants