cmu-cs-academy / desktop-cmu-graphics

BSD 3-Clause "New" or "Revised" License
15 stars 9 forks source link

Support for Gamepads/Joysticks #45

Closed rriley closed 1 year ago

rriley commented 1 year ago

I'm currently working on adapting cmu-graphics to an arcade cabinet we have here at CMU Qatar. As part of that, I needed joystick/gamepad support. This is a set of commits to add basic support for joysticks by making the pygame joystick events visible to the cmu-graphics user through callbacks, similar to how the keyboard and mouse are currently handled.

I'm interested in seeing this merged and am open to making changes and adaptations based on feedback.

austin-schick commented 1 year ago

Hi Ryan,

Thanks for the PR! Evan Mallory and I talked this over, and we wanted to run an alternate solution by you: What if we exposed a way for users to respond to arbitrary pygame events? This could look something like an onPygameEvent event handler function, or maybe we could let you overwrite a function like app.externalEventHandler.

Either way, this would allow you to write a file like gamepads.py that uses pygame events to extend the animation framework. Then your students would just need to download and import one additional file to get your extra features.

We like this solution because it allows you and other users to extend the animation framework in interesting ways, without us having to approve / make canon / test / commit to not breaking those extensions moving forward.

Would that address the problem you were hoping to solve by merging these changes? If not, we're definitely open to other options! I'm curious to hear why you were hoping to get this merged to master rather than distributing a custom zip file, for example.

Thanks again, Austin

rriley commented 1 year ago

Hi Austin,

Thanks for your reply, and for you and Evan taking the time to talk this through.

I like your idea of making pygame events more accessible and having me write my own library. That gives me the freedom to quickly iterate on it for my students (and gives them a chance to tweak it if needed) but makes sure it stays easily compatible with new versions of cmu-graphics. It also gives a nice way for students to extend cmu-graphics with more pygame stuff if they want to for their own projects.

My desire for an upstream merge instead of distributing my own custom zip was fairly straightforward and self-serving: I don't want to deal with re-merging every time your team does a useful update. However, I very much see your point of not wanting to take on responsibility for not freaking whatever feature random people like me want to add. You've got enough on your plate.

I'm open to any option you have for making the pygame events accessible. The easiest option sounds like just tweaking the main pygame event loop to just call onPyGameEvent in the else, but whatever works best and feels the most maintainable to you all would be great.

Again, I appreciate your efforts!

schmave commented 1 year ago

Thanks for this additional information! We'll take the approach of exposing pygame events to some function outside of cmu_graphics itself. Unfortunately we won't have a chance to work on it this week. We expect to get this back to you next week. Thanks for your patience.

rriley commented 1 year ago

These changes look great. Thanks for doing this, I really appreciate you putting in so much time.

austin-schick commented 1 year ago

Thanks for opening the PR @rriley! I think we ended up with a nice addition to the library, and I'm excited to see what cool extensions you and other educators make with it.