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.

Sign up for GitHub

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

Jump to bottom

added Vector2.from_polar and Vector3.from_spherical classmethods #2141

Draft
wants to merge 13 commits into
base: main
Choose a base branch
from
Draft

added Vector2.from_polar and Vector3.from_spherical classmethods #2141

wants to merge 13 commits into from

Conversation

oddbookworm
Copy link
Member

@oddbookworm oddbookworm requested a review from a team as a code owner April 30, 2023 19:12
@oddbookworm oddbookworm linked an issue Apr 30, 2023 that may be closed by this pull request
Implement a Vector2.from_polar class creation method #2139
Open
@oddbookworm oddbookworm added the math pygame.math label Apr 30, 2023
@Starbuck5
Copy link
Member

I'm surprised upstream chose to make the same name method have two different signatures in two different scenarios.

Is there a reason that Vector2().from_polar shouldn't return itself? That way it could be:

Vector2.from_polar((r, phi)) -> Vector2
Vector2().from_polar((r, phi)) -> Vector2

And maybe that would have simpler code? The implementation they chose works and is clever, but is a lot of complexity to add.

@oddbookworm
Copy link
Member Author

I don't see why we can't have the same signature for both. I'll have to dedicate more time to rewriting it, so it might have to wait until this weekend. I have at least found a way to clean up the stub for the current implementation and I'll push a commit for that

@ankith26 ankith26 added the New API This pull request may need extra debate as it adds a new class or function to pygame label May 25, 2023
@MyreMylar MyreMylar closed this Oct 8, 2023
@MyreMylar MyreMylar reopened this Oct 8, 2023
@MyreMylar
Copy link
Member

Just toggling this to re-run the CI as I think it is ready for review.

MyreMylar
MyreMylar approved these changes Oct 8, 2023
View reviewed changes
Copy link
Member

@MyreMylar MyreMylar left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Good compatibility with upstream fixes.

@MyreMylar
Copy link
Member

Looks like this will need to merge with main to clear the CircleCI failure.

@MyreMylar MyreMylar requested a review from Starbuck5 October 15, 2023 10:29
@Starbuck5
Copy link
Member

It would be good to get this rebased and have the original author of this be added as a coauthor to the commit.

MyreMylar and others added 2 commits November 26, 2023 12:37
@MyreMylar
Merge branch 'main' into Vector2.from_polar
f7da8bf 
# Conflicts:
#	buildconfig/stubs/pygame/math.pyi
#	docs/reST/ref/math.rst
#	src_c/doc/math_doc.h
@MyreMylar @joaquin30
Add coauthor
f4586b7 
Co-authored-by: joaquin30 <jpinoz@tuta.io>
@MyreMylar MyreMylar added this to the 2.4.0 milestone Nov 26, 2023
ankith26
ankith26 reviewed Dec 4, 2023
View reviewed changes
src_c/math.c Outdated Show resolved Hide resolved
@Starbuck5 Starbuck5 modified the milestones: 2.4.0, 2.5.0 Dec 25, 2023
@itzpr3d4t0r
Copy link
Member

Why not make these functions METH_FASTCALL? Instead of doing it in a separate PR doubling review time we should implement it here i think.

@oddbookworm oddbookworm requested a review from itzpr3d4t0r April 27, 2024 15:37
MyreMylar
MyreMylar reviewed Apr 28, 2024
View reviewed changes
src_c/math.c Outdated Show resolved Hide resolved
@oddbookworm @MyreMylar
Fix some grammar
69743f2 
Co-authored-by: Dan Lawrence <danintheshed@gmail.com>
MyreMylar
MyreMylar approved these changes Apr 28, 2024
View reviewed changes
Copy link
Member

@MyreMylar MyreMylar left a comment

Choose a reason for hiding this comment

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

OK, still looks good to me after recent improvements 👍

I have little idea of how popular usage of these coordinate systems actually is among pygame-ce users but improving compatibility with upstream where it is doing generally useful things is a plus.

pmp-p
pmp-p requested changes May 2, 2024
View reviewed changes
src_c/math.c
@@ -33,6 +33,7 @@
#define NO_PYGAME_C_API
#include "doc/math_doc.h"

#include "base.c"
Copy link
Member

Choose a reason for hiding this comment

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

i don't think this is a good idea

src_c/base.c
@@ -2296,8 +2366,10 @@ MODINIT_DEFINE(base)
c_api[26] = pg_TwoDoublesFromFastcallArgs;
Copy link
Member

Choose a reason for hiding this comment

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

but pg_TwoDoublesFromFastcallArgs is declared a static inline.

@Starbuck5 Starbuck5 modified the milestones: 2.5.0, 2.5.1 Jun 2, 2024
@Starbuck5 Starbuck5 modified the milestones: 2.5.1, 2.5.2 Aug 7, 2024
@Starbuck5 Starbuck5 removed this from the 2.5.2 milestone Oct 13, 2024
aatle
aatle reviewed Oct 19, 2024
View reviewed changes
buildconfig/stubs/pygame/math.pyi
Comment on lines +222 to +224
@overload # type: ignore
@classmethod
def from_polar(cls, value: Tuple[float, float], /) -> Vector2: ...
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
@overload # type: ignore
@classmethod
def from_polar(cls, value: Tuple[float, float], /) -> Vector2: ...
@overload
def from_polar(value: Tuple[float, float], /) -> Vector2: ... # type: ignore

It looks like the hack for typing fails for the class call. A bit of experimentation gives a different fix, in mypy.
This should also be applied to from_spherical.

P.S.: The new Point and SequenceLike types should be integrated into the PR if it is going to be merged.

@ankith26 ankith26 marked this pull request as draft November 2, 2024 09:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ankith26 ankith26 ankith26 left review comments

@aatle aatle aatle left review comments

@pmp-p pmp-p pmp-p requested changes

@MyreMylar MyreMylar MyreMylar approved these changes

@Starbuck5 Starbuck5 Awaiting requested review from Starbuck5

@itzpr3d4t0r itzpr3d4t0r Awaiting requested review from itzpr3d4t0r

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement a Vector2.from_polar class creation method
7 participants
@oddbookworm @Starbuck5 @MyreMylar @itzpr3d4t0r @pmp-p @ankith26 @aatle