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

Remove unnecessary pixeloffset member of subsurface data #3014

Merged

Conversation

Starbuck5
Copy link
Member

@Starbuck5 Starbuck5 commented Jul 22, 2024

I was digging around our structs and found this member barely used anywhere. All it did was set the subsurf pixel pointer to the correct location when pgSurface_Prep is called. But the subsurf pixel pointer is set up to point at the right location in surface.subsurface already, and never changes from that position. So storing the offset and re-setting it occasionally is unnecessary effort.

How did I verify it was unnecessary?

  • No other bits of code set pixels to something else on a surface. (So if nothing is messing it up, no need to reset it)
  • I put in an assert in the previous prep code to fail if the current pixel pointer is different to the one it was being set to, and through the entire test suite that assertion held true.

This makes the subsurface code a tiny bit simpler and more memory efficient.

@Starbuck5 Starbuck5 requested a review from a team as a code owner July 22, 2024 07:34
@damusss damusss added the Code quality/robustness Code quality and resilience to changes label Jul 22, 2024
I was digging around our structs and found this member barely used anywhere. All it did was set the subsurf pixel pointer to the correct location when pgSurface_Prep is called. But the subsurf pixel pointer is set up to point at the right location in surface.subsurface already, and never changes from that position. So storing the offset and re-setting it occasionally is unnecessary effort.
@Starbuck5 Starbuck5 force-pushed the remove-unnecessary-subsurf-data branch from 107daff to 8b48d66 Compare July 23, 2024 07:12
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 👍
I can find no other usage of the pixeloffset member, as you say it just seems to be setting the surface->pixels pointer to the place it is already pointing at.

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! 🎈

pgSurface_LockBy((pgSurfaceObject *)data->owner, (PyObject *)surfobj);
surf->pixels = ((char *)owner->pixels) + data->pixeloffset;
Copy link
Member

Choose a reason for hiding this comment

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

The code being removed here is so weird, I wonder if there was any intention behind this... IG we will never know

Copy link
Member Author

Choose a reason for hiding this comment

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

I investigated a little further, it's been like this since surflock.c was created 24 years ago! pygame/pygame@45784b3

@ankith26 ankith26 merged commit 6a1560c into pygame-community:main Jul 28, 2024
25 checks passed
@ankith26 ankith26 added this to the 2.5.1 milestone Jul 28, 2024
@Starbuck5 Starbuck5 deleted the remove-unnecessary-subsurf-data branch July 28, 2024 20:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code quality/robustness Code quality and resilience to changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants