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

Revert #55978 "Fixed event spam when using the Nintendo Switch controller" #56028

Conversation

madmiraal
Copy link
Contributor

#55978 will reintroduce #42876. As discussed in #43674, instead of imposing a fixed limit on the sensitivity of the joystick, we need a user configurable project settings with Godot defaults for both the joystick sensitivity and the joystick deadzone.

@Calinou
Copy link
Member

Calinou commented Dec 17, 2021

As I understand it, this needs to be made optional with a project setting so that users who need more precision (steering wheels/HOTAS) can opt in. Otherwise, this filter should remain enabled by default as having only 128 levels of pressure isn't an issue for analog sticks or analog keyboards.

Most indie games don't need (good) steering wheel or HOTAS support, but good gamepad support is important to most games.

@akien-mga
Copy link
Member

CC @slouken

@akien-mga akien-mga changed the title Revert #55978 Revert #55978 "Fixed event spam when using the Nintendo Switch controller" Dec 17, 2021
@akien-mga
Copy link
Member

akien-mga commented Dec 17, 2021

As discussed in #43674, instead of imposing a fixed limit on the sensitivity of the joystick, we need a user configurable project settings with Godot defaults for both the joystick sensitivity and the joystick deadzone.

Yeah I reviewed and merged this a bit fast (and it's included in 3.4.1 which I'm about to release - but I think the issue it reintroduces is less problematic than the bug it's fixing, so it's not too bad IMO. We can improve this in 3.4.2.).

I agree that this filter seems mostly relevant for joysticks at rest, and thus a minimal deadzone (which can be configurable, but should have a good default value) makes sense. And then another option for sensitivity can be useful if there's a need indeed, though it seems the main issue is really the idle point.

@slouken
Copy link
Contributor

slouken commented Dec 17, 2021

A deadzone would also solve the issue with "not pressed" events being continually sent, but note that the problem occurs anytime the joystick is outside the deadzone but less than 0.5. The deadzone also varies significantly between controllers and even individual controllers, so if someone is testing with a PS4 controller and creates a tight deadzone (e.g. 4000) based on observed values, then this issue will crop up again.

Maybe it makes sense for the input logic to be changed so if you have pressed events, they override not-pressed events?

@madmiraal
Copy link
Contributor Author

madmiraal commented Dec 17, 2021

A deadzone would also solve the issue with "not pressed" events being continually sent, but note that the problem occurs anytime the joystick is outside the deadzone but less than 0.5.

Exactly. The only problem #55978 is solving is #43674 (noisy events generated by sensitive joysticks), which IMO is less significant than #42876. Note: #55978 would make #42876 worse, because now only motions greater than 0.05 (previously it was 0.01) would be detected.

Maybe it makes sense for the input logic to be changed so if you have pressed events, they override not-pressed events?

Note: #47599 fixes #45628 i.e. the problem with not pressed events being sent.

@akien-mga
Copy link
Member

Otherwise, this filter should remain enabled by default as having only 128 levels of pressure isn't an issue for analog sticks or analog keyboards.

IINM (and I should have spotted that on first review before merging), this actually limits sensitivity to 20 levels (5% increments) per direction, which seems quite extreme. The previous filter or 0.01 was more reasonable IMO.

I agree with reverting this PR for now, but I guess we should keep track of the discussion and proposed solution(s) in a new issue (or PR directly)?

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Approved as it fixes a regression, albeit it reintroduces a bug that will need a different fix.

@akien-mga akien-mga merged commit 5ffee63 into godotengine:master Dec 21, 2021
@akien-mga
Copy link
Member

Thanks!

@madmiraal
Copy link
Contributor Author

we should keep track of the discussion and proposed solution(s) in a new issue (or PR directly)?

I'll open a proposal where it can be discussed in a central location as the issues are now spread across multiple issues and PRs.

@madmiraal madmiraal deleted the revert-55978-fix-switch-controller-event-spam branch December 21, 2021 11:16
@madmiraal
Copy link
Contributor Author

I've created godotengine/godot-proposals#3709.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants