-
-
Notifications
You must be signed in to change notification settings - Fork 181
Implemented the Line.project #3402
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
base: main
Are you sure you want to change the base?
Conversation
Not my hideous drawings used as official documentation 😭 |
(discussion) Personal API opinion: the positional/keyword clamp argument should be made keyword-only because positional boolean arguments are bad - see https://docs.astral.sh/ruff/rules/boolean-type-hint-positional-argument/ for reasoning. |
I tried to satisfy every request I got here but I made one choice that might should be evaluated. def project(self, point: tuple[float, float], / clamp: bool = False) -> tuple[float, float]: ... aka saying that the first arg is positional only but I want to get this approved I guess |
@@ -197,3 +197,6 @@ class Line: | |||
def scale_ip(self, factor_and_origin: Point, /) -> None: ... | |||
def flip_ab(self) -> Line: ... | |||
def flip_ab_ip(self) -> None: ... | |||
def project( | |||
self, point: tuple[float, float], clamp: bool = False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I wasn't clear. Only the return type should be changed to tuple[float, float]
; the argument should stay Point
.
The reason is that while the input argument should be as broad and abstract of a type as possible, the return type needs to be more concrete and narrow to allow easy usage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now when talking about this, not related to this PR, but maybe we should rename the typing.(Int)Point to typing.(Int)PointLike, just to be the same as the rest of typing module, and a little bit more clear to avoid stuff like that in the future. On the other hand, it is hard to do that now since the typing module is exposed in pygame API, so maybe in pg3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would normally agree with increasing naming consistency.
However, Point
is not necessarily like the other types in that it is not a union of similar, acceptable types. It is also not a raw protocol like SequenceLike
.
The main reason of course is verbosity; originally, CoordinateLike
would be too long which might be the original reason, but PointLike
is still long too.
clarify the docs for the Line.project methdo Co-authored-by: aatle <168398276+aatle@users.noreply.github.com>
grammar fix in the docs for the Line.project method Co-authored-by: aatle <168398276+aatle@users.noreply.github.com>
small update to the docs for Line.project Co-authored-by: aatle <168398276+aatle@users.noreply.github.com>
Can't approve / disapprove but what i asked for was addressed in one form or another.
I may have gone a little overboard, but here's my test script. """
https://github.com/pygame-community/pygame-ce/pull/3402 testing
"""
import math
import pygame
import pygame.geometry
pygame.init()
screen_width, screen_height = 720, 500
screen = pygame.display.set_mode((screen_width, screen_height))
pygame.display.set_caption("Line.project example")
clock = pygame.Clock()
font = pygame.font.SysFont("Arial", 24)
line = pygame.geometry.Line((screen_width/2-20,250), (screen_width/2+20,250))
instructions = """
Use WASD/arrows to adjust the two points, respectively.
The position of the mouse is projected onto the blue line.
Toggle clamp by pressing C.
"""
instructions_surf = font.render(instructions.strip(), True, "black")
speed = 250
clamp = False
while True:
dt = clock.tick(144) / 1000
for event in pygame.event.get():
if event.type == pygame.QUIT or (event.type == pygame.KEYDOWN and event.key == pygame.K_ESCAPE):
pygame.quit()
raise SystemExit
if event.type == pygame.KEYDOWN and event.key == pygame.K_c:
clamp = not clamp
keys = pygame.key.get_pressed()
if keys[pygame.K_w]:
line.ay -= speed * dt
if keys[pygame.K_a]:
line.ax -= speed * dt
if keys[pygame.K_s]:
line.ay += speed * dt
if keys[pygame.K_d]:
line.ax += speed * dt
if keys[pygame.K_UP]:
line.by -= speed * dt
if keys[pygame.K_LEFT]:
line.bx -= speed * dt
if keys[pygame.K_DOWN]:
line.by += speed * dt
if keys[pygame.K_RIGHT]:
line.bx += speed * dt
screen.fill("purple")
screen.blit(instructions_surf, (10, 10))
pygame.draw.line(screen, "blue", line.a, line.b, width=3)
pygame.draw.circle(screen, "blue", line.a, 5)
pygame.draw.circle(screen, "blue", line.b, 5)
mouse = pygame.mouse.get_pos()
proj_point = line.project(mouse, clamp=clamp)
if not math.isnan(proj_point[0]) and not math.isnan(proj_point[1]):
pygame.draw.line(screen, "red", mouse, proj_point)
pygame.display.flip() |
I ran into problems with this function returning NaNs (not a number) in some scenarios, pygame.draw.line freezes indefinitely when it gets one of them, opened an issue for that: #3412. Is there a way this could not return NaN? Like if it's going to do that, return the original coordinate instead? IDK about implementation or whether that makes sense. Another option would be to add a warning in the docs that this can return NaN, because I think people will want to do stuff with this value and having it be NaN will mess those calculations up.
On another subject, the current implementation of fastcall and keywords has several issues. Yes, the code that should work works, but also code that shouldn't work works. For example, this line completely works: I checked, there are 0 instances of a normal function usage of fastcall and keywords in the pygame-ce codebase, so this is not a standard this PR should be held to. Manual kwarg implementations are error prone and will have inconsistent error messaging relative to PyArg_ParseTupleAndKeywords. I think this PR should either be positional only or should do keywords the normal way. |
Surface.get_rect and get_frect are one example. Line 292 in be3d05a
|
Yeah, those aren't normal functions. Normal functions use the kwargs directly, surface.get_rect and rect.move_to (the other example) both just forward the keywords to another function. AKA they don't process them themselves. |
So I can again rewrite it again to not be a fastcall I agree that implementing kwargs parsing for fastcall would be super error prone. I will also look into why it returns NaN and fix it.
I can look but I dont think so. What problems do you have with the art? I can try to improve it |
Just to be clear, I wasn't referring to the graphics you put in the docs, I was referring to the NaN problem. "Prior art" meaning how have others dealt with this. |
Okay, so I want to discuss how we are going to handle the NaN problem. The issue arises when the line has a length of zero, meaning both points A and B are at the same position. Mathematically, there is no way to project the point. If clamping is on, I can technically say the point should be equal to A or B, but if clamping is off, I don't know what it would return. The only solution I see as a possibility is to throw a ZeroDivisionError and mention it in the docs |
Yes, if clamping is on you can just return A (not any different from B) and that makes sense. If the clamping is off, we can't lie. We should throw a similar error to what vector2 does when normalizing a zero-length vector which is to throw a ValueError. ZeroDivisionError makes it seem like we have a bug, also because users don't know there's any division, value error seems consistent. and of course both paths have to be mentioned in the docs, yeah. |
I need help I dont understand why the github test aren't passing |
Do you need those extra imports in lines 4/5. Because I can't see them anywhere else |
yeah I will delete them my editor added them I didnt need them thanks I didnt know that can cause an issue |
all new requested changes anything else? |
This PR should add the feature requested in #3019
I added the
project()
method to thepygame.geometry.Line
object I added the docs and test for the method