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

New functions : Vector2.angle and Vector2.angle_rad and the tests and documentation needed #3216

Closed
wants to merge 6 commits into from

Conversation

AntoineMamou
Copy link
Contributor

As explains in this issue : #3195, I added the two functions angle and angle_rad in the module Vector2.
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].

I also created tests and documentation for these two functions

@AntoineMamou AntoineMamou requested a review from a team as a code owner November 7, 2024 13:06
Copy link
Member

@oddbookworm oddbookworm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’d rather see these implemented as read-only attributes vs methods. There’s also a discussion to be had about how it should work for Vector3 because we should have parity in features between the two as much as possible

@AntoineMamou
Copy link
Contributor Author

Ok, I'll try to modify my code and create attributes and not methods and wait for how the Vector3 features should work.

@@ -34,6 +34,8 @@
#define DOC_MATH_VECTOR2_ANGLETO "angle_to(Vector2, /) -> float\ncalculates the angle to a given vector in degrees."
#define DOC_MATH_VECTOR2_ASPOLAR "as_polar() -> (r, phi)\nreturns a tuple with radial distance and azimuthal angle."
#define DOC_MATH_VECTOR2_FROMPOLAR "from_polar((r, phi), /) -> None\nSets x and y from a polar coordinates tuple."
#define DOC_MATH_VECTOR2_ANGLERAD "angle_rad() -> float\nreturns the angle of the vector in radians relative to the positive X-axis."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that this macros have been created, but the doc RST file has not been edited, making me believe you added them manually. To add a new feature you should edit the math.rst found somewhere under the docs folder and when you added proper documentation run the command py dev.py docs (or py -m buildconfig docs) that will generate them automatically.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I'm new so I didn't know that. I'm going to fix that. But I don't understand what this macros is doing if it's not to create a doc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, with @oddbookworm comment. Do you think I should update my code and make a new PR for this issue or not ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AntoineMamou for the docs header, did you not read the first line of the file? Where it says it's autogenerated from the actual docs?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which part of my comment do you want to push off to another PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I didn't see it when I make the PR

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was just thinking about the attributes angle and angle_rad

else if (angle_deg <= -180) {
angle_deg += 360;
}
return Py_BuildValue("d", angle_deg);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why have you used Py_BuildValue here and PyFloat_FromDouble in the other function? I feel like they should both use the latter. also, I feel like there are a few unnecessary declarations that could be avoided reducing a bit the lines, if that doesn't decrease readability too much (but formatting the code might bring it back)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes you are right. Because I begin in this project, I saw that there is these two function and I try it both. Since it works, I didn't see the problem but if both should use PyFloat_FromDouble, it's ok.

@yunline 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 9, 2024
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these changes relevant to this pr?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello,

No, not at all. This was a mistake I made because I was working on an other issue at that time. By the way, this PR should be close I think because it's this PR that is link to the issue of .angle and .angle_rad : #3222

@ankith26
Copy link
Member

Closing this PR in favour of the newer PR #3222 made by OP

@ankith26 ankith26 closed this Nov 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
math pygame.math New API This pull request may need extra debate as it adds a new class or function to pygame
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants