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

Implement divide functionality for Vec2, Vec3 and Vec4 classes #2942

Closed
mvaligursky opened this issue Mar 5, 2021 · 4 comments · Fixed by #2946
Closed

Implement divide functionality for Vec2, Vec3 and Vec4 classes #2942

mvaligursky opened this issue Mar 5, 2021 · 4 comments · Fixed by #2946
Assignees
Labels
good first issue Good for newcomers

Comments

@mvaligursky
Copy link
Contributor

Currently we have:

  • mul - multiplies vector by vector
  • scale - multiplies vector by scalar

We should consider similar functionality for divide operation.

@willeastcott
Copy link
Contributor

willeastcott commented Mar 5, 2021

How about if the API was like this:

Vec3#mulScalar(n: number) : Vec3
Vec3#mul(vec: Vec3) : Vec3
Vec3#mul2(vec1: Vec3, vec2: Vec3) : Vec3

Vec3#divScalar(n: number) : Vec3
Vec3#div(vec: Vec3) : Vec3
Vec3#div2(vec1: Vec3, vec2: Vec3) : Vec3

Vec3#addScalar(n: number) : Vec3
Vec3#add(vec: Vec3) : Vec3
Vec3#add2(vec1: Vec3, vec2: Vec3) : Vec3

Vec3#subScalar(n: number) : Vec3
Vec3#sub(vec: Vec3) : Vec3
Vec3#sub2(vec1: Vec3, vec2: Vec3) : Vec3

And we deprecated Vec3#scale.

And same for Vec2 and Vec4 of course.

@mvaligursky
Copy link
Contributor Author

sounds good but not sure we need these, it's not common to do that:

Vec3#addScalar(n: number) : Vec3
Vec3#subScalar(n: number) : Vec3

Maybe we should add these:

Vec3#mulScalar2(n: number, vec: Vec3)
Vec3#divScalar2(n: number, vec: Vec3)

@Maksims
Copy link
Contributor

Maksims commented Mar 5, 2021

Won't it become many ways of doing the same thing?
.scale - is very common across all various math libs and languages. .scale(1/s) is same as .div(s).
Although "div" is conflicting name in the web with DOM div.

For readability of code, I rarely see Devs using mul2.

@willeastcott
Copy link
Contributor

willeastcott commented Mar 5, 2021

@Maksims This issue originated from here: https://forum.playcanvas.com/t/dividing-vectors/18583/6

The problem is that, for you or I, it's obvious that div(x) is scale(1/x). But to some developers, it's just not obvious, or even intuitive if you are using autocomplete in the code editor. It seems reasonable that a lot of developers will think "I want to divide this vector" and start typing vec.div to bring up autocomplete (only to find it doesn't exist). I think we're just used to the current API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants