Initial _render.Image implementation. #3618
Conversation
📝 WalkthroughWalkthroughImage was implemented as a public surface-image type: added Python C-type lifecycle, initialization from Texture/Image, rendering and get_rect methods, many property accessors, weakref support in the C struct, updated stubs, and new tests exercising behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Image
participant Texture
participant Renderer
User->>Image: __init__(texture_or_image, srcrect)
activate Image
Image->>Image: Validate source type & srcrect
Image->>Image: Set defaults (angle, origin, flip, alpha, blend_mode)
Image->>Image: Store texture/srcrect and register weakref
deactivate Image
User->>Image: draw(srcrect?, dstrect?)
activate Image
Image->>Texture: Access underlying texture
Image->>Image: Apply color/alpha modulation
Image->>Image: Apply transforms (origin, flip, angle)
Image->>Renderer: Submit draw (src, dst, blend_mode)
Renderer-->>User: Rendered pixels
deactivate Image
User->>Image: get_rect()
activate Image
Image-->>User: Return rect (srcrect or texture size)
deactivate Image
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src_c/include/_pygame.h (1)
580-585: Bug: pgRenderer_Check references the wrong symbol.pgRenderer_Check uses &pgRender_Type (missing “er”), but the actual type is pgRenderer_Type. This will miscompile or misbehave in modules relying on the public header.
Apply this fix:
-#define pgRenderer_Check(x) \ - (PyObject_IsInstance((x), (PyObject *)&pgRender_Type)) +#define pgRenderer_Check(x) \ + (PyObject_IsInstance((x), (PyObject *)&pgRenderer_Type))
🧹 Nitpick comments (5)
test/render_test.py (2)
566-570: Replace setattr calls with direct assignment in tests.setattr with constant attribute names is unnecessary; assign the property directly. Keeps tests clear and satisfies Ruff B010.
- with self.assertRaises(TypeError): - setattr(self.image, 'srcrect', None) - with self.assertRaises(TypeError): - setattr(self.image, 'srcrect', 42) + with self.assertRaises(TypeError): + self.image.srcrect = None + with self.assertRaises(TypeError): + self.image.srcrect = 42
571-595: Add a regression check that Image.draw doesn’t mutate the Texture’s mods.Given draw currently sets color/alpha/blend on the underlying texture, assert they’re unchanged after draw. This protects against state leaks.
# After self.image.draw(...): tex = self.image.texture self.assertEqual(tex.color, pygame.Color(255, 255, 255, 255)) self.assertEqual(tex.alpha, 255) self.assertEqual(tex.blend_mode, pygame.BLENDMODE_NONE)buildconfig/stubs/pygame/_render.pyi (1)
155-158: Optional: Align Image.get_rect signature with Texture.get_rect.Texture.get_rect(**kwargs) supports placement helpers; Image.get_rect currently takes none. If you want parity, expose kwargs and delegate later when implemented.
- def get_rect(self) -> Rect: ... + def get_rect(self, **kwargs: Any) -> Rect: ...src_c/render.c (2)
1375-1391: srcrect setter lacks validation.Reject negative sizes and out-of-bounds values relative to the current texture.
- rectptr = pgRect_FromObject(arg, &temp); + rectptr = pgRect_FromObject(arg, &temp); if (!rectptr) { RAISERETURN(PyExc_TypeError, "srcrect must be a Rect", -1); } + if (!self->texture) { + RAISERETURN(pgExc_SDLError, "Image has no texture", -1); + } + if (temp.x < 0 || temp.y < 0 || temp.w < 0 || temp.h < 0 || + temp.x + temp.w > self->texture->width || + temp.y + temp.h > self->texture->height) { + RAISERETURN(PyExc_ValueError, "srcrect out of texture bounds", -1); + }
1618-1622: Optional: Support kwargs in Image.get_rect for parity with Texture.get_rect.Return a placed rect via pgObject_getRectHelper when kwargs are provided; otherwise return srcrect.
static PyObject * image_get_rect_method(pgImageObject *self, PyObject *const *args, Py_ssize_t nargs, PyObject *kwargs) { - PyObject *rect = pgRect_New(&self->srcrect->r); - return rect; + if (!kwargs || PyDict_Size(kwargs) == 0) { + return pgRect_New(&self->srcrect->r); + } + PyObject *rect = pgRect_New(&self->srcrect->r); + return pgObject_getRectHelper(rect, args, nargs, kwargs, "rect"); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
buildconfig/stubs/pygame/_render.pyi(1 hunks)src_c/include/_pygame.h(1 hunks)src_c/render.c(3 hunks)test/render_test.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
test/render_test.py (2)
buildconfig/stubs/pygame/_render.pyi (23)
renderer(106-106)Renderer(17-75)Texture(78-146)from_surface(108-108)Image(149-171)width(102-102)height(104-104)blend_mode(94-94)blend_mode(96-96)color(98-98)color(100-100)color(169-169)color(171-171)alpha(90-90)alpha(92-92)get_rect(145-145)get_rect(155-155)clear(33-33)blit(26-32)to_surface(47-49)draw(12-14)draw(109-117)draw(156-158)src_c/render.c (1)
_render(1690-1779)
buildconfig/stubs/pygame/_render.pyi (3)
buildconfig/stubs/pygame/_sdl2/video.pyi (9)
Texture(42-102)get_rect(65-65)get_rect(110-110)draw(66-74)draw(111-113)color(62-62)color(64-64)color(124-124)color(126-126)buildconfig/stubs/pygame/rect.pyi (1)
Rect(307-307)buildconfig/stubs/pygame/color.pyi (1)
Color(12-94)
src_c/render.c (3)
src_c/color.c (2)
pgColor_NewLength(2364-2374)pg_RGBAFromObjEx(2421-2477)src_c/base.c (1)
pg_TwoFloatsFromObj(507-568)src_c/rect.c (1)
rect(780-849)
🪛 Ruff (0.14.0)
test/render_test.py
567-567: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
569-569: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
⏰ 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: AMD64
- GitHub Check: x86
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: build (windows-latest)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.13.5)
- GitHub Check: build (macos-14)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.9.23)
- GitHub Check: dev-check
- GitHub Check: build (ubuntu-22.04)
- GitHub Check: msys2 (ucrt64, ucrt-x86_64)
- GitHub Check: Pyodide build
- GitHub Check: Debian (Bookworm - 12) [s390x]
- GitHub Check: Debian (Bookworm - 12) [armv6]
- GitHub Check: Debian (Bookworm - 12) [ppc64le]
- GitHub Check: Debian (Bookworm - 12) [armv7]
- GitHub Check: msys2 (mingw64, x86_64)
- GitHub Check: aarch64
- GitHub Check: msys2 (clang64, clang-x86_64)
- GitHub Check: i686
- GitHub Check: x86_64
🔇 Additional comments (1)
src_c/include/_pygame.h (1)
573-575: Weakref support for pgImageObject — good addition.The new weakreflist member lines up with pgImage_Type.tp_weaklistoffset usage and the tests’ GC expectations.
| static int | ||
| image_set_texture(pgImageObject *self, PyObject *arg, void *closure) | ||
| { | ||
| if (!pgTexture_Check(arg)) { | ||
| RAISERETURN(PyExc_TypeError, "texture must be a Texture", -1); | ||
| } | ||
| Py_XINCREF(arg); | ||
| Py_XDECREF(self->texture); | ||
| self->texture = (pgTextureObject *)arg; | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
Changing texture without reconciling srcrect can leave it out of bounds.
Either reset srcrect to the new texture size or validate and raise.
self->texture = (pgTextureObject *)arg;
+ /* Ensure srcrect is valid for the new texture */
+ if (!self->srcrect ||
+ self->srcrect->r.x < 0 || self->srcrect->r.y < 0 ||
+ self->srcrect->r.w < 0 || self->srcrect->r.h < 0 ||
+ self->srcrect->r.x + self->srcrect->r.w > self->texture->width ||
+ self->srcrect->r.y + self->srcrect->r.h > self->texture->height) {
+ Py_XDECREF(self->srcrect);
+ self->srcrect = (pgRectObject *)pgRect_New4(0, 0,
+ self->texture->width,
+ self->texture->height);
+ }
return 0;There was a problem hiding this comment.
That seems reasonable, though if we're enforcing the srcrect fit inside the texture, shouldn't be also do so in image_set_srcrect ?
There was a problem hiding this comment.
@cherez You're absolutely correct! For consistency, image_set_srcrect should also validate that the new rectangle fits within the texture bounds. Currently it accepts any rectangle without checking.
Here's an updated suggestion that adds validation to both functions:
For image_set_srcrect:
static int
image_set_srcrect(pgImageObject *self, PyObject *arg, void *closure)
{
SDL_Rect temp;
SDL_Rect *rectptr;
if (!arg || Py_IsNone(arg)) {
RAISERETURN(PyExc_TypeError, "srcrect must be a Rect", -1);
}
rectptr = pgRect_FromObject(arg, &temp);
if (!rectptr) {
RAISERETURN(PyExc_TypeError, "srcrect must be a Rect", -1);
}
/* Validate bounds against current texture */
if (self->texture) {
if (temp.x < 0 || temp.y < 0 || temp.w < 0 || temp.h < 0 ||
temp.x + temp.w > self->texture->width ||
temp.y + temp.h > self->texture->height) {
RAISERETURN(PyExc_ValueError, "rect values are out of range", -1);
}
}
Py_XDECREF(self->srcrect);
self->srcrect = (pgRectObject *)pgRect_New(rectptr);
return 0;
}For image_set_texture:
static int
image_set_texture(pgImageObject *self, PyObject *arg, void *closure)
{
if (!pgTexture_Check(arg)) {
RAISERETURN(PyExc_TypeError, "texture must be a Texture", -1);
}
pgTextureObject *new_texture = (pgTextureObject *)arg;
/* Validate existing srcrect against new texture bounds */
if (self->srcrect &&
(self->srcrect->r.x < 0 || self->srcrect->r.y < 0 ||
self->srcrect->r.w < 0 || self->srcrect->r.h < 0 ||
self->srcrect->r.x + self->srcrect->r.w > new_texture->width ||
self->srcrect->r.y + self->srcrect->r.h > new_texture->height)) {
/* Reset srcrect to full texture dimensions if out of bounds */
Py_XDECREF(self->srcrect);
self->srcrect = (pgRectObject *)pgRect_New4(0, 0,
new_texture->width,
new_texture->height);
}
Py_XINCREF(arg);
Py_XDECREF(self->texture);
self->texture = new_texture;
return 0;
}This ensures consistent validation across both setters, matching the behavior already present in image_init.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src_c/render.c (4)
1293-1302: Semantic inconsistency: Image.draw() should offset area coordinates relative to self->srcrect.When
areais provided todraw(), it should be interpreted relative to this Image's srcrect (like the constructor does at line 1258), not as absolute texture coordinates. This makesareaspecify a sub-region of the Image, which aligns with the expected semantics.Apply this diff to make area relative:
if (!Py_IsNone(area)) { if (!(areasrcptr = pgRect_FromObject(area, &areasrc))) { PyErr_SetString(PyExc_ValueError, "srcrect must be a Rect or None"); return 0; } + /* Make area relative to this Image's srcrect */ + areasrc.x += self->srcrect->r.x; + areasrc.y += self->srcrect->r.y; } else { areasrcptr = &self->srcrect->r; }
1311-1314: Default dest size should use Image's srcrect dimensions, not full Texture size.When
destis a point, the width/height should default to the area being drawn (fromareasrcptrif present, otherwise fromself->srcrect), not the full texture dimensions.Apply this diff:
destrect.w = - (float)(areasrcptr ? areasrcptr->w : self->texture->width); + (float)(areasrcptr ? areasrcptr->w : self->srcrect->r.w); destrect.h = - (float)(areasrcptr ? areasrcptr->h : self->texture->height); + (float)(areasrcptr ? areasrcptr->h : self->srcrect->r.h);
1335-1347: Critical: draw() mutates the underlying Texture's state without restoring it.The code modifies the texture's color mod, alpha mod, and blend mode but never restores the previous values. This leaks state to other Images or code using the same Texture, violating encapsulation.
Save the texture's state before modifying, then restore it after rendering:
+ /* Save current texture state */ + Uint8 prev_r, prev_g, prev_b, prev_a; + SDL_BlendMode prev_blend; + if (SDL_GetTextureColorMod(self->texture->texture, &prev_r, &prev_g, &prev_b) < 0 || + SDL_GetTextureAlphaMod(self->texture->texture, &prev_a) < 0 || + SDL_GetTextureBlendMode(self->texture->texture, &prev_blend) < 0) { + PyErr_SetString(pgExc_SDLError, SDL_GetError()); + return 0; + } + + /* Apply Image's state */ RENDERER_PROPERTY_ERROR_CHECK(SDL_SetTextureColorMod( self->texture->texture, rgba[0], rgba[1], rgba[2])) RENDERER_PROPERTY_ERROR_CHECK( SDL_SetTextureAlphaMod(self->texture->texture, (Uint8)self->alpha)) RENDERER_PROPERTY_ERROR_CHECK( SDL_SetTextureBlendMode(self->texture->texture, self->blend_mode)) - if (SDL_RenderCopyExF(self->texture->renderer->renderer, - self->texture->texture, areasrcptr, destrectptr, - (double)self->angle, originptr, flip) < 0) { + int rc = SDL_RenderCopyExF(self->texture->renderer->renderer, + self->texture->texture, areasrcptr, destrectptr, + (double)self->angle, originptr, flip); + + /* Restore texture state (ignore errors) */ + SDL_SetTextureColorMod(self->texture->texture, prev_r, prev_g, prev_b); + SDL_SetTextureAlphaMod(self->texture->texture, prev_a); + SDL_SetTextureBlendMode(self->texture->texture, prev_blend); + + if (rc < 0) { PyErr_SetString(pgExc_SDLError, SDL_GetError()); return 0; }
1362-1372: Changing texture without validating srcrect can leave it out of bounds.The setter allows replacing the texture without checking if the current
srcrectis valid for the new texture's dimensions. This could lead to rendering errors or crashes.Consider either resetting srcrect to the new texture's full dimensions or validating and raising an error:
if (!pgTexture_Check(arg)) { RAISERETURN(PyExc_TypeError, "texture must be a Texture", -1); } + pgTextureObject *new_texture = (pgTextureObject *)arg; + /* Validate srcrect against new texture */ + if (self->srcrect && + (self->srcrect->r.x < 0 || self->srcrect->r.y < 0 || + self->srcrect->r.w < 0 || self->srcrect->r.h < 0 || + self->srcrect->r.x + self->srcrect->r.w > new_texture->width || + self->srcrect->r.y + self->srcrect->r.h > new_texture->height)) { + /* Reset srcrect to cover new texture */ + Py_XDECREF(self->srcrect); + self->srcrect = (pgRectObject *)pgRect_New4(0, 0, + new_texture->width, + new_texture->height); + } Py_XINCREF(arg); Py_XDECREF(self->texture); - self->texture = (pgTextureObject *)arg; + self->texture = new_texture; return 0;
🧹 Nitpick comments (3)
test/render_test.py (2)
530-531: Consider being more specific about the expected exception type.The test accepts either
TypeErrororValueErrorfor invalid alpha values. The C implementation at line 1458 raisesValueErrorfor out-of-range values, so you could test that more precisely.- with self.assertRaises((TypeError, ValueError)): + with self.assertRaises(ValueError): self.image.alpha = -1
66-66: Remove obsolete TODO comment.The TODO comment indicates uncertainty about Image implementation, but the tests at lines 573-597 now exercise Image.draw() functionality. This comment can be removed.
- self.renderer.blit(image, dest, area) # TODO Assert after Image implementation + self.renderer.blit(image, dest, area)src_c/render.c (1)
1640-1646: Consider supporting kwargs in Image.get_rect() for API consistency.Unlike
Texture.get_rect()(line 690-695), which supports keyword arguments for positioning,Image.get_rect()doesn't. This creates an API inconsistency.If the intent is to match
Texture.get_rect()behavior, you could use the same pattern:static PyObject * image_get_rect_method(pgImageObject *self, PyObject *const *args, Py_ssize_t nargs, PyObject *kwargs) { PyObject *rect = pgRect_New(&self->srcrect->r); - return rect; + return pgObject_getRectHelper(rect, args, nargs, kwargs, "rect"); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
buildconfig/stubs/pygame/_render.pyi(1 hunks)src_c/include/_pygame.h(1 hunks)src_c/render.c(3 hunks)test/render_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src_c/include/_pygame.h
- buildconfig/stubs/pygame/_render.pyi
🧰 Additional context used
🧬 Code graph analysis (2)
src_c/render.c (3)
src_c/color.c (2)
pgColor_NewLength(2364-2374)pg_RGBAFromObjEx(2421-2477)src_c/base.c (1)
pg_TwoFloatsFromObj(507-568)src_c/rect.c (1)
rect(780-849)
test/render_test.py (2)
buildconfig/stubs/pygame/_render.pyi (23)
renderer(106-106)Renderer(17-75)Texture(78-146)from_surface(108-108)Image(149-171)width(102-102)height(104-104)blend_mode(94-94)blend_mode(96-96)color(98-98)color(100-100)color(169-169)color(171-171)alpha(90-90)alpha(92-92)get_rect(145-145)get_rect(155-155)clear(33-33)blit(26-32)to_surface(47-49)draw(12-14)draw(109-117)draw(156-158)src_c/render.c (1)
_render(1725-1814)
🪛 Ruff (0.14.0)
test/render_test.py
569-569: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
571-571: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
⏰ 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: AMD64
- GitHub Check: x86_64
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: aarch64
- GitHub Check: build (windows-latest)
- GitHub Check: i686
- GitHub Check: build (macos-14)
- GitHub Check: Debian (Bookworm - 12) [s390x]
- GitHub Check: Debian (Bookworm - 12) [armv6]
- GitHub Check: debug_coverage (ubuntu-24.04, 3.14.0rc1)
- GitHub Check: msys2 (clang64, clang-x86_64)
- GitHub Check: Pyodide build
- GitHub Check: debug_coverage (ubuntu-24.04, 3.9.23)
- GitHub Check: dev-check
- GitHub Check: debug_coverage (ubuntu-24.04, 3.13.5)
- GitHub Check: msys2 (mingw64, x86_64)
- GitHub Check: msys2 (ucrt64, ucrt-x86_64)
- GitHub Check: build (ubuntu-22.04)
- GitHub Check: x86
| static int | ||
| image_set_srcrect(pgImageObject *self, PyObject *arg, void *closure) | ||
| { | ||
| SDL_Rect temp; | ||
| SDL_Rect *rectptr; | ||
| if (!arg || Py_IsNone(arg)) { | ||
| RAISERETURN(PyExc_TypeError, "srcrect must be a Rect", -1); | ||
| } | ||
| rectptr = pgRect_FromObject(arg, &temp); | ||
| if (!rectptr) { | ||
| RAISERETURN(PyExc_TypeError, "srcrect must be a Rect", -1); | ||
| } | ||
| Py_XDECREF(self->srcrect); | ||
| /* Create a new Rect from the exact values returned by pgRect_FromObject */ | ||
| self->srcrect = (pgRectObject *)pgRect_New(rectptr); | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
srcrect setter should validate bounds against texture dimensions.
Unlike image_init (which validates at lines 1252-1256), the srcrect setter doesn't check if the new rectangle is within the texture's bounds. This could allow invalid srcrects that cause rendering errors.
Add bounds validation:
rectptr = pgRect_FromObject(arg, &temp);
if (!rectptr) {
RAISERETURN(PyExc_TypeError, "srcrect must be a Rect", -1);
}
+ /* Validate bounds against texture */
+ if (self->texture &&
+ (rectptr->x < 0 || rectptr->y < 0 ||
+ rectptr->w < 0 || rectptr->h < 0 ||
+ rectptr->x + rectptr->w > self->texture->width ||
+ rectptr->y + rectptr->h > self->texture->height)) {
+ RAISERETURN(PyExc_ValueError, "srcrect is out of texture bounds", -1);
+ }
Py_XDECREF(self->srcrect);
/* Create a new Rect from the exact values returned by pgRect_FromObject */
self->srcrect = (pgRectObject *)pgRect_New(rectptr);🤖 Prompt for AI Agents
In src_c/render.c around lines 1384 to 1400, the image_set_srcrect setter
assigns a new Rect without checking that the rect lies within the underlying
texture dimensions; replicate the same bounds validation used in image_init
(around lines 1252-1256) after pgRect_FromObject returns rectptr: verify
rectptr->x and rectptr->y are >= 0 and rectptr->w and rectptr->h are >= 0 and
that rectptr->x + rectptr->w <= texture_width and rectptr->y + rectptr->h <=
texture_height (use the same fields or helpers used elsewhere to obtain the
surface/texture width and height); if validation fails, return an appropriate
Python exception (using RAISERETURN as in image_init) instead of assigning the
rect, otherwise proceed to Py_XDECREF and set self->srcrect as currently done.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src_c/render.c (4)
1293-1303: Make draw(srcrect=...) relative to the Image’s srcrect (match init).Currently, a non-None srcrect is treated as absolute texture coords. Offset it by self->srcrect->r.{x,y} and validate within the Image’s srcrect.
if (!Py_IsNone(area)) { if (!(areasrcptr = pgRect_FromObject(area, &areasrc))) { PyErr_SetString(PyExc_ValueError, "srcrect must be a Rect or None"); return 0; } + /* Interpret as relative to this Image's srcrect (like __init__) */ + areasrc.x += self->srcrect->r.x; + areasrc.y += self->srcrect->r.y; + /* Optional: validate against Image bounds */ + if (areasrc.w < 0 || areasrc.h < 0 || + areasrc.x < self->srcrect->r.x || + areasrc.y < self->srcrect->r.y || + areasrc.x + areasrc.w > self->srcrect->r.x + self->srcrect->r.w || + areasrc.y + areasrc.h > self->srcrect->r.y + self->srcrect->r.h) { + PyErr_SetString(PyExc_ValueError, "srcrect out of Image bounds"); + return 0; + } + areasrcptr = &areasrc; } else { areasrcptr = &self->srcrect->r; }
1335-1347: Fix error handling and restore texture state around draw.Two issues: (1) RENDERER_PROPERTY_ERROR_CHECK returns -1, but this function signals failure with 0; callers check “!ret”, so -1 is misinterpreted as success. (2) draw mutates the Texture’s color/alpha/blend and does not restore, leaking state.
- RENDERER_PROPERTY_ERROR_CHECK(SDL_SetTextureColorMod( - self->texture->texture, rgba[0], rgba[1], rgba[2])) - RENDERER_PROPERTY_ERROR_CHECK( - SDL_SetTextureAlphaMod(self->texture->texture, (Uint8)self->alpha)) - RENDERER_PROPERTY_ERROR_CHECK( - SDL_SetTextureBlendMode(self->texture->texture, self->blend_mode)) - - if (SDL_RenderCopyExF(self->texture->renderer->renderer, - self->texture->texture, areasrcptr, destrectptr, - (double)self->angle, originptr, flip) < 0) { - PyErr_SetString(pgExc_SDLError, SDL_GetError()); - return 0; - } + /* Save current texture mods */ + Uint8 prev_r, prev_g, prev_b, prev_a; + SDL_BlendMode prev_blend; + if (SDL_GetTextureColorMod(self->texture->texture, &prev_r, &prev_g, &prev_b) < 0 || + SDL_GetTextureAlphaMod(self->texture->texture, &prev_a) < 0 || + SDL_GetTextureBlendMode(self->texture->texture, &prev_blend) < 0) { + PyErr_SetString(pgExc_SDLError, SDL_GetError()); + return 0; + } + /* Apply Image mods */ + if (SDL_SetTextureColorMod(self->texture->texture, rgba[0], rgba[1], rgba[2]) < 0 || + SDL_SetTextureAlphaMod(self->texture->texture, (Uint8)self->alpha) < 0 || + SDL_SetTextureBlendMode(self->texture->texture, self->blend_mode) < 0) { + PyErr_SetString(pgExc_SDLError, SDL_GetError()); + return 0; + } + int rc = SDL_RenderCopyExF(self->texture->renderer->renderer, + self->texture->texture, areasrcptr, destrectptr, + (double)self->angle, originptr, flip); + /* Restore previous mods (ignore restore errors) */ + SDL_SetTextureColorMod(self->texture->texture, prev_r, prev_g, prev_b); + SDL_SetTextureAlphaMod(self->texture->texture, prev_a); + SDL_SetTextureBlendMode(self->texture->texture, prev_blend); + if (rc < 0) { + PyErr_SetString(pgExc_SDLError, SDL_GetError()); + return 0; + }
1362-1372: Changing texture should reconcile srcrect.Switching textures without validating or resetting srcrect can leave it out-of-bounds and render garbage or error.
Py_XINCREF(arg); Py_XDECREF(self->texture); self->texture = (pgTextureObject *)arg; + /* Ensure srcrect fits the new texture */ + if (!self->srcrect || + self->srcrect->r.x < 0 || self->srcrect->r.y < 0 || + self->srcrect->r.w < 0 || self->srcrect->r.h < 0 || + self->srcrect->r.x + self->srcrect->r.w > self->texture->width || + self->srcrect->r.y + self->srcrect->r.h > self->texture->height) { + Py_XDECREF(self->srcrect); + self->srcrect = (pgRectObject *)pgRect_New4( + 0, 0, self->texture->width, self->texture->height); + } return 0;
1384-1400: Validate srcrect setter against texture bounds.Setter accepts any Rect; must reject out-of-bounds rectangles to avoid invalid reads.
rectptr = pgRect_FromObject(arg, &temp); if (!rectptr) { RAISERETURN(PyExc_TypeError, "srcrect must be a Rect", -1); } + if (self->texture && + (rectptr->x < 0 || rectptr->y < 0 || + rectptr->w < 0 || rectptr->h < 0 || + rectptr->x + rectptr->w > self->texture->width || + rectptr->y + rectptr->h > self->texture->height)) { + RAISERETURN(PyExc_ValueError, "srcrect is out of texture bounds", -1); + } Py_XDECREF(self->srcrect); /* Create a new Rect from the exact values returned by pgRect_FromObject */ self->srcrect = (pgRectObject *)pgRect_New(rectptr);
🧹 Nitpick comments (1)
src_c/render.c (1)
1231-1262: Constructor srcrect semantics: relative offset check is good; consider clearer error text.Bounds check then offset mirrors intended “relative to base image” behavior. Consider changing the error to “srcrect is out of Image bounds” for clarity, matching setters/draw.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
buildconfig/stubs/pygame/_render.pyi(1 hunks)src_c/render.c(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- buildconfig/stubs/pygame/_render.pyi
🧰 Additional context used
🧬 Code graph analysis (1)
src_c/render.c (3)
src_c/color.c (2)
pgColor_NewLength(2364-2374)pg_RGBAFromObjEx(2421-2477)src_c/base.c (1)
pg_TwoFloatsFromObj(507-568)src_c/rect.c (1)
rect(780-849)
⏰ 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: debug_coverage (ubuntu-24.04, 3.13.5)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.14.0rc1)
- GitHub Check: AMD64
- GitHub Check: x86
- 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: build (windows-latest)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: build (macos-14)
- GitHub Check: dev-check
- GitHub Check: aarch64
- GitHub Check: x86_64
- GitHub Check: Pyodide build
- GitHub Check: i686
- GitHub Check: msys2 (ucrt64, ucrt-x86_64)
- GitHub Check: msys2 (mingw64, x86_64)
- GitHub Check: build (ubuntu-22.04)
- GitHub Check: msys2 (clang64, clang-x86_64)
🔇 Additional comments (2)
src_c/render.c (2)
1311-1315: Good: default dst size uses Image’s rect.Using areasrc (if provided) else self->srcrect for width/height when dst is a point matches expected semantics.
1648-1661: LGTM: image.draw forwards to renderer path.Argument parsing and delegation to image_renderer_draw look correct.
| static PyObject * | ||
| image_get_rect_method(pgImageObject *self, PyObject *const *args, | ||
| Py_ssize_t nargs, PyObject *kwargs) | ||
| { | ||
| PyObject *rect = pgRect_New(&self->srcrect->r); | ||
| return rect; | ||
| } |
There was a problem hiding this comment.
get_rect should honor kwargs (center=, topleft=, etc.) like Texture/Surface.
Current implementation ignores args/kwargs and returns a copy of srcrect. Use pgObject_getRectHelper with a 0,0 rect sized to the Image’s srcrect to match pygame semantics.
- PyObject *rect = pgRect_New(&self->srcrect->r);
- return rect;
+ PyObject *rect =
+ pgRect_New4(0, 0, self->srcrect->r.w, self->srcrect->r.h);
+ return pgObject_getRectHelper(rect, args, nargs, kwargs, "rect");🤖 Prompt for AI Agents
In src_c/render.c around lines 1640 to 1646, image_get_rect_method currently
ignores args/kwargs and always returns a copy of srcrect; modify it to build a
temporary rect at (0,0) with the width/height from self->srcrect->r, call
pgObject_getRectHelper with that rect and the provided args/kwargs (nargs, args,
kwargs) to honor center/topleft/etc., handle errors returned from the helper,
and return the helper's PyObject result (ensuring correct reference counting and
no memory leaks).
2 participants
Not quite ready to go, as I'm not sure how the
srcrectpassed to draw should behave, but here is most of a port of _sdl2.video.Image into C.I tried to maintain the original behavior as closely as possible, including some behaviors I think are maybe not intuitive (like draw overwriting the blend mode of its texture). It might be worth considering if any of these should be changed.