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

Adding buffer geometry utilities for merging. #9529

Closed
wants to merge 8 commits into
base: dev
from

Conversation

Projects
None yet
4 participants
@kylegmaxwell

kylegmaxwell commented Aug 16, 2016

These are some buffer geometry utilities that I wrote for merging geometry and related operations. I am submitting this after discussion on {mrdoob}/three.js#8162 with @WestLangley
#8162 (comment)
The main idea is that to merge buffer geometry requires creating a new geometry, hence the functions are static on the BufferGeometry class. I also created a jsfiddle as an example.
https://jsfiddle.net/kylu/c1rt9dnb/

@mrdoob

This comment has been minimized.

Show comment
Hide comment
@mrdoob

mrdoob Sep 7, 2016

Owner

I'm thinking of creating a InstancedBufferGeometry. splitVertices would then become something like if ( geometry.isInstancedBufferGeometry ) geometry2 = geometry.toBufferGeometry();, right?

Owner

mrdoob commented Sep 7, 2016

I'm thinking of creating a InstancedBufferGeometry. splitVertices would then become something like if ( geometry.isInstancedBufferGeometry ) geometry2 = geometry.toBufferGeometry();, right?

@mrdoob

This comment has been minimized.

Show comment
Hide comment
@mrdoob
Owner

mrdoob commented Sep 7, 2016

@kylegmaxwell

This comment has been minimized.

Show comment
Hide comment
@kylegmaxwell

kylegmaxwell Sep 8, 2016

Oh, good call. I just updated the PR with another commit to use the toNonIndexed function.

kylegmaxwell commented Sep 8, 2016

Oh, good call. I just updated the PR with another commit to use the toNonIndexed function.

Show outdated Hide outdated src/core/BufferGeometry.js Outdated
Show outdated Hide outdated src/core/BufferGeometry.js Outdated
Show outdated Hide outdated src/core/BufferGeometry.js Outdated
Show outdated Hide outdated src/core/BufferGeometry.js Outdated
@kylegmaxwell

This comment has been minimized.

Show comment
Hide comment
@kylegmaxwell

kylegmaxwell Sep 9, 2016

ok, the third commit should address the comments so far, and now the PR is much more concise.

kylegmaxwell commented Sep 9, 2016

ok, the third commit should address the comments so far, and now the PR is much more concise.

Show outdated Hide outdated src/core/BufferGeometry.js Outdated
@WestLangley

This comment has been minimized.

Show comment
Hide comment
@WestLangley

WestLangley Sep 10, 2016

Collaborator

So the restriction is geometries 2, 3, ..., N must have all the attributes of the first geometry in the array -- and any additional attributes in geometries 2, 3, ..., N are ignored?

Collaborator

WestLangley commented Sep 10, 2016

So the restriction is geometries 2, 3, ..., N must have all the attributes of the first geometry in the array -- and any additional attributes in geometries 2, 3, ..., N are ignored?

@kylegmaxwell

This comment has been minimized.

Show comment
Hide comment
@kylegmaxwell

kylegmaxwell Sep 12, 2016

Yes, I can supply a default value instead, but it seemed more straightforward to drop the inconsistent attributes. Then the user can set them ahead of time with the appropriate defaults on a case by case basis.

kylegmaxwell commented Sep 12, 2016

Yes, I can supply a default value instead, but it seemed more straightforward to drop the inconsistent attributes. Then the user can set them ahead of time with the appropriate defaults on a case by case basis.

@WestLangley

This comment has been minimized.

Show comment
Hide comment
@WestLangley

WestLangley Sep 12, 2016

Collaborator

So if the first geometry is a special case, then would it make more sense to start with a geometry?

bufferGeometry.merge( geometries ); // geometries can be a single geometry or an array

In other words, avoid the static method approach.

Collaborator

WestLangley commented Sep 12, 2016

So if the first geometry is a special case, then would it make more sense to start with a geometry?

bufferGeometry.merge( geometries ); // geometries can be a single geometry or an array

In other words, avoid the static method approach.

@kylegmaxwell

This comment has been minimized.

Show comment
Hide comment
@kylegmaxwell

kylegmaxwell Sep 15, 2016

Yes, that's a good idea. Your suggestion to make it a member function seems to fit better with the current design of the class.

Though, this reminds me, there is already a merge function, which seems to merge two buffer geometries, but assumes that the buffers have already been allocated. I must say, I found that counter intuitive at first, coming from using the merge function on Geometry, I expected similar behavior, though I understand buffer geometry has different constraints.

Anyways, I renamed my function join, so as to not conflict with merge.

kylegmaxwell commented Sep 15, 2016

Yes, that's a good idea. Your suggestion to make it a member function seems to fit better with the current design of the class.

Though, this reminds me, there is already a merge function, which seems to merge two buffer geometries, but assumes that the buffers have already been allocated. I must say, I found that counter intuitive at first, coming from using the merge function on Geometry, I expected similar behavior, though I understand buffer geometry has different constraints.

Anyways, I renamed my function join, so as to not conflict with merge.

@alvaromongon

This comment has been minimized.

Show comment
Hide comment
@alvaromongon

alvaromongon Dec 3, 2017

This code seems to work as expected, will it be merged?

alvaromongon commented Dec 3, 2017

This code seems to work as expected, will it be merged?

kylegmaxwell added some commits Dec 5, 2017

Remove extra line from merge
I mistakenly left that in.
Add blank line
Restores original formatting at end of file.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment