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

vector2 support for Surface.get_at and set_at #2111

Merged
merged 20 commits into from
Jul 22, 2023

Conversation

ScriptLineStudios
Copy link
Member

the ability to use vector2's to specify coordinates for surface set_ats and get_ats

@ScriptLineStudios ScriptLineStudios requested a review from a team as a code owner April 10, 2023 14:36
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 think you should edit the typestub info in buildconfig/stubs/pgyame/surface.pyi so that linters don't throw a fit when using a Vector2 in these methods. Also, adding documentation for this new functionality would be a good idea IMO. Maybe make sure to mention that any floating point values will be truncated, not rounded

@novialriptide
Copy link
Member

novialriptide commented Apr 15, 2023

@oddbookworm has mentioned that linters would complain about introducing Vector2s to get_at() and set_at(), this got me thinking that I'm not entirely sure if this PR should go through as stuff like this becomes valid.

# this pr
import pygame
a = pygame.Surface((50,50))
a.set_at(pygame.Vector2(50,50.5), (25,25,25))
a.set_at((50,50.5), (25,25,25))

whereas before, this would've thrown an error

# pygame-ce 2.2.1
import pygame
a = pygame.Surface((50,50))
a.set_at((50,50.5), (25,25,25))
TypeError: 'float' object cannot be interpreted as an integer

If we favor introducing this, we can mention in the docs that we now support float values for get_at() and set_at().

@narilee2006
Copy link
Contributor

I agree

@Starbuck5
Copy link
Member

this got me thinking that I'm not entirely sure if this PR should go through as stuff like this becomes valid

Many things in pygame support floats now, and didn't always. Transform.scale used to need integer dimensions, for example. I think it's perfectly fine to have integer things accept floats and just truncate them.

But you're right, this can be documented and hinted as support for a 2-sequence of floats.

@Starbuck5 Starbuck5 added the Surface pygame.Surface label Apr 18, 2023
src_c/surface.c Outdated Show resolved Hide resolved
src_c/surface.c Outdated Show resolved Hide resolved
@Starbuck5
Copy link
Member

Starbuck5 commented Apr 30, 2023

Also I'm curious what kind of performance impact this has on get_at/set_at.

docs/reST/ref/surface.rst Outdated Show resolved Hide resolved
docs/reST/ref/surface.rst Outdated Show resolved Hide resolved
src_c/surface.c Outdated Show resolved Hide resolved
@Starbuck5
Copy link
Member

I did some performance testing, and set_at with this PR is 1.5% faster now! (This is pretty low, it could be a fluke)
But this means that there isn't a big performance regression here we're not noticing.

I'm not sure whether pg_TwoIntFromObj or that PyArg_ParseTuple is more expensive, but METH_O is less expensive than METH_VARARGS so maybe that helps it.

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.

LGTM! (After a quick merge conflict fix)

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.

The docs, tests and stubs for get_at_mapped aren't updated yet

@ankith26 ankith26 added this to the 2.3.1 milestone Jul 21, 2023
This reverts commit 4fae8fc.
Copy link
Member

@Starbuck5 Starbuck5 left a comment

Choose a reason for hiding this comment

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

Looks good!

Thanks for working on this. Getting stuff supporting floats when appropriate is a good initiative throughout the pygame-ce API.

@Starbuck5 Starbuck5 dismissed ankith26’s stale review July 22, 2023 09:05

Changes were made

@Starbuck5 Starbuck5 merged commit 4920fcf into pygame-community:main Jul 22, 2023
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Surface pygame.Surface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants