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

Fix action state when multiple events are assigned #80859

Merged

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Aug 21, 2023

Fixes #45628

This PR simply changes Action.pressed (internal struct) from bool to int and increments it when action is pressed and decrements when it's released. Input.is_action_pressed() and related actions return true when at least one even is pressed.

There is some weirdness when releasing actions with modifier keys. They can be released without being pressed. I added a small workaround, but the changes need some testing in general.

@KoBeWi KoBeWi added this to the 4.2 milestone Aug 21, 2023
@KoBeWi KoBeWi requested a review from a team as a code owner August 21, 2023 17:14
@Sauermann
Copy link
Contributor

The concept looks good from a quick glance at the changes.
Do you think that this PR supersedes #47599?
This PR should probably revert #30890.

@KoBeWi KoBeWi force-pushed the prepare_for_action_and_make_it_double branch from e84cf58 to c9773c6 Compare August 21, 2023 20:57
@KoBeWi KoBeWi requested a review from a team as a code owner August 21, 2023 20:57
@KoBeWi
Copy link
Member Author

KoBeWi commented Aug 21, 2023

Do you think that this PR supersedes #47599?

Yes. That PR has so many changes that I'm not even sure what it's trying to do.

This PR should probably revert #30890.

Done.

Copy link
Contributor

@Sauermann Sauermann left a comment

Choose a reason for hiding this comment

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

This PR reverts #30890, which originally solved issue #30888.
I have tested this PR and can confirm, that this PR fixes the implementation and resolves #30888. Implementation looks good.

I can't test if this PR solves issue #45628, because I don't have a PS4 controller.

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

KoBeWi commented Aug 22, 2023

I can't test if this PR solves issue #45628, because I don't have a PS4 controller.

You can test with a keyboard, with 2 keys assigned to the same action (e.g. the default ui_accept).

@Sauermann
Copy link
Contributor

I have tested this PR with 2 keys assigned to the same action.
So #45628 is a duplicate of #30888!?

@KoBeWi
Copy link
Member Author

KoBeWi commented Aug 22, 2023

Seems so, although it's weird that it says that it worked in Godot 3 🤔
I'll test with a controller.

@KoBeWi KoBeWi force-pushed the prepare_for_action_and_make_it_double branch from c9773c6 to 0d1aa91 Compare August 22, 2023 10:25
@KoBeWi
Copy link
Member Author

KoBeWi commented Aug 22, 2023

Ok I pushed some changes to also fix action strength.

I can't exactly reproduce the original issue (i.e. "controls randomly not working"), so it needs to be tested by affected users.

@YuriSizov YuriSizov requested review from Sauermann and a team August 25, 2023 10:03
Copy link
Contributor

@Sauermann Sauermann left a comment

Choose a reason for hiding this comment

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

I can't replicate #45628, so it is difficult for me to verify, if the new changes fix that issue.

Comment on lines +725 to +724
action.strength = p_event->get_action_strength(E.key);
action.raw_strength = p_event->get_action_raw_strength(E.key);
Copy link
Contributor

Choose a reason for hiding this comment

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

This implementation changes the action.strength on every event, which might lead to alternating values, if two axis inputs are mapped to the same action and both axis are moved.

The comment #45628 (comment) suggests to use as strength the maximum strength of all related actions. Perhaps that would be a better solution?

Copy link
Member Author

Choose a reason for hiding this comment

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

But then what happens on release? It should assign max strength of still pressed actions, but they are not tracked. This means that if you use analog and then press a key, your analog strength will be ignored until it's released.
Maybe it's fine, idk.

Copy link
Contributor

Choose a reason for hiding this comment

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

It should assign max strength of still pressed actions, but they are not tracked

Perhaps we could track it with the following, but I haven thought it through yet.

diff --git a/core/input/input_map.h b/core/input/input_map.h
index b4d5beacb3..b59e898730 100644
--- a/core/input/input_map.h
+++ b/core/input/input_map.h
@@ -48,10 +48,17 @@ public:
         */
        static int ALL_DEVICES;
 
+       struct EventWithStrength {
+               Ref<InputEvent> event;
+               int pressed = 0;
+               float strength = 0.0f;
+               float raw_strength = 0.0f;
+       };
+
        struct Action {
                int id;
                float deadzone;
-               List<Ref<InputEvent>> inputs;
+               List<EventWithStrength> inputs;
        };
 
 private:

Copy link
Member Author

Choose a reason for hiding this comment

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

This can be addressed later if someone opens an issue. Let's see if the current behavior is ok for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good to me.

@KoBeWi KoBeWi force-pushed the prepare_for_action_and_make_it_double branch from 0d1aa91 to f1d00ab Compare August 25, 2023 15:07
@romlok
Copy link
Contributor

romlok commented Aug 25, 2023

Stumbled upon this PR while investigating some related behaviour, so I tested this under Linux:

When idle, the (my) DS4's analogue sticks often send out a stream of events with very low values (|n| < 0.1), alternating between two ±0.01 values (eg. 0.06, 0.07 in quick succession).

So if a stick axis is mapped to an action, and that action is triggered through a different input than the stick, the action will tend to be "released" after a short amount of time.


I also noticed that, with this PR, if the action is triggered via the stick, it will not always release immediately when the stick is released. The associated input events will be fired, but Input.is_action_pressed will continue to be true for a while afterward. Seemingly until some amount of low-value axis events are fired?


Also, unrelated to stick axes (I disconnected the DS4 just to be sure!), it appears that the "inputs held" counter doesn't reset to zero when the window loses focus. If I hold down two keys to trigger an action, and move focus to another window, Input.is_action_pressed continues to return true. For a single held key the action is considered released when the window loses focus.

FWIW, this was my test project: is-action-pressed.zip

@Sauermann
Copy link
Contributor

alternating between two ±0.01 values (eg. 0.06, 0.07 in quick succession).

Have you tried to set the deadzone to 0.1? That might help.

Input.is_action_pressed continues to return true

That issue is tracked in #36223.

Copy link
Member

@reduz reduz left a comment

Choose a reason for hiding this comment

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

Looks good

@reduz
Copy link
Member

reduz commented Aug 25, 2023

To me this was always the right approach to solve the original issue btw.

@romlok
Copy link
Contributor

romlok commented Aug 25, 2023

alternating between two ±0.01 values (eg. 0.06, 0.07 in quick succession).

Have you tried to set the deadzone to 0.1? That might help.

The deadzone specified in the input map is the default 0.5. AFAICT deadzone is used to determine pressed/not pressed, but doesn't actually filter out any events. Or is there another deadzone setting? 🤔

Input.is_action_pressed continues to return true

That issue is tracked in #36223.

I'm using Linux, but that issue appears to be specific to MacOS? 😕
The current behaviour for me in 4.1 is that actions are considered released when the window loses focus, no matter how many of that action's inputs are being pressed. This patch changes that behaviour for multiple simultaneous inputs.

It seems to be that with this patch the "held keys" is reduced by one on each focus loss, so if an action does get "stuck" in this way, one can release it by repeatedly focus-defocusing the window.

@KoBeWi KoBeWi force-pushed the prepare_for_action_and_make_it_double branch from f1d00ab to ad1abca Compare August 25, 2023 21:10
@KoBeWi
Copy link
Member Author

KoBeWi commented Aug 25, 2023

It seems to be that with this patch the "held keys" is reduced by one on each focus loss, so if an action does get "stuck" in this way, one can release it by repeatedly focus-defocusing the window.

Fixed.

@akien-mga akien-mga merged commit 76bc5a6 into godotengine:master Aug 29, 2023
@akien-mga
Copy link
Member

Thanks!


I actually merged this by mistake xD
I had the tab open because I wanted to do some testing and make sure I can both reproduce the original bug, and confirm the fix. I guess I can still do that post merge.

@jordanlis
Copy link

jordanlis commented Sep 9, 2023

The deadzone specified in the input map is the default 0.5. AFAICT deadzone is used to determine pressed/not pressed, but doesn't actually filter out any events. Or is there another deadzone setting? 🤔

Haa ok, I understand now. So the deadzone doesn't apply if the button is not pressed by the user ? (like these random events) ? I don't really see why we don't just apply this deadzone to any events, it should fix the original issue of really small and random events, no ?

@KoBeWi
Copy link
Member Author

KoBeWi commented Sep 9, 2023

The deadzone is always used. It determines whether action is pressed or not.

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.

Movement doesn't work as expected when mapping keys and axes to the same action
7 participants