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

Matrix3/Matrix4: Refactored .applyToBuffer() #10355

Merged
merged 5 commits into from
Dec 15, 2016
Merged

Matrix3/Matrix4: Refactored .applyToBuffer() #10355

merged 5 commits into from
Dec 15, 2016

Conversation

Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented Dec 14, 2016

This PR renames .applyToBuffer() to . applyToBufferAttribute(). Besides, the methods now support InterleavedBufferAttribute.


Multiplies (applies) the upper 3x3 matrix of this matrix to every 3D vector in the [page:ArrayBuffer buffer].
Multiplies (applies) the upper 3x3 matrix of this matrix to every 3D vector in the [page:BufferAttribute attribute].
Copy link
Collaborator

Choose a reason for hiding this comment

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

upper 3x3 ? Also, what is the use case for this method?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've removed that part of the sentence. The following commit originally added the methods 1b6effb.

Default is the last element in the array.<br /><br />
[page:BufferAttribute attribute] - An attribute of floats that represent 3D vectors.<br />
[page:Number offset] - (optional) index in the attribute of the first vector. Default is 0.<br />
[page:Number count] - (optional) index in the attribute of the last vector. Default is the last element in the attribute.<br /><br />
Copy link
Collaborator

Choose a reason for hiding this comment

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

count is a quantity, not an index.

[page:Number offset] - (optional) index in the array of the first vector's x component. Default is 0.<br />
[page:Number length] - (optional) index in the array of the last vector's z component.
Default is the last element in the array.<br /><br />
[page:BufferAttribute attribute] - An attribute of floats that represent 3D vectors.<br />
Copy link
Collaborator

Choose a reason for hiding this comment

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

This method only makes sense with certain buffer attributes, right? -- even though it is generically-named.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can't think of a better name than . applyToBufferAttribute(). Any suggestions?

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Dec 14, 2016

Um, looks like these methods are not used in the entire repo so far...

@WestLangley
Copy link
Collaborator

Um, looks like these methods are not used in the entire repo so far...

Right. What is the use case? And why offset and count?

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Dec 14, 2016

And why offset and count?

I think the original author added these parameters (offset and length) to keep the interface and logic of applyToBuffer similar to applyToVector3Array.

@WestLangley
Copy link
Collaborator

Let's see what others think...

@mrdoob
Copy link
Owner

mrdoob commented Dec 14, 2016

Feel free to remove what's not needed 👍

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Dec 14, 2016

We need these methods if we want to make BufferGeometry .applyMatrix() compatible to InterleavedBufferAttribute, see here.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Dec 14, 2016

Why not start with this simplified approach? . applyToBufferAttribute() without offset and count. In this way the methods can be used in BufferGeometry .applyMatrix().

@WestLangley
Copy link
Collaborator

Why not start with this simplified approach? . applyToBufferAttribute() without offset and count.

That seems reasonable to me. Can you remove applyToVector3Array()? There is only one example use case, which can be refactored or inlined if necessary.

@mrdoob mrdoob merged commit 61736b1 into mrdoob:dev Dec 15, 2016
@mrdoob
Copy link
Owner

mrdoob commented Dec 15, 2016

Thanks!

@unphased
Copy link
Contributor

unphased commented Apr 18, 2017

Was this really necessary...? Breaking my stuff as I upgrade to r84. I just pasted it back in though and rebuilt.

So this was taken out to make the code 0.1% smaller? A noble pursuit, I suppose.

@mrdoob
Copy link
Owner

mrdoob commented Apr 18, 2017

@unphased Is not only about the size. It's also about the API, documentation, user support, etc. Sorry that I broke your code.

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.

4 participants