new angle and angle_rad property in vector2 #3222

AntoineMamou · 2024-11-12T17:43:12Z

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].

new file: test.txt

oddbookworm

Needs documentation, and there are a few issues I see offhand. But thanks for submitting! 🎉

oddbookworm · 2024-11-12T18:16:24Z

src_c/math.c

+    pgVector *vec = (pgVector *)self;
+
+    if (vec->coords[0] == 0.0 && vec->coords[1] == 0.0) {
+        return PyFloat_FromDouble(0.0);


I’d rather an exception get raised here. The zero vector is special and getting it’s angle makes no sense

ok, I thought it should be 0 because of the initial issue #3195, but I can change and raise an exception

@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.

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

link to standard I used

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.

I believe your example (of using the angle of a scaled vector instead of other way around) is uncommon and already bad to implement by itself because of floating point limits. The zero vector itself does not have to be reached for the angle to change because the design of the code encounters imprecision. If one were to actually hit the zero vector and face the special behavior, they wouldn't even be able to grow it back.

We do not have to assert any answer. Like python atan2, it can be left undocumented but still give a value instead of an error, even if its implementation specific (which we can prevent).
The point is, using an error is worse than an arbitrary but valid angle, for various reasons. Mathematically, this isn't even really bad.

How is raising an error worse than giving an undocumented result? At least an error gives the user some kind of feedback that maybe something went wrong rather than just silently accept that the user knows what they're doing (which is never a given)

First, mathematically it is okay to give e.g. 0.0 for the angle. All vectors with a magnitude of 0 are equivalent regardless of their direction, and polar coordinates needs the origin anyway. Imagine if the vector were to be implemented with magnitude + direction internally instead of components.

Having a possible error for getting or setting an angle is very annoying and would create rare crashes, cases that would most likely be better off having a defined and even undocumented angle for the zero vector.
One thing is that the angle is a property and is essentially an attribute. Nobody likes it when their attribute accesses raise a ValueError depending on the contents of the object.

Second, using a valid angle for it is superior as default behavior than raising an error. Imagine an unknowing user, implementing something that requires the angle between two points. When the two points are equivalent which is very rare, 99% of cases you want it to give any angle instead of blowing up and necessitating extra code checks.
My pymunk code would be exploding if its Vec2d gave an error for the angle of the zero vector, and much more error-prone.

Mathematically, anything is acceptable because it's undefined. 0 is just as valid as 180 degrees/pi radians. That doesn't mean we should. All polar vectors of magnitude 0 are equivalent, regardless of their angle. If you have a vector defined with r=0, then theta does not matter at all. There's absolutely no rule in math that it must be 0, because that's nonsense. It doesn't make sense at all to even talk about the angle of the zero vector.

No, I 100% disagree that having an undocumented return in one special case is better than actually requiring the user to be responsible. We are a graphics library (plus other stuff). That's not our job. If we were an actual game engine that abstracted that away, then sure. But we're not. Our own opinions should not make it into the library. Be explicit. Make the user validate that their thing makes sense. Also, I will not support setting angle using these methods. Read-only attribute. If you need to set the angle, do it properly using the components or one of the rotate methods.

"Superior" is subjective. IMO the superior thing to do would be to check if the two points are the same before even thinking about getting the angle between them. Give me one good use-case where not checking that beforehand is overly burdensome to the user. I think in most cases, it would actually allow for short-circuits in code. Location and destination are the same? Don't do anything then. Easy. Fast. Error-free.

Continuing the discussion on discord

oddbookworm · 2024-11-12T18:17:00Z

src_c/math.c

+static PyObject *
+vector_get_angle(pgVector *self, void *closure)
+{
+    pgVector *vec = (pgVector *)self;


Why this cast to the type it already is?

oddbookworm · 2024-11-12T18:17:44Z

src_c/math.c

+
+    double angle = atan2(vec->coords[1], vec->coords[0]) * 180.0 / M_PI;
+
+    if (angle > 180.0) {


Make constants instead of reusing the same hardcoded numbers

oddbookworm · 2024-11-12T18:19:02Z

src_c/math.c

+}
+
+static PyObject *
+vector_get_angle_rad(pgVector *self, void *closure)


Just reuse the function you already have instead of writing the same code twice. Since you already got the output in a form you like, just convert it from degrees to radians

oddbookworm · 2024-11-12T18:20:07Z

src_c/math.c

@@ -2585,6 +2631,8 @@ static PyMethodDef vector2_methods[] = {
 static PyGetSetDef vector2_getsets[] = {
    {"x", (getter)vector_getx, (setter)vector_setx, NULL, NULL},
    {"y", (getter)vector_gety, (setter)vector_sety, NULL, NULL},
+    {"angle", (getter)vector_get_angle, NULL, NULL, NULL},


Missing the docs component of these structs, which should be generated in the docs header, which is generated from the documentation updates you need to make

I have to make a doc in the file math.rst, right ? I'm not understanding everything about the doc yet. But I'm working on it !

…these properties

Gabriele and others added 5 commits October 16, 2024 14:29

Changes to be commited:

1932b82

new file: test.txt

Delete test.txt

49dbee5

Merge branch 'pygame-community:main' into main

ddf0c48

add angle and angle_rad property for vector2

131d956

syntax error correction

91fe678

AntoineMamou requested a review from a team as a code owner November 12, 2024 17:43

oddbookworm requested changes Nov 12, 2024

View reviewed changes

yunline added New API This pull request may need extra debate as it adds a new class or function to pygame math pygame.math labels Nov 13, 2024

code modification for properties angle and angle rad and new doc for …

faf77a8

…these properties

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

new angle and angle_rad property in vector2 #3222

new angle and angle_rad property in vector2 #3222

AntoineMamou commented Nov 12, 2024

oddbookworm left a comment •

edited

Loading

oddbookworm Nov 12, 2024

AntoineMamou Nov 13, 2024

aatle Nov 14, 2024

oddbookworm Nov 14, 2024 •

edited

Loading

oddbookworm Nov 14, 2024

aatle Nov 14, 2024

oddbookworm Nov 14, 2024

aatle Nov 14, 2024

oddbookworm Nov 14, 2024

aatle Nov 14, 2024

oddbookworm Nov 12, 2024

oddbookworm Nov 12, 2024

oddbookworm Nov 12, 2024

oddbookworm Nov 12, 2024

AntoineMamou Nov 13, 2024

oddbookworm Nov 13, 2024


		double angle = atan2(vec->coords[1], vec->coords[0]) * 180.0 / M_PI;

		if (angle > 180.0) {

new angle and angle_rad property in vector2 #3222

Are you sure you want to change the base?

new angle and angle_rad property in vector2 #3222

Conversation

AntoineMamou commented Nov 12, 2024

oddbookworm left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oddbookworm Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oddbookworm left a comment •

edited

Loading

oddbookworm Nov 14, 2024 •

edited

Loading