Skip to content
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

Remove event._joy_instance_map #2048

Merged
merged 3 commits into from
May 29, 2023
Merged

Conversation

yunline
Copy link
Contributor

@yunline yunline commented Mar 23, 2023

Fix issues:
#2047
pygame/pygame#3634

Removing for 2 reasons.

  • _joy_instance_map is mapping instance_id to instance_id, which is pointless.
  • _joy_instance_map also causes KeyError (which is not caught and causes the SystemError) when a controller is being unplugged but was not in the dict.

@yunline yunline added event pygame.event joystick pygame.joystick bugfix PR that fixes bug labels Mar 23, 2023
@yunline yunline requested a review from a team as a code owner March 23, 2023 08:54
Copy link
Member

@zoldalma999 zoldalma999 left a comment

Choose a reason for hiding this comment

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

This will break code as implemented.

joy_instance_map does not map instance_id to instance_id, it maps instance_id to device_index (or it tries its best to). Which is needed for backwards compatibility. With your implementation, we will be returning instance_ids instead of device_indices Every code that uses event.joy with joystick.get_id will break, but not instantly. Only after they unplug and plug a joystick back in (since until this happens, the to ids should match up, though it is not guaranteed). And not in an obvious way, like an error, but their event just won't match up with their joystick.

However I do agree with removing the mapping dictionary. A better solution to this would be to add a joystick function that returns a joystick object from an instance id. This should work since 1) joysticks don't send events 'till created and 2) the pgJoystickObject keeps track of its id already (or if it cannot find it, it would just set the joy attribute to -1 or something like that. There is no proper way to fix this, since the two ids don't properly match up). It would fix the bug mentioned and would work in a backwards compatible manner. The only place where this is not needed is the JOYDEVICEADDED event (which already uses device_indices).

If you need help, or are unsure of something, I'd be happy to help you complete the pr. I'd also be open to implementing the change myself on this branch directly.

@yunline
Copy link
Contributor Author

yunline commented Mar 24, 2023

This will break code as implemented.

Sorry about this. It's my bad.
I'll try to fix it soon.

@yunline
Copy link
Contributor Author

yunline commented Mar 25, 2023

I added a new c api pgJoystick_GetDeviceIndexByInstanceID() to get the device index.
But there's a issue, when a joystick is initialized by controller module instead of joystick module, the "joy" attribute will be always -1 because the joystick object is not created.
However, I assume that people who use the deprecated "joy" attribute don't use _sdl2.controller. So I think we don't have to solve this problem.

Copy link
Member

@zoldalma999 zoldalma999 left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks for the pr!🎉

But there's a issue, when a joystick is initialized by controller module instead of joystick module, the "joy" attribute will be always -1 because the joystick object is not created.

Yeah, that is fine. We should deprecate event.joy anyway, since it is not really supposed to be used anymore (but instance_id instead)

Also, sorry for the long delay. I should be (hopefully) more active now

Copy link
Member

@Starbuck5 Starbuck5 left a comment

Choose a reason for hiding this comment

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

Zoldalma walked me through what this does, and it seems legit.

@Starbuck5 Starbuck5 merged commit e88edd1 into pygame-community:main May 29, 2023
@Starbuck5 Starbuck5 added this to the 2.3 milestone May 29, 2023
@yunline yunline deleted the joystick-fix branch May 30, 2023 00:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix PR that fixes bug event pygame.event joystick pygame.joystick
Projects
None yet
3 participants