WIP #70 - Added controller support #347
Conversation
Adds common controller support to PGZero. - Controllers are added and removed automatically. - Support for checking states of buttons and axis is fully implemented. - A default controller can be accessed via `joy()`. If possible, this will be shortened to just `joy` in the future. - Users can define PGZero events `on_joy_move`, `on_joy_down`, `on_joy_up`, `on_joy_added` and `on_joy_removed`. These currently only function correctly without specifying parameters. Proper support like with the other event functions will be added in the future. - Using multiple controllers in one game is possible. - Controllers can be hotplugged. - Switching game controls based on whether a controller is easy via `if joy(): ... else: ...`. Again, this should become even easier when `joy()` is improved. - Simultaneous controls with keyboard, mouse and controllers are also possible and do not conflict. One problem with adding parameter support is that mouse events also use the generic `button` parameter which means that overloading it for use with `mouse.LEFT` and so on prevents an easy solution for joysticks. This is still a work in progress. Fixes lordmauve#70.
Added support for parameters in user joystick functions. - Controller specific button and axis integer values are automatically mapped to generic enums with identifiers to make user functions agnostic to specific controller model. - User function parameters for axis movement reflect deadzone settings for joysticks. Buttons and axis not known to the controller layout now don't crash the program anymore. Instead the are exposed to the user as value UNKNOWN to allow simply ignoring unknown inputs or reacting to them.
ImportantI've found out that there is a class This opens the question of whether we should use those options instead. There are both benefits and drawback I can see: Benefits to using prebuilt controller classMostly maintanability. The custom solution I wrote is able to handle all relevant controller input, so the prebuilt one is not needed for functionality. The big benefit would be that all controller models supported by the pygame._sdl2 package would automatically be supported in PGZero. If we use my custom solution, we would have to make sure that we have up-to-date mappings for current controller models. Drawbacks to using prebuilt controller classThe prebuilt class seems to be less well supported by the community than the more central ´joystick´ class of pygame. Over time, this could introduce problems. I'm also not sure if we rely on SDL2 anywhere else in the codebase. If not, this would force the issue, which might not be desirable. Benefits to using custom joystick variantMost of the work is done and from here it is easy to customize code that does not rely on any fringe features of pygame but rather a core class that is well supported and documented. Drawbacks to using custom joystick variantMore manual maintenance required to stay up-to-date with current controller mappings. Currently, I lean more towards staying with the more core joystick class to be conservative about depending on SDL2 and using more well supported systems. I would very much like input though, especially from @lordmauve since deciding on one or the other likely locks it in for a while. |
There was a problem hiding this comment.
It's good to see this. I wrote the hotplugging support in Pygame a few years ago but haven't got around to supporting it in Pygame Zero.
I think it's going to need quite a bit more work on the API we offer to users, I think this is too low-level. In particular I think we should have an API like
player1, player2 = request_joysticks(2)and then Pygame Zero might pop up a controller connection screen (like on Switch) that runs until two joysticks are connected and reappears if one of the selected joysticks is removed.
| # These variables are used to give a lockout time for physical | ||
| # HAT button simulation to prevent ghost inputs. | ||
| self._DU_open = True | ||
| self._DD_open = True | ||
| self._DL_open = True | ||
| self._DR_open = True | ||
|
|
||
| # Used with clock.schedule to prevent ghost button presses via timeouts. | ||
| def _unlock_DU(self): | ||
| self._DU_open = True | ||
|
|
||
| def _unlock_DD(self): | ||
| self._DD_open = True | ||
|
|
||
| def _unlock_DL(self): | ||
| self._DL_open = True | ||
|
|
||
| def _unlock_DR(self): | ||
| self._DR_open = True |
There was a problem hiding this comment.
I can see the point of this debouncing but I think the entire thing should be encapsulated separately from this class and probably using game ticks instead of the clock class. The clock is likely to be pausable and support slowmo (based on work that is already in Wasabi2D).
There was a problem hiding this comment.
Actually if we do that maybe we just make a "master clock" for pgzero internal use, so that would be Ok
There was a problem hiding this comment.
At least in the current state of the project, I think using clock is very idiomatic. I think when such a change comes having the internal clock would be a very sensible way to handle the resultant problems. Since that would be a rather small change, I think I would prefer to leave that for when it actually happens. Right now it doesn't feel idiomatic to make a single other clock for this future use case when nowhere else in the project works like that.
src/pgzero/joystick.py
Outdated
| @property | ||
| def face_down(self): | ||
| """Returns whether the lower face button is pressed.""" | ||
| return self._pressed[self._btn_map.FD] |
There was a problem hiding this comment.
You can use the descriptor protocol to remove all this code duplication. Off the top of my head it's
@dataclass
class Pressed:
button: str
def __get__(self, cls: type['Joystick'], inst: 'Joystick') -> bool:
return inst._pressed[getattr(inst._btm_map, self.button)]then here
class Joystick:
face_down = Pressed('FD')There was a problem hiding this comment.
I must admit I don't quite understand how the functionality is supposed to be retained this way. Disclaimer: I don't know dataclasses very well.
The point of these properties is that the user can do if joy().face_up: .... This is not possible here, because with the change applied, joy().face_up returns the class instance and since that is not None, if joy().face_up is always True, regardless of the underlying info in _pressed of the joystick. Maybe my implementation is wrong?
A second point which remains is that this approach allows the user to change the value of face_up to whatever they like. Since it's not a property anymore, nothing stops reassignment. Of course this isn't that likely to happen, but I still think we should avoid it.
I agree the current solution is ugly, but reading some of the documentation of dataclass I couldn't find how to get the desired behavior. It wasn't that hard to change it using __getattr__() however, so that is the first approach I took for now. Maybe you can give it another look and see if that works for you? There's still a slight downside in that trying to write to face_up won't give the clear description that properties can't be written to but will instead claim that the attribute doesn't exist which might be confusing for users.
|
Hi, sorry for the absence. I agree that a more automated use is desirable but I would question if that should necessarily happen in the same step / PR. I think first having a way to interact with controllers similar to the mouse or keyboard would be idiomatic. Then building on top of that with an extension that pauses the game and waits for connections etc. in another step seems viable and less likely to stall to me (as such a system might have requirements of other parts of the codebase that might not be accounted for yet). I will address your review as soon as I find some time to work on it. |
|
Thanks for the review, I addressed all the easy stuff and what remains now is the automatic mapping. I'm working on that. One other thing is that the reduction of duplicate code is a bit confusing in respect to user facing error messages. I get not wanting wasted space in the file, but maybe that's preferable to solutions that introduce their own problems? I made the change for now, just tell me what you think. |
…gned all parts of the controller system accordingly.
|
@lordmauve Okay, the automatic controller mapping works, although I'll probably have to test it some more. I've split the file you linked into three parts based on platform to reduce overhead when looking for the mapping. While this is some manual work, it could likely also be automated with a script and done with a single command when the host repo updates their files. |
|
Okay, big change made. |
|
@lordmauve I'll be writing the unittests for this functionality next I think, but I'd really love some feedback if you're happy with what we have now before that. |
|
Welp, probably not gonna happen, so I did the tests from the current state. |
|
@lordmauve Any chance you might be able to look at something? |
Adds common controller support to PGZero.
joyto check the state of buttons and axes (similar tokeyboard.afor example). It is always present and safe to access. If no actual joysticks are in use, it simply reports all buttons as unpressed and all axes in their neutral position. It tracks all inputs by joystick devices. This means that when a single controller is connected, it always acts as a safe way to create joystick mappings without having to assign controllers manually or deal with them disconnecting. If two or more controllers are connected,joyreflects all changes made to any of them. If a game wants to support two or more controllers, actual individual controllers have to be accessed. Their access however is exactly the same as forjoyso code can easily be adjusted.on_joy_move,on_joy_down,on_joy_up,on_joy_addedandon_joy_removed. Users can give these the parametersjoybtn,axisandvaluefor button and axis events respectively as well asinstance_idto differentiate controllers. Like with mouse or keyboard, the used buttons or axes can be checked viaif joybtn == joybutton.FACE_UP: ...andif axis == joyaxis.LEFT_X: ....joyis always safe to access, nothing breaks if no controllers are present and the controls kick in once a controller is connected.joyshould not define the same controls as KBM since that would be the previous functionality.'joysticks' is the interface to the internal joystick management class and is a dict-like object with which individual joysticks can be accessed via their instance_ids like
p1_con = joysticks[0]andp2_con = joysticks[3]to give each player their own controller with separate controls.Automatic controller mappings rely on this community project and can automatically be updated by running
update_controllerdb.pyonce from the project root.This is still a work in progress.
Fixes #70.