Merge pull request pygame-community#2939 from oddbookworm/fix-surf-fi…

…ll-negative-rect

Fix `Surface.fill` with rects with negative positions that overlap the surface

docs/reST/ref/surface.rst

@@ -306,6 +306,11 @@
 
       This will return the affected Surface area.
 
+      .. note:: As of pygame-ce version 2.5.1, a long-standing bug has been fixed!
+         Now when passing in a ``Rect`` with negative ``x`` or negative ``y`` (or both),
+         the ``Rect`` filled will no longer be shifted to ``(0, 0)``, but instead only the
+         part of the ``Rect`` overlapping the window's ``Rect`` will be filled.
+
       .. ## Surface.fill ##
 
    .. method:: scroll

src_c/surface.c

@@ -1755,53 +1755,32 @@ surf_fill(pgSurfaceObject *self, PyObject *args, PyObject *keywds)
         rect = &temp;
     }
 
-    if (rect->w < 0 || rect->h < 0 || rect->x > surf->w || rect->y > surf->h) {
-        sdlrect.x = sdlrect.y = 0;
-        sdlrect.w = sdlrect.h = 0;
+    // In SDL3, SDL_IntersectRect is renamed to SDL_GetRectIntersection
+    SDL_Rect surfrect = {0, 0, surf->w, surf->h};
+    if (!SDL_IntersectRect(rect, &surfrect, &sdlrect)) {
+        sdlrect.x = 0;
+        sdlrect.y = 0;
+        sdlrect.w = 0;
+        sdlrect.h = 0;
     }
-    else {
-        sdlrect.x = rect->x;
-        sdlrect.y = rect->y;
-        sdlrect.w = rect->w;
-        sdlrect.h = rect->h;
-
-        // clip the rect to be within the surface.
-        if (sdlrect.x + sdlrect.w <= 0 || sdlrect.y + sdlrect.h <= 0) {
-            sdlrect.w = 0;
-            sdlrect.h = 0;
-        }
-
-        if (sdlrect.x < 0) {
-            sdlrect.x = 0;
-        }
-        if (sdlrect.y < 0) {
-            sdlrect.y = 0;
-        }
-
-        if (sdlrect.x + sdlrect.w > surf->w) {
-            sdlrect.w = sdlrect.w + (surf->w - (sdlrect.x + sdlrect.w));
-        }
-        if (sdlrect.y + sdlrect.h > surf->h) {
-            sdlrect.h = sdlrect.h + (surf->h - (sdlrect.y + sdlrect.h));
-        }
 
-        if (sdlrect.w <= 0 || sdlrect.h <= 0) {
-            return pgRect_New(&sdlrect);
-        }
+    if (sdlrect.w <= 0 || sdlrect.h <= 0) {
+        return pgRect_New(&sdlrect);
+    }
 
-        if (blendargs != 0) {
-            result = surface_fill_blend(surf, &sdlrect, color, blendargs);
-        }
-        else {
-            pgSurface_Prep(self);
-            pgSurface_Lock((pgSurfaceObject *)self);
-            result = SDL_FillRect(surf, &sdlrect, color);
-            pgSurface_Unlock((pgSurfaceObject *)self);
-            pgSurface_Unprep(self);
-        }
-        if (result == -1)
-            return RAISE(pgExc_SDLError, SDL_GetError());
+    if (blendargs != 0) {
+        result = surface_fill_blend(surf, &sdlrect, color, blendargs);
+    }
+    else {
+        pgSurface_Prep(self);
+        pgSurface_Lock((pgSurfaceObject *)self);
+        result = SDL_FillRect(surf, &sdlrect, color);
+        pgSurface_Unlock((pgSurfaceObject *)self);
+        pgSurface_Unprep(self);
     }
+    if (result == -1)
+        return RAISE(pgExc_SDLError, SDL_GetError());
+
     return pgRect_New(&sdlrect);
 }
 

test/surface_test.py

@@ -4268,6 +4268,38 @@ def test_fill(self):
         for y in range(5, 480, 10):
             self.assertEqual(screen.get_at((10, y)), screen.get_at((330, 480 - y)))
 
+    def test_fill_negative_rectpos(self):
+        """Regression test for https://github.com/pygame-community/pygame-ce/issues/2938"""
+        screen = pygame.display.set_mode((640, 480))
+        other_surface = screen.copy()
+
+        negative_x_rect = pygame.Rect(-10, 10, 20, 20)
+        negative_x_rect_drawn = pygame.Rect(0, 10, 10, 20)
+        negative_y_rect = pygame.Rect(100, -10, 20, 20)
+        negative_y_rect_drawn = pygame.Rect(100, 0, 20, 10)
+        negative_both = pygame.Rect(-10, -10, 20, 20)
+        negative_both_drawn = pygame.Rect(0, 0, 10, 10)
+
+        red_rect_1 = screen.fill("red", negative_x_rect)
+        blue_rect_1 = screen.fill("blue", negative_y_rect)
+        green_rect_1 = screen.fill("green", negative_both)
+
+        red_rect_2 = other_surface.fill("red", negative_x_rect_drawn)
+        blue_rect_2 = other_surface.fill("blue", negative_y_rect_drawn)
+        green_rect_2 = other_surface.fill("green", negative_both_drawn)
+
+        self.assertEqual(red_rect_1, red_rect_2)
+        self.assertEqual(blue_rect_1, blue_rect_2)
+        self.assertEqual(green_rect_1, green_rect_2)
+
+        # verify both have the same pixels
+        width, height = screen.get_size()
+        for row in range(height):
+            for col in range(width):
+                self.assertEqual(
+                    screen.get_at((col, row)), other_surface.get_at((col, row))
+                )
+
 
 if __name__ == "__main__":
     unittest.main()