Skip to content

Fix stubs for buffer types #3398

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

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

Conversation

aatle
Copy link
Contributor

@aatle aatle commented Apr 9, 2025

Python 3.12 introduced the python-visible buffer protocol with __buffer__ and __release_buffer__. It also defined collections.abc.Buffer.
This PR allows static type checkers to recognize some pygame types as buffers, helpful for Surface buffer operations with other libraries like numpy.

Stub changes:

  • Add __buffer__() to Sound, BufferProxy, Mask, Color, PixelArray. Also add __release_buffer__() to first three.
  • Replace internal image._BufferLike with typing_extensions.Buffer. Replace Sound.__init__() Any and numpy.ndarray overloads with one typing_extensions.Buffer overload.
  • Minor change to type of BufferProxy.write() buffer argument

@aatle aatle requested a review from a team as a code owner April 9, 2025 01:25
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.

This PR is failing stubtest, it is complaining that __buffer__ and __release_buffer__ are not actually present at runtime

@aatle
Copy link
Contributor Author

aatle commented Apr 9, 2025

Yes, I'll probably have to add a regex to mypy_allow_list.txt.
But I am confused why stubtest may sometimes give different results. (Possibly python version?) It failed for one environment with python 3.10.
And the pygame-ce main branch fails stubtest locally on my Windows computer due to AbstractGroup.__class_getitem__ not having positional-only arguments (python 3.13.0).

@ankith26
Copy link
Member

It is probably dependent on the python version, I would suggest adding a python version check in the code instead of adding it to mypy allow list

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.

I'm not exactly clear what is the logic behind some places having __release_buffer__ and some places not having it. In my understanding from the docs, __release_buffer__ is only needed when some cleanup must be done.

Can you please clarify, and also maybe document it in the code/stubs with comments

@aatle
Copy link
Contributor Author

aatle commented Apr 13, 2025

I didn't know why some had __release_buffer__ either. The stubs directly reflect the PyBufferProcs of the code.
After some research, I can give some reasons why:

  • Sound: if requested as multi-dimensional buffer, it has to allocate a new buffer, which it must release

  • BufferProxy: always allocates a new view object, which must be deallocated

  • Mask: allocates a buffer for use (which it caches until buffer is released)

  • Color: gives its stored internal data for the view, requiring no allocation. May only be read-only buffer.

  • PixelArray: gives itself for use in the view. It itself references a surface's pixels, which it cannot release.

I won't document this in the code nor in stubs with comments, it's not of this PR's concern to explain C code internal implementations.

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 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants