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

Add LenSqr functions #74

Merged
merged 3 commits into from
Apr 16, 2019
Merged

Add LenSqr functions #74

merged 3 commits into from
Apr 16, 2019

Conversation

Zyl9393
Copy link
Contributor

@Zyl9393 Zyl9393 commented Nov 1, 2018

Added SqrLen() functions which return squared vector length. This can make some lines shorter when variable names are long, e.g.:
worldSpaceFragmentCenter.Dot(worldSpaceFragmentCenter)
vs.
worldSpaceFragmentCenter.SqrLen()
Understandable if not wanted, though. Mostly learning how 2 github over here. Have a nice day.

Copy link
Member

@pwaller pwaller left a comment

Choose a reason for hiding this comment

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

Sorry it has taken a while for someone to get back to you on this. I can't see anything wrong with this other than a minor bug in the automatically generated files. Can you rectify and I'll merge assuming there aren't any objections.

mgl64/matstack/transformstack_test.go Outdated Show resolved Hide resolved
@pwaller
Copy link
Member

pwaller commented Apr 15, 2019

Thanks for fixing the issues I raised!

One thing that occured to me as I go to merge it:

How about LenSquared as the name of the function? Then if you're using an editor with autocomplete, it will show up as you're looking for Len - improving the discoverability. Also it would appear next to Len in the documentation: https://godoc.org/github.com/go-gl/mathgl/mgl32

Let me know what you think. Happy to accept it as is otherwise - though I will squash the history down to just "Add ... functions", to avoid the merge commit going from master into the development branch.

@MMulthaupt
Copy link

I would expect autocomplete to list the function regardless of where in the name the substring is found, but regarding the documentation, you raise a good point. Maybe call it LenSqr then, or LenSq even? Having it short was one of main reasons for making this, and Sq is a well understood shortcut in anything math-related; even Java refrains from its usual verbosity, calling the square root function in its Math package sqrt.

@pwaller
Copy link
Member

pwaller commented Apr 15, 2019

OK, then 👍 to LenSqr from me.

@Zyl9393
Copy link
Contributor Author

Zyl9393 commented Apr 15, 2019

Should be good now.

@pwaller pwaller changed the title Add SqrLen functions Add LenSqr functions Apr 16, 2019
@pwaller pwaller merged commit c4601bc into go-gl:master Apr 16, 2019
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.

3 participants