Skip to content

p5.Vector.setHeading uses radians, even when angleMode(DEGREES) is specified #5497

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

Closed
1 of 17 tasks
oleboleskole3 opened this issue Dec 8, 2021 · 10 comments · Fixed by #5736
Closed
1 of 17 tasks

p5.Vector.setHeading uses radians, even when angleMode(DEGREES) is specified #5497

oleboleskole3 opened this issue Dec 8, 2021 · 10 comments · Fixed by #5736

Comments

@oleboleskole3
Copy link

Most appropriate sub-area of p5.js?

  • Accessibility (Web Accessibility)
  • Build tools and processes
  • Color
  • Core/Environment/Rendering
  • Data
  • DOM
  • Events
  • Friendly error system
  • Image
  • IO (Input/Output)
  • Localization
  • Math
  • Unit Testing
  • Typography
  • Utilities
  • WebGL
  • Other (specify if possible)

Details about the bug:

  • p5.js version: 1.4.0
  • Web browser and version: Chrome v96.0.4664.45
  • Operating System: Windows
  • Steps to reproduce this:
function setup() {
  angleMode(DEGREES)
  const vector = createVector(0, 1)
  vector.setHeading(1)
  console.log(`Expected: 1, Received: ${vector.heading()}`)
}

Logs Expected: 1, Received: 57.29577951308232 to the console, but

function setup() {
  angleMode(RADIANS)
  const vector = createVector(0, 1)
  vector.setHeading(1)
  console.log(`Expected: 1, Received: ${vector.heading()}`)
}

Logs Expected: 1, Received: 1 to the console

@welcome
Copy link

welcome bot commented Dec 8, 2021

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.

@awelles
Copy link
Contributor

awelles commented Dec 8, 2021

p5.Vector.fromAngle() and p5.Vector.fromAngles() also ignore angleMode(DEGREES)

@limzykenneth limzykenneth mentioned this issue Dec 15, 2021
2 tasks
awelles added a commit to awelles/p5.js that referenced this issue Dec 17, 2021
This reverts commit fbe4ed6.
@limzykenneth
Copy link
Member

Fixing this would probably count as a breaking change so I'm not 100% sure if we want to fix this as this stage. Alternatively on the vector and angle related functions documentation we can add a note about this and suggestion to use radians() to convert the values where needed.

@Willyham
Copy link

Willyham commented Mar 5, 2022

Please add this to the docs 🙏 . I just got tripped up by this for a few hours until I read the source and realised it only took radians as input.

@limzykenneth
Copy link
Member

I'm having a reverse on opinion here and think all angle related methods under p5.Vector probably should be respecting angleMode() because heading() and angleBetween() are already doing so but the rest isn't, creating a pretty inconsistent experience that will more likely to be viewed as a bug.

@KevinGrajeda
Copy link
Contributor

KevinGrajeda commented Jul 17, 2022

Functions in p5.Vector taking angleMode() into consideration:

should we make setHeading(),fromAngle() and fromAngles() use the current angleMode()?
@limzykenneth i would like to have your opinion on this so i can start making changes.

@KevinGrajeda
Copy link
Contributor

@jeffawang, @AdilRabbani, I would also like to know your opinion.

@limzykenneth
Copy link
Member

@KevinGrajeda You can file PRs to apply angle mode to those functions.

@jeffawang
Copy link

@jeffawang, @AdilRabbani, I would also like to know your opinion.

Thanks for finding other instances that need to change. I agree witih @limzykenneth's conclusion that it's a breaking change but that it's worth fixing for consistency.

@KevinGrajeda
Copy link
Contributor

hey @limzykenneth i think #5495 closed this by mistake.

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.

7 participants