Accumulated input instance sharing regression in Input.parse_input_event()

[EIREXE]

Godot version

3.x (b7ded89)

System information

Arch Linux

Issue description

Previously, passing a reused instance to parse_input_event even with accumulated input enabled would not cause the event to be shared between emitted events.

Steps to reproduce

Create two input events with input accumulation enabled:

        Input.set_use_accumulated_input(true)
	var a = InputEventAction.new()
	for action in ["test1", "test2"]:
		a.action = action
		Input.parse_input_event(a)

Parse them in another node:

func _input(event):
	if event is InputEventAction:
		print(event.action)

event.action will return test2 in all cases, in an older version it would return test1 and test2 in alternating fashion

Minimal reproduction project

InputBug.zip

[akien-mga]

I can reproduce the issue with the MRP. The behavior changed between 3.4 beta 3 and 3.4 beta 4, which likely points at #42220. CC @RandomShaper

I don't know yet if it's an actual bug or a bugfix, I'll let @RandomShaper assess.

To me both behaviors seem non-intuitive when reading the description of set_use_accumulated_input. I tweaked the project to count how many events are sent and received in 3 seconds, and it's the same number for both. So I'm not sure what accumulation happens at all here.

InputBug.zip

[EIREXE]

I can reproduce the issue with the MRP. The behavior changed between 3.4 beta 3 and 3.4 beta 4, which likely points at #42220. CC @RandomShaper

I don't know yet if it's an actual bug or a bugfix, I'll let @RandomShaper assess.

To me both behaviors seem non-intuitive when reading the description of set_use_accumulated_input. I tweaked the project to count how many events are sent and received in 3 seconds, and it's the same number for both. So I'm not sure what accumulation happens at all here.

InputBug.zip

I think accumulation is done in the OS implementation, and since we are firing events ourselves it's likely those don't get accumulated, but don't quote me on that.

[RandomShaper]

To me both behaviors seem non-intuitive when reading the description of set_use_accumulated_input. I tweaked the project to count how many events are sent and received in 3 seconds, and it's the same number for both. So I'm not sure what accumulation happens at all here.

I guess the description could be a bit more precise. Only "flowing" events like mouse or touch drags can be accumulated. Presses/unpresses of keys, mouse buttons, etc. are kept because they are relevant actions that can't be squashed together without losing important information.

I think accumulation is done in the OS implementation, and since we are firing events ourselves it's likely those don't get accumulated, but don't quote me on that.

It isn't. It's InputDefault, the OS agnostic input management class, where accumulation happens.

I don't know yet if it's an actual bug or a bugfix, I'll let @RandomShaper assess.

In 3.4-beta3, Input::parse_input_event() bypasses input event accumulation (it processes the event immediately).
In later versions, Input::parse_input_event() processes immediately if accumulation is disabled or accumulates if it's enabled.

Under the hood, the situation was that in 3.4-beta3 we had InputDefault::parse_input_event() and InputDefault::accumulate_input_event(), the latter not being exposed. Therefore, there was no way to send an input event from a script obeying whether accumulation was enabled. Newer versions got InputDefault::accumulate_input_event() removed. Scripting still has InputDefault::parse_input_event(), which can accumulate or process immediately appropriately.

Therefore, it's a bit hard to tell if older versions had a bug from the scripting perspective (Input::parse_input_event() ignores if accumulation is enabled) or if instead compatibility has been broken in newer versions. In order to decide, since (again, from the scripting perspective) accumulate_input_event() was never exposed, I'd say that the problem was in older versions, where you couldn't accumulate events from script and so the newer versions have a corrected behavior.

Furthermore, if we considered the bug is in the newer implementation, things could get a bit akward:

• Scripting InputDefault::parse_input_event() wouldn't map to C++ InputDefault::parse_input_event(), but to some new function that would lock and call _parse_input_event_impl().
• We'd have to expose InputDefault::accumulate_input_event(), which wouldn't make much sense, since in case accumulation is disabled, events sent via that method would get nowhere.

---

