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: Skip .userData in .mergeBufferGeometries() #24754

Conversation

donmccurdy
Copy link
Collaborator

@donmccurdy donmccurdy commented Oct 6, 2022

Currently .mergeBufferGeometries() adds a bunch of empty data to the .userData field, even if the original geometries did not use .userData themselves. This change ignores .userData if none of the objects have content. With this change we'll skip gathering .userData into an array. Since it is application-specific data, users can likely do something more appropriate with it easily. To reproduce the previous behavior, for example:

geometry.userData.mergeUserData = geometries.map( ( geometry ) => geometry.userData );

From https://threejs.org/examples/webgl_geometry_minecraft.html

Before:
Screen Shot 2022-10-06 at 3 38 49 PM

After:
Screen Shot 2022-10-06 at 3 36 10 PM

@donmccurdy
Copy link
Collaborator Author

Hm, maybe we should just skip gathering the .userData entirely? I think this was added with #13241, because I wanted to avoid throwing away data during the loading process. We no longer use the function in GLTFLoader. And for users calling the function directly, it would be far simpler if we ask users to write:

geometry.userData.mergeUserData = geometries.map( ( geometry ) => geometry.userData );

So I don't think it's particularly relevant for BufferGeometryUtils to do this any more.

@Mugen87
Copy link
Collaborator

Mugen87 commented Oct 7, 2022

TBH, as a user I would expected that userData fields are honored by a merge operation, too.

@Mugen87
Copy link
Collaborator

Mugen87 commented Oct 7, 2022

On the other hand.....we don't know how users actually utilize the userData field on app level so the current merge might not be that useful.

Maybe it's indeed better to remove the exiting code. If someone complains, we can reconsider the code of this PR.

@donmccurdy donmccurdy changed the title BufferGeometryUtils: Skip collecting unused .userData in .mergeBufferGeometries() BufferGeometryUtils: Skip .userData in .mergeBufferGeometries() Oct 7, 2022
@donmccurdy
Copy link
Collaborator Author

donmccurdy commented Oct 7, 2022

Yeah, mostly I think it's hard to say what honoring .userData really means when it's application-specific data. It turns .mergedUserData into a sort of public API. I'd vote to remove this, and have updated the PR.

@Mugen87 Mugen87 added this to the r146 milestone Oct 10, 2022
@Mugen87 Mugen87 merged commit cbbff66 into mrdoob:dev Oct 10, 2022
@donmccurdy donmccurdy deleted the hotfix/buffergeometryutils-merge-no-empty-userdata branch October 10, 2022 21:46
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.

2 participants