Fix lost click event when opening popup

[0HyperCube]

Brief Description of What This PR Does

When opening a popup such as the organelle context menu, the mouse release event will be marked as handled by the popup. This means that the input manager won't call the OnInput function:

Thrive/src/engine/input/InputManager.cs

Lines 573 to 575 in dce65af

	if (unhandledInput || !attribute.OnlyUnhandled)
	{
	if (attribute.OnInput(@event))

This caused the HeldDown field of RunOnKeyAttribute to get stuck in the true state.

I changed the RunOnKeyDownAttribute to rely on Input.IsActionJustPressed rather than the HeldDown state to avoid this issue.

Related Issues
I think this closes #5217?

Progress Checklist

Note: before starting this checklist the PR should be marked as non-draft.

• PR author has checked that this PR works as intended and doesn't
  break existing features:
  https://wiki.revolutionarygamesstudio.com/wiki/Testing_Checklist
  (this is important as to not waste the time of Thrive team
  members reviewing this PR)
• Initial code review passed (this and further items should not be checked by the PR author)
• Functionality is confirmed working by another person (see above checklist link)
• Final code review is passed and code conforms to the
  styleguide.

Before merging all CI jobs should finish on this PR without errors, if
there are automatically detected style issues they should be fixed by
the PR author. Merging must follow our
styleguide.

[revolutionary-bot]

The lead programmer for Thrive is currently on vacation until 2025-01-07. Until then other programmers will try to make pull request reviews, but please be patient if your PR is not getting reviewed.

PRs may be merged after multiple programmers have approved the changes (especially making sure to ensure style guide conformance and gameplay testing are good). If there are no active experienced programmers who can perform merges, PRs may need to wait until the lead programmer is back to be merged.

[hhyyrylainen]

Wouldn't the actual fix be to make sure that up events are always handled? Because that's how I tried to design the input system in the first place. I'm sure I put in at least some code that should feed handled inputs that are release type events into the system so that the held down statuses wouldn't get stuck.

[0HyperCube]

As far as I can see, HeldDown is only set to false in the OnInput and FocusLost methods. OnInput is only called if the input is unhandled (or the attribute has OnlyUnhandled). 'FocusLost' is only called when the window has focus lost.

Could you please point me towards the existing code? Thanks.

[hhyyrylainen]

That's probably exactly the bug, OnInput only passes events it sees but it has code that further key handlers are given all events if they are up events. So this situation likely needs a bit of refactoring so that handled up events would still be fed as up events into all the listeners (so I'd split OnInput into two parts, one that does the normal stuff and one that has just the event forwarding, and then I'd pass handled events that are key up events to the second part of the method).

[0HyperCube]

I'm struggling to implement your proposed solution.

The Godot input code calls the _Input() method before the _UnhandledInput(). This means that inputs that end up being unhandled are processed twice: first as a handled input and then as an unhandled input. You proposed to update the HeldDown state for handled inputs. Therefore HeldDown would never be updated for the unhandled input event (due to the aforementioned double event processing).

Therefore I think that it is not possible to rely on the HeldDown state to determine if the key is pressed. The only solution I have thought of is the Input.IsActionJustPressed (as implemented in this PR). Although perhaps I have misunderstood your proposed approach?

[hhyyrylainen]

Sorry I don't fully understand what you mean. Cannot the input method be modified to only process key up events? (and for efficiency the unhandled input could be made to ignore up events to not process them twice)

[hhyyrylainen]

That would be true if only key up events were processed but my suggestion is:

• Add a new method that runs for all key events that are released (even consumed ones)
• Keep the old method that runs on not consumed events

That way passing the events from that new method and the old method to a handling place, would correctly handle both situations.

When I talked about only processing key up events it was specifically for that new listener, because otherwise it would totally break the input system design as it would react to also consumed input.

[0HyperCube]

Wouldn't that approach break the RunOnKeyUpAttribute? Since both that and the key down one inherit from RunOnKeyAttribute which actually handles the HeldDown state.

Although I suppose I can change it so that the HeldDown state is updated only in the child classes.

[hhyyrylainen]

I did foresee that but I think it is probably better to check if anything would break and then add a big warning for the key up that it will trigger even for consumed inputs so it needs to be used carefully.

[0HyperCube]

I propose that it is better to make all of the attributes work properly by using the Input.IsActionJustPressed and Input.IsActionJustReleased`.

However I also see it is probably advantageous to have the HeldDown state correct, which would involve calling the OnInput for handled input but have it only dispatch the methods if the input is unhandled.

[hhyyrylainen]

I propose that it is better to make all of the attributes work properly by using the Input.IsActionJustPressed and Input.IsActionJustReleased`.

I don't prefer this approach mainly because then I would need to review a big PR with a lot of potential implications all over the codebase. That way I suggested it works with much more minimal changes. And I think that it is already the case that events that are up are already fed to all of the input listeners so it wouldn't be that big of a change to also send GUI-consumed inputs if they are up events to everywhere.