Final note: it makes sense that a re-used input event with accumulation enabled exhibits that behavior. Accumulation works by queueing references to the events for a later flush at the end of the frame. You are effectively buffering the same object twice. "Fixing" that would involve making copies of every accumulated event, which could harm performance. The guideline would be: once you've sent an input event to the engine, forget about it. (Also, accumulation will modify the last queued event if the next one can be fused to it, so you can't count on having control over it.)

[EIREXE]

To me both behaviors seem non-intuitive when reading the description of set_use_accumulated_input. I tweaked the project to count how many events are sent and received in 3 seconds, and it's the same number for both. So I'm not sure what accumulation happens at all here.

I guess the description could be a bit more precise. Only "flowing" events like mouse or touch drags can be accumulated. Presses/unpresses of keys, mouse buttons, etc. are kept because they are relevant actions that can't be squashed together without losing important information.

I think accumulation is done in the OS implementation, and since we are firing events ourselves it's likely those don't get accumulated, but don't quote me on that.

It isn't. It's InputDefault, the OS agnostic input management class, where accumulation happens.

I don't know yet if it's an actual bug or a bugfix, I'll let @RandomShaper assess.

In 3.4-beta3, Input::parse_input_event() bypasses input event accumulation (it processes the event immediately). In later versions, Input::parse_input_event() takes processes immediately if accumulation is disabled or accumulates if it's enabled.

Under the hood, the situation was that in 3.4-beta3 we had InputDefault::parse_input_event() and InputDefault::accumulate_input_event(), the latter not being exposed. Therefore, there was no way to send an input event from a script obeying whether accumulation was enabled. Newer versions got InputDefault::accumulate_input_event() removed. Scripting still have InputDefault::parse_input_event(), which can accumulate or process immediately appropriately.

Therefore, it's a bit hard to tell if older versions had a bug from the scripting perspective (Input::parse_input_event() ignores if accumulation is enabled) or if instead compatibility has been broken in newer versions. In order to decide, since (again, from the scripting perspective) accumulate_input_event() was never exposed, I'd say that the problem was in older versions, where you couldn't accumulate events from script and so the newer versions have a corrected behavior.

Furthermore, if we considered the bug is in the newer implementation, things could get a bit akward:

* Scripting `InputDefault::parse_input_event()` wouldn't map to C++ `InputDefault::parse_input_event()`, but to some new function that would lock and call `_parse_input_event_impl()`.

* We'd have to expose `InputDefault::accumulate_input_event()`, which wouldn't make much sense, since in case accumulation is disabled, events sent via that method would get nowhere.

Final note: it makes sense that a re-used input event with accumulation enabled exhibits that behavior. Accumulation works by queueing references to the events for a later flush at the end of the frame. You are effectively buffering the same object twice. "Fixing" that would involve making copies of every accumulated event, which could harm performance. The guideline would be: once you've sent an input event to the engine, forget about it. (Also, accumulation will modify the last queued event if the next one can be fused to it, so you can't count on having control over it.)

I don't think such behaviour is necessarily wrong or correct, I'm just saying it's behaving differently.

[akien-mga]

So given the above discussion, I think this just requires some better documentation?

[RandomShaper]

In short, I'd say so. The unfortunate thing here is that an implementation detail can be noticed from the surface and at least a piece of documentation warning about the fact that input events are reference counted and the engine doesn't get an unique view of them once passed the API boundary, or something like that.

That said, it would be possible to put some code that detects this kind of usage and duplicates the events under the hood, but I'm already foreseeing some unintended side effects of that can happen as well. So, let's go the docs route.

By the way, @EIREXE, ¡Hola!

[Rubonnek]

This issue has been fixed in master at #64423 and in 3.x at #64424.

Running the MRP now yields the following message:

WARNING: An input event object is being parsed more than once in the same frame, which is unsafe.
If you are generating events in a script, you have to instantiate a new event instead of sending the same one more than once, unless the original one was sent on an earlier frame.
You can call duplicate() on the event to get a new instance with identical values.

Closing.