-
-
Notifications
You must be signed in to change notification settings - Fork 201
Compile _sdl2.controller and _sdl2.touch with SDL3 #3427
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
6813ed3
9f7700b
a3e5250
382a881
6af411a
a75bcb7
43bd259
8b69605
3a7d8bf
73a0997
c7576cd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,7 +30,11 @@ controller_module_init(PyObject *module, PyObject *_null) | |
return RAISE(pgExc_SDLError, SDL_GetError()); | ||
} | ||
} | ||
#if SDL_VERSION_ATLEAST(3, 0, 0) | ||
SDL_SetGamepadEventsEnabled(SDL_TRUE); | ||
#else | ||
SDL_GameControllerEventState(SDL_ENABLE); | ||
#endif | ||
|
||
Py_RETURN_NONE; | ||
} | ||
|
@@ -87,7 +91,16 @@ controller_module_get_count(PyObject *module, PyObject *_null) | |
{ | ||
CONTROLLER_INIT_CHECK(); | ||
|
||
#if SDL_VERSION_ATLEAST(3, 0, 0) | ||
int count; | ||
SDL_JoystickID *joysticks = SDL_GetJoysticks(&count); | ||
if (!joysticks) { | ||
return RAISE(pgExc_SDLError, SDL_GetError()); | ||
} | ||
SDL_free(joysticks); | ||
#else | ||
int count = SDL_NumJoysticks(); | ||
#endif | ||
if (count < 0) { | ||
return RAISE(pgExc_SDLError, SDL_GetError()); | ||
} | ||
|
@@ -290,7 +303,11 @@ controller_set_mapping(pgControllerObject *self, PyObject *args, | |
|
||
char guid_str[64]; | ||
SDL_Joystick *joy = SDL_GameControllerGetJoystick(self->controller); | ||
#if SDL_VERSION_ATLEAST(3, 0, 0) | ||
SDL_GUIDToString(SDL_JoystickGetGUID(joy), guid_str, 63); | ||
#else | ||
SDL_JoystickGetGUIDString(SDL_JoystickGetGUID(joy), guid_str, 63); | ||
#endif | ||
|
||
PyObject *key, *value; | ||
const char *key_str, *value_str; | ||
|
@@ -371,10 +388,17 @@ controller_rumble(pgControllerObject *self, PyObject *args, PyObject *kwargs) | |
low_freq = MAX(MIN(low_freq, 1.0f), 0.0f) * 65535; | ||
high_freq = MAX(MIN(high_freq, 1.0f), 0.0f) * 65535; | ||
|
||
#if SDL_VERSION_ATLEAST(3, 0, 0) | ||
SDL_bool success = SDL_GameControllerRumble( | ||
self->controller, (Uint16)low_freq, (Uint16)high_freq, duration); | ||
|
||
return PyBool_FromLong(success); | ||
#else | ||
int success = SDL_GameControllerRumble(self->controller, (Uint16)low_freq, | ||
(Uint16)high_freq, duration); | ||
|
||
return PyBool_FromLong(success == 0); | ||
|
||
#endif | ||
} | ||
|
||
static PyObject * | ||
|
@@ -384,7 +408,9 @@ controller_stop_rumble(pgControllerObject *self, PyObject *_null) | |
if (!self->controller) { | ||
return RAISE(pgExc_SDLError, "Controller is not initialized"); | ||
} | ||
SDL_GameControllerRumble(self->controller, 0, 0, 1); | ||
if (!SDL_GameControllerRumble(self->controller, 0, 0, 1)) { | ||
return RAISE(pgExc_SDLError, SDL_GetError()); | ||
} | ||
Py_RETURN_NONE; | ||
} | ||
|
||
|
@@ -467,7 +493,18 @@ controller_new(PyTypeObject *subtype, PyObject *args, PyObject *kwargs) | |
|
||
CONTROLLER_INIT_CHECK(); | ||
|
||
if (id >= SDL_NumJoysticks() || !SDL_IsGameController(id)) { | ||
#if SDL_VERSION_ATLEAST(3, 0, 0) | ||
int joycount; | ||
SDL_JoystickID *joysticks = SDL_GetJoysticks(&joycount); | ||
if (!joysticks) { | ||
return RAISE(pgExc_SDLError, SDL_GetError()); | ||
} | ||
SDL_free(joysticks); | ||
#else | ||
int joycount = SDL_NumJoysticks(); | ||
#endif | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. given this is used in 2 places, does it make more sense to define this as a helper function? |
||
|
||
if (id >= joycount || !SDL_IsGameController(id)) { | ||
return RAISE(pgExc_SDLError, "Invalid index"); | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -25,20 +25,48 @@ | |||||||||||||||||||||||||||
static PyObject * | ||||||||||||||||||||||||||||
pg_touch_num_devices(PyObject *self, PyObject *_null) | ||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||
return PyLong_FromLong(SDL_GetNumTouchDevices()); | ||||||||||||||||||||||||||||
int count; | ||||||||||||||||||||||||||||
#if SDL_VERSION_ATLEAST(3, 0, 0) | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
SDL_TouchID *devices = SDL_GetTouchDevices(&count); | ||||||||||||||||||||||||||||
if (devices == NULL) { | ||||||||||||||||||||||||||||
return RAISE(pgExc_SDLError, SDL_GetError()); | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
SDL_free(devices); | ||||||||||||||||||||||||||||
#else | ||||||||||||||||||||||||||||
count = SDL_GetNumTouchDevices(); | ||||||||||||||||||||||||||||
#endif | ||||||||||||||||||||||||||||
return PyLong_FromLong(count); | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
static PyObject * | ||||||||||||||||||||||||||||
pg_touch_get_device(PyObject *self, PyObject *index) | ||||||||||||||||||||||||||||
pg_touch_get_device(PyObject *self, PyObject *index_obj) | ||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||
SDL_TouchID touchid; | ||||||||||||||||||||||||||||
if (!PyLong_Check(index)) { | ||||||||||||||||||||||||||||
if (!PyLong_Check(index_obj)) { | ||||||||||||||||||||||||||||
return RAISE(PyExc_TypeError, | ||||||||||||||||||||||||||||
"index must be an integer " | ||||||||||||||||||||||||||||
"specifying a device to get the ID for"); | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
touchid = SDL_GetTouchDevice(PyLong_AsLong(index)); | ||||||||||||||||||||||||||||
int index = PyLong_AsLong(index_obj); | ||||||||||||||||||||||||||||
if (PyErr_Occurred()) { | ||||||||||||||||||||||||||||
return NULL; // exception already set | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
#if SDL_VERSION_ATLEAST(3, 0, 0) | ||||||||||||||||||||||||||||
int count; | ||||||||||||||||||||||||||||
SDL_TouchID *devices = SDL_GetTouchDevices(&count); | ||||||||||||||||||||||||||||
if (devices == NULL) { | ||||||||||||||||||||||||||||
return RAISE(pgExc_SDLError, SDL_GetError()); | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
if (index < 0 || index >= count) { | ||||||||||||||||||||||||||||
SDL_free(devices); | ||||||||||||||||||||||||||||
return RAISE(PyExc_IndexError, "index is out of bounds"); | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
touchid = devices[index]; | ||||||||||||||||||||||||||||
SDL_free(devices); | ||||||||||||||||||||||||||||
#else | ||||||||||||||||||||||||||||
touchid = SDL_GetTouchDevice(index); | ||||||||||||||||||||||||||||
#endif | ||||||||||||||||||||||||||||
if (touchid == 0) { | ||||||||||||||||||||||||||||
/* invalid index */ | ||||||||||||||||||||||||||||
return RAISE(pgExc_SDLError, SDL_GetError()); | ||||||||||||||||||||||||||||
|
@@ -47,21 +75,32 @@ pg_touch_get_device(PyObject *self, PyObject *index) | |||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
static PyObject * | ||||||||||||||||||||||||||||
pg_touch_num_fingers(PyObject *self, PyObject *device_id) | ||||||||||||||||||||||||||||
pg_touch_num_fingers(PyObject *self, PyObject *device_id_obj) | ||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||
int fingercount; | ||||||||||||||||||||||||||||
if (!PyLong_Check(device_id)) { | ||||||||||||||||||||||||||||
if (!PyLong_Check(device_id_obj)) { | ||||||||||||||||||||||||||||
return RAISE(PyExc_TypeError, | ||||||||||||||||||||||||||||
"device_id must be an integer " | ||||||||||||||||||||||||||||
"specifying a touch device"); | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
SDL_TouchID device_id = PyLong_AsLongLong(device_id_obj); | ||||||||||||||||||||||||||||
if (PyErr_Occurred()) { | ||||||||||||||||||||||||||||
return NULL; // exception already set | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
VIDEO_INIT_CHECK(); | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
fingercount = SDL_GetNumTouchFingers(PyLong_AsLongLong(device_id)); | ||||||||||||||||||||||||||||
#if SDL_VERSION_ATLEAST(3, 0, 0) | ||||||||||||||||||||||||||||
SDL_Finger **fingers = SDL_GetTouchFingers(device_id, &fingercount); | ||||||||||||||||||||||||||||
if (fingers == NULL) { | ||||||||||||||||||||||||||||
return RAISE(pgExc_SDLError, SDL_GetError()); | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
SDL_free(fingers); | ||||||||||||||||||||||||||||
#else | ||||||||||||||||||||||||||||
fingercount = SDL_GetNumTouchFingers(device_id); | ||||||||||||||||||||||||||||
if (fingercount == 0) { | ||||||||||||||||||||||||||||
return RAISE(pgExc_SDLError, SDL_GetError()); | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
#endif | ||||||||||||||||||||||||||||
Comment on lines
+99
to
+103
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do not treat “0 fingers” as an error in the SDL2 path. SDL_GetNumTouchFingers returns a negative value on error; 0 is a valid “no active fingers” state. Current code raises SDLError when fingercount == 0, which is incorrect and will spuriously error on valid states. Fix by checking for < 0 instead of == 0. See SDL2 docs. Apply this diff: #else
- fingercount = SDL_GetNumTouchFingers(device_id);
- if (fingercount == 0) {
- return RAISE(pgExc_SDLError, SDL_GetError());
- }
+ fingercount = SDL_GetNumTouchFingers(device_id);
+ if (fingercount < 0) {
+ return RAISE(pgExc_SDLError, SDL_GetError());
+ }
#endif 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fake news There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. , meaning the current code in the diff is actually correct in treating My previous suggestion was wrong - the code should indeed raise an SDL error when ✏️ Learnings added
|
||||||||||||||||||||||||||||
return PyLong_FromLong(fingercount); | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
#if !defined(BUILD_STATIC) | ||||||||||||||||||||||||||||
|
@@ -95,14 +134,28 @@ pg_touch_get_finger(PyObject *self, PyObject *args, PyObject *kwargs) | |||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
VIDEO_INIT_CHECK(); | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
#if SDL_VERSION_ATLEAST(3, 0, 0) | ||||||||||||||||||||||||||||
int fingercount; | ||||||||||||||||||||||||||||
SDL_Finger **fingers = SDL_GetTouchFingers(touchid, &fingercount); | ||||||||||||||||||||||||||||
if (fingers == NULL) { | ||||||||||||||||||||||||||||
return RAISE(pgExc_SDLError, SDL_GetError()); | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
if (index < 0 || index >= fingercount) { | ||||||||||||||||||||||||||||
SDL_free(fingers); | ||||||||||||||||||||||||||||
return RAISE(PyExc_IndexError, "index is out of bounds"); | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
finger = fingers[index]; | ||||||||||||||||||||||||||||
SDL_free(fingers); | ||||||||||||||||||||||||||||
#else | ||||||||||||||||||||||||||||
Comment on lines
136
to
+149
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fix use-after-free in pg_touch_get_finger (SDL3 path). SDL_GetTouchFingers returns a single allocation (array + finger data) that must be freed with SDL_free; finger pointers become invalid after free. The code currently assigns finger = fingers[index], calls SDL_free(fingers), and then dereferences finger later to build the dict — this is a UAF bug. Copy the fields you need before freeing, and then build the Python dict from those copies. See SDL3 docs: returned array is a single allocation to free once. Apply this diff: #if SDL_VERSION_ATLEAST(3, 0, 0)
int fingercount;
SDL_Finger **fingers = SDL_GetTouchFingers(touchid, &fingercount);
if (fingers == NULL) {
return RAISE(pgExc_SDLError, SDL_GetError());
}
if (index < 0 || index >= fingercount) {
SDL_free(fingers);
return RAISE(PyExc_IndexError, "index is out of bounds");
}
- finger = fingers[index];
- SDL_free(fingers);
+ finger = fingers[index];
+ SDL_FingerID fid = finger->id;
+ float fx = finger->x;
+ float fy = finger->y;
+ float fpressure = finger->pressure;
+ SDL_free(fingers);
#else
- if (!(finger = SDL_GetTouchFinger(touchid, index))) {
+ if (!(finger = SDL_GetTouchFinger(touchid, index))) {
Py_RETURN_NONE;
}
+ SDL_FingerID fid = finger->id;
+ float fx = finger->x;
+ float fy = finger->y;
+ float fpressure = finger->pressure;
#endif
@@
- _pg_insobj(fingerobj, "id", PyLong_FromLongLong(finger->id));
- _pg_insobj(fingerobj, "x", PyFloat_FromDouble(finger->x));
- _pg_insobj(fingerobj, "y", PyFloat_FromDouble(finger->y));
- _pg_insobj(fingerobj, "pressure", PyFloat_FromDouble(finger->pressure));
+ _pg_insobj(fingerobj, "id", PyLong_FromLongLong(fid));
+ _pg_insobj(fingerobj, "x", PyFloat_FromDouble(fx));
+ _pg_insobj(fingerobj, "y", PyFloat_FromDouble(fy));
+ _pg_insobj(fingerobj, "pressure", PyFloat_FromDouble(fpressure)); Also applies to: 160-163 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I dont think so There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. . The user ankith26 has disagreed with my assessment, and given their retrieved learning about SDL internals, they likely have a better understanding. Let me acknowledge this and step back. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hang on the bot is correct actually, just checked the source. This is an actual issue, though the way I would fix it is by freeing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
||||||||||||||||||||||||||||
if (!(finger = SDL_GetTouchFinger(touchid, index))) { | ||||||||||||||||||||||||||||
Py_RETURN_NONE; | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
#endif | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
fingerobj = PyDict_New(); | ||||||||||||||||||||||||||||
if (!fingerobj) | ||||||||||||||||||||||||||||
if (!fingerobj) { | ||||||||||||||||||||||||||||
return NULL; | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
_pg_insobj(fingerobj, "id", PyLong_FromLongLong(finger->id)); | ||||||||||||||||||||||||||||
_pg_insobj(fingerobj, "x", PyFloat_FromDouble(finger->x)); | ||||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,9 @@ | ||
from pygame.version import SDL | ||
|
||
if __import__("sys").platform not in ("wasi", "emscripten"): | ||
from .audio import * # pylint: disable=wildcard-import; lgtm[py/polluting-import] | ||
if SDL < (3, 0, 0): | ||
from .audio import * # pylint: disable=wildcard-import; lgtm[py/polluting-import] | ||
from .sdl2 import * # pylint: disable=wildcard-import; lgtm[py/polluting-import] | ||
from .video import * # pylint: disable=wildcard-import; lgtm[py/polluting-import] | ||
|
||
if SDL < (3, 0, 0): | ||
from .video import * # pylint: disable=wildcard-import; lgtm[py/polluting-import] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this breaks the SDL2 case. Why not leave this code as it is?