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

Use unit norm when finding plane distanceToPoint #11821

Closed
wants to merge 1 commit into from

Conversation

casawa
Copy link
Contributor

@casawa casawa commented Jul 22, 2017

In case the normal supplied is not of unit length.

In case the normal supplied is not of unit length.
@WestLangley
Copy link
Collaborator

Thanks for checking the code. The more the better!

All normal vectors in three.js are assumed to be of unit length. This is not something we check.

We can add a comment in the Plane docs.

@casawa
Copy link
Contributor Author

casawa commented Jul 22, 2017

Ah -- is it worth it to maybe call normalize upon initialization? That way, say I want the norm to be in the direction of <0.5, 1, 0>, then as a client I don't necessarily need to always calculate the normalized version and respective constant on my end.

@WestLangley
Copy link
Collaborator

is it worth it to maybe call normalize upon initialization?

If we did that, the plane constant would be immediately changed after setting it, which may lead to more confusion.

We assume normals and direction vectors have unit length. It is best to be consistent throughout the library, so users know what to expect.

@casawa
Copy link
Contributor Author

casawa commented Jul 22, 2017

Makes sense; will try to add a small comment to the Plane docs.

@casawa casawa closed this Jul 22, 2017
@casawa casawa deleted the patch-1 branch July 22, 2017 04:43
@WestLangley
Copy link
Collaborator

Makes sense; will try to add a small comment to the Plane docs.

Yes, please do.

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.

None yet

2 participants