new angle and angle_rad property in vector2 #3222
Conversation
src_c/math.c
Outdated
| pgVector *vec = (pgVector *)self; | ||
|
|
||
| if (vec->coords[0] == 0.0 && vec->coords[1] == 0.0) { | ||
| return PyFloat_FromDouble(0.0); |
There was a problem hiding this comment.
I’d rather an exception get raised here. The zero vector is special and getting it’s angle makes no sense
There was a problem hiding this comment.
ok, I thought it should be 0 because of the initial issue #3195, but I can change and raise an exception
There was a problem hiding this comment.
@oddbookworm
I strongly think the angle properties should be consistent with the existing behavior of .as_polar(), .angle_to(), .rotate() and more importantly python math.atan2() (which is based off the a C standard) and polar coordinates.
These functions don't raise errors and (for not nan args) don't return nan, and most actually do not document this special case.
There was a problem hiding this comment.
I still don't agree that we should silently just say the angle of the zero vector is zero. I don't like those functions treating that special case in the way they do either. And actually, the C standard does not guarantee that atan2(0, 0) returns 0. Here's a snippet from the C standard
There was a problem hiding this comment.
One reason I hate that behavior is because it can lead to very confusing behavior. Consider the case where you're just continuously scaling down a vector without changing it's direction. At some point, you'll go from a constant angle to 0, regardless of what the starting angle is.
There was a problem hiding this comment.
Hello @aatle and @oddbookworm,
I saw on discord that there is many vision for what should be done for the zero vector. And I wanted to know if people agree on one of these in order to maybe change my code
There was a problem hiding this comment.
Hi @AntoineMamou, it has been settled that we will imitate python's math.atan2() function behavior. (Returns certain value depending on sign of zeroes, for zero vector.)
See https://github.com/python/cpython/blob/ed81971e6b26c34445f06850192b34458b029337/Lib/test/test_math.py#L327 for the relevant test cases.
Here's the old CPython 3.7.2 implementation for special cases: https://github.com/python/cpython/blob/v3.7.2/Modules/mathmodule.c#L566-L600
Recent CPython relies on a library, with atan2 implemented here:
https://github.com/rust-lang/libm/blob/master/src/math/atan2.rs#L20-L31
There was a problem hiding this comment.
Hi @aatle, so if we want to imitate math.atan2(), for the case where x=y=0, it should return 0, right ?
There was a problem hiding this comment.
And I'm using atan2 to find the angle, so basically I can skip the if statement ?
There was a problem hiding this comment.
Maybe I'm missing something here
yunline
added
New API
|
I see that two checks were not successful but I don't understand why. Can someone help me ? |
There was a problem hiding this comment.
Hi, sorry for taking a while to get to this proposal.
Personally I really like the overall idea. Definitely +1 for keeping it as a property. A couple of things to discuss
- Do we really need both
angleandangle_rad? Yes, I can understand why it was done (consistency with other vector methods), but because this is a new property, we can consider dropping one of these. I'm just putting this idea up for discussion, personally have no opinion here - Why restrict it to read only property? I can imagine that setting an angle without changing magnitude would be a useful feature as well. Though that could be a separate follow up PR too.
Again, thanks for taking up this proposal and contributing to pygame-ce
|
To answer those questions,
I guess since it is a property, it may be appropriate to allow setting; however, there should be a non-inplace equivalent. This functionality can be always be added in a later PR though. |
|
Requesting a re-review from @oddbookworm here as they had previously reviewed it. |
Update version to 2.5.5
|
As soon as everything passes, I'll merge it |
This PR follows this one : #3216
I make the changes needed and add two properties :
angle_rad: Returns the vector’s angle in radians relative to the positive x-axis.angle: Returns the angle in degrees, normalized to (180,180].