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

Implementation of p5.Vector setHeading() #4767

Closed
samdelong opened this issue Aug 26, 2020 · 10 comments · Fixed by #4770
Closed

Implementation of p5.Vector setHeading() #4767

samdelong opened this issue Aug 26, 2020 · 10 comments · Fixed by #4770

Comments

@samdelong
Copy link
Contributor

samdelong commented Aug 26, 2020

I am opening a new issue to implement a setHeading method for the p5.Vector class. Currently, p5 contains a function rotate(), which rotates the vector by a certain value, but no such method exists for setting the vector's rotation to an explicit direction. I currently have something working, and am planning to pull request an implementation.

An implementation would look as follows:

p5.Vector.setHeading = function(angle){
  this.rotate(-this.heading())
  this.rotate(angle)
}

This is my first time attempting to contribue to open source, so any tips would be greatly appreciated!

@welcome
Copy link

welcome bot commented Aug 26, 2020

Welcome! 👋 Thanks for opening your first issue here! And to ensure the community is able to respond to your issue, be sure to follow the issue template if you haven't already.

@samdelong samdelong changed the title Implmementation of p5.Vector setHeading Implementation of p5.Vector setHeading() Aug 26, 2020
samdelong added a commit to samdelong/p5.js that referenced this issue Aug 26, 2020
This commit adds the setHeading() function to the p5.Vector class. There are
some remaining issues here. For example, there are no tests.
@mishazawa
Copy link
Contributor

Hello.

no such method exists for setting the vector's rotation to an explicit direction

AFAIK: vector is object that have direction and length. By creating vector you already set his rotation that point to direction.

Take a look at fromAngle function. I think it doing exactly what you want.

Probably this operation may be cheaper (for CPU cycles) when you create new vector and rotate it. Because heading() calculates arctangent, rotate() calculates sine and cosine (for coordinates [x, y] that will be point to vector with 0 angle), then new rotation calculates sine and cosine again.

@samdelong
Copy link
Contributor Author

Hi mishazawa

The issue with creating a new vector altogether with fromAngle() would be any user-defined characteristics of the vector would be erased. However I can see why my implementation would be poor practice for performance.

Perhaps we could copy the code from fromAngle(), but instead of returning a new vector, we just modify the vector in the current context?

@mishazawa
Copy link
Contributor

any user-defined characteristics of the vector would be erased

Which ones? Vector has only [x, y, z] properties.
If you trying to store any other data inside vector f.e.:

let vec = createVector(1, 2, 3);
vec.offset = createVector(2, 2, 2);

I highly recommend not to do it. Because vector is designed to store only direction and length (through coordinates).
Instead you could create your own structure f.e.:

let arrow = {
  direction: createVector(1,2,3),
  offset: createVector(2, 2, 2),
}

we could copy the code from fromAngle()

Proper way is to create new vector. I don't see any use cases when you should modify vector instead just re-write it.

@samdelong
Copy link
Contributor Author

I can see where you're coming from, but often times p5.js is used by beginners, and having to create a new vector using fromAngle() is pretty unintuitive. I think a setHeading() function would help ease some confusion. If not it's own implementation, an alias would still be helpful.

Also just to note, a vector's z property would be lost if using the fromAngle() function. Even though the heading() function should only be used on 2D vectors, it would be beneficial in reducing some confusion.

@burnedsap
Copy link

I agree, I think it would be handy to have a setHeading() function.

samdelong added a commit to samdelong/p5.js that referenced this issue Dec 11, 2020
@samdelong
Copy link
Contributor Author

As per @mishazawa s suggestion, I edited the setHeading() functions implementation to more closely represent the fromAngle() function, but without the need to create a new object. I still think this feature would improve the p5.js library.

@limzykenneth
Copy link
Member

Thanks for the discussions! I'm ok with this being added, can someone else have a look at this proposal and if it's ok, we'll merge it? @lmccart @montoyamoraga @outofambit @stalgiag

@montoyamoraga
Copy link
Member

i agree that this will be a good contribution to the library. My only doubt is if this would break any 3D vector? i saw on the docs that vectors can be x,yz too, right?

@samdelong
Copy link
Contributor Author

I included that this method would only work for 2d vectors in the method documentation. Im not sure if something should prevent a user from rotating a 3d vector...maybe a console warning would be helpful here?

lmccart added a commit that referenced this issue Dec 12, 2020
Implementing p5.Vector setHeading() (Issue #4767)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants