pygame-ce icon indicating copy to clipboard operation
pygame-ce copied to clipboard

Port the controller module to C.

Open zoldalma999 opened this issue 2 years ago • 7 comments
trafficstars

While the port is meant to be a full replacement, there are some changes made to the module. But apart from the removed functions, there are not a lot of differences, and they are meant to make the module more like the joystick module. However they are/can be breaking changes.

Changes:

  • removed controller.set_eventstate
  • removed controller.get_eventstate
  • removed controller.update
  • removed controller.name_forindex
  • Controller.get_axis returns in the range of -1 to 1 now
  • Controllers that have the same id are the same object
  • Keyword argument names might not be the same everywhere

removed controller.set_eventstate removed controller.get_eventstate removed controller.update

I removed the controller.get/set_eventstate functions, since there is no other place we expose something like this. If someone wants to block controller events, they can use pygame.event.set_blocked anyway. And controller.update is only needed if one disables events with set_eventstate, so that got removed too.

removed controller.name_forindex

controller.name_forindex is kind of an awkward function, since it uses device indicies as its parameter, which is inconsistent, and should not be used a lot. Even SDL removed it in SDL3 to replace it with a similar function, just with instance_ids.

Controller.get_axis returns in the range of -1 to 1 now

Controller.get_axis returning -1 to 1 values is a change that should have been made a while ago honestly. Nowhere else do we use the -32768 to 32767 range. Changed the CONTROLLERAXISMOTION event as well, while I was at it.

Controllers that have the same id are the same object

This is done mostly so that it matches up with the joystick module's behaviour.

Questions:

  • What to do with the docs of the removed functions? There is no "removed in" tag, I don't think?
  • What to do with attributes (name, id)? For now I did not document them, but did add them, but there was some discussion about it (also id should be removed and replaced with instance_id anyway).
  • What to do with the old module? Should it be removed now or should we wait a bit with it?

Test script and testing:

Adapted from the joystick example, this should test out most of the functionality of the module. Things to check:

  • Runs at all?
  • All information is correct on the screen (ie button presses or axis values are correct)
  • When pressing the A button (or cross on ps controllers), the B (or circle) button should be the one returning True.
  • Pressing the B (/circle) button should start a light rumble on the controller
  • Hotplugging works as it should

If something is broken, set NEW_CONTROLLER_MODULE to False to see if it is a regression or not. You can also run the controller tests, though not sure how good those are.

Test script
import sys
import pygame

USE_NEW_CONTROLLER_MODULE = True
if USE_NEW_CONTROLLER_MODULE:
    import pygame._sdl2.controller

    pygame.controller = pygame._sdl2.controller
else:
    import pygame._sdl2.controller_old

    pygame.controller = pygame._sdl2.controller_old

pygame.init()
pygame.controller.init()


button_name_pairs = {
    pygame.CONTROLLER_BUTTON_A: "A",
    pygame.CONTROLLER_BUTTON_B: "B",
    pygame.CONTROLLER_BUTTON_X: "X",
    pygame.CONTROLLER_BUTTON_Y: "Y",
    pygame.CONTROLLER_BUTTON_DPAD_UP: "dpad up",
    pygame.CONTROLLER_BUTTON_DPAD_DOWN: "dpad down",
    pygame.CONTROLLER_BUTTON_DPAD_LEFT: "dpad left",
    pygame.CONTROLLER_BUTTON_DPAD_RIGHT: "dpad right",
    pygame.CONTROLLER_BUTTON_LEFTSHOULDER: "left shoulder",
    pygame.CONTROLLER_BUTTON_RIGHTSHOULDER: "right shoulder",
    pygame.CONTROLLER_BUTTON_LEFTSTICK: "left stick",
    pygame.CONTROLLER_BUTTON_RIGHTSTICK: "right stick",
    pygame.CONTROLLER_BUTTON_BACK: "back",
    pygame.CONTROLLER_BUTTON_GUIDE: "guide",
    pygame.CONTROLLER_BUTTON_START: "start",
}


axis_name_pairs = {
    pygame.CONTROLLER_AXIS_LEFTX: "left x",
    pygame.CONTROLLER_AXIS_LEFTY: "left y",
    pygame.CONTROLLER_AXIS_RIGHTX: "right x",
    pygame.CONTROLLER_AXIS_RIGHTY: "right y",
    pygame.CONTROLLER_AXIS_TRIGGERLEFT: "trigger left",
    pygame.CONTROLLER_AXIS_TRIGGERRIGHT: "trigger right",
}


class TextPrint:
    def __init__(self):
        self.reset()
        self.counter = 0
        self.font = pygame.font.Font(None, 25)

    def tprint(self, screen, text):
        text_bitmap = self.font.render(text, True, (255, 255, 255))
        screen.blit(text_bitmap, (self.x, self.y))
        self.y += self.line_height
        self.counter += 1

    def reset(self):
        self.x = 10
        self.y = 10
        self.line_height = 22
        self.counter = 0

    def shift_right(self):
        self.x += 350
        self.y -= self.line_height * self.counter
        self.counter = 0

    def indent(self):
        self.x += 10

    def unindent(self):
        self.x -= 10


screen = pygame.display.set_mode((1280, 720))
clock = pygame.time.Clock()
controllers = {}
text_print = TextPrint()

done = False
while not done:
    for event in pygame.event.get():
        if event.type == pygame.QUIT:
            done = True
        if event.type == pygame.KEYDOWN and event.key == pygame.K_ESCAPE:
            done = True

        if event.type == pygame.CONTROLLERBUTTONDOWN:
            if event.button == pygame.CONTROLLER_BUTTON_A:
                controller = controllers[event.instance_id]
                if controller.rumble(0.2, 0.7, 500):
                    print(f"Rumble effect played on controller {event.instance_id}")

        if event.type == pygame.CONTROLLERDEVICEADDED:
            if not pygame.controller.is_controller(event.device_index):
                continue

            controller = pygame.controller.Controller(event.device_index)
            controllers[controller.as_joystick().get_instance_id()] = controller
            print(f"Controller {controller.as_joystick().get_instance_id()} connencted")
            mapping = controller.get_mapping()
            mapping["a"] = "b1"
            mapping["b"] = "b0"
            controller.set_mapping(mapping)

        if event.type == pygame.CONTROLLERDEVICEREMOVED:
            del controllers[event.instance_id]
            print(f"Controller {event.instance_id} disconnected")

    screen.fill((20, 20, 20))
    text_print.reset()

    joystick_count = pygame.controller.get_count()
    text_print.tprint(screen, f"Number of joysticks: {joystick_count}")
    text_print.indent()

    for controller in controllers.values():
        jid = controller.as_joystick().get_instance_id()

        text_print.tprint(screen, f"Controller {jid}")
        text_print.indent()
        text_print.tprint(screen, f"Controller name: {controller.name}")

        text_print.tprint(screen, "")
        text_print.tprint(screen, "Axes:")
        text_print.indent()
        for const, name in axis_name_pairs.items():
            text_print.tprint(screen, f"{name}: {controller.get_axis(const):.2f}")
        text_print.unindent()

        text_print.tprint(screen, "")
        text_print.tprint(screen, "Buttons:")
        text_print.indent()
        for const, name in button_name_pairs.items():
            text_print.tprint(screen, f"{name}: {controller.get_button(const)}")
        text_print.unindent()

        text_print.unindent()
        text_print.shift_right()
        text_print.tprint(screen, "Mapping:")
        text_print.indent()
        for name, value in controller.get_mapping().items():
            text_print.tprint(screen, f"{name}: {value}")
        text_print.unindent()

        text_print.shift_right()

    pygame.display.flip()
    clock.tick(30)

pygame.quit()

PS.: I wanted to (and should have) done this earlier. But late is better than never.

zoldalma999 avatar Mar 26 '23 10:03 zoldalma999

Unit tests failing because the ubuntu 20.04 build uses SDL 2.0.10, which doesn't have SDL_strtokr. Try using strtok_r instead, our compilers probably all support it?

Other fail is about the stubs of Controller __init__, not sure the best way to fix that one.

Starbuck5 avatar Mar 29 '23 04:03 Starbuck5

Notes for self, review:

  • Stuff non obviously changed should go in docs
  • Keyword arguments? (docs vs source vs old source)

Starbuck5 avatar May 14 '23 07:05 Starbuck5

@zoldalma999 Once the merge conflicts are fixed, I'm willing to give this a final test with several controllers I own to make sure it works properly, and then merge it when all things look good. If you'd like, I can fix the merge conflicts and push to your branch

oddbookworm avatar May 25 '24 18:05 oddbookworm

Sorry to come up with this so late, but I’m not sure the ā€œport to C PRā€ and the ā€œchange a couple APIsā€ PR should be the same PR. I’d be happier with this PR if it was a drop in replacement.

Starbuck5 avatar May 25 '24 18:05 Starbuck5

@zoldalma999 Once the merge conflicts are fixed, I'm willing to give this a final test with several controllers I own to make sure it works properly, and then merge it when all things look good. If you'd like, I can fix the merge conflicts and push to your branch

I'd appreciate that. I'll message you whenever it is ready.

Sorry to come up with this so late, but I’m not sure the ā€œport to C PRā€ and the ā€œchange a couple APIsā€ PR should be the same PR. I’d be happier with this PR if it was a drop in replacement.

The changes to the API are more like removes, apart from "Controller.get_axis returns in the range of -1 to 1 now". I can change it to be like the cython version, and add functions that will be removed in a later pr anyway, but it made sense for me to just not add those functions.

zoldalma999 avatar May 29 '24 17:05 zoldalma999

The changes to the API are more like removes, apart from "Controller.get_axis returns in the range of -1 to 1 now". I can change it to be like the cython version, and add functions that will be removed in a later pr anyway, but it made sense for me to just not add those functions.

Thanks for pointing that out. I'd like it if you changed that to be like the Cython version, then that change can be evaluated independently.

Starbuck5 avatar May 30 '24 06:05 Starbuck5

Looking at the fails I'm guessing this needs to make some modifications to the meson build now:

..\src_py\_sdl2\meson.build:2:17: ERROR: File controller.py does not exist.

MyreMylar avatar May 31 '24 11:05 MyreMylar

With my nintendo switch joycons, I get a segfault on Windows 11 when I reach this line in the test script :

# In pygame.CONTROLLERDEVICEADDED event check
mapping = controller.get_mapping()

EDIT : Traceback coming when I'll figure out which debugger can give the traceback I want on Windows.

Turned out this line (line 252) was the culprit :

        if (value[0] != '\0') {

Here you have to check if value[0] is not NULL.

bilhox avatar Aug 26 '24 12:08 bilhox

Merging this now.

MyreMylar avatar Aug 30 '24 08:08 MyreMylar