Conversation
commit 813f384 Author: Josip Komljenović <josipkoma@gmail.com> Date: Wed Jan 29 15:43:45 2025 +0100 Add interface for _sdl2.video classes commit 702958c Author: Josip Komljenović <josipkoma@gmail.com> Date: Wed Jan 29 15:38:01 2025 +0100 Add interface for _sdl2.video classes commit 081b032 Author: Josip Komljenović <josipkoma@gmail.com> Date: Wed Jan 29 15:33:20 2025 +0100 Add interface for _sdl2.video classes commit c9ffd1c Author: Josip Komljenović <josipkoma@gmail.com> Date: Wed Jan 29 15:25:10 2025 +0100 Add interface for _sdl2.video classes
commit 813f384 Author: Josip Komljenović <josipkoma@gmail.com> Date: Wed Jan 29 15:43:45 2025 +0100 Add interface for _sdl2.video classes commit 702958c Author: Josip Komljenović <josipkoma@gmail.com> Date: Wed Jan 29 15:38:01 2025 +0100 Add interface for _sdl2.video classes commit 081b032 Author: Josip Komljenović <josipkoma@gmail.com> Date: Wed Jan 29 15:33:20 2025 +0100 Add interface for _sdl2.video classes commit c9ffd1c Author: Josip Komljenović <josipkoma@gmail.com> Date: Wed Jan 29 15:25:10 2025 +0100 Add interface for _sdl2.video classes
|
@MightyJosip The other ones have been merged, now it's this PRs turn! Looks like it needs to be updated and done whatever needs to happen to get it ready for review. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a full Image public API in C and stubs: constructor, properties (alpha, angle, blend_mode, color, flip_x, flip_y, origin, srcrect, texture), methods get_rect and draw (srcrect/dstrect, origin, flips, color/alpha/blend), SDL texture helpers, type init/dealloc, and tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Py as Python
participant Img as Image
participant Tex as SDL_Texture
participant Ren as SDL_Renderer
Py->>Img: draw(srcrect?, dstrect?)
activate Img
Img->>Img: resolve srcrect / dstrect (defaults)
Img->>Img: compute origin, angle, flip flags
Img->>Tex: apply color modulation / alpha / blend mode (helpers)
Img->>Ren: SDL_RenderCopyExF(Tex, srcrect, dstrect, angle, origin, flips)
Ren-->>Img: render complete
deactivate Img
Img-->>Py: return / raise
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 |
|
I did a little bit of the merging work so we can see what we are working with here. Still needs tests and documentation. |
|
Ran this through some AI suggestions on my home PC and they seemed sensible changes. We'll see what the PR review AI has to say. Otherwise, this seems about as complete as the previous PRs in this series. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (2)
test/render_test.py (2)
617-623: Add an assertion that verifies actual Image rendering via blit()Currently we draw with Image.draw and validate pixels, but Renderer.blit(Image, ...) has no pixel assertion. Adding a minimal pixel check will catch regressions in renderer->image path.
Proposed addition:
@@ # Test the actual rendering result self.renderer.clear() self.image.draw(dstrect=pygame.Rect(20, 20, 50, 50)) result = self.renderer.to_surface() # The center of the drawn image should have the expected color self.assertEqual(result.get_at((45, 45)), pygame.Color(80, 120, 160, 255)) + + # Also verify the blit(Image, ...) path renders correctly + self.renderer.clear() + self.renderer.blit(self.image, pygame.Rect(10, 10, 50, 50)) + result = self.renderer.to_surface() + self.assertEqual(result.get_at((35, 35)), pygame.Color(80, 120, 160, 255))
653-682: Cover edge cases: srcrect=None reset, alpha clamping, and state isolation
- Test that setting
srcrect = Noneresets to full texture.- Verify alpha is clamped to [0,255] (no wrap to 246 for -10).
- Ensure per-Image modulation does not leak to the Texture or other Images.
Suggested tests:
@@ def test_property_type_errors(self): @@ with self.assertRaises(TypeError): self.image.texture = "invalid" + + def test_srcrect_none_resets_to_full(self): + self.image.srcrect = pygame.Rect(10, 10, 30, 30) + self.image.srcrect = None + self.assertEqual(self.image.get_rect(), pygame.Rect(0, 0, 50, 50)) + + def test_alpha_is_clamped_and_no_wrap(self): + self.image.alpha = -10 + self.assertEqual(self.image.alpha, 0.0) + self.image.alpha = 300 + self.assertEqual(self.image.alpha, 255.0) + + def test_texture_state_is_restored_after_image_draw(self): + # Set texture mods to known values, then draw a red-tinted Image. + self.texture.color = pygame.Color(255, 255, 255, 255) + self.texture.alpha = 255 + self.renderer.clear() + img = _render.Image(self.texture) + img.color = (255, 0, 0) + img.alpha = 128 + img.draw(dstrect=pygame.Rect(10, 10, 30, 30)) + # Draw the raw texture afterwards; it should not be stuck red/128 alpha. + self.texture.draw(dstrect=pygame.Rect(50, 10, 30, 30)) + result = self.renderer.to_surface() + self.assertEqual(result.get_at((65, 25)), pygame.Color(80, 120, 160, 255))
📜 Review details
Configuration used: Path: .coderabbit.yaml
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/_render.pyi(1 hunks)src_c/render.c(5 hunks)test/render_test.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
test/render_test.py (1)
buildconfig/stubs/pygame/_render.pyi (36)
renderer(106-106)Renderer(17-75)texture(188-188)texture(190-190)Texture(78-146)from_surface(108-108)Image(149-196)srcrect(184-184)srcrect(186-186)alpha(90-90)alpha(92-92)alpha(156-156)alpha(158-158)angle(160-160)angle(162-162)color(98-98)color(100-100)color(168-168)color(170-170)blend_mode(94-94)blend_mode(96-96)blend_mode(164-164)blend_mode(166-166)flip_x(172-172)flip_x(174-174)flip_y(176-176)flip_y(178-178)origin(180-180)origin(182-182)get_rect(145-145)get_rect(191-191)draw(12-14)draw(109-117)draw(192-196)clear(33-33)to_surface(47-49)
buildconfig/stubs/pygame/_render.pyi (3)
buildconfig/stubs/pygame/_sdl2/video.pyi (8)
color(62-62)color(64-64)color(124-124)color(126-126)get_rect(65-65)get_rect(110-110)draw(66-74)draw(111-113)buildconfig/stubs/pygame/color.pyi (1)
Color(12-94)buildconfig/stubs/pygame/rect.pyi (1)
Rect(303-303)
src_c/render.c (3)
src_c/base.c (1)
pg_TwoFloatsFromObj(507-568)src_c/color.c (3)
color(2508-2572)pg_RGBAFromObjEx(2421-2477)pgColor_NewLength(2364-2374)src_c/rect.c (1)
rect(772-841)
⏰ 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: arm64
- GitHub Check: x86_64
- GitHub Check: Debian (Bookworm - 12) [ppc64le]
- GitHub Check: Debian (Bookworm - 12) [armv6]
- GitHub Check: Debian (Bookworm - 12) [armv7]
- GitHub Check: Debian (Bookworm - 12) [s390x]
- GitHub Check: dev-check
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: msys2 (clang64, clang-x86_64)
- GitHub Check: msys2 (ucrt64, ucrt-x86_64)
- GitHub Check: build (ubuntu-22.04)
- GitHub Check: msys2 (mingw64, x86_64)
- GitHub Check: x86
- GitHub Check: AMD64
- GitHub Check: x86_64
- GitHub Check: aarch64
- GitHub Check: debug_coverage (ubuntu-24.04, 3.14.0rc1)
- GitHub Check: i686
- GitHub Check: debug_coverage (ubuntu-24.04, 3.13.5)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.9.23)
| def alpha(self) -> float: ... | ||
| @alpha.setter | ||
| def alpha(self, value: float) -> None: ... | ||
| @property | ||
| def angle(self) -> float: ... | ||
| @angle.setter | ||
| def angle(self, value: float) -> None: ... |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Alpha type differs from Texture.alpha
Image.alpha is typed as float here, while Texture.alpha is int. If fractional alpha isn’t intended as a public API difference, consider aligning Image.alpha to int for consistency, or document the divergence. Also, clamp in C to [0, 255] to avoid wrap-around on negative or >255 values.
🏁 Script executed:
#!/bin/bash
# Inspect recent lines around Image.alpha in _render.pyi
sed -n '140,200p' buildconfig/stubs/pygame/_render.pyi
# Locate the Texture class definition
rg -nP 'class Texture' -C3 buildconfig/stubs/pygame/_render.pyi
# Find all occurrences of .alpha in this file to compare types
rg -nP '\.alpha' -C2 buildconfig/stubs/pygame/_render.pyiLength of output: 2169
🏁 Script executed:
#!/bin/bash
# List all .pyi stub files in pygame stubs directory
find buildconfig/stubs/pygame -type f -name '*.pyi'
# Search for Texture.alpha or any alpha definitions within Texture class
rg -nP 'class Texture' -C20 buildconfig/stubs/pygame/*.pyi | sed -n '1,100p'
rg -nP 'Texture.*alpha' -C2 buildconfig/stubs/pygame/*.pyi
# Search across all stub files for 'alpha(self)' to see type annotations
rg -nP 'def alpha' -C2 buildconfig/stubs/pygame/*.pyiLength of output: 10391
Align or document alpha type mismatch: In buildconfig/stubs/pygame/_render.pyi, Texture.alpha (lines 90–92) is typed as int but Image.alpha (lines 156–158) uses float. Unless fractional alpha is a deliberate feature, change Image.alpha to int for consistency or document the intended float behavior. Also ensure the C implementation clamps values to [0, 255] to prevent wrap-around.
🤖 Prompt for AI Agents
In buildconfig/stubs/pygame/_render.pyi around lines 156–162 (and note
Texture.alpha at lines 90–92), Image.alpha is typed as float while Texture.alpha
is int; update Image.alpha to int for consistency (or if float is intended, add
a docstring explaining fractional alpha and behavior), adjust the setter
signature to accept int and return None, and ensure the C implementation clamps
input to the [0,255] range to avoid wrap-around (document clamping behavior in
the stub as well).
| static int | ||
| image_set_alpha(pgImageObject *self, PyObject *arg, void *closure) | ||
| { | ||
| if (!PyNumber_Check(arg)) { | ||
| RAISERETURN(PyExc_TypeError, "alpha must be real number", -1); | ||
| } | ||
| self->alpha = (float)PyFloat_AsDouble(arg); | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clamp alpha to [0, 255] to avoid unsigned wrap-around
Negative or >255 values currently wrap when cast to Uint8 in draw paths. Clamp once at assignment.
Apply:
static int
image_set_alpha(pgImageObject *self, PyObject *arg, void *closure)
{
if (!PyNumber_Check(arg)) {
RAISERETURN(PyExc_TypeError, "alpha must be real number", -1);
}
- self->alpha = (float)PyFloat_AsDouble(arg);
+ float a = (float)PyFloat_AsDouble(arg);
+ if (a < 0.0f) a = 0.0f;
+ if (a > 255.0f) a = 255.0f;
+ self->alpha = a;
return 0;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| static int | |
| image_set_alpha(pgImageObject *self, PyObject *arg, void *closure) | |
| { | |
| if (!PyNumber_Check(arg)) { | |
| RAISERETURN(PyExc_TypeError, "alpha must be real number", -1); | |
| } | |
| self->alpha = (float)PyFloat_AsDouble(arg); | |
| return 0; | |
| } | |
| static int | |
| image_set_alpha(pgImageObject *self, PyObject *arg, void *closure) | |
| { | |
| if (!PyNumber_Check(arg)) { | |
| RAISERETURN(PyExc_TypeError, "alpha must be real number", -1); | |
| } | |
| float a = (float)PyFloat_AsDouble(arg); | |
| if (a < 0.0f) a = 0.0f; | |
| if (a > 255.0f) a = 255.0f; | |
| self->alpha = a; | |
| return 0; | |
| } |
🤖 Prompt for AI Agents
src_c/render.c around lines 1311 to 1319: the setter currently casts the Python
number to float then later casts to Uint8 causing negative or >255 values to
wrap; read the value with PyFloat_AsDouble (check for error and return -1 if it
fails), clamp the resulting double to the range [0.0, 255.0], then assign the
clamped value to self->alpha (cast to float if needed) and return 0.
| static int | ||
| image_set_srcrect(pgImageObject *self, PyObject *arg, void *closure) | ||
| { | ||
| SDL_Rect *rect, temp; | ||
| pgRectObject *old_srcrect; | ||
| if (!(rect = pgRect_FromObject(arg, &temp))) { | ||
| RAISERETURN(PyExc_TypeError, "srcrect must be a rectangle", -1); | ||
| } | ||
| old_srcrect = self->srcrect; | ||
| self->srcrect = (pgRectObject *)pgRect_New(rect); | ||
| Py_XDECREF(old_srcrect); | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
srcrect setter should support None to reset to full bounds (matches stubs and init)
Allow None to mean “full texture” and rebuild srcrect accordingly.
Apply:
static int
image_set_srcrect(pgImageObject *self, PyObject *arg, void *closure)
{
- SDL_Rect *rect, temp;
- pgRectObject *old_srcrect;
- if (!(rect = pgRect_FromObject(arg, &temp))) {
- RAISERETURN(PyExc_TypeError, "srcrect must be a rectangle", -1);
- }
- old_srcrect = self->srcrect;
- self->srcrect = (pgRectObject *)pgRect_New(rect);
- Py_XDECREF(old_srcrect);
- return 0;
+ pgRectObject *old_srcrect = self->srcrect;
+ if (Py_IsNone(arg)) {
+ SDL_Rect full = {0, 0, self->texture->width, self->texture->height};
+ self->srcrect = (pgRectObject *)pgRect_New(&full);
+ Py_XDECREF(old_srcrect);
+ return 0;
+ }
+ SDL_Rect temp, *rect = pgRect_FromObject(arg, &temp);
+ if (!rect) {
+ RAISERETURN(PyExc_TypeError, "srcrect must be a rectangle or None", -1);
+ }
+ self->srcrect = (pgRectObject *)pgRect_New(rect);
+ Py_XDECREF(old_srcrect);
+ return 0;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| static int | |
| image_set_srcrect(pgImageObject *self, PyObject *arg, void *closure) | |
| { | |
| SDL_Rect *rect, temp; | |
| pgRectObject *old_srcrect; | |
| if (!(rect = pgRect_FromObject(arg, &temp))) { | |
| RAISERETURN(PyExc_TypeError, "srcrect must be a rectangle", -1); | |
| } | |
| old_srcrect = self->srcrect; | |
| self->srcrect = (pgRectObject *)pgRect_New(rect); | |
| Py_XDECREF(old_srcrect); | |
| return 0; | |
| } | |
| static int | |
| image_set_srcrect(pgImageObject *self, PyObject *arg, void *closure) | |
| { | |
| pgRectObject *old_srcrect = self->srcrect; | |
| if (Py_IsNone(arg)) { | |
| SDL_Rect full = {0, 0, self->texture->width, self->texture->height}; | |
| self->srcrect = (pgRectObject *)pgRect_New(&full); | |
| Py_XDECREF(old_srcrect); | |
| return 0; | |
| } | |
| SDL_Rect temp, *rect = pgRect_FromObject(arg, &temp); | |
| if (!rect) { | |
| RAISERETURN(PyExc_TypeError, "srcrect must be a rectangle or None", -1); | |
| } | |
| self->srcrect = (pgRectObject *)pgRect_New(rect); | |
| Py_XDECREF(old_srcrect); | |
| return 0; | |
| } |
| static int | ||
| image_init(pgImageObject *self, PyObject *args, PyObject *kwargs) | ||
| { | ||
| PyObject *texture_or_imageobj, *srcrectobj = Py_None; | ||
| pgTextureObject *textureprt; | ||
| SDL_Rect *rect, temp, old_srcrect; | ||
| SDL_BlendMode blend_mode; | ||
| Uint8 rgba[4] = {255, 255, 255, 255}; | ||
| char *keywords[] = {"texture_or_image", "srcrect", NULL}; | ||
| if (!PyArg_ParseTupleAndKeywords(args, kwargs, "O|O", keywords, | ||
| &texture_or_imageobj, &srcrectobj)) { | ||
| return -1; | ||
| } | ||
| if (pgTexture_Check(texture_or_imageobj)) { | ||
| textureprt = (pgTextureObject *)texture_or_imageobj; | ||
| temp = (SDL_Rect){0, 0, textureprt->width, textureprt->height}; | ||
| } | ||
| else if (pgImage_Check(texture_or_imageobj)) { | ||
| textureprt = ((pgImageObject *)texture_or_imageobj)->texture; | ||
| temp = ((pgImageObject *)texture_or_imageobj)->srcrect->r; | ||
| } | ||
| else { | ||
| RAISERETURN(PyExc_AttributeError, | ||
| "First argument must be either Texture or Image object", | ||
| -1); | ||
| } | ||
| self->texture = textureprt; | ||
| Py_INCREF(self->texture); | ||
| if (Py_IsNone(srcrectobj)) { | ||
| self->srcrect = (pgRectObject *)pgRect_New(&temp); | ||
| } | ||
| else { | ||
| old_srcrect = temp; | ||
| if (!(rect = pgRect_FromObject(srcrectobj, &temp))) { | ||
| RAISERETURN(PyExc_TypeError, "srcrect must be a rectangle or None", | ||
| -1); | ||
| } | ||
| if (rect->x < 0 || rect->y < 0 || rect->w < 0 || rect->h < 0 || | ||
| rect->x + rect->w > old_srcrect.w || | ||
| rect->y + rect->h > old_srcrect.h) { | ||
| RAISERETURN(PyExc_ValueError, "srcrect values are out of range", | ||
| -1); | ||
| } | ||
| rect->x += old_srcrect.x; | ||
| rect->y += old_srcrect.y; | ||
| self->srcrect = (pgRectObject *)pgRect_New(rect); | ||
| } | ||
| RENDERER_PROPERTY_ERROR_CHECK( | ||
| SDL_GetTextureBlendMode(self->texture->texture, &blend_mode)); | ||
| self->angle = 0; | ||
| self->blend_mode = blend_mode; | ||
| self->origin.x = 0; | ||
| self->origin.y = 0; | ||
| self->has_origin = SDL_FALSE; | ||
| self->flip_x = SDL_FALSE; | ||
| self->flip_y = SDL_FALSE; | ||
| self->alpha = 255; | ||
| self->color = (pgColorObject *)pgColor_NewLength(rgba, 4); | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
image_init: missing NULL checks on allocations
Check for allocation failures from pgRect_New and pgColor_NewLength and early-out cleanly to avoid crashes.
Apply:
@@
- if (Py_IsNone(srcrectobj)) {
- self->srcrect = (pgRectObject *)pgRect_New(&temp);
- }
+ if (Py_IsNone(srcrectobj)) {
+ self->srcrect = (pgRectObject *)pgRect_New(&temp);
+ if (!self->srcrect) {
+ return -1;
+ }
+ }
@@
- self->color = (pgColorObject *)pgColor_NewLength(rgba, 4);
- return 0;
+ self->color = (pgColorObject *)pgColor_NewLength(rgba, 4);
+ if (!self->color) {
+ return -1;
+ }
+ return 0;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| static int | |
| image_init(pgImageObject *self, PyObject *args, PyObject *kwargs) | |
| { | |
| PyObject *texture_or_imageobj, *srcrectobj = Py_None; | |
| pgTextureObject *textureprt; | |
| SDL_Rect *rect, temp, old_srcrect; | |
| SDL_BlendMode blend_mode; | |
| Uint8 rgba[4] = {255, 255, 255, 255}; | |
| char *keywords[] = {"texture_or_image", "srcrect", NULL}; | |
| if (!PyArg_ParseTupleAndKeywords(args, kwargs, "O|O", keywords, | |
| &texture_or_imageobj, &srcrectobj)) { | |
| return -1; | |
| } | |
| if (pgTexture_Check(texture_or_imageobj)) { | |
| textureprt = (pgTextureObject *)texture_or_imageobj; | |
| temp = (SDL_Rect){0, 0, textureprt->width, textureprt->height}; | |
| } | |
| else if (pgImage_Check(texture_or_imageobj)) { | |
| textureprt = ((pgImageObject *)texture_or_imageobj)->texture; | |
| temp = ((pgImageObject *)texture_or_imageobj)->srcrect->r; | |
| } | |
| else { | |
| RAISERETURN(PyExc_AttributeError, | |
| "First argument must be either Texture or Image object", | |
| -1); | |
| } | |
| self->texture = textureprt; | |
| Py_INCREF(self->texture); | |
| if (Py_IsNone(srcrectobj)) { | |
| self->srcrect = (pgRectObject *)pgRect_New(&temp); | |
| } | |
| else { | |
| old_srcrect = temp; | |
| if (!(rect = pgRect_FromObject(srcrectobj, &temp))) { | |
| RAISERETURN(PyExc_TypeError, "srcrect must be a rectangle or None", | |
| -1); | |
| } | |
| if (rect->x < 0 || rect->y < 0 || rect->w < 0 || rect->h < 0 || | |
| rect->x + rect->w > old_srcrect.w || | |
| rect->y + rect->h > old_srcrect.h) { | |
| RAISERETURN(PyExc_ValueError, "srcrect values are out of range", | |
| -1); | |
| } | |
| rect->x += old_srcrect.x; | |
| rect->y += old_srcrect.y; | |
| self->srcrect = (pgRectObject *)pgRect_New(rect); | |
| } | |
| RENDERER_PROPERTY_ERROR_CHECK( | |
| SDL_GetTextureBlendMode(self->texture->texture, &blend_mode)); | |
| self->angle = 0; | |
| self->blend_mode = blend_mode; | |
| self->origin.x = 0; | |
| self->origin.y = 0; | |
| self->has_origin = SDL_FALSE; | |
| self->flip_x = SDL_FALSE; | |
| self->flip_y = SDL_FALSE; | |
| self->alpha = 255; | |
| self->color = (pgColorObject *)pgColor_NewLength(rgba, 4); | |
| return 0; | |
| } | |
| static int | |
| image_init(pgImageObject *self, PyObject *args, PyObject *kwargs) | |
| { | |
| PyObject *texture_or_imageobj, *srcrectobj = Py_None; | |
| pgTextureObject *textureprt; | |
| SDL_Rect *rect, temp, old_srcrect; | |
| SDL_BlendMode blend_mode; | |
| Uint8 rgba[4] = {255, 255, 255, 255}; | |
| char *keywords[] = {"texture_or_image", "srcrect", NULL}; | |
| if (!PyArg_ParseTupleAndKeywords(args, kwargs, "O|O", keywords, | |
| &texture_or_imageobj, &srcrectobj)) { | |
| return -1; | |
| } | |
| if (pgTexture_Check(texture_or_imageobj)) { | |
| textureprt = (pgTextureObject *)texture_or_imageobj; | |
| temp = (SDL_Rect){0, 0, textureprt->width, textureprt->height}; | |
| } | |
| else if (pgImage_Check(texture_or_imageobj)) { | |
| textureprt = ((pgImageObject *)texture_or_imageobj)->texture; | |
| temp = ((pgImageObject *)texture_or_imageobj)->srcrect->r; | |
| } | |
| else { | |
| RAISERETURN(PyExc_AttributeError, | |
| "First argument must be either Texture or Image object", | |
| -1); | |
| } | |
| self->texture = textureprt; | |
| Py_INCREF(self->texture); | |
| if (Py_IsNone(srcrectobj)) { | |
| self->srcrect = (pgRectObject *)pgRect_New(&temp); | |
| if (!self->srcrect) { | |
| return -1; | |
| } | |
| } | |
| else { | |
| old_srcrect = temp; | |
| if (!(rect = pgRect_FromObject(srcrectobj, &temp))) { | |
| RAISERETURN(PyExc_TypeError, "srcrect must be a rectangle or None", | |
| -1); | |
| } | |
| if (rect->x < 0 || rect->y < 0 || rect->w < 0 || rect->h < 0 || | |
| rect->x + rect->w > old_srcrect.w || | |
| rect->y + rect->h > old_srcrect.h) { | |
| RAISERETURN(PyExc_ValueError, "srcrect values are out of range", | |
| -1); | |
| } | |
| rect->x += old_srcrect.x; | |
| rect->y += old_srcrect.y; | |
| self->srcrect = (pgRectObject *)pgRect_New(rect); | |
| } | |
| RENDERER_PROPERTY_ERROR_CHECK( | |
| SDL_GetTextureBlendMode(self->texture->texture, &blend_mode)); | |
| self->angle = 0; | |
| self->blend_mode = blend_mode; | |
| self->origin.x = 0; | |
| self->origin.y = 0; | |
| self->has_origin = SDL_FALSE; | |
| self->flip_x = SDL_FALSE; | |
| self->flip_y = SDL_FALSE; | |
| self->alpha = 255; | |
| self->color = (pgColorObject *)pgColor_NewLength(rgba, 4); | |
| if (!self->color) { | |
| return -1; | |
| } | |
| return 0; | |
| } |
🤖 Prompt for AI Agents
In src_c/render.c around lines 1474-1533, image_init currently assumes
pgRect_New and pgColor_NewLength succeed; add NULL checks after each allocation
and cleanly return -1 on failure while releasing any owned references: after
Py_INCREF(self->texture) if pgRect_New(...) returns NULL decref self->texture
and return -1 (do this for both the default srcrect and the srcrect created from
rect), and after creating self->color check for NULL and on failure DECREF
self->srcrect (if set) and self->texture before returning -1; ensure no memory
or reference leaks in each early-exit path.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@buildconfig/stubs/pygame/_render.pyi`:
- Around line 148-152: The __init__ signature in this stub uses unimported
typing names Union and Optional; replace them with the modern PEP 604 style (use
Texture | Image and RectLike | None or RectLike | None as appropriate) to avoid
missing imports and match the rest of the file — update the __init__ parameter
types in the class (the constructor def __init__(self, texture_or_image: ... ,
srcrect: ... ) -> None) and apply the same change to every other Optional/Union
usage in this class (the other method/attribute annotations that currently use
Optional/Union) so the file consistently uses X | Y and X | None instead of
typing.Optional/typing.Union.
- Around line 181-184: The srcrect property annotation is incorrect: the getter
always returns a pgRectObject (Rect) per the C implementation
(image_get_srcrect), so change the getter return type from Optional[RectLike] to
Rect; also update the setter signature to accept Optional[RectLike] (allowing
None to reset bounds) or remove Optional if the C setter does not accept
None—ensure the annotations for "srcrect" (getter and `@srcrect.setter`) match the
C behavior (image_get_srcrect and the image setter) so consumers get accurate
types.
In `@src_c/render.c`:
- Around line 1347-1355: In image_set_blend_mode, after calling
PyLong_AsLong(arg) check for overflow by testing the result and PyErr_Occurred
(or use PyLong_Check + PyLong_AsLong and then if value == -1 &&
PyErr_Occurred()) before assigning to self->blend_mode; if an error occurred, do
not assign and return -1 to propagate the Python exception, otherwise cast the
long to SDL_BlendMode and return 0. Ensure you still return 0 on success and -1
on error so the setter follows the expected Python C API convention.
- Around line 1331-1339: The setter image_set_angle currently calls
PyFloat_AsDouble and silently treats -1.0 as a valid angle; change it to detect
and propagate conversion errors: call PyFloat_AsDouble into a double (e.g.,
double val = PyFloat_AsDouble(arg)); if val == -1.0 && PyErr_Occurred() return
-1 so the exception is propagated, otherwise assign self->angle = (float)val and
return 0; keep the existing type check (PyNumber_Check) and RAISERETURN usage
for the non-number path.
🧹 Nitpick comments (2)
test/render_test.py (2)
532-534: Ambiguous test — silently passes whether or not clamping is implementedThe comment says "should not raise error but may clamp", but neither the stored value nor the clamped range is asserted. If clamping is the intended behavior (as suggested by past reviews), assert the result:
self.image.alpha = -10 self.assertEqual(self.image.alpha, 0.0) # clamped to lower bound self.image.alpha = 300 self.assertEqual(self.image.alpha, 255.0) # clamped to upper boundIf unclamped storage is intentional, assert that instead and document why.
606-614:get_rectkwargs not tested — consider adding acenter=test
Texture.get_rectsupports kwargs likecenter=(tested at line 300-302). IfImage.get_rectis intended to match (per the stub), add a similar test:rect = self.image.get_rect(center=(50, 50)) self.assertEqual(rect, pygame.Rect(25, 25, 50, 50))This would also catch the current C-side METH_NOARGS mismatch.
| def __init__( | ||
| self, | ||
| texture_or_image: Union['Texture', 'Image'], | ||
| srcrect: Optional[RectLike] = None, | ||
| ) -> None: ... |
There was a problem hiding this comment.
Union and Optional are used but not imported — this will fail type checking
Lines 150–151 use Union and Optional, but they aren't imported at line 2. The rest of this file uses the modern X | Y and X | None syntax (e.g., lines 26–28). Switch to the same style for consistency and to avoid the missing import.
🐛 Proposed fix
def __init__(
self,
- texture_or_image: Union['Texture', 'Image'],
- srcrect: Optional[RectLike] = None,
+ texture_or_image: "Texture" | "Image",
+ srcrect: RectLike | None = None,
) -> None: ...Apply the same pattern to all other Optional usages in this class (lines 178, 180, 182, 184, 192, 193).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def __init__( | |
| self, | |
| texture_or_image: Union['Texture', 'Image'], | |
| srcrect: Optional[RectLike] = None, | |
| ) -> None: ... | |
| def __init__( | |
| self, | |
| texture_or_image: "Texture" | "Image", | |
| srcrect: RectLike | None = None, | |
| ) -> None: ... |
🤖 Prompt for AI Agents
In `@buildconfig/stubs/pygame/_render.pyi` around lines 148 - 152, The __init__
signature in this stub uses unimported typing names Union and Optional; replace
them with the modern PEP 604 style (use Texture | Image and RectLike | None or
RectLike | None as appropriate) to avoid missing imports and match the rest of
the file — update the __init__ parameter types in the class (the constructor def
__init__(self, texture_or_image: ... , srcrect: ... ) -> None) and apply the
same change to every other Optional/Union usage in this class (the other
method/attribute annotations that currently use Optional/Union) so the file
consistently uses X | Y and X | None instead of typing.Optional/typing.Union.
| @property | ||
| def srcrect(self) -> Optional[RectLike]: ... | ||
| @srcrect.setter | ||
| def srcrect(self, value: Optional[RectLike]) -> None: ... |
There was a problem hiding this comment.
srcrect getter return type should be Rect, not Optional[RectLike]
The C implementation (image_get_srcrect) always returns a pgRectObject — it never returns None. The getter return type should be Rect to match reality and provide accurate type information to consumers.
Also, as noted in a prior review, the setter should accept None (to reset to full-texture bounds) or the Optional annotation should be removed if None is not supported.
🔧 Proposed fix (assuming C setter is updated to accept None)
`@property`
- def srcrect(self) -> Optional[RectLike]: ...
+ def srcrect(self) -> Rect: ...
`@srcrect.setter`
- def srcrect(self, value: Optional[RectLike]) -> None: ...
+ def srcrect(self, value: RectLike | None) -> None: ...📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @property | |
| def srcrect(self) -> Optional[RectLike]: ... | |
| @srcrect.setter | |
| def srcrect(self, value: Optional[RectLike]) -> None: ... | |
| `@property` | |
| def srcrect(self) -> Rect: ... | |
| `@srcrect.setter` | |
| def srcrect(self, value: RectLike | None) -> None: ... |
🤖 Prompt for AI Agents
In `@buildconfig/stubs/pygame/_render.pyi` around lines 181 - 184, The srcrect
property annotation is incorrect: the getter always returns a pgRectObject
(Rect) per the C implementation (image_get_srcrect), so change the getter return
type from Optional[RectLike] to Rect; also update the setter signature to accept
Optional[RectLike] (allowing None to reset bounds) or remove Optional if the C
setter does not accept None—ensure the annotations for "srcrect" (getter and
`@srcrect.setter`) match the C behavior (image_get_srcrect and the image setter)
so consumers get accurate types.
| static int | ||
| image_set_angle(pgImageObject *self, PyObject *arg, void *closure) | ||
| { | ||
| if (!PyNumber_Check(arg)) { | ||
| RAISERETURN(PyExc_TypeError, "angle must be real number", -1); | ||
| } | ||
| self->angle = (float)PyFloat_AsDouble(arg); | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
PyFloat_AsDouble error not checked in image_set_angle
Same pattern as alpha: PyFloat_AsDouble can fail (returns -1.0 and sets an exception) even after PyNumber_Check passes, e.g. if __float__ raises. The error is silently swallowed and self->angle is set to -1.0.
🔧 Proposed fix
self->angle = (float)PyFloat_AsDouble(arg);
+ if (self->angle == -1.0f && PyErr_Occurred()) {
+ return -1;
+ }🤖 Prompt for AI Agents
In `@src_c/render.c` around lines 1331 - 1339, The setter image_set_angle
currently calls PyFloat_AsDouble and silently treats -1.0 as a valid angle;
change it to detect and propagate conversion errors: call PyFloat_AsDouble into
a double (e.g., double val = PyFloat_AsDouble(arg)); if val == -1.0 &&
PyErr_Occurred() return -1 so the exception is propagated, otherwise assign
self->angle = (float)val and return 0; keep the existing type check
(PyNumber_Check) and RAISERETURN usage for the non-number path.
| static int | ||
| image_set_blend_mode(pgImageObject *self, PyObject *arg, void *closure) | ||
| { | ||
| if (!PyLong_Check(arg)) { | ||
| RAISERETURN(PyExc_TypeError, "Blend mode must be an integer", -1); | ||
| } | ||
| self->blend_mode = (SDL_BlendMode)(PyLong_AsLong(arg)); | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
PyLong_AsLong overflow not checked in image_set_blend_mode
If arg is a Python int that overflows long, PyLong_AsLong returns -1 and sets OverflowError, but the code doesn't check for this, leaving a pending exception while still assigning blend_mode = -1.
🔧 Proposed fix
static int
image_set_blend_mode(pgImageObject *self, PyObject *arg, void *closure)
{
if (!PyLong_Check(arg)) {
RAISERETURN(PyExc_TypeError, "Blend mode must be an integer", -1);
}
- self->blend_mode = (SDL_BlendMode)(PyLong_AsLong(arg));
+ long val = PyLong_AsLong(arg);
+ if (val == -1 && PyErr_Occurred()) {
+ return -1;
+ }
+ self->blend_mode = (SDL_BlendMode)val;
return 0;
}🤖 Prompt for AI Agents
In `@src_c/render.c` around lines 1347 - 1355, In image_set_blend_mode, after
calling PyLong_AsLong(arg) check for overflow by testing the result and
PyErr_Occurred (or use PyLong_Check + PyLong_AsLong and then if value == -1 &&
PyErr_Occurred()) before assigning to self->blend_mode; if an error occurred, do
not assign and return -1 to propagate the Python exception, otherwise cast the
long to SDL_BlendMode and return 0. Ensure you still return 0 on success and -1
on error so the setter follows the expected Python C API convention.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src_c/render.c`:
- Around line 1216-1218: The return in image_renderer_draw uses the RAISE macro
which expands to return a PyObject* causing a type mismatch for the function
that returns int; replace the RAISE usage with RAISERETURN or explicitly call
PyErr_SetString (or similar) and then return 0 so the function returns an int
while setting the Python error; update the branch that currently does `return
RAISE(PyExc_ValueError, "dstrect must be a point, Rect, or None");` to use
RAISERETURN or PyErr_SetString + `return 0` to fix the compilation error and
preserve the Python exception (refer to RAISE, RAISERETURN, and
image_renderer_draw).
- Around line 1214-1222: The dstrectptr remains NULL when
pgFRect_FromObject(dest, &dstrect) fails but pg_TwoFloatsFromObj(dest,
&dstrect.x, &dstrect.y) succeeds, so SDL_RenderCopyExF gets a NULL destination
and ignores the position; fix by setting dstrectptr = &dstrect after the
successful pg_TwoFloatsFromObj fallback in the draw function (and make the
identical change in image_draw), ensuring subsequent calls (SDL_RenderCopyExF)
receive the address of dstrect instead of NULL; reference symbols:
pgFRect_FromObject, pg_TwoFloatsFromObj, dstrectptr, dstrect, SDL_RenderCopyExF,
and image_draw.
🧹 Nitpick comments (2)
src_c/render.c (2)
1359-1371:image_set_coloroverrides the exception already set bypg_RGBAFromObjEx.When
pg_RGBAFromObjExfails, it sets its own descriptive exception. The subsequentRAISERETURN(PyExc_TypeError, ...)replaces it with a generic message, losing context. Simplyreturn -1instead to propagate the original error.♻️ Proposed fix
if (!pg_RGBAFromObjEx(arg, color, PG_COLOR_HANDLE_ALL)) { - RAISERETURN(PyExc_TypeError, "Unable to convert argument to Color", - -1); + return -1; }
1459-1471:image_set_texturedoesn't updatesrcrectwhen texture changes.When the texture is replaced, the existing
srcrectmay reference dimensions from the old texture and could now be out of bounds for the new one. Consider whethersrcrectshould be reset to the new texture's full bounds (matching__init__behavior), or at minimum document that it's the caller's responsibility.
| if (!(dstrectptr = pgFRect_FromObject(dest, &dstrect))) { | ||
| if (!pg_TwoFloatsFromObj(dest, &dstrect.x, &dstrect.y)) { | ||
| return RAISE(PyExc_ValueError, | ||
| "dstrect must be a point, Rect, or None"); | ||
| } | ||
| dstrect.w = (float)self->texture->width; | ||
| dstrect.h = (float)self->texture->height; | ||
| } | ||
| } |
There was a problem hiding this comment.
dstrectptr remains NULL when dest is a point — draw ignores the position.
When pgFRect_FromObject fails (returns NULL into dstrectptr) and the pg_TwoFloatsFromObj fallback succeeds, dstrect is populated but dstrectptr is never reassigned to &dstrect. The subsequent SDL_RenderCopyExF call receives NULL as the destination, rendering to the full viewport instead of the requested position.
The same bug exists in image_draw at lines 1275–1283.
🐛 Proposed fix
if (!Py_IsNone(dest)) {
if (!(dstrectptr = pgFRect_FromObject(dest, &dstrect))) {
if (!pg_TwoFloatsFromObj(dest, &dstrect.x, &dstrect.y)) {
- return RAISE(PyExc_ValueError,
- "dstrect must be a point, Rect, or None");
+ RAISERETURN(PyExc_ValueError,
+ "dstrect must be a point, Rect, or None", 0);
}
dstrect.w = (float)self->texture->width;
dstrect.h = (float)self->texture->height;
+ dstrectptr = &dstrect;
}
}And similarly in image_draw (lines 1275–1283):
if (!Py_IsNone(dstrectobj)) {
if (!(dstrectptr = pgFRect_FromObject(dstrectobj, &dstrect))) {
if (!pg_TwoFloatsFromObj(dstrectobj, &dstrect.x, &dstrect.y)) {
return RAISE(PyExc_ValueError,
"dstrect must be a point, Rect, or None");
}
dstrect.w = (float)self->texture->width;
dstrect.h = (float)self->texture->height;
+ dstrectptr = &dstrect;
}
}🧰 Tools
🪛 GitHub Actions: python3 dev.py all
[error] 1216-1216: Compilation error in C extension: returning a void* from a function with return type int due to RAISE macro expansion in render.c (RAISE expands to returning NULL). This causes metadata generation for the wheel to fail.
🤖 Prompt for AI Agents
In `@src_c/render.c` around lines 1214 - 1222, The dstrectptr remains NULL when
pgFRect_FromObject(dest, &dstrect) fails but pg_TwoFloatsFromObj(dest,
&dstrect.x, &dstrect.y) succeeds, so SDL_RenderCopyExF gets a NULL destination
and ignores the position; fix by setting dstrectptr = &dstrect after the
successful pg_TwoFloatsFromObj fallback in the draw function (and make the
identical change in image_draw), ensuring subsequent calls (SDL_RenderCopyExF)
receive the address of dstrect instead of NULL; reference symbols:
pgFRect_FromObject, pg_TwoFloatsFromObj, dstrectptr, dstrect, SDL_RenderCopyExF,
and image_draw.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@test/render_test.py`:
- Around line 485-494: The file test/render_test.py failed ruff-format; run ruff
format on this file (e.g., `ruff format test/render_test.py`) or apply the same
formatting changes locally, ensuring the ImageTest class and its setUp method
(references: class ImageTest, def setUp, self.window, self.renderer,
self.texture, self.image) match ruff's output, then commit the formatted file so
CI no longer reports formatting differences.
- Around line 656-662: The test test_invalid_init_params currently expects
AttributeError for a wrong-type argument to _render.Image, which is
unconventional; update the code so that the Image constructor raises TypeError
for invalid argument types (validate the argument type early in the C
implementation where Image is constructed—e.g., the Image_init / Image_new
routine that currently probes `.texture`) by setting
PyErr_SetString(PyExc_TypeError, ...) and returning NULL, and then update this
test (test_invalid_init_params) to assert TypeError for both
_render.Image("invalid") and _render.Image(self.texture, "invalid_rect") to
match the corrected behavior; if the AttributeError is intentional, instead add
a clarifying comment in the C constructor and leave the test as-is.
- Around line 532-534: The test currently sets out-of-range alpha values but
doesn't verify the stored result; update the test around the self.image.alpha
assignments to assert the implementation's post-set behavior (e.g., if alpha
should be clamped, assert self.image.alpha == 0 after assigning -10 and assert
self.image.alpha == 255 after assigning 300), referencing the self.image.alpha
property to locate the assertions to add.
🧹 Nitpick comments (3)
test/render_test.py (3)
486-486: Uninformative skip reason; consider linking to a tracking issue.The skip message
"Skip"doesn't explain why the suite is disabled. If it's blocked on the C implementation landing first, say so (e.g.,"Image C implementation not yet merged"). This helps anyone triaging test results understand whether the skip is intentional or stale.
633-654: Transformation draws are smoke-tested only — no pixel verification.Every
draw()call here just asserts "no crash." Consider adding at least oneto_surface()+get_at()check to confirm a transformation actually took effect (e.g., verify that a flipped image differs from the unflipped case, or that a rotated image places pixels in a different location).
509-514: Consider verifying that other properties propagate from the source Image.When constructing from another Image, only
textureandsrcrectare verified. If the source image has non-defaultalpha,angle,color,flip_x,flip_y,origin, orblend_mode, do those transfer? If yes, asserting them would strengthen the test. If no, asserting the defaults would document that behavior.
|
|
||
| @unittest.skip("Skip") | ||
| class ImageTest(unittest.TestCase): | ||
| def setUp(self): | ||
| self.window = pygame.Window(size=(100, 100)) | ||
| self.renderer = _render.Renderer(self.window) | ||
| self.surface = pygame.Surface((50, 50)) | ||
| self.surface.fill(pygame.Color(80, 120, 160, 255)) | ||
| self.texture = _render.Texture.from_surface(self.renderer, self.surface) | ||
| self.image = _render.Image(self.texture) |
There was a problem hiding this comment.
Pipeline failure: ruff-format reports formatting differences.
The CI run failed because ruff-format reformatted this file. Run ruff format test/render_test.py locally and commit the result.
🤖 Prompt for AI Agents
In `@test/render_test.py` around lines 485 - 494, The file test/render_test.py
failed ruff-format; run ruff format on this file (e.g., `ruff format
test/render_test.py`) or apply the same formatting changes locally, ensuring the
ImageTest class and its setUp method (references: class ImageTest, def setUp,
self.window, self.renderer, self.texture, self.image) match ruff's output, then
commit the formatted file so CI no longer reports formatting differences.
| # Test invalid alpha values (should not raise error but may clamp) | ||
| self.image.alpha = -10 | ||
| self.image.alpha = 300 |
There was a problem hiding this comment.
Assert the resulting value after out-of-range alpha assignments.
Setting -10 and 300 without checking the stored result means you don't know whether the implementation clamps, wraps, or stores the raw value. Add assertions for the expected post-set behavior.
Proposed fix (assuming clamping to 0–255)
- # Test invalid alpha values (should not raise error but may clamp)
- self.image.alpha = -10
- self.image.alpha = 300
+ # Out-of-range values should be clamped
+ self.image.alpha = -10
+ self.assertGreaterEqual(self.image.alpha, 0.0)
+ self.image.alpha = 300
+ self.assertLessEqual(self.image.alpha, 255.0)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Test invalid alpha values (should not raise error but may clamp) | |
| self.image.alpha = -10 | |
| self.image.alpha = 300 | |
| # Out-of-range values should be clamped | |
| self.image.alpha = -10 | |
| self.assertGreaterEqual(self.image.alpha, 0.0) | |
| self.image.alpha = 300 | |
| self.assertLessEqual(self.image.alpha, 255.0) |
🤖 Prompt for AI Agents
In `@test/render_test.py` around lines 532 - 534, The test currently sets
out-of-range alpha values but doesn't verify the stored result; update the test
around the self.image.alpha assignments to assert the implementation's post-set
behavior (e.g., if alpha should be clamped, assert self.image.alpha == 0 after
assigning -10 and assert self.image.alpha == 255 after assigning 300),
referencing the self.image.alpha property to locate the assertions to add.
| def test_invalid_init_params(self): | ||
| """Test Image initialization with invalid parameters""" | ||
| with self.assertRaises(AttributeError): | ||
| _render.Image("invalid") | ||
|
|
||
| with self.assertRaises(TypeError): | ||
| _render.Image(self.texture, "invalid_rect") |
There was a problem hiding this comment.
AttributeError for a wrong-type argument is unconventional — consider TypeError.
Line 658 expects AttributeError when passing "invalid" to Image(). The standard Python convention for a wrong-type argument is TypeError. If the C implementation currently raises AttributeError because it probes for a .texture attribute, that's an implementation leak — it should ideally validate the type first and raise TypeError.
If this matches the current C behavior intentionally, a comment explaining why would help. Otherwise, update both the C code and this test.
🤖 Prompt for AI Agents
In `@test/render_test.py` around lines 656 - 662, The test
test_invalid_init_params currently expects AttributeError for a wrong-type
argument to _render.Image, which is unconventional; update the code so that the
Image constructor raises TypeError for invalid argument types (validate the
argument type early in the C implementation where Image is constructed—e.g., the
Image_init / Image_new routine that currently probes `.texture`) by setting
PyErr_SetString(PyExc_TypeError, ...) and returning NULL, and then update this
test (test_invalid_init_params) to assert TypeError for both
_render.Image("invalid") and _render.Image(self.texture, "invalid_rect") to
match the corrected behavior; if the AttributeError is intentional, instead add
a clarifying comment in the C constructor and leave the test as-is.
4 participants
This PR converts pygame._sdl2.video.Image to C code. It has the implementation of all the Image methods and attributes. It doesn't have docs/type hints/tests or stuff like that.
Leaving this as the draft atm, because it depends on #3317, #3327 and #3330, pushing just for the internal testing purposes