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

Implementation of pygame.(F)Rect.rel_center #3089

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

bilhox
Copy link
Contributor

@bilhox bilhox commented Aug 31, 2024

Implementation of #1242 according to MyreMylar name proposal.

While I'm bringing this feature to a potential release, what's your opinion on rel_centerx and rel_centery ?

Closes #1242 .

Tests + documentation added.

@bilhox bilhox requested a review from a team as a code owner August 31, 2024 22:11
@damusss
Copy link
Contributor

damusss commented Sep 1, 2024

I'm not sure about the x and y variants, as
(r.w/2+N, r.h/2+M) is shorter than (r.rel_centerx+N, r.rel_centery+M) (where N and M are arbitrary numbers not to make the expression useless), but I like rel_center, it makes sense.

@damusss damusss added New API This pull request may need extra debate as it adds a new class or function to pygame rect pygame.rect labels Sep 1, 2024
@@ -2804,6 +2814,33 @@ RectExport_getcenter(RectObject *self, void *closure)
self->r.y + (self->r.h / 2));
}

/*center*/
Copy link
Contributor

Choose a reason for hiding this comment

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

relcenter

Copy link
Contributor

Choose a reason for hiding this comment

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

you brought up a point, should it be rel_center or relcenter

Copy link
Member

Choose a reason for hiding this comment

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

I kind of hate it, but probably relcenter for consistency with the other pre-existing rect attributes.

@aatle
Copy link
Contributor

aatle commented Sep 2, 2024

rel_centerx and rel_centery are probably less readable than width/2, and less useful compared to centerx.

It seems to me like most uses would be related to Surfaces.
I think more concrete use cases for this feature need to be listed.

@bilhox
Copy link
Contributor Author

bilhox commented Sep 3, 2024

rel_centerx and rel_centery are probably less readable than width/2, and less useful compared to centerx.

It seems to me like most uses would be related to Surfaces. I think more concrete use cases for this feature need to be listed.

With the argument you gave with damus, indeed rel_centerx and rel_centery is not needed.
Except that, I think the person who opened the issue gave a fairly good usecase, and I personally support the feature for the same reason.

@aatle
Copy link
Contributor

aatle commented Sep 4, 2024

I agree that the feature is logical and should probably be implemented, but I wonder if there are more use cases than the one given:

I often find myself evaluating (rect.w / 2, rect.h / 2) to find the middle of a rect, relative to itself. This is useful when drawing a circle onto a surface for a sprite, for example.

If I understand this case correctly, one could use surface.get_rect().center instead, right? For rects from surfaces, the position is (0,0) since only size is relevant, so rel_center is not needed.
I personally don't see any use cases in my own project codebase, but that's probably because I use Vector2 for this stuff.

docs/reST/ref/rect.rst Outdated Show resolved Hide resolved
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.

Can we rename it relcenter ? Then I think we can get this in.

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

Successfully merging this pull request may close these issues.

Rect.size_rect (2429)
5 participants