Fix duplicated button press event on touch input

[syntaxerror247]

Currently touch input and drag input is being handled twice. Once as the screen touch and then below as mouse events.

This PR returns early if p_event->get_device() == InputEvent::DEVICE_ID_EMULATION because we have logic for handling both screen touch and mouse events, so there's no need for emulated events here.

closes #120439

[Nintorch]

• I'm afraid this PR basically reverts Fix Input.parse_input_event Button regression #119329, and that would reopen (regression) Using Input.parse_input_event can make button presses stop working in some circumstances #119320 :(

[syntaxerror247]

@Nintorch I tested it but can't reproduce that regression.
Tested on Fedora linux and Android.

[Nintorch]

This PR seems to work correctly for me on both Windows and Android, I can reproduce the issue on master but not here.

I'm not sure why this doesn't reintroduce the problem I mentioned earlier, it's possible it could have been fixed by something else besides my PR. But that's good!

This also begs the question: should we really ignore emulated events altogether in BaseButton? Could there be a project that relies on BaseButton reacting to artificial InputEventMouseButton and InputEventMouseMotion events generated by the game code?
But if it's not a big deal, I think this approach looks good!

[syntaxerror247]

This also begs the question: should we really ignore emulated events altogether in BaseButton? Could there be a project that relies on BaseButton reacting to artificial InputEventMouseButton and InputEventMouseMotion events generated by the game code?

According to implementation, if emulate_mouse_from_touch is true, every touch input generates a corresponding mouse event. Likewise, if emulate_touch_from_mouse is true, every mouse input generates a corresponding touch event. So, we do need to ignore one event.

The same logic also applies when the user calls Input.parse_input_event(...) from game code and these calls are not marked as emulated, unless user manually sets the event.device = InputEvent.DEVICE_ID_EMULATION.

So, if the event's device is explicitly set to InputEvent.DEVICE_ID_EMULATION, in that case it would now be ignored. However, there's little to no reason for users to even set device in the first place. This is the only case that is breaking.
I think the solution here would be to document that InputEvent.DEVICE_ID_EMULATION should only be used for checking the physical input and not be used with Input.parse_input_event(...) to set device id and mention this in 4.7 migration guide?

InputEvent.DEVICE_ID_EMULATION class reference already makes it very clear that it's only for checking physical input.

[Nintorch]

Hm, I see, in this case I think the PR looks good to me!

We may also want to mark it as breaks compat so it gets mentioned in the migration guide.

[syntaxerror247]

This also needs to be cherrypicked to 4.7, and breaks compat and cherrypick don't usually go together. I think it's probably fine in this case since the behavior change is very minor?

[KoBeWi]

I can't test, but the change makes sense.

[Repiteo]

Thanks!

[KurodaKayn]

I still think this fix is too broad and not precise enough.

Returning early for every InputEvent::DEVICE_ID_EMULATION event means BaseButton now ignores more than just mouse events generated from touch input. This can also block:

• User-created InputEventMouseButton events if the script sets device = InputEvent.DEVICE_ID_EMULATION.
• Other emulated mouse events that are not generated from touch input.
• Any future synthetic input path that reuses DEVICE_ID_EMULATION.

The bug here is specifically about the duplicated mouse event generated from touch input, so I think the fix should distinguish that source instead of treating all emulated events as equivalent.

Why not use the more precise approach from my PR? It marks mouse events generated from touch input and makes BaseButton ignore only those. Without the tests, the actual implementation is still under 30 lines, and it avoids changing the behavior of unrelated emulated mouse input.

[ace24713]

Has anyone tested how this interacts with a button_mask set to right click and/or the project setting enable_long_press_as_right_click?

[syntaxerror247]

I still think this fix is too broad and not precise enough.

Returning early for every InputEvent::DEVICE_ID_EMULATION event means BaseButton now ignores more than just mouse events generated from touch input. This can also block:

• User-created InputEventMouseButton events if the script sets device = InputEvent.DEVICE_ID_EMULATION.

Yes, but it is documented that DEVICE_ID_EMULATION is used for touch to mouse and mouse to touch emulation. I'd expect users to not set this in the first place. Anyway, I'll mention this compact break in 4.7 migration guide.

• Other emulated mouse events that are not generated from touch input.

The only other emulated event is for mouse to touch, and that will have a corresponding mouse event, so it will be handled too.

• Any future synthetic input path that reuses DEVICE_ID_EMULATION.

As I said, DEVICE_ID_EMULATION is documented that it's for touch-mouse emulation, so I'd say it shouldn't be resued for any other purpose.

The bug here is specifically about the duplicated mouse event generated from touch input, so I think the fix should distinguish that source instead of treating all emulated events as equivalent.

Why not use the more precise approach from my PR? It marks mouse events generated from touch input and makes BaseButton ignore only those. Without the tests, the actual implementation is still under 30 lines, and it avoids changing the behavior of unrelated emulated mouse input.

Your PR is essentially doing the same thing. Instead of returning early, it ntroduces a new property and add checks for emulated events.

[syntaxerror247]

Has anyone tested how this interacts with a button_mask set to right click and/or the project setting enable_long_press_as_right_click?

Touch events don't have a button mask, so this has no effect on touch input. However, if you have encountered unexpected behavior, please open a bug report.

Edit: tested with enable_long_press_as_right_click, there's a bug i found #120962

[KurodaKayn]

Your PR is essentially doing the same thing. Instead of returning early, it ntroduces a new property and add checks for emulated events.

Small correction: my PR does not check for emulated events in general.

It checks for mouse events specifically generated from touch input.
DEVICE_ID_EMULATION remains broader: touch->mouse, mouse->touch, and user-dispatched events using that device id.

So difference is not early return vs extra property. Difference is device-wide filtering vs source-specific filtering.

I see your point about DEVICE_ID_EMULATION being documented for touch-mouse emulation.

My concern is where policy is applied. Returning early in BaseButton makes DEVICE_ID_EMULATION mean “Button must ignore this event”. Docs say it identifies emulated input, not that GUI controls should drop it.

That also creates a compat break for user-dispatched InputEventMouseButton with device = DEVICE_ID_EMULATION. If this needs migration guide mention, then behavior change is real.

I don’t think my PR does same thing. It only marks mouse events generated from touch input, then BaseButton ignores only that duplicate mouse path. Regular emulated mouse events still work.

Bug is specifically “real touch + generated mouse both handled”. So fix can suppress only generated mouse sibling, not all emulated input.

[syntaxerror247]

That also creates a compat break for user-dispatched InputEventMouseButton with device = DEVICE_ID_EMULATION. If this needs migration guide mention, then behavior change is real.

This compact break is intentional and I think this is only issue. If there's any other behaviour change that you observed then please create a bug report for that.

[Repiteo]

Despite being minor, maintenance releases aren't the place for compat-breaking changes. I would instead encourage making a dedicated 4.7 PR that documents the issue instead