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

Vector3 proposed method name changes #4090

Closed
gregtatum opened this issue Nov 8, 2013 · 7 comments
Closed

Vector3 proposed method name changes #4090

gregtatum opened this issue Nov 8, 2013 · 7 comments

Comments

@gregtatum
Copy link
Contributor

Vector3

getColumnFromMatrix

Vector3 has a method getColumnFromMatrix, but it actually sets the vector based on that matrix. I would propose that it's changed to setColumnFromMatrix.

applyAxisAngle

This is a non-intuitive name for rotating on an axis. I would propose it's changed to rotateOnAxis(axis, angle).

@erichlof
Copy link
Contributor

erichlof commented Nov 8, 2013

Hi TatumCreative, thanks for taking a crack at the todos in the documentation, for all the rest of us.

IMHO setColumnFromMatrix is also misleading because it sounds like you are trying to set the column. Here's two more suggestions that tell the programmer exactly what the method does, although the names are a little longer:

applyColumnFromMatrix (you are applying the supplied matrix's column to your Vector3's x, y, and z components)
or
setFromMatrixColumn (set your Vector3's components from the supplied matrix's column)

While were at it, the 2 methods right above this one in Vector3.js - namely getPositionFromMatrix, and getScaleFromMatrix have the same naming problem - they really don't 'get' anything. Maybe setPositionFromMatrix and setScaleFromMatrix would be more readable.

applyAxisAngle is less bothersome, but could become setFromAxisAngle or rotateOnAxisAngle (I like the word Angle being included in method name because it directly tells you that you must supply both an Axis AND an Angle.

These changes would probably break a lot of demos though :(

Thanks again for filling in some of the documentation. I might take a crack at some parts one of these days. :)

@gregtatum
Copy link
Contributor Author

setFromMatrixColumn makes a lot of sense!

I think the angle on the rotateOnAxis would be implied. I suspect applyAxisAngle would be more familiar to someone who uses 3d math a lot more, but it feels a little jargony to me. http://en.wikipedia.org/wiki/Axis%E2%80%93angle_representation

The name changes could include the old method names with a reference to the new name with a deprecation notice to ease a transition.

No problem on the docs :) It's helping me to become more familiar with the ins and outs of Three.js.

@mrdoob
Copy link
Owner

mrdoob commented Nov 9, 2013

@WestLangley sounds good?

@WestLangley
Copy link
Collaborator

Well, what it really is, is

Vector3.setFromPositionComponentOfMatrix()

That's a little wordy, so if you want to make a change, I think it would be OK to use these:

Vector3.setFromMatrixColumn()
Vector3.setFromMatrixPosition()
Vector3.setFromMatrixScale()

I would leave these alone:

Vector3.applyAxisAngle()
Vector3.applyMatrix3()
Vector3.applyMatrix4()
Vector3.applyQuaternion()
Vector3.applyProjection()

@erichlof
Copy link
Contributor

erichlof commented Nov 9, 2013

@WestLangley
Vector3.setFromMatrixColumn()
Vector3.setFromMatrixPosition()
Vector3.setFromMatrixScale()

You have my vote! +1 for these - thanks. They are more descriptive of what's going on 'under the hood'. All the other naming conventions for the rest of Three.js are perfect in my opinion - they are clear, concise and descriptive.

Thanks so much for all your work and expertise @WestLangley and @mrdoob !

@mrdoob
Copy link
Owner

mrdoob commented Nov 16, 2013

These sound good to me too! :)

@gero3
Copy link
Contributor

gero3 commented Nov 16, 2013

I agree. It is better

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants