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

Rework Input action pressed state to support multiple controllers #84943

Merged
merged 1 commit into from
Dec 8, 2023

Conversation

groud
Copy link
Member

@groud groud commented Nov 15, 2023

This implements the following changes:

  • Rename Action to ActionState, it's a bit clearer IMO.
  • Input now store a pressed state per device ID and per event type.
  • For performance (or more like to keep it like it was before maybe? ^_^"), it computes a pressed, strength and raw_strength value in an _update_action_cache function, only when an event is received.

I tested the PR with a keyboard and two controllers connected to the same computer and it worked fine.

Disclaimer: the only thing it does not support right now is controllers correctly being unpressed automatically if they get disconnected. I think it's a rare situation, and it would be hard to fix as I didn't find a way to detect when a device was unplugged. This is now fixed.

I did set the milestone to 4.3 as I think it's quite late for such a change for 4.2. I set both bug and enhancement tags as I am not sure which one it is.

@groud groud added this to the 4.3 milestone Nov 15, 2023
@groud groud requested a review from a team as a code owner November 15, 2023 16:54
@KoBeWi
Copy link
Member

KoBeWi commented Nov 15, 2023

I think it's a rare situation

Not really... At most uncommon.

I didn't find a way to detect when a device was unplugged

Input singleton has a signal for that.

@groud
Copy link
Member Author

groud commented Nov 15, 2023

Not really... At most uncommon.

I meant, having two controllers mapped to the same action, with one being disconnect with one button being pressed might be a be rare. Maybe if there's a connectivity issue though.

Input singleton has a signal for that.

Ah damn, I didn't see that and was checking for signals in display servers. I'll see if I can fix that.

@Calinou
Copy link
Member

Calinou commented Nov 15, 2023

How does this PR compare to #84685? When I tested that PR, multiple input devices interacted seamlessly between each other (exactly like I'd expect them to).

@groud
Copy link
Member Author

groud commented Nov 15, 2023

How does this PR compare to #84685? When I tested that PR, multiple input devices interacted seamlessly between each other (exactly like I'd expect them to).

I think you might not have tested the situations where two controllers (both connected) have the same event mapped to a given action. Releasing on one of the controller should release the action, even if the other controller still pressed the action.

The solution in #84685 does not take the device_id into account, while this PR does. (Unless I missed something, that could be my bad)

@groud
Copy link
Member Author

groud commented Nov 16, 2023

Fixed CI issues (and fixed the joypad disconnection behavior issue too)

@KoBeWi
Copy link
Member

KoBeWi commented Nov 17, 2023

This breaks is_action_just_released() with joypad buttons. It's always false.

@groud
Copy link
Member Author

groud commented Nov 21, 2023

This breaks is_action_just_released() with joypad buttons. It's always false.

Thanks, I tested locally and it should be fixed now.

core/input/input.cpp Outdated Show resolved Hide resolved
@KoBeWi
Copy link
Member

KoBeWi commented Nov 21, 2023

I am not so sure about iterating an array of bools after every event. My bitwise approach was faster. Also I had a small optimization to avoid updating strength when it wouldn't have effect. Though I guess it's maybe not feasible with the new code 🤔

core/input/input.h Outdated Show resolved Hide resolved
core/input/input.cpp Outdated Show resolved Hide resolved
@groud
Copy link
Member Author

groud commented Nov 21, 2023

I am not so sure about iterating an array of bools after every event. My bitwise approach was faster. Also I had a small optimization to avoid updating strength when it wouldn't have effect. Though I guess it's maybe not feasible with the new code 🤔

Yeah. I agree it's slower but this is really not a bottleneck at all, as the array will be always be super small anyway. Inputs management needs to be optimized when processed at the node level, but in the Input singleton, as they are not propagated to the tree, it's less of an issue.

core/input/input.cpp Outdated Show resolved Hide resolved
@KoBeWi
Copy link
Member

KoBeWi commented Nov 21, 2023

I noticed that when you have connected joypad, it sends a released event at the beginning. It's a pre-existing issue though.

core/input/input.cpp Outdated Show resolved Hide resolved
core/input/input.cpp Outdated Show resolved Hide resolved
@groud groud force-pushed the rework_input branch 2 times, most recently from 0d98e0a to 5bc0e1f Compare November 24, 2023 13:41
@groud
Copy link
Member Author

groud commented Nov 24, 2023

Updated the PR with Kobewi's feedback. I reorganized the code a bit but it seems to be working correctly.

core/input/input.cpp Outdated Show resolved Hide resolved
core/input/input.cpp Outdated Show resolved Hide resolved
core/input/input.h Outdated Show resolved Hide resolved
Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally, I can confirm this keeps test_analog_and_digital_input_2.zip working as expected.

@groud Could you upload a testing project that specifically tests the issue this is supposed to fix? I have multiple controllers to test it, but am not sure of a practical test scenario.

@groud
Copy link
Member Author

groud commented Dec 4, 2023

With the test project you provided, just replace the axis in the bindings by a controller button.

This is what happens in 4.2:
video

Basically, the action is set as "unpressed" when releasing the button on the blue controller, while I still press the button on the black controller. This PR fixes that, and only releases the action when both controllers are released.

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

This makes sense given the explanation given here: #84943 (comment)

@YuriSizov YuriSizov merged commit f56765e into godotengine:master Dec 8, 2023
15 checks passed
@YuriSizov
Copy link
Contributor

Thanks!

@golddotasksquestions
Copy link

golddotasksquestions commented Dec 21, 2023

I think you might not have tested the situations where two controllers (both connected) have the same event mapped to a given action. Releasing on one of the controller should release the action, even if the other controller still pressed the action.

I just tested this and now I'm even more confused than before.

What I would expect given two joypads are mapped to the same action (let's call the action "joy_a") and both press the mapped input (let's say button A on two Xbox controller for example), then one of the joypads released the button and then the other ...

func _process(delta):
	if Input.is_action_just_pressed("joy_a"):
		print("joy_a is just pressed")
	if Input.is_action_just_released("joy_a"):
		print("joy_a is just released")

I would expect this to print:

joy_a is just pressed
joy_a is just pressed
joy_a is just released
joy_a is just released

So all of the joypad "presses" and "releases" I would expect to register and print.
I tested this in Godot 3.5.3, 4.2.1 and 4.3.dev1, none of those produce this result:

Godot 3.5.3:

joy_a is just pressed
joy_a is just released

Godot 4.2.1:

joy_a is just pressed
joy_a is just released
joy_a is just released

Godot 4.3 dev1:

joy_a is just pressed
joy_a is just released

I also tested all of this with a single Input (All Devices) mapping, and with two mappings (for both Device 0, Device 1), same result. To be clear, I would expected the result to be the same. I just wanted to be sure this is not correlated.

It first glance it seems like Godot 3.5.3 and Godot 4.3. Dev1 behave the same. But they do not. If you press and hold A on joypad_0, then repeatedly quickly press A on the other joypad_1 (while still holding A on joypad__0), in Godot 3.5.3, the first "press" is from the joypad_0, the first "release" is from joypad_1, and then it's only joypad_1 printing "press" and "release":

Godot 3.5.3:

joy_a is just pressed (joypad_0)
joy_a is just released (joypad_ 1)
joy_a is just pressed (joypad_ 1)
joy_a is just released (joypad_ 1)
joy_a is just pressed (joypad_ 1)
joy_a is just released (joypad_ 1)
joy_a is just pressed (joypad_ 1)
joy_a is just released (joypad_ 1)

Note: When finally releasing A on joypad_0, (which was held all while pressing A joypad_1 repeatedly), it won't print "released"!

In Godot 4.2.1:

joy_a is just pressed (joypad_0)
joy_a is just released (joypad_ 1)
joy_a is just pressed (joypad_ 1)
joy_a is just released (joypad_ 1)
joy_a is just pressed (joypad_ 1)
joy_a is just released (joypad_ 1)
joy_a is just pressed (joypad_ 1)
joy_a is just released (joypad_ 1)
joy_a is just released (joypad_0)

In contrast to Godot 3.5.3, joypad_0 will print "release" once it is finally released! But the first joypad_1 "pressed" is not printed.

In Godot 4.3 Dev1:

joy_a is just pressed (joypad_0)
joy_a is just released (joypad_0)

Godot 4.3 Dev1 is totally different once again, not printing either "pressed" nor "released" of joypad_1, regardless how often you press A on joypad_1, while Button A of joypad_0 is held.

To reiterate, what I would expect is all joypad presses and released to register and print. So in the above example of A on joypad_0 being held while A on joypad_1 is repeadedly pressed, I would expect this to print:

joy_a is just pressed (joypad_0)
joy_a is just pressed (joypad_ 1)
joy_a is just released (joypad_ 1)
joy_a is just pressed (joypad_ 1)
joy_a is just released (joypad_ 1)
joy_a is just pressed (joypad_ 1)
joy_a is just released (joypad_ 1)
joy_a is just pressed (joypad_ 1)
joy_a is just released (joypad_ 1)
joy_a is just released (joypad_0)

However none of the Godot versions I tested behaved as expected.

@Zireael07
Copy link
Contributor

@golddotasksquestions Please post this as a separate issue, not a comment on a PR merged 2 weeks ago (people are likely to miss it)

@groud
Copy link
Member Author

groud commented Dec 22, 2023

Just to answer quickly, I am not sure this would be the expected behavior for everyone:

joy_a is just pressed
joy_a is just pressed
joy_a is just released
joy_a is just released

Since like some may not expect an action to be pressed again once it is already in the pressed state, or, similarly, to be released again once in the "not pressed" state. Also, this means that after the first "is just released" event, then the action will still be in the "pressed" state, which seems also unexpected.

I think this thus deserves a dedicated issue or proposal.

@golddotasksquestions
Copy link

@Zireael07:
#86439

@groud
I disagree. With Godots current behavior, the Godot dev does not have the ability to decide what to do with an input, because the input is not registered in the script.

If one of my players presses or releases any Input, as the dev, I want to know about it.

@KoBeWi
Copy link
Member

KoBeWi commented Dec 22, 2023

Well, the events are received normally. This only affects action's pressed state.

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.

7 participants