Improve, fix and clarify Surface docs #3455
Conversation
buildconfig/stubs/pygame/surface.pyi
Outdated
| @@ -801,13 +797,11 @@ class Surface: | |||
| ``SDL_video.h`` | |||
|
|
|||
| :: | |||
|
|
|||
| SRCALPHA 0x00010000 # Blit uses source alpha blending | |||
There was a problem hiding this comment.
I'm not sure this moving here is the right call
There was a problem hiding this comment.
Why would SRCALPHA be private if you use it often when constructing a surface?
There was a problem hiding this comment.
Latest commit should address this I believe
|
I've addressed most of @oddbookworm 's comments and I also fixed get_alpha again. I took the liberty of moving locking related methods at the bottom, and I would like the liberty of moving the width, height and size properties at the top. |
…A, improve flags docs
| the format will best match the display Surface if one was initialized, otherwise | ||
| the most suitable format is chosen for the current platform. | ||
|
|
||
| One way to control the pixel format/charateristics is providing a bitmask of flags: |
| the most suitable format is chosen for the current platform. | ||
|
|
||
| One way to control the pixel format/charateristics is providing a bitmask of flags: | ||
| * ``SRCALPHA``: The Surface will include an alpha channel. |
There was a problem hiding this comment.
I think this should mention "per-pixel alpha", mention transparency in some way.
It is not really stated that SRCALPHA is how to initialize a surface with transparency; I often see people using .convert_alpha() immediately after Surface creation instead.
|
|
||
| One way to control the pixel format/charateristics is providing a bitmask of flags: | ||
| * ``SRCALPHA``: The Surface will include an alpha channel. | ||
| * ``HWSURFACE``: Creates the image in video memory but is obsolete in pygame 2. |
There was a problem hiding this comment.
IMO the wording can be improved or rearranged about "obsolete": the original description with (obsolete in pygame 2) creates the image in video memory is more clear in my opinion.
| * ``SRCALPHA``: The Surface will include an alpha channel. | ||
| * ``HWSURFACE``: Creates the image in video memory but is obsolete in pygame 2. | ||
|
|
||
| Otherwise The pixel format can be controlled in different ways: |
There was a problem hiding this comment.
The "Otherwise" is not appropriate because this is not mutually exclusive with the flags. Actually, the two options listed here are mutually exclusive, which is not really mentioned.
(also, there is a random capitalization of "The")
Perhaps this section could be merged with the flags section, as all describe how to initialize a Surface with characteristics and formats.
| * ``HWSURFACE``: Creates the image in video memory but is obsolete in pygame 2. | ||
|
|
||
| Otherwise The pixel format can be controlled in different ways: | ||
| * Providing another ``Surface`` after the flags argument. In this case that Surface's |
There was a problem hiding this comment.
Wording nitpick: I think the "In this case" is unnecessary.
Additionally, I noticed that the per-pixel alpha (SRCALPHA) flag does not seem to be used by the new surface. If that is accurate, it could be mentioned.
| method. Changing the pixels referenced by either the original Surface or | ||
| the subsurface will have an effect on both. | ||
|
|
||
| There is support for pixel access for Surfaces but it is slow on hardware |
There was a problem hiding this comment.
A note, it was hardly explained what "hardware acceleration" and "hardware surface" are in the original docs, and many have thought that hardware acceleration meant GPU.
|
|
||
| There is support for pixel access for Surfaces but it is slow on hardware | ||
| Surfaces. You can use the :meth:`get_at()` and :meth:`set_at()` functions | ||
| but while they are fine for simple access will be slow when doing pixel work |
There was a problem hiding this comment.
missing a subject between "access" and "will" (grammar)
| explained in the :class:`Surface` constructor. Flags can also be | ||
| provided, for example ``SRCALPHA`` will request an alpha channel. | ||
| * Passing the bitmasks of the pixels and flags. Note that the bit depth | ||
| could be calculated wrong, therefore it is not adviced to use this |
| * ``HWACCEL``: Blit uses hardware acceleration | ||
| * ``SRCCOLORKEY``: Blit uses a source color key | ||
| * ``RLEACCELOK``: Private flag | ||
| * ``RLEACCEL``: Surface is RLE encoded |
There was a problem hiding this comment.
"RLE encoded" does not make sense ("run-length encoding encoded")
| * ``RLEACCEL``: Surface is RLE encoded | |
| * ``RLEACCEL``: Surface has run-length encoding |
| :meth:`Surface.premul_alpha` for more. | ||
| * Global alpha: A single alpha values controlling all the pixels. | ||
| * Colorkey: A color flagged to be considered transparent in operations. | ||
|
|
There was a problem hiding this comment.
I think it would be useful to have information on which is generally fastest on what platforms.
| Unlike the :meth:`convert()` method, the pixel format for the new | ||
| surface will not be exactly the same as the display surface, but it will | ||
| be optimized for fast alpha blitting to it. | ||
| Creates a new copy of the Surface with a different format that contains |
There was a problem hiding this comment.
The original wording was "a new copy of the surface with the desired pixel format", which is more accurate, because the Surface could already be in the most ideal format.
|
Great idea for an improvement, Damus. It may be wise to split this out into smaller PRs to get it through, there's already been a ton of conversation and probably will be a bunch more, this is that kind of PR. But parts of this likely could have gotten through already, like you doing surface -> Surface all over the place. |
Some parts of the docs had smaller errors. Some parts were basically lying. Big methods and the constructor where very convoluted throwing information randomly. I didn't change everything, but rephrased things. Also, in the constructor and in convert, I thought that the docs weren't even trying to explain how the arguments worked, so I looked at the actual code and refined them. I'm open for suggestions of course.