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

Improved blit() docs #2262

Merged
merged 20 commits into from
Nov 26, 2023

Conversation

itzpr3d4t0r
Copy link
Member

@itzpr3d4t0r itzpr3d4t0r commented Jun 19, 2023

Follows #2259. This one aims to improve the Surface.blit() function's docs since it's one of pygame's most used functions. I heavily encourage feedback as this is just a starting point. I tried to make docs easy to read and comprehend to a beginner but i may have rephrased some things wrong and/or made big mistakes.

Added:

  • More detailed explanations of the dest and area parameters.
  • More notes about when the blit will be skipped/positioning with not normalized rects.
  • Explaination of what the special_flags parameter does (something that was completely absent before) and a whole “Special Flags” section that explains the different blending flags and how they affect the blit operation.

Removed:

  • Mentions of specific versions of pygame where certain special flags were added.

OLD:
image
NEW:
image

The blit docs no longer list all blend flags and the blend flag descriptions have been moved to a separate section in the Surface object description:
image
image
This is to offload function docs of a repetitive blend flags description while providing a more in depth description of what each flag does.

@itzpr3d4t0r

This comment was marked as outdated.

@itzpr3d4t0r itzpr3d4t0r marked this pull request as ready for review June 19, 2023 13:57
@itzpr3d4t0r itzpr3d4t0r requested a review from a team as a code owner June 19, 2023 13:57
@itzpr3d4t0r

This comment was marked as outdated.

@itzpr3d4t0r itzpr3d4t0r changed the title Blit docs rewrite Improved blit() docs Jun 19, 2023
@Starbuck5
Copy link
Member

Here's an idea to chew on. What if the special flags explanation was a completely separate page, like the list of default colors in pygame.color. Right now the function is more daunting to look at because the description is so detailed, and a lot of that length is now coming from the special_flags documentation.

@itzpr3d4t0r
Copy link
Member Author

itzpr3d4t0r commented Jun 20, 2023

Here's an idea to chew on. What if the special flags explanation was a completely separate page, like the list of default colors in pygame.color. Right now the function is more daunting to look at because the description is so detailed, and a lot of that length is now coming from the special_flags documentation.

Yeah I have kind of the same feeling there, but for now i guess i could just move the flags to the surface class and mention that in the blit docs. While this may not be a long term solution, it should give me some more time to work on the Surface class, make that better, and move flags out of there. Though idk if it's strictly necessary to create a separate page as for now we only got a handful of flags so yeah, we shall see.

@Starbuck5
Copy link
Member

Starbuck5 commented Jun 20, 2023

Just for reference this is what I'm talking about: https://pyga.me/docs/ref/color_list.html. Whole separate page not in the modules top bar.

@itzpr3d4t0r

This comment was marked as resolved.

@itzpr3d4t0r

This comment was marked as outdated.

@itzpr3d4t0r

This comment was marked as outdated.

@itzpr3d4t0r
Copy link
Member Author

Here's an idea to chew on. What if the special flags explanation was a completely separate page, like the list of default colors in pygame.color.

Should this be implemented in this PR or a followup?

@Starbuck5
Copy link
Member

I just don't like that this PR seems to make the blit docs significantly longer, when part of the issue being brought up by OP was the docs being too long and overcomplicated for entry level users. I'm not sure how you'd like to tackle this in PRs.

@itzpr3d4t0r
Copy link
Member Author

I don't find these docs too overcomplicated, maybe a bit too long still but mind that writing docs isn't an easy task, as is demonstrated by how poorly our docs are written (my fblits docs as well). With this PR I think I added a lot more information that wasn't even there before, and organised it in a way in which if you're a beginner you can simply read the first 6 lines and you are fine but if you're an advanced user you can go more in-depth further down. Sorry but aside from it beeing a bit too long and the possibility of making a separate page for special flags i just don't see how these docs are bad. They are better structured, have a clear begginner at the top and advanced at the bottom structure, add meaningful information that you could've only gotten with errors/debug.

@itzpr3d4t0r

This comment was marked as outdated.

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

Went over this a bit again.

  • Weird to have a bulleted list followed by 4 paragraphs explaining in further detail. All the detail could be in the bulleted list of parameters in the first place.
  • Lets do a separate page for the flags, like color_list
  • The notes section needs to get restructured, because the notes all look very high importance, but they're not that important. I suggested having them as a paragraph of bits and bobs after the list of parameters.

Uses premultiplied alpha blending for faster and accurate results when
the color channels are already multiplied by the image alpha channel.
You should only use this blend mode if you previously premultiplied the Surface with
:meth:`premul_alpha()`. Check out :doc:`surface`'s :meth:`premul_alpha()` method for more
Copy link
Member

Choose a reason for hiding this comment

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

Rather than referencing a class and then referencing a method, you could just reference Surface.premul_alpha directly.

@Starbuck5
Copy link
Member

So now the list of parameters is not a bulleted list at all, which looks weird to me. The pygame functions usually have a bulleted list of parameters (draw) or paragraphs.

There is now a bulleted list of notes, which is better than before. But it's just weird that almost all the notes directly map to one parameter. Like why not have the information in the parameter descriptions?

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.

Looks good over all - I added a couple of small suggestions.

Thanks for the hard work on this 👍

@MyreMylar
Copy link
Member

Looks like this one needs a merge with main to clear the CircleCI fail.

itzpr3d4t0r and others added 20 commits November 21, 2023 22:49
added separators and slight correction

shortened blit docs

removed redundant note, slightly better default blend_flag description

removed special flags section in blit

added some highlights to the last note

better notes, separated param descriptions into separate sections

various rephrases to improve passive voice use and readability

added more info about the default blend flag. cleanup

moved the special flags section to Surface(),
addressed reviews.

changed the way sections are made

removed "-"s to align all cases

substantially changed special flags descriptions

added comment on how the alpha channel is treated

some other minor changes

some corrections
- moved noted to bullet list
- moved special flags list to a separate page
Co-authored-by: Dan Lawrence <danintheshed@gmail.com>
@itzpr3d4t0r itzpr3d4t0r dismissed oddbookworm’s stale review November 26, 2023 20:41

Outdated review and reviewer wasn't active for a long time

@itzpr3d4t0r itzpr3d4t0r merged commit da69f21 into pygame-community:main Nov 26, 2023
30 checks passed
@itzpr3d4t0r itzpr3d4t0r added this to the 2.4.0 milestone Nov 26, 2023
@itzpr3d4t0r itzpr3d4t0r deleted the blit_docs_enhancements branch December 2, 2023 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants