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

Fix Input::remove_joy_mapping #98792

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

MJacred
Copy link
Contributor

@MJacred MJacred commented Nov 3, 2024

Erasing a joypad mapping can invalidate other attached joypads and the fallback mapping guid

Fixes #91257 (cannot confirm crash on Linux. Untested on Windows where it was observed)

If this PR is merged, it needs to be cherry-picked/backported to all supported Godot versions, afaik.

NOTE

  • I'm not sure if I handled fallback_mapping correctly, because according to Input::is_joy_known the fallback does not count as a known device… Therefore, feedback on this would be nice
    • If it's only meant as an internal fallback, a more minimal change of this PR to the fallback logic would be to deny removing the mapping for that one. Then I could remove the setter and getter for the fallback. -> Removed bindings
  • I renamed some members in windows joypad file to differentiate between direct INPUT and direct X. I found d_joypads, and it feels like the d_ prefix was meant as a direct input abbreviation. So I used that. I'm open to alternative naming convention, though. Some renaming should happen to untangle INPUT and X. -> Renaming is done in another PR

@MJacred MJacred requested review from a team as code owners November 3, 2024 11:42
@MJacred MJacred changed the title Fix Input::remove_joy_mapping Draft: Fix Input::remove_joy_mapping Nov 3, 2024
@MJacred MJacred force-pushed the fix_remove_joy_mapping branch from 0c33bde to 42f5514 Compare November 3, 2024 11:51
@MJacred MJacred changed the title Draft: Fix Input::remove_joy_mapping Fix Input::remove_joy_mapping Nov 3, 2024
@MJacred MJacred force-pushed the fix_remove_joy_mapping branch from 42f5514 to fd8aa95 Compare November 3, 2024 12:02
@AThousandShips AThousandShips added bug topic:input crash cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release labels Nov 3, 2024
@AThousandShips AThousandShips added this to the 4.4 milestone Nov 3, 2024
@MJacred MJacred force-pushed the fix_remove_joy_mapping branch from fd8aa95 to ce6fae4 Compare November 3, 2024 15:28
@MJacred MJacred requested a review from a team as a code owner November 3, 2024 15:28
@MJacred MJacred force-pushed the fix_remove_joy_mapping branch from ce6fae4 to 556f90b Compare December 14, 2024 11:37
doc/classes/Input.xml Outdated Show resolved Hide resolved
doc/classes/Input.xml Outdated Show resolved Hide resolved
doc/classes/Input.xml Outdated Show resolved Hide resolved
@MJacred MJacred force-pushed the fix_remove_joy_mapping branch from 8a92f94 to 18bce29 Compare December 15, 2024 12:39
Erasing a joypad mapping can invalidate other attached joypads and the fallback mapping guid
@MJacred MJacred force-pushed the fix_remove_joy_mapping branch from 18bce29 to 4083238 Compare December 16, 2024 19:27
@MJacred
Copy link
Contributor Author

MJacred commented Dec 16, 2024

status update:

  • Rebased PR to latest master, resolving merge conflict.
  • Removed bindings in Godot's spirit of "don't give bindings to things people don't need"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release crash topic:input
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove a non-xinput device joymapping cause game crash
3 participants