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

Crafty.math.Vector3D #735

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Conversation

mucaho
Copy link
Contributor

@mucaho mucaho commented Feb 14, 2014

this PR was split from #708

adds the Crafty.math.Vector3D feature and tests

@kevinsimper
Copy link
Contributor

Looks cool to me

result.setValues(this);
return result.crossProduct(vecRH).normalize();
}; // getNormal

Copy link
Member

Choose a reason for hiding this comment

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

It's not clear to me that this is the equivalent of the 2D function of the same name -- I think that actually finds a unit vector that is normal to the line between the two input vectors, as opposed to being normal to both of them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So 2D getNormal ==

an unit vector that is perpendicular to the line between two vectors

and 3D getNormal ==

an unit vector that is perpendicular to the plane defined by the two 3D vectors

Do you want me to rename one of those methods? They both kind of have similar semantics though

Copy link
Member

Choose a reason for hiding this comment

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

Honestly just looking at the name, I'd expect the behavior you implemented here -- maybe the 2D one should be renamed? They probably shouldn't have the same name, as that might be misleading.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well I can't think of a better name than getNormal for both 2D and 3D :)
I think we can still leave the names as they are, but rather improve the documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I thought of one: rename to getPerpendicular for 2D. See #745

@mucaho
Copy link
Contributor Author

mucaho commented Mar 8, 2014

adjusted the documentation for Vector3D.getNormal

@mucaho
Copy link
Contributor Author

mucaho commented Mar 23, 2014

@starwed
Is the inclusion of this feature worth the extra space in crafty-min.js? If not, is there a way to place it into the commit history and remove it right away (it may come in handy in the future)?

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