flood fill #2840
flood fill #2840
Conversation
This is no longer fun. Is my local install of clang-format bugged?
yunline
added
New API
… up, I will add ascii art to illustrate drawing algorithms. This commit message is longer than the diff!
|
This has been stalled for 10 months now. |
WalkthroughAdds a new pygame.draw.flood_fill API: type stub and typing import updated, C-level implementation with BFS supporting solid color or pattern surfaces, a documentation macro and method-table entry, and tests added (the flood-fill test class was inserted twice). Returns a bounding Rect or a 0-sized Rect at the start when nothing is drawn. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant PyAPI as pygame.draw.flood_fill (stub)
participant CWrapper as draw.c:flood_fill
participant BFS as flood_fill_inner (BFS)
participant Target as TargetSurface (SDL_Surface)
participant Pattern as PatternSurface (SDL_Surface)
User->>PyAPI: flood_fill(surface, color|pattern, start_pos)
PyAPI->>CWrapper: call C wrapper with parsed args
CWrapper->>Target: lock target surface
alt pattern provided
CWrapper->>Pattern: lock pattern surface & sample pixels
CWrapper->>BFS: run BFS using pattern lookups
else solid color
CWrapper->>BFS: run BFS using supplied color value
end
BFS-->>CWrapper: bounding box (or empty)
CWrapper->>Target: unlock target surface
alt pixels filled
CWrapper-->>PyAPI: return bounding Rect
else nothing drawn
CWrapper-->>PyAPI: return 0-sized Rect at start_pos
end
PyAPI-->>User: Rect
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
src_c/draw.c (1)
110-116: Prototypes are in place (resolves earlier “missing definition” concern).The forward declarations for flood_fill_inner and unsafe_set_at now exist and match their later definitions. Nothing to change.
🧹 Nitpick comments (4)
test/draw_test.py (2)
7315-7317: Rename class to reflect usage (it’s a concrete TestCase, not a mixin).The class inherits unittest.TestCase and is discovered directly. Naming it “Mixin” is inconsistent with the rest of the file and can mislead future contributors.
Apply this diff:
-class DrawFloodFillMixin(unittest.TestCase): +class DrawFloodFillTest(unittest.TestCase): """Mixin tests for flood fill."""
7318-7359: Add coverage for return Rect and no-op behavior; consider clip behavior tests.
- Missing assertions on the returned Rect, unlike other draw tests (e.g., line/rect/polygon). Recommend asserting the bounding rect equals the actually modified area.
- Add a no-op case: starting on a pixel already equal to the target color should return a 0-sized Rect at start_pos (matches C implementation).
- Consider a test ensuring flood_fill respects Surface.set_clip, as other draw functions do.
Proposed additions (minimal, self-contained):
def test_flood_fill(self): """Ensures flood fill fills with solid color""" surf = pygame.Surface((100, 100)) surf.fill((0, 0, 0)) pygame.draw.line(surf, (255, 0, 0), (10, 10), (90, 90), 5) self.assertEqual( surf.get_at((10, 10)), (255, 0, 0), "line drawing precondition" ) self.assertEqual( surf.get_at((90, 90)), (255, 0, 0), "line drawing precondition" ) - pygame.draw.flood_fill(surf, (255, 255, 255), (90, 90)) + changed = pygame.draw.flood_fill(surf, (255, 255, 255), (90, 90)) self.assertEqual( surf.get_at((90, 90)), (255, 255, 255), "flood fill start point" ) self.assertEqual( surf.get_at((10, 10)), (255, 255, 255), "flood fill reaching the end" ) + # Sanity: returned rect should bound modified pixels. + # Using helper from this module: any pixel != background is "changed". + from_rect = create_bounding_rect(surf, pygame.Color("black"), (90, 90)) + self.assertEqual(changed, from_rect) def test_flood_pattern(self): """Ensures flood fill fills in a pattern""" surf = pygame.Surface((100, 100)) surf.fill((0, 0, 0)) pattern = pygame.Surface((2, 2)) pattern.fill((255, 255, 255)) pattern.set_at((0, 0), (255, 0, 0)) pattern.set_at((1, 1), (0, 0, 255)) pygame.draw.line(surf, (0, 0, 0), (5, 95), (95, 5)) pygame.draw.line(surf, (0, 0, 0), (50, 0), (50, 95)) pygame.draw.flood_fill(surf, pattern, (95, 95)) for pt in [(0, 0), (0, 1), (1, 0), (1, 1)]: self.assertEqual(surf.get_at(pt), pattern.get_at(pt), pt) + + def test_flood_fill_noop_returns_zero_rect(self): + """Starting on already-filled color should no-op and return a 0-sized Rect.""" + surf = pygame.Surface((10, 10)) + surf.fill((10, 20, 30)) + start = (3, 4) + changed = pygame.draw.flood_fill(surf, (10, 20, 30), start) + self.assertEqual(changed, pygame.Rect(start, (0, 0)))If you want, I can also add a clip test once we confirm the implementation respects the surface clip.
src_c/draw.c (2)
2506-2507: Prefer PG_GetSurfaceClipRect for consistency.Elsewhere in this file we use PG_GetSurfaceClipRect; using it here keeps API uniform (no functional difference expected).
114-116: Consider inlining unsafe_set_at.This function is hot inside flood_fill and inner loops of other draw routines; marking it
static inlinecan help compilers inline and remove dispatch overhead.-static void +static inline void unsafe_set_at(SDL_Surface *surf, int x, int y, Uint32 color)Also applies to: 2674-2701
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
buildconfig/stubs/pygame/draw.pyi(1 hunks)src_c/doc/draw_doc.h(1 hunks)src_c/draw.c(5 hunks)test/draw_test.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
test/draw_test.py (2)
src_c/draw.c (3)
draw(4109-4143)line(226-295)flood_fill(1300-1385)buildconfig/stubs/pygame/draw.pyi (2)
line(374-424)flood_fill(572-596)
buildconfig/stubs/pygame/draw.pyi (2)
src_c/draw.c (1)
flood_fill(1300-1385)buildconfig/stubs/pygame/surface.pyi (1)
Surface(42-1051)
src_c/draw.c (4)
src_c/color.c (1)
color(2508-2572)buildconfig/stubs/pygame/draw.pyi (1)
flood_fill(572-596)src_c/base.c (1)
pg_TwoIntsFromObj(413-474)src_c/surflock.c (2)
pgSurface_Lock(61-65)pgSurface_Unlock(67-71)
🪛 GitHub Actions: python3 dev.py all
buildconfig/stubs/pygame/draw.pyi
[error] 1-1: Trailing whitespace detected. Pre-commit hook 'trailing-whitespace' modified the file.
[error] 572-572: ruff: SyntaxError: unindent does not match any outer indentation level.
[error] 572-572: ruff-format: Failed to parse buildconfig/stubs/pygame/draw.pyi due to indentation error.
[error] 1-1: clang-format: Code style issues fixed in this file; please re-run pre-commit.
src_c/draw.c
[error] 1-1: clang-format: Code style issues fixed in this file; please re-run pre-commit.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: x86_64
- GitHub Check: arm64
- GitHub Check: debug_coverage (ubuntu-24.04, 3.14.0rc1)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.9.23)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.13.5)
- GitHub Check: aarch64
- GitHub Check: x86_64
- GitHub Check: i686
- GitHub Check: build (macos-14)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: Debian (Bookworm - 12) [armv7]
- GitHub Check: Debian (Bookworm - 12) [armv6]
- GitHub Check: Debian (Bookworm - 12) [s390x]
- GitHub Check: Debian (Bookworm - 12) [ppc64le]
- GitHub Check: x86
- GitHub Check: AMD64
- GitHub Check: msys2 (clang64, clang-x86_64)
- GitHub Check: msys2 (ucrt64, ucrt-x86_64)
- GitHub Check: msys2 (mingw64, x86_64)
🔇 Additional comments (3)
src_c/draw.c (3)
1-1: Please re-run clang-format (pre-commit fixed style).CI reports clang-format changes were applied automatically. Re-run the repo’s formatter locally (e.g.,
pre-commit run -aorpython3 dev.py fmt) and push the result.
2474-2497: SURF_GET_AT macro: looks correct and consistent.Covers 1,2,3,4 bpp using SDL endianness; matches existing set_at logic. No change requested.
2580-2582: Pattern modulo semantics LGTM.Using
x % pattern->wandy % pattern->his the expected tiling behavior. No change requested.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
buildconfig/stubs/pygame/draw.pyi (3)
571-596: Fix indentation, align parameter name to start_pos, and replace Union with precise overloads to satisfy CI and match C APITwo issues are breaking CI: top-level indentation for the new def block and a missing dedent at EOF. Also, the stub currently exposes a Union in the parameter type and uses start_point, which does not match the C keyword start_pos. Replace the whole block with overloads that reflect the two accepted value types for the second argument (ColorLike or Surface) while keeping the runtime keyword name color, then provide a single docstring on the dispatcher. This mirrors the pattern used by aacircle and avoids runtime keyword mismatches.
Apply this diff:
@@ - def flood_fill( - surface: Surface, - color: Union[ColorLike, Surface], - start_point: Point -) -> Rect: - """Replace the color of a cluster of connected same-color pixels, beginning - from the starting point, with a repeating pattern or solid single color - - :param Surface surface: surface to draw on - :param color: color, or surface pattern, to draw with. The alpha value is optional if using a - tuple ``(RGB[A])`` - :type color: :data:`pygame.typing.ColorLike` or a pattern to fill with, as a Surface - :param start_point: starting point as a sequence of 2 ints/floats, - e.g. ``(x, y)`` - :type starting_point: tuple(int or float, int or float) or - list(int or float, int or float) or Vector2(int or float, int or float) - - :returns: a rect bounding the changed pixels, if nothing is drawn the - bounding rect's position will be the position of the starting point - and its width and height will be 0 - :rtype: Rect - - .. versionadded:: 2.5.6 - """ +@overload +def flood_fill( + surface: Surface, + color: ColorLike, + start_pos: Point, +) -> Rect: ... + +@overload +def flood_fill( + surface: Surface, + color: Surface, + start_pos: Point, +) -> Rect: ... + +def flood_fill(*args, **kwargs): # type: ignore + """Replace the color of a cluster of connected same-color pixels, starting at + ``start_pos``, with a repeating pattern or a solid single color. + + :param Surface surface: surface to draw on + :param color: color to draw with, or a Surface pattern to fill with. The alpha + value is optional if using a tuple ``(RGB[A])`` + :type color: :data:`pygame.typing.ColorLike` or :class:`pygame.Surface` + :param start_pos: starting point as a sequence of 2 ints/floats, e.g. ``(x, y)`` + :type start_pos: tuple(int or float, int or float) or + list(int or float, int or float) or Vector2(int or float, int or float) + + :returns: a rect bounding the changed pixels; if nothing is drawn the + bounding rect's position will be the position of the starting point + and its width and height will be 0 + :rtype: Rect + + .. versionadded:: 2.5.6 + """Notes:
- Kept the keyword name color in both overloads to match the C keywords = {"surface","color","start_pos"} and avoid runtime keyword mismatches.
- Moved the decorators/def to column 0 to fix “unindent does not match any outer indentation level”.
- Renamed start_point to start_pos for consistency with src_c/draw.c and other draw APIs.
571-596: Align stub parameter names with C runtime keywordsThe C implementation of
flood_fillregisters its keyword arguments as{"surface", "color", "start_pos", NULL}(see static keywords array at lines 1314–1316 insrc_c/draw.c). However, the stub inbuildconfig/stubs/pygame/draw.pyicurrently usesstart_pointas the third parameter name and in its docstring. This mismatch will cause calls likeflood_fill(surface=…, color=…, start_point=(x, y))to raise aTypeErrorat runtime, since the interpreter only recognizesstart_pos.Please update the stub so that the signature and all doc references use
start_posinstead ofstart_point, and ensure no straystart_pointorstarting_pointtokens remain:• File: buildconfig/stubs/pygame/draw.pyi
– Change
python def flood_fill( surface: Surface, color: Union[ColorLike, Surface], start_point: Point ) -> Rect:
to
python def flood_fill( surface: Surface, color: Union[ColorLike, Surface], start_pos: Point ) -> Rect:– In the docstring, rename:
-:param start_point:→:param start_pos:
-:type start_point:→:type start_pos:• Note on overloads: If you later introduce an overload for pattern fills, do not expose a new keyword like
pattern_surface; the C API only acceptscolorfor both solid and pattern fills. Any stub overload must continue to use the keyword namecolor.These changes are critical to ensure that type‐checked calls in the stubs align with the actual runtime behavior and prevent keyword‐argument errors.
571-596: IndentationError still present indraw.pyistub; refactor required
The AST parse check onbuildconfig/stubs/pygame/draw.pyicontinues to fail with:IndentationError: unindent does not match any outer indentation level at line 572Please correct the indentation around the
def flood_fillblock (lines 571–596):
- Ensure the
def flood_fill(signature and its parameter lines use the same number of spaces as the surrounding stubs (no mixing of tabs/spaces).- Align the opening parenthesis, parameters, closing parenthesis, and docstring consistently.
After refactoring, rerun:
python3 dev.py all python3 - <<'PY' import ast ast.parse(open('buildconfig/stubs/pygame/draw.pyi', encoding='utf-8').read()) print('OK') PYto confirm there are no remaining indentation or EOF dedent issues.
♻️ Duplicate comments (1)
buildconfig/stubs/pygame/draw.pyi (1)
581-595: Docstring wording/naming consistency and Sphinx friendlinessThe docstring should reference start_pos (not “starting point”/start_point) and avoid documenting parameters that are not actual keywords (e.g., pattern_surface) to prevent Sphinx parameter validation warnings. The proposed docstring above compresses this into a single “color” param that may be either ColorLike or Surface.
Incorporated prior feedback (“color, or surface pattern, to draw with…”) while retaining the runtime keyword name.
🧹 Nitpick comments (1)
buildconfig/stubs/pygame/draw.pyi (1)
31-31: Remove unused Union import after switching to overloadsUnion becomes unnecessary once you add overloads. Drop it to keep the stubs clean and satisfy linters.
Apply this diff:
-from typing import overload, Union +from typing import overload
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
buildconfig/stubs/pygame/draw.pyi(2 hunks)src_c/doc/draw_doc.h(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src_c/doc/draw_doc.h
🧰 Additional context used
🧬 Code graph analysis (1)
buildconfig/stubs/pygame/draw.pyi (2)
src_c/draw.c (1)
flood_fill(1300-1385)buildconfig/stubs/pygame/rect.pyi (1)
Rect(303-303)
🪛 GitHub Actions: MSYS2
buildconfig/stubs/pygame/draw.pyi
[error] 572-572: Sphinx AutoAPI extension error during docs build. Parsing Python code failed in 'pygame.draw' at line 572: unindent does not match any outer indentation level.
🪛 GitHub Actions: python3 dev.py all
buildconfig/stubs/pygame/draw.pyi
[error] 572-572: ruff: SyntaxError: unindent does not match any outer indentation level.
[error] 596-596: ruff: SyntaxError: Expected dedent, found end of file.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: arm64
- GitHub Check: x86_64
- GitHub Check: i686
- GitHub Check: x86_64
- GitHub Check: aarch64
- GitHub Check: debug_coverage (ubuntu-24.04, 3.13.5)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.9.23)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.14.0rc1)
- GitHub Check: Debian (Bookworm - 12) [armv6]
- GitHub Check: Debian (Bookworm - 12) [armv7]
- GitHub Check: Debian (Bookworm - 12) [s390x]
- GitHub Check: Debian (Bookworm - 12) [ppc64le]
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
src_c/doc/draw_doc.h (1)
13-13: Unify keyword name with the rest of draw. (start_pos vs start_point).*Current macro uses start_point, while most draw APIs use start_pos (see DOC_DRAW_LINE/DOC_DRAW_AALINE). Pick one and use it consistently across docs, stubs, and C kwlist. Suggest start_pos for parity with existing APIs.
Apply this diff if you go with start_pos:
-#define DOC_DRAW_FLOODFILL "flood_fill(surface, color, start_point) -> Rect\nflood_fill(surface, pattern_surface, start_point) -> Rect\nFill a connected area of same-color pixels." +#define DOC_DRAW_FLOODFILL "flood_fill(surface, color, start_pos) -> Rect\nflood_fill(surface, pattern_surface, start_pos) -> Rect\nFill a connected area of same-color pixels."src_c/draw.c (1)
1397-1423: Bitset math bugs: non-portable WORD_BITS and incorrect clear-mask.
- Use CHAR_BIT instead of hardcoded 8.
- Use unsigned shifts and a single-bit mask; current clear path
(~(1) << n)clears lower bits too.Apply:
-#define WORD_BITS (8 * sizeof(unsigned int)) +#define WORD_BITS (CHAR_BIT * sizeof(unsigned int)) @@ static inline void _bitarray_set(unsigned int *bitarray, size_t idx, SDL_bool value) { - if (value) { - bitarray[idx / WORD_BITS] |= (1 << (idx % WORD_BITS)); - } - else { - bitarray[idx / WORD_BITS] &= (~(1) << (idx % WORD_BITS)); - } + const size_t word = idx / WORD_BITS; + const unsigned int mask = (1u << (idx % WORD_BITS)); + if (value) { + bitarray[word] |= mask; + } else { + bitarray[word] &= ~mask; + } } @@ _bitarray_get(unsigned int *bitarray, size_t idx) { - if (bitarray[idx / WORD_BITS] & (1 << (idx % WORD_BITS))) + if (bitarray[idx / WORD_BITS] & (1u << (idx % WORD_BITS))) return SDL_TRUE; else return SDL_FALSE; }And add the missing header near the includes:
#include <float.h> +#include <limits.h> // for CHAR_BIT
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src_c/doc/draw_doc.h(1 hunks)src_c/draw.c(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src_c/draw.c (4)
src_c/color.c (1)
color(2508-2572)src_c/transform.c (4)
if(1689-1710)if(1732-1833)if(2322-2324)if(4181-4183)src_c/base.c (1)
pg_TwoIntsFromObj(413-474)src_c/surflock.c (2)
pgSurface_Lock(61-65)pgSurface_Unlock(67-71)
🪛 GitHub Actions: python3 dev.py all
src_c/draw.c
[error] 1-1: clang-format: Code formatting changes were applied by the hook. Please review and commit.
🪛 GitHub Actions: SDL3 build
src_c/draw.c
[error] 1335-1335: SDL_ConvertSurface: too many arguments to function 'SDL_ConvertSurface' (SDL3 signature expects fewer parameters).
[error] 2475-2475: PG_FORMAT_BytesPerPixel usage leads to invalid type argument of '->' during macro expansion.
[error] 2548-2548: SURF_GET_AT macro expansion triggers type-mismatch error related to PG_FORMAT_BytesPerPixel.
[error] 2572-2572: SURF_GET_AT macro expansion triggers type-mismatch error related to PG_FORMAT_BytesPerPixel.
[error] 2580-2580: SURF_GET_AT macro expansion triggers type-mismatch error related to PG_FORMAT_BytesPerPixel.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: i686
- GitHub Check: x86_64
- GitHub Check: aarch64
- GitHub Check: debug_coverage (ubuntu-24.04, 3.14.0rc1)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.13.5)
- GitHub Check: Debian (Bookworm - 12) [armv6]
- GitHub Check: Debian (Bookworm - 12) [s390x]
- GitHub Check: Debian (Bookworm - 12) [armv7]
- GitHub Check: Debian (Bookworm - 12) [ppc64le]
🔇 Additional comments (3)
src_c/draw.c (3)
1353-1371: Early-return leak fixes covered by wrapper refactor.With the refactor above, both target and pattern surfaces are unlocked on all early returns. If you keep the old structure, you must unlock/free in the TypeError/lock-failure paths too. The refactor supersedes this.
1-1: Formatting hook changed files.The CI indicates clang-format applied changes. Please run the project formatter (e.g., python3 dev.py format) and commit the result to satisfy the hook.
4095-4098: Keyword consistency confirmed for flood_fill
Theflood_fillfunction usesstart_pointuniformly in its C kwlist, in src_c/doc/draw_doc.h, and in buildconfig/stubs/pygame/draw.pyi. No updates are needed.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src_c/draw.c (2)
111-117: Forward declarations are fine and resolve prior “missing definition?” concern.The flood_fill_inner and unsafe_set_at prototypes match later definitions; this addresses the earlier reviewer’s “I can't actually find a definition for this?” remark.
1301-1397: Keyword name consistency: start_point vs start_pos. Pick one and align across C/docs/stubs.Other draw APIs here use start_pos/end_pos. Either stick to start_pos for parity or update stubs/docs to start_point. Current code uses start_point.
If you choose start_pos, change here:
@@ - static char *keywords[] = {"surface", "color", "start_point", NULL}; + static char *keywords[] = {"surface", "color", "start_pos", NULL}; @@ - if (!pg_TwoIntsFromObj(start, &startx, &starty)) { - return RAISE(PyExc_TypeError, "invalid start_point argument"); - } + if (!pg_TwoIntsFromObj(start, &startx, &starty)) { + return RAISE(PyExc_TypeError, "invalid start_pos argument"); + }To verify consistency repo-wide:
#!/bin/bash # Check keyword naming consistency for flood_fill across C, docs, and stubs. rg -nP 'flood_fill\s*\(' --type=c src_c rg -n 'flood_fill\(' buildconfig/stubs/pygame/draw.pyi rg -nP 'DOC_DRAW_FLOODFILL|start_(pos|point)' src_c/doc/draw_doc.h buildconfig/stubs/pygame/draw.pyi
🧹 Nitpick comments (3)
src_c/draw.c (3)
1347-1361: Locking style inconsistencies with draw.c norm.Most draw.* wrappers lock/unlock unconditionally via pgSurface_Lock/Unlock; here you guard with SDL_MUSTLOCK. Not wrong, but inconsistent and harder to reason about.
Minimal alignment:
- if (SDL_MUSTLOCK(surf)) { - did_lock_surf = SDL_TRUE; - if (!pgSurface_Lock(surfobj)) { - return RAISE(PyExc_RuntimeError, "error locking surface"); - } - } + did_lock_surf = SDL_TRUE; + if (!pgSurface_Lock(surfobj)) { + return RAISE(PyExc_RuntimeError, "error locking surface"); + } @@ - if (pattern && SDL_MUSTLOCK(pattern)) { - did_lock_pat = SDL_TRUE; - if (!pgSurface_Lock(pat_surfobj)) { + if (pattern) { + did_lock_pat = SDL_TRUE; + if (!pgSurface_Lock(pat_surfobj)) { if (did_lock_surf) { pgSurface_Unlock(surfobj); } return RAISE(PyExc_RuntimeError, "error locking pattern surface"); } }
1408-1436: Bitset helpers look correct; good masks and unsigned shifts.
- WORD_BITS uses CHAR_BIT.
- Masking uses 1u and ~mask.
- Access via idx/WORD_BITS is correct.
Minor nit: consider using size_t for x/y in point2d or add casts at assignments to avoid implicit narrowing.
2487-2511: SURF_GET_AT macro uses PG_FORMAT_BytesPerPixel; ensure callers always pass a PG_PixelFormat.*In this file you do pass a PG_PixelFormat* (“format”), which is fine. The earlier SDL3 failures typically came from passing surf->format (Uint32). Keep it this way or, for parity with the rest of draw.c helpers, switch to PG_SURF_BytesPerPixel(p_surf) and drop the p_format parameter.
Optional macro + call-site refactor:
-#define SURF_GET_AT(p_color, p_surf, p_x, p_y, p_pixels, p_format, p_pix) \ - switch (PG_FORMAT_BytesPerPixel(p_format)) { \ +#define SURF_GET_AT(p_color, p_surf, p_x, p_y, p_pixels, p_pix) \ + switch (PG_SURF_BytesPerPixel(p_surf)) { \ case 1: \ p_color = (Uint32) * \ ((Uint8 *)(p_pixels) + (p_y) * p_surf->pitch + (p_x)); \ break; \ @@ default: /* case 4: */ \ p_color = \ *((Uint32 *)(p_pixels + (p_y) * p_surf->pitch) + (p_x)); \ break; \ }And update the three call sites in flood_fill_inner by removing the format argument.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
buildconfig/stubs/pygame/draw.pyi(2 hunks)src_c/doc/draw_doc.h(1 hunks)src_c/draw.c(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src_c/doc/draw_doc.h
- buildconfig/stubs/pygame/draw.pyi
🧰 Additional context used
🧬 Code graph analysis (1)
src_c/draw.c (4)
src_c/surface.c (3)
if(2245-2248)if(4534-4579)if(4581-4598)src_c/base.c (1)
pg_TwoIntsFromObj(413-474)src_c/surflock.c (2)
pgSurface_Lock(61-65)pgSurface_Unlock(67-71)src_c/_pygame.h (3)
PG_GetSurfaceFormat(130-134)PG_GetSurfaceFormat(243-247)PG_GetSurfaceClipRect(375-380)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: aarch64
- GitHub Check: x86_64
- GitHub Check: i686
- GitHub Check: debug_coverage (ubuntu-24.04, 3.13.5)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.9.23)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.14.0rc1)
- GitHub Check: x86
- GitHub Check: AMD64
- GitHub Check: Debian (Bookworm - 12) [s390x]
- GitHub Check: Debian (Bookworm - 12) [armv7]
- GitHub Check: Debian (Bookworm - 12) [ppc64le]
- GitHub Check: Debian (Bookworm - 12) [armv6]
- GitHub Check: build (windows-latest)
- GitHub Check: build (macos-14)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: dev-check
- GitHub Check: build (ubuntu-22.04)
- GitHub Check: msys2 (clang64, clang-x86_64)
- GitHub Check: msys2 (ucrt64, ucrt-x86_64)
- GitHub Check: msys2 (mingw64, x86_64)
🔇 Additional comments (2)
src_c/draw.c (2)
32-32: Good portability add: use CHAR_BIT instead of hard-coded 8.Including limits.h to drive WORD_BITS via CHAR_BIT is correct and future-proof.
4117-4118: API table entry looks correct.flood_fill is added with METH_VARARGS | METH_KEYWORDS and DOC_DRAW_FLOODFILL. Assuming docs/stubs match the chosen keyword name, this is good.
There was a problem hiding this comment.
OK, this seems to be compiling nicely for the CI now and I've tested it locally as well with the unit tests and some test code.
There are probably further extensions we could make to this idea (like color difference thresholds when propagating the fill) but I think this is useful - I could have used it when making my 'Pygame Paint' app. We could also tweak the implementation somewhat (tidy that macro somewhere?), and probably the type hints (overload decorator instead of Union?), but what we have here seems workable to me.
I will leave it for a little bit longer for anymore feedback, before merging. Just had the idea to consider renaming the 'color' parameter to 'fill' to encompass that it can be a colour or a Surface...
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
test/draw_test.py (3)
7316-7332: Add return-rect assertions to validate API contract.These tests verify pixel results but not the Rect returned by draw.flood_fill. As other draw tests assert bounding rects, add a quick check to ensure the returned Rect matches what was actually drawn.
Apply:
@@ - pygame.draw.flood_fill(surf, (255, 255, 255), (90, 90)) + ret = pygame.draw.flood_fill(surf, (255, 255, 255), (90, 90)) + self.assertIsInstance(ret, pygame.Rect) + expected = create_bounding_rect(surf, pygame.Color("black"), (90, 90)) + self.assertEqual(ret, expected)
7342-7359: Remove no-op lines and strengthen tiling expectations for pattern fill.
- Two black lines drawn on a black surface are no-ops; they add cost without effect.
- Consider asserting tiling is anchored consistently (e.g., to (0,0)) by checking additional points.
Apply:
@@ - pygame.draw.line(surf, (0, 0, 0), (5, 95), (95, 5)) - pygame.draw.line(surf, (0, 0, 0), (50, 0), (50, 95)) + # (optional) If future semantics change, barriers can be added here + # with a contrasting color to constrain the fill region. pygame.draw.flood_fill(surf, pattern, (95, 95)) for pt in [(0, 0), (0, 1), (1, 0), (1, 1)]: self.assertEqual(surf.get_at(pt), pattern.get_at(pt), pt) + + # Validate global tiling alignment: color at (x,y) equals pattern at (x%w,y%h). + for (x, y) in [(2, 0), (2, 1), (3, 3), (10, 17), (95, 95)]: + self.assertEqual( + surf.get_at((x, y)), + pattern.get_at((x % pattern.get_width(), y % pattern.get_height())), + (x, y), + )
7361-7391: Also assert Rect and kwargs behavior; keep pixel-by-pixel comparison.
- Capture and assert the returned Rect equals the actual drawn bounds (consistent with other draw tests).
- Add a small kwargs test to align with the suite’s style (args/kwargs/invalids are commonly covered across draw APIs).
Apply:
@@ - pygame.draw.flood_fill(surf, (255,255,255), (10, 50)) + r1 = pygame.draw.flood_fill(surf, (255,255,255), (10, 50)) + self.assertIsInstance(r1, pygame.Rect) + self.assertEqual(r1, create_bounding_rect(surf, pygame.Color("black"), (10, 50))) @@ - pygame.draw.flood_fill(surf, (255,255,255), (1,1)) + r2 = pygame.draw.flood_fill(surf, (255,255,255), (1,1)) + self.assertIsInstance(r2, pygame.Rect) @@ - pygame.draw.flood_fill(surf, (255,0,0), (50,50)) + r3 = pygame.draw.flood_fill(surf, (255,0,0), (50,50)) + self.assertIsInstance(r3, pygame.Rect) + self.assertEqual(r3, create_bounding_rect(surf, pygame.Color("white"), (50, 50))) @@ for pt in itertools.product(range(100), range(100)): self.assertEqual(surf.get_at(pt), surf2.get_at(pt)) + + def test_flood_fill_kwargs(self): + """Ensures flood_fill accepts kwargs and returns a Rect without drawing when no-op.""" + surf = pygame.Surface((5, 5)) + surf.fill("white") + # No-op fill (same color) should return a 0-sized Rect at the start position. + ret = pygame.draw.flood_fill(surface=surf, color=pygame.Color("white"), start_pos=(0, 0)) + self.assertIsInstance(ret, pygame.Rect) + self.assertEqual(ret, pygame.Rect(0, 0, 0, 0))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
test/draw_test.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
test/draw_test.py (1)
src_c/draw.c (5)
draw(4140-4174)line(227-296)flood_fill(1301-1405)set_at(1570-1599)circle(806-916)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: x86_64
- GitHub Check: arm64
- GitHub Check: Debian (Bookworm - 12) [armv7]
- GitHub Check: Debian (Bookworm - 12) [armv6]
- GitHub Check: Debian (Bookworm - 12) [ppc64le]
- GitHub Check: Debian (Bookworm - 12) [s390x]
- GitHub Check: i686
- GitHub Check: aarch64
- GitHub Check: x86_64
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: build (ubuntu-22.04)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.9.23)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.13.5)
- GitHub Check: msys2 (mingw64, x86_64)
- GitHub Check: msys2 (clang64, clang-x86_64)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.14.0rc1)
- GitHub Check: x86
- GitHub Check: msys2 (ucrt64, ucrt-x86_64)
- GitHub Check: dev-check
- GitHub Check: AMD64
🔇 Additional comments (1)
test/draw_test.py (1)
5-5: LGTM: import itertools is appropriate and scoped to new tests.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
test/draw_test.py (2)
7378-7396: Optional: reduce per-pixel comparisons for speed.The double 10k-pixel loops are fine but slow. Consider comparing:
- bounding rects, plus
- a small set of perimeter/center sample points via
test_utils.rect_area_ptsandrect_outer_bounds.
7343-7360: Document pattern tiling anchor semantics: The implementation in flood_fill_inner usesx % pattern->wandy % pattern->h, anchoring the pattern at the surface origin. Add a note to the function’s header/doc to lock in this behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
test/draw_test.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
test/draw_test.py (1)
src_c/draw.c (5)
draw(4140-4174)line(227-296)flood_fill(1301-1405)set_at(1570-1599)circle(806-916)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: arm64 deps
- GitHub Check: x86_64
- GitHub Check: i686
- GitHub Check: aarch64
- GitHub Check: debug_coverage (ubuntu-24.04, 3.14.0rc1)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.13.5)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.9.23)
- GitHub Check: x86
- GitHub Check: AMD64
- GitHub Check: Debian (Bookworm - 12) [ppc64le]
- GitHub Check: Debian (Bookworm - 12) [armv6]
- GitHub Check: Debian (Bookworm - 12) [s390x]
- GitHub Check: Debian (Bookworm - 12) [armv7]
- GitHub Check: build (ubuntu-22.04)
- GitHub Check: msys2 (ucrt64, ucrt-x86_64)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: msys2 (mingw64, x86_64)
- GitHub Check: msys2 (clang64, clang-x86_64)
- GitHub Check: dev-check
🔇 Additional comments (2)
test/draw_test.py (2)
1-1: Import looks good.
itertoolsis used bytest_flood_circle; no issues.
7316-7319: DrawFloodFillTest appears only once; no duplicates found.
MyreMylar
left a comment
MyreMylar
left a comment
There was a problem hiding this comment.
OK, LGTM. If anyone does want to tweak the API before a public release then they can still do so, but I don't have any super compelling ideas.
Flood fill drawing operation, like in GIMP and ASEprite.
This still needs docs and tests, and perhaps some more special-case handling of indexed surfaces. Can take a pattern surface as color argument. If you like this, maybe we can add pattern surfaces to polygons also, but in a pinch, you can draw a polygon and then use flood fill, or draw a polygon on an intermediate surface, use flood fill, and then blit the surface.
One thing this can't do that you can do in GIMP is gradients.
Test Program
Summary by CodeRabbit
New Features
Documentation
Tests