[3.x] Reset joy axes and buttons after joy disconnection

[orlambda]

Fixes #105047

[lawnjelly]

I'm not super expert in the joystick handling but I think this looks correct.

Would be nice to get some input ( 😁 ) from a more regular input contributor to check so I'll post on rocketchat before going further.

[MJacred]

I probably don't count as a more regular input contributor, but here I go:

The approach is a hack-fix, as it basically sends another event for a device that has been disconnected.

this results currently in…

• one InputEventJoypadMotion event in _input (with axis == 0.0) for each axis I used before disconnecting. It makes no difference whether I hold the stick on disconnect or not
• the first _process() call after disconnect outputs the axis value when using Input.get_action_strength , not 0.0

[lawnjelly]

Just posting this for the author so they can see as discussed in input on rocketchat:
@orlambda

MJacred
2:27 PM
reviewed the code and tested it under various circumstances (and checked against 3.6)

lawnjelly
3:08 PM
The events on disconnect I'm not so sure is a problem (this seems logical to receive cancellation of buttons and axis in user code?), but you could be right about the first _process() call outputting the axis value rather than 0.0 that doesn't sound right... 🤔
Essentially if a device is disconnected, is there a problem with sending events that effectively cancel anything that is pressed?
That means user code can respond to it how you would expect I would think? 🤔

MJacred
3:46 PM
I don't think it's an issue if you get a proper "cancel" event.
both bugs address nonsense behaviour:
_process() gives a value which should have been reset for a device which is not addressable anymore
_input() triggers events on disconnect for actions that happened somewhere in the past. and triggers them again, if you re-connect your joypad and disconnect again w/o touching an axis

lawnjelly
3:49 PM
Is there a problem caused by receiving event from disconnected device? Will it crash, or is it possibility of connecting another device to the same ID or something?
In most cases you would have thought user code would react ok to a 0.0 event (or press being false)... 🤔
It might be possible to log on the joystick buttons whether they have been ever pressed while connected, but is it actually worth it I'm wondering.
I.e. is this a hypothetical problem rather than real world, and could we fix that independently if it was reported as a bug later?
I do take the point that it is not perfect, but we also see a lot of "perfect is the enemy of good enough" so we don't want to lose out on a fix if it makes the situation better than the current.
I'm not an input guy, but that's my thoughts for release management.

MJacred
3:54 PM
regarding the _input(): I reckon a _joy_axis.clear(); is missing, i.e. InputDefault::release_pressed_events() is not used. that's why it re-triggers the same events over and over on disconnect

lawnjelly
3:59 PM
So maybe something like:
if (_joy_axis.has(c) && _joy_axis[c] != 0)
{
_axis_event(p_idx, i, 0.0f);
}

or something like that (pseudocode)?

MJacred
4:00 PM
that should resolve it for _input()
though it won't do proper clearing like InputDefault::release_pressed_events() does
but it could be good enough

lawnjelly
4:04 PM
So it needs to remove the axis from _joy_axis map I'm guessing to be complete.

MJacred
4:07 PM
that would be the clean solution (possibly also clearing other vars).
all things considered, afaik, nobody else uses _joy_axis, so your pseudocode looks good enough to solve this issue

[orlambda]

I tried to address the issues mentioned in the latest commit:

Axes are now removed from the map using _joy_axis.erase(c). (Using _joy_axis.clear() would remove axes from all joypads.)

An event is now only sent for an axis if its value is not 0.0.

Events are now parsed immediately, rather than pushed to buffered_events.

I couldn't observe the first _process() call outputting the axis value rather than 0.0 mentioned by @MJacred, what platform is that on? Does that still happen with this commit?

(I'm using a PS5 controller on macOS 15.5. On disconnection I see values change to a 1 for Up and Left axes for both sticks and 0 for all other axes - these are the values leftovers that weren't being reset for me. Now I sometimes see them output for one or two _process() calls, before on_joy_connection_changed() prints its message but not after. I think fixing that would probably be its own issue.

#84943 addresses this in a different way in Godot 4 by updating action_states so this could possibly be backported as an alternative?

[lawnjelly]

Does this PR deal with buffered_events in parse_input_event?

i.e. could it clear out the current state, but then when buffered_events gets flushed (after the controller disconnected) they could get set back to invalid values?

Maybe we need to go through buffered_events and remove any that concern this controller?

Another possible flaw is whether it is actually ok to call _parse_input_event_impl() at this point in the game loop. Maybe it is always ok because these could be coming in from a separate thread (but then on those platforms maybe these use buffered_events instead).

More concerns have appeared to resolve.

[orlambda]

I don't know if it's ok to use _parse_input_event_impl(), I'd need someone with more knowledge judge that.

Yes we'd need to deal with buffered_events. Would looping through the list be acceptable for performance? Another approach could be to check in _parse_input_event_impl() whether a joypad is connected and ignore the event if not. I'm not sure how this would work with multithreading to make sure no invalid values are parsed before the connected is changed to false. Would this be a good check to add anyway?

I think it's premature to implement the above before it's clear that pushing the reset events ahead of buffered_events is actually safe and beneficial. It would be good to hear how this works on other systems as I don't see a benefit to this on mine.

If I understand correctly, the point of this is to eliminate a one frame delay in resetting the values. I wonder if that would be an acceptably minor issue given the contexts in which joypads would usually be disconnected?

[orlambda]

Any thoughts on what this needs to move forward?

[lawnjelly]

Any thoughts on what this needs to move forward?

Sorry I didn't see your original reply.

It's a while since I read the PR but my own main concern I think was the buffered_events on OSes that use these could still maybe contain events from the controller being deleted.

So the implication is that when removing a controller we should probably also go through buffered_events and remove any that concern the removed controller, so we don't later start getting events coming in from a removed controller.

As I say I'm not an input specialist so I'm having to play it safe here unless we get an approval from an input maintainer.

Another alternative might be to post the controller removed message to the buffered_events, and process everything in order. 🤔

It would be nice to have some testing additionally on platforms that use buffered input.

Don't get me wrong, I suspect the PR as is is probably better than what we have already, but it would be nice to get it all fixed here in a sensible way rather than leaving potential further bugs open, especially on platforms that use buffered input.

[MJacred]

I couldn't observe the first _process() call outputting the axis value rather than 0.0 mentioned by @MJacred, what platform is that on? Does that still happen with this commit?

Ubuntu.
Don't have the time to re-test right now.

[orlambda]

Thanks, is there anyone you know from input you could bring in to have a look?