Skip to content

Conversation

@Starbuck5
Copy link
Member

Rather than relying on pygame.init() being called, since not all workflows would do that.

This just moves where the code added in #3654 triggers. Thanks to reviewers on that for reviewing it so quickly, I'm sort of embarrassed to already want to change it.

But I was thinking about what Ankith said in his review, that we wouldn't want SDL to ever override signal handlers from Python. And my previous PR didn't do that. It just turned it off if one happened to call pygame.init(). Like in my audio example script in my other recent PR I don't call pygame.init(), I just call audio.init().

Rather than relying on pygame.init() being called, since not all workflows would do that.
@Starbuck5 Starbuck5 requested a review from a team as a code owner December 10, 2025 08:37
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 10, 2025

📝 Walkthrough

Walkthrough

The change relocates SDL3 signal handler hint configuration from pg_init to base module initialization, preserving functionality while altering the initialization sequence.

Changes

Cohort / File(s) Summary
SDL3 Signal Handler Hint Relocation
src_c/base.c
Removed SDL_SetHint(SDL_HINT_NO_SIGNAL_HANDLERS, "1") from pg_init SDL3 conditional block and added it to the base module initialization (MODINIT_DEFINE(base)) SDL3 path

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~5–10 minutes

  • Verify that moving the hint configuration from pg_init to base module initialization does not alter the initialization order or introduce timing issues
  • Confirm that the hint is still applied early enough in the initialization sequence to be effective

Possibly related PRs

Suggested labels

sdl3

Suggested reviewers

  • gresm
  • ankith26

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Move SDL hint so it always triggers on import' clearly and specifically describes the main change: relocating SDL hint initialization from pygame.init() to module import time.
Description check ✅ Passed The description is directly related to the changeset, explaining the motivation for moving the SDL hint from pygame.init() to import-time to handle workflows that don't call pygame.init().
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6f3b01a and 2cebfb2.

📒 Files selected for processing (1)
  • src_c/base.c (1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: oddbookworm
Repo: pygame-community/pygame-ce PR: 0
File: :0-0
Timestamp: 2025-11-03T19:52:35.950Z
Learning: In translation PRs for the pygame-ce repository: When identifying potential new content or enhancements, ask the user if they want an issue opened to track adding the content to the English version first, rather than suggesting it directly in the translation PR.
Learnt from: Starbuck5
Repo: pygame-community/pygame-ce PR: 3573
File: src_c/_pygame.h:108-133
Timestamp: 2025-09-01T20:18:57.500Z
Learning: In pygame-ce PR #3573, PG_SURF_BitsPerPixel was changed from a simple macro to a static inline function to handle SDL2/SDL3 differences. SDL2's BitsPerPixel includes padding bits (e.g., RGBX => 32) while SDL3's SDL_BITSPERPIXEL excludes padding (e.g., RGBX => 24). The new implementation bridges this gap rather than removing the helper entirely.
Learnt from: Starbuck5
Repo: pygame-community/pygame-ce PR: 3573
File: src_c/_camera.c:129-131
Timestamp: 2025-08-30T21:11:00.240Z
Learning: When doing SDL2 to SDL3 compatibility changes, Starbuck5 prefers to maintain exact existing behaviors even when other improvements could be made, focusing solely on the compatibility aspect rather than mixing in behavioral fixes.
📚 Learning: 2025-09-01T20:18:57.500Z
Learnt from: Starbuck5
Repo: pygame-community/pygame-ce PR: 3573
File: src_c/_pygame.h:108-133
Timestamp: 2025-09-01T20:18:57.500Z
Learning: In pygame-ce PR #3573, PG_SURF_BitsPerPixel was changed from a simple macro to a static inline function to handle SDL2/SDL3 differences. SDL2's BitsPerPixel includes padding bits (e.g., RGBX => 32) while SDL3's SDL_BITSPERPIXEL excludes padding (e.g., RGBX => 24). The new implementation bridges this gap rather than removing the helper entirely.

Applied to files:

  • src_c/base.c
📚 Learning: 2025-08-30T21:11:00.240Z
Learnt from: Starbuck5
Repo: pygame-community/pygame-ce PR: 3573
File: src_c/_camera.c:129-131
Timestamp: 2025-08-30T21:11:00.240Z
Learning: When doing SDL2 to SDL3 compatibility changes, Starbuck5 prefers to maintain exact existing behaviors even when other improvements could be made, focusing solely on the compatibility aspect rather than mixing in behavioral fixes.

Applied to files:

  • src_c/base.c
⏰ 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). (15)
  • GitHub Check: build (ubuntu-24.04)
  • GitHub Check: build (windows-latest)
  • GitHub Check: debug_coverage (ubuntu-24.04, 3.10.17)
  • GitHub Check: debug_coverage (ubuntu-24.04, 3.14.0)
  • GitHub Check: dev-check
  • GitHub Check: msys2 (mingw64, x86_64)
  • GitHub Check: x86
  • GitHub Check: build (ubuntu-22.04)
  • GitHub Check: AMD64
  • GitHub Check: msys2 (ucrt64, ucrt-x86_64)
  • GitHub Check: msys2 (clang64, clang-x86_64)
  • GitHub Check: i686
  • GitHub Check: x86_64
  • GitHub Check: Pyodide build
  • GitHub Check: aarch64
🔇 Additional comments (1)
src_c/base.c (1)

2124-2131: LGTM! This change correctly achieves the PR objective.

The SDL3 hint is now set at module import time, ensuring it applies regardless of whether pygame.init() is called. This properly handles workflows that only call subsystem initializers like audio.init(). The placement is appropriate since SDL_SetHint() can be safely called before SDL initialization and will take effect when SDL is later initialized.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@Starbuck5 Starbuck5 added the sdl3 label Dec 10, 2025
// this leaves people unable to quit their script. Plus it's different
// than SDL2 behavior.
SDL_SetHint(SDL_HINT_NO_SIGNAL_HANDLERS, "1");
#endif
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that this logic has been moved here, have you tested that something like say, quit+init doesn't reset this hint? I feel like something like this must be done at one of the init functions, say display.init or the internal event init function.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dang I didn't test that because I expected hints to be global. But no, I guess quitting SDL resets them.

It needs to get called on every possible event path that could initialize event? mixer.init, display.init, others?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test program

import pygame

pygame.display.init()
clock = pygame.time.Clock()
running = True
try:
    while running:
        for event in pygame.event.get():
            if event.type == pygame.QUIT:
                print("RECEIVED QUIT EVENT OMG 1")
                running = False

        clock.tick(60)
except KeyboardInterrupt:
    print("KEYBOARD INTERRUPTED 1")

pygame.display.quit()



pygame.init()
print("initialized 1")
clock = pygame.time.Clock()
running = True

try:
    while running:
        for event in pygame.event.get():
            if event.type == pygame.QUIT:
                print("RECEIVED QUIT EVENT OMG 1")
                running = False

        clock.tick(60)
except KeyboardInterrupt:
    print("KEYBOARD INTERRUPTED 1")

pygame.quit()

pygame.init()
print("initialized 2")

try:
    while running:
        for event in pygame.event.get():
            if event.type == pygame.QUIT:
                print("RECEIVED QUIT EVENT OMG 2")
                running = False

        clock.tick(60)
except KeyboardInterrupt:
    print("KEYBOARD INTERRUPTED 2")

pygame.quit()

Copy link
Member

@ankith26 ankith26 Dec 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the hint should be set in pgEvent_AutoInit, any other init function that needs event to be init must call pg_mod_autoinit(IMPPREFIX "event") (like display.init already does it)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some other SDL things initialize event internally, so it can be potentially non obvious. Like I ran into this problem with SDL_INIT_AUDIO.

@Starbuck5 Starbuck5 marked this pull request as draft December 13, 2025 05:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants