py-sdl2 icon indicating copy to clipboard operation
py-sdl2 copied to clipboard

[RFC] Could not draw a diagonal line on a surface with 1 bpp.

Open A-Wpro opened this issue 2 years ago • 2 comments

PR Description

Could not draw a diagonal line on a surface with 1 bpp. I had a problem to draw a diagonal line, probably I should have using byte-wises access. But for a bpp == 1. I think is easier to load a : pxbuf = ctypes.cast(rtarget.pixels, ctypes.POINTER(ctypes.c_uint8))

pxbuf was a int cause of : pxbuf = rtarget.pixels # byte-wise access. But later we ask for pxbuf[int(y1 * frac + x1)] = color

Now pxbuf for bpp == 1, pxbuf is a sdl2.audio.LP_c_ubyte. Btw, if someone know why we use sdl2.audio for graphical purpose, I would love to know.

Merge Checklist

  • [ ] the PR has been reviewed and all comments are resolved
  • [ ] all CI checks pass
  • [ ] (if applicable): the PR description includes the phrase closes #<issue-number> to automatically close an issue
  • [ ] (if applicable): bug fixes, new features, or API changes are documented in news.rst

A-Wpro avatar Jul 29 '22 13:07 A-Wpro

@A-Wpro Thanks for this! Would you be willing to add a unit test for this as well, just so I can be sure this doesn't get broken again in the future? Also, can you remove the whitespace change on line 159?

Now pxbuf for bpp == 1, pxbuf is a sdl2.audio.LP_c_ubyte. Btw, if someone know why we use sdl2.audio for graphical purpose, I would love to know.

Where are you seeing this? That sounds like a mistake somewhere in PySDL2, but I can't seem to find anything in the code that would cause it. Regardless, that return type is just an alias for ctypes.POINTER(ctypes.c_ubyte) so it shouldn't actually cause problems apart from some user confusion.

a-hurst avatar Jul 29 '22 17:07 a-hurst

@A-Wpro Thanks for this! Would you be willing to add a unit test for this as well, just so I can be sure this doesn't get broken again in the future?

Yes, I will do it asap, in which file should I write this unit test? sdl2ext_draw_test ?

Also, can you remove the whitespace change on line 159?

Done.

Where are you seeing this? That sounds like a mistake somewhere in PySDL2, but I can't seem to find anything in the code that would cause it. Regardless, that return type is just an alias for ctypes.POINTER(ctypes.c_ubyte) so it shouldn't actually cause problems apart from some user confusion.

When you try to print type right after pxbuf initialization, after line 158 for example, you can see sdl2.audio.LP_c_ubyte as output.

A-Wpro avatar Jul 29 '22 18:07 A-Wpro

@A-Wpro Sorry for the delay! Got busy with other things and haven't had a ton of time for PySDL2. To avoid adding new test images to the repository I wrote a different unit test for the bug, but I've applied that and your fix to the main branch here: https://github.com/py-sdl/py-sdl2/commit/3ee0ae4aa7f5501f544223fe1eeaf59a9ea57789

Thanks again for finding and fixing this!

a-hurst avatar Sep 02 '22 20:09 a-hurst