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

Allow getting Input "axis" and "vector" values by specifying multiple actions #42976

Merged
merged 2 commits into from
Nov 16, 2020

Conversation

aaronfranke
Copy link
Member

@aaronfranke aaronfranke commented Oct 21, 2020

Implements and closes godotengine/godot-proposals#519

In a nutshell, this PR allows shortening this:

var walk = WALK_FORCE * (Input.get_action_strength("move_right") - Input.get_action_strength("move_left"))

To this:

var walk = WALK_FORCE * Input.get_axis("move_left", "move_right")

And (roughly) this:

var velocity = Vector2(Input.get_action_strength("move_right") - Input.get_action_strength("move_left"), 
		Input.get_action_strength("move_back") - Input.get_action_strength("move_forward")).clamped(1)

To this (a deadzone can also be specified):

var velocity = Input.get_vector("move_left", "move_right", "move_forward", "move_back")

Some discussion remains on what to do with the deadzones for get_vector. @groud Let me know what you think the best approach is for this. As it is now, it behaves exactly like the above code claims, which many Godot projects already use.

Minimal test project: InputGetAxisTest.zip

@groud
Copy link
Member

groud commented Oct 22, 2020

So, my idea is that the get_vector function should have, instead of a p_limit_length argument, an argument to determine the shape of the output. This would be either "Circle" or "Square".

When selecting the "Square" mode, the values are not clamped, and the deadzones also should be square shaped:
squred_deadzone

When in the "Circle" mode, the value should be clamped and the deadzones should be circle-shaped:
circle_deadzone

Note that, since we have one deadzone for each action, I think we should like, stretch the deadzone on each half axis to make things consistent. Here an example where the up deadzone is greater than other deadzones:
egg_deadzone

In any case, the circle deadzone requires you to access the underlying raw values for each action. The value returned by get_action_strengh() being modified by the deadzone value, it cannot be used here to transform the deadzone in a circle.

As a final note, I think we should have negative and positive arguments order reversed. It's quite weird, when the API is written in the a left to right language to have to type "right" first and "left" second.They are literally in the reverse order considering their position in the code line.

@aaronfranke
Copy link
Member Author

aaronfranke commented Oct 23, 2020

@groud What is the correct way to get the raw input amount and deadzone amount for a particular action? There doesn't seem to be an API for this, and the deadzone truncation is very deep.

Input::get_action_strength gets values from action_state populated by _parse_input_event_impl which calls InputEvent::get_action_strength which calls InputMap::event_get_action_status which calls either InputEventAction::get_strength() or InputMap::_find_event which calls InputEvent::action_match which is overridden in many places but for joypads it clamps the value returned from InputEventJoypadMotion::get_axis_value()

Should I add an API for this to Input or InputEvent or Input::Action or InputEventAction or InputMap? I don't want to make spaghetti code to fix this. Also, I noticed Input::_handle_deadzone is a completely unused declaration in the header.

@groud
Copy link
Member

groud commented Oct 23, 2020

Yep, that's why it's a tricky problem. We may require the user to set the deadzones to zero then add a custom deadzone on top of it, but I really don't like this idea as it is not an obvious behavior.

Instead, I believe that we should store a "raw_strength" along the "strength" in the input map, so that we can make our custom computations in the get_vector() function. This requires some changes here and there but it should not be that hard to implement I believe (basically, it's copying what we do with the strength value but without applying the deadzone)

@aaronfranke aaronfranke force-pushed the input-get-axis branch 2 times, most recently from 3647485 to 268ec66 Compare October 23, 2020 22:32
@aaronfranke
Copy link
Member Author

aaronfranke commented Oct 24, 2020

I think the behavior is good now. I tested it with a Steam controller and it works nice. The first commit by itself is a "just a wrapper over get_action_strength" implementation like when I first submitted the PR, and the last 2 commits change this to handle deadzones nicely as described above. Note that currently it uses the deadzone value from the positive X action only. I think implementing a stretched circle would be way overcomplicated and wouldn't actually provide any real world benefit.

What remains is to decide on what order the arguments should be. The current order of (positive, negative) was mostly for similarity with subtracting two get_action_strength values, but indeed it does look confusing especially for right/left. I asked on Discord and there were no strong opinions. I would like community feedback on this, but I think we probably will change it to (negative, positive) as you suggest, I just don't want to change it and then change it back again :P

@groud
Copy link
Member

groud commented Oct 24, 2020

Note that currently it uses the deadzone value from the positive X action only. I think implementing a stretched circle would be way overcomplicated and wouldn't actually provide any real world benefit.

That is exactly what I wanted to avoid. This is doing a quite unpredictable thing behind the user back. If we don't provide a per half axis deadzone (with such a stretched circle), then we should ask the user, in the get_vector() function, to provide the deadzone override.

What remains is to decide on what order the arguments should be. The current order of (positive, negative) was mostly for similarity with subtracting two get_action_strength values, but indeed it does look confusing especially for right/left. I asked on Discord and there were no strong opinions. I would like community feedback on this, but I think we probably will change it to (negative, positive) as you suggest, I just don't want to change it and then change it back again :P

I agree, let's ask for more opinions on this then.

core/input/input_event.cpp Outdated Show resolved Hide resolved
core/input/input_event.cpp Outdated Show resolved Hide resolved
core/input/input_event.cpp Show resolved Hide resolved
core/input/input_event.cpp Show resolved Hide resolved
core/input/input_event.cpp Show resolved Hide resolved
@aaronfranke aaronfranke force-pushed the input-get-axis branch 2 times, most recently from 8e2fba6 to bc6ede7 Compare October 24, 2020 10:22
@aaronfranke aaronfranke force-pushed the input-get-axis branch 2 times, most recently from 43c9934 to ebae03b Compare November 10, 2020 07:06
@aaronfranke
Copy link
Member Author

aaronfranke commented Nov 10, 2020

@groud I made the negative/positive change and I think this is ready for review now.

As for the deadzone, now it can either be manually specified, or if a negative value is passed in (default is -1) it will be calculated automatically from the average of the action deadzones. A deadzone of zero would disable the deadzone. All possible float values except for NaNs are handled (ex: a deadzone of INF means the output is always zero).

@aaronfranke aaronfranke requested a review from groud November 10, 2020 21:11
doc/classes/Input.xml Outdated Show resolved Hide resolved
Copy link
Member

@groud groud left a comment

Choose a reason for hiding this comment

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

Good work, people are going to be happy with that. :)

@akien-mga akien-mga merged commit 19f27ab into godotengine:master Nov 16, 2020
@akien-mga
Copy link
Member

Thanks!

@@ -149,6 +149,9 @@ void Input::_bind_methods() {
ClassDB::bind_method(D_METHOD("is_action_just_pressed", "action"), &Input::is_action_just_pressed);
ClassDB::bind_method(D_METHOD("is_action_just_released", "action"), &Input::is_action_just_released);
ClassDB::bind_method(D_METHOD("get_action_strength", "action"), &Input::get_action_strength);
ClassDB::bind_method(D_METHOD("get_action_raw_strength", "action"), &Input::get_action_strength);
Copy link
Member Author

@aaronfranke aaronfranke Jul 23, 2021

Choose a reason for hiding this comment

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

@groud Do you remember if this was supposed to be bound (or we can re-decide now I guess)? It seems that I bound this incorrectly, but I'm not even sure that it should be bound. If so, this needs to be get_action_raw_strength, and also a string needs to be added to the if statement in get_argument_options (branch).

Copy link
Member

Choose a reason for hiding this comment

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

Ah yeah, it's indeed bound incorrectly.

I don't think accessing the raw value is needed anymore, now that we have get_vector() or get_axis(). But I guess it does not hurt that much to expose it I guess, if anyone want to implement their own deadzone system.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in #50789

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.

Allow getting Input "axis" values by specifying multiple actions
3 participants