pygame-community / pygame-ce

🐍🎮 pygame - Community Edition is a FOSS Python library for multimedia applications (like games). Built on top of the excellent SDL library.
https://pyga.me
764 stars 120 forks source link

`Surface.fill()` `rect` argument: negative coordinates incorrectly set to 0 #2938

Closed aatle closed 23 hours ago

aatle commented 1 week ago

Environment:

pygame.print_debug_info() ``` Platform: Linux-6.5.0-1022-gcp-x86_64-with-glibc2.39 System: Linux System Version: #24~22.04.1-Ubuntu SMP Tue May 28 16:34:13 UTC 2024 Processor: Architecture: Bits: 64bit Linkage: ELF Python: CPython 3.11.9 (main, Apr 2 2024, 08:25:04) [GCC 13.2.0] pygame version: 2.5.0 SDL versions: Linked: 2.30.3 Compiled: 2.30.3 SDL Mixer versions: Linked: 2.8.0 Compiled: 2.8.0 SDL Font versions: Linked: 2.22.0 Compiled: 2.22.0 SDL Image versions: Linked: 2.8.2 Compiled: 2.8.2 Freetype versions: Linked: 2.13.2 Compiled: 2.13.2 Display Driver: x11 ( xwayland == False ) Mixer Driver: Mixer Not Initialized ```

Current behavior:

Surface.fill incorrectly handles negative coordinates for the rect argument if the rect partially overlaps with the surface. Each negative coordinate (x, y) will be set to 0 before drawing without changing size, which basically changes the intended position of the rect. Unless, the rect does not overlap the surface at all, in which case nothing is drawn which is correct. For example, the rect (-50, 20, 80, 80) would be drawn as (0, 20, 80, 80) instead of (0, 20, 30, 80), much further right than expected. This affects the top and left edges.

pygame.draw.rect does not suffer from this bug. Filling a rect going off the right or bottom edges works normally. This bug seems to be unrelated to Surface clipping area.

Expected behavior:

Surface.fill treats negative coordinates intuitively and does not 'move' the rect inside the surface. Surface.fill should be able to draw just a part of the rect on the left and top edges if there are negative coordinates. A possible fix implementation would be to add the negative coordinate(s) to the size before clamping them to 0.

Steps to reproduce:

  1. Create a basic pygame program that shows the display.
  2. Using either the display surface or a second surface that is blit on the display, call surf.fill('green', (-60,-5,80,80)) .
  3. Optionally, swap out that call with pygame.draw.rect(surf, 'green', (-60,-5,80,80)) to see the correct drawing.

Test code

import pygame as pg

display = pg.display.set_mode((640,360))

# display.set_clip((50,50,50,50))

while True:
  for event in pg.event.get():
    if event.type == pg.QUIT:
      raise Exception
  display.fill((16,16,16))
  print(display.fill('green', (-60,-5,80,80)))
  # print(pg.draw.rect(display, 'green', (-60,-5,80,80)))
  pg.display.update()

Original issue

138

https://www.github.com/pygame/pygame/issues/123 The issue was from bitbucket (copied to github) in 2012, got a PR fixing an unrelated bug, then got closed

oddbookworm commented 1 week ago

Thanks for the report @aatle! I've put up a tentative fix pull request. If you provide me the OS and version of python you're on, I can get you a development wheel from the CI runs for you to test on if you would like

aatle commented 1 week ago

Cool. Also, all the OS, python version stuff is in the pygame.print_debug_info() dropdown at the top of the post.

oddbookworm commented 1 week ago

🤦‍♂️ oof, that was a dumb question lol. I'm even the one that originally wrote that function. When the CI run for your OS and python version completes, I'll ping you and upload a wheel for you. If you don't know how to install a wheel, it's just pip install path/to/file.whl

oddbookworm commented 1 week ago

pygame_ce-2.5.1.dev1-cp311-cp311-manylinux_2_17_x86_64.manylinux2014_x86_64.zip @aatle try this, you'll have to unzip it first. Github doesn't like raw wheel files

aatle commented 1 week ago

Tested. Everything looks all good now using that wheel. Glad to have this decade-old bug fixed!

oddbookworm commented 1 week ago

I'm actually going to keep this issue open and tie it to that pull request. It's not actually solved until the fix is merged into main after all