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

Allow degenerate circles (radius = 0) #2913

Merged
merged 5 commits into from
Jun 26, 2024

Conversation

damusss
Copy link
Member

@damusss damusss commented Jun 6, 2024

note: I've modified the circle init error message to include the radius, as it was very unclear before. If you don't want degenerate circles, this modification should still be added IMHO, so let me know.

I've slightly modified the code, the tests and the docs to allow degenerate circles to exist.
I've run through every line where the radius attribute is used and I checked for divisions with the radius but they were not present.
Why?

  • You can create 'degenerate' rects by passing 0 as width and height, so for consistency I think you should be able to create degenerate circles aswell
  • It will unintentionally be treated like a point when checking for collisions, and virtual attributes will be automatically affected
  • degenerate circles are still considered circles in analitic geometry so I would consider them colliders
  • If degenerate rects are valid colliders/shapes, degenerate circles should aswell

Tell me if you'd like more tests or if I missed something.
Of course you can not agree with allowing degenerate circles, I made this pull request so that if you think they should be there, you'll find a PR ready for it.

@damusss damusss requested a review from a team as a code owner June 6, 2024 18:20
@oddbookworm oddbookworm added the geometry pygame.geometry label Jun 7, 2024
@oddbookworm
Copy link
Member

I'm requesting a review from @itzpr3d4t0r as the resident pygame-geometry expert

@oddbookworm oddbookworm requested a review from itzpr3d4t0r June 7, 2024 23:55
src_c/circle.c Outdated Show resolved Hide resolved
@oddbookworm
Copy link
Member

oddbookworm commented Jun 8, 2024

I tested this locally with this test script, and it seems to work exactly as I was expecting it to

import pygame
import pygame.geometry

screen = pygame.display.set_mode((50, 50), pygame.SCALED)
stationary_circle = pygame.geometry.Circle(*screen.get_rect().center, 0)

radius = 10
moving_circle = pygame.geometry.Circle(screen.get_rect().center, radius)

running = True
while running:
    for event in pygame.event.get():
        if event.type == pygame.QUIT:
            running = False

        if event.type == pygame.MOUSEWHEEL:
            radius = radius + event.y
            if radius < 0:
                radius = 0

    pos = pygame.mouse.get_pos()
    moving_circle.center = pos
    moving_circle.radius = radius

    color = "green"
    if moving_circle.collidecircle(stationary_circle):
        color = "red"

    screen.fill("black")
    screen.set_at(stationary_circle.center, "blue")
    if radius:
        pygame.draw.aacircle(screen, color, moving_circle.center, moving_circle.radius, 1)
    else:
        screen.set_at(moving_circle.center, color)
    pygame.display.flip()

Copy link
Member

@itzpr3d4t0r itzpr3d4t0r left a comment

Choose a reason for hiding this comment

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

I'm fine with this, still can't see where it would be useful but i guess it's no harm.

src_c/circle.c Outdated Show resolved Hide resolved
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.

Alright, with the clarified error messages, I'm happy with this. I don't see much use here that isn't already covered by a point, but we allow 0-sized Rects, so I'm not going to object here

Copy link
Member

@ankith26 ankith26 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the PR 🥳

@ankith26 ankith26 added this to the 2.5.1 milestone Jun 26, 2024
@ankith26 ankith26 merged commit 717c861 into pygame-community:main Jun 26, 2024
39 checks passed
@damusss damusss deleted the circle-zero-radius branch July 23, 2024 10:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
geometry pygame.geometry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants