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

Add support for mouse axes in the input map #80259

Closed
wants to merge 1 commit into from
Closed

Add support for mouse axes in the input map #80259

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Aug 4, 2023

It felt odd to me that relative mouse motion could only be referenced by manually handling events in scripts so this PR adds the ability to bind actions to the various mouse axes. It looks like this:

2023-08-04 07-59-15

This lets me setup a nice first person input map entirely in the editor:

2023-08-03 22-39-11

With that input map setup one can configure a first person controller using only actions which feels a lot more intuitive:

extends CharacterBody3D

const SPEED = 5.0
const LOOK_HORIZONTAL_SENSITIVITY = 0.01
const LOOK_VERTICAL_SENSITIVITY = 0.01

var gravity = ProjectSettings.get_setting("physics/3d/default_gravity")

@onready var neck = $Neck
@onready var camera = $Neck/Camera

func _ready():
    Input.mouse_mode = Input.MOUSE_MODE_CAPTURED

func _physics_process(delta):
    if not is_on_floor():
        velocity.y -= gravity * delta
        
    var look_horizontal = Input.get_axis("Look Right", "Look Left") * LOOK_HORIZONTAL_SENSITIVITY
    neck.rotate_y(look_horizontal)

    var look_vertical = Input.get_axis("Look Down", "Look Up") * LOOK_VERTICAL_SENSITIVITY
    camera.rotate_x(look_vertical)
    camera.rotation_degrees.x = clamp(camera.rotation_degrees.x, -85, 85)

    var input_dir = Input.get_vector("Move Left", "Move Right", "Move Forward", "Move Backward")
    var direction = (neck.basis * Vector3(input_dir.x, 0, input_dir.y)).normalized()
    if direction:
        velocity.x = direction.x * SPEED
        velocity.z = direction.z * SPEED
    else:
        velocity.x = move_toward(velocity.x, 0, SPEED)
        velocity.z = move_toward(velocity.z, 0, SPEED)

    move_and_slide()

It's my first time contributing to Godot so hopefully I followed all the patterns expected. I tried to keep my code formatted as similar to the surrounding code.

@ghost ghost requested review from a team as code owners August 4, 2023 15:01
@AThousandShips
Copy link
Member

AThousandShips commented Aug 4, 2023

This kind of change requires a proposal, please open one here, this is to gauge support and various details

One already exists that seems to fit this:

@ghost
Copy link
Author

ghost commented Aug 4, 2023

Ah sorry, I didn't realize there was a proposal process for this. I've added a comment to that issue and linked this PR.

@@ -91,6 +92,17 @@ String EventListenerLineEdit::get_event_text(const Ref<InputEvent> &p_event, boo
if (text.is_empty()) {
text = "(" + TTR("Unset") + ")";
}
} else if (mouse_motion.is_valid()) {
Vector2 rel = mouse_motion->get_relative();
if (rel.x < 0){
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (rel.x < 0){
if (rel.x < 0) {

Please run clang-format as you have several errors like this and indentation errors

Copy link
Author

Choose a reason for hiding this comment

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

My apologies. I'll get that setup and run later when I'm back on my personal machine.

Comment on lines +97 to +105
if (rel.x < 0){
text = TTR("Mouse X (Left)");
} else if (rel.x > 0){
text = TTR("Mouse X (Right)");
} else if (rel.y < 0){
text = TTR("Mouse Y (Up)");
} else {
text = TTR("Mouse Y (Down)");
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't tested this, but on paper it sounds like checking for < or > exactly 0 would be problematic.

Not only do you have to keep your mouse perfectly still after pressing the button, to avoid a misdetected mouse input. But also you'd have to more your mouse perfectly straight to avoid detecting the wrong axis (or in the case of this code, the X axis is easier to trigger than the Y axis).

Suggestion would be to have a higher movement threshold value (e.g. something like "move at least 10 pixels"), and then take the axis with the highest magnitude and use that to decide which input was made. This avoids both issues.

Copy link
Author

Choose a reason for hiding this comment

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

This is for displaying the configured axis in the input configuration window. The events bound to those actions are always unit length on one axis. This is not a general to_string implementation for mouse moved events nor is it part of the logic of detecting the action.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oops my bad, I just looked at the class name and assumed that's what the code was doing without taking a closer look 😁

Copy link
Author

Choose a reason for hiding this comment

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

No worries. :)

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, it works as expected.

Testing project: test_pr_80259.zip

Some comments:

  • Unfortunately, this PR will run into Movement doesn't work as expected when mapping keys and axes to the same action #45628 until it's fixed. Considering the main use case of this PR, I think this is a blocking issue as it breaks the point of this PR. You can see this in the testing project by changing the value of NO_MOUSE in the CharacterBody3D script, which uses a separate action without mouse axis inputs.

  • I find that you can't really have an ideal mouse sensitivity and gamepad sensitivity with a single value.1 In the testing project, I've set my mouse sensitivity to be acceptable (still a bit high), but the gamepad sensitivity then becomes extremely low.

  • Most games expose mouse sensitivity and gamepad sensitivity as separate toggles, while we don't have any kind of built-in overrides for captured mouse sensitivity. As a result, you still need to identify the source of the input action to apply a dedicated sensitivity modifier. This kind of breaks the point of this PR, unfortunately.

  • Dedicated editor icons for mouse axes are missing, unlike joypad axes which have separate icons.

  • The class reference documentation for InputEventMouseMotion (doc/classes/InputEventMouseMotion) should mention that the action's deadzone is ignored for mouse axes. This might be obvious to some, but it's better to mention it explicitly 🙂

I think the core idea of this PR is encouraging, but it needs further work before it's really a net win in most situations.

Footnotes

  1. For reference, I use an ASUS ROG Harpe Ace at 1600 dpi (2.4 GHz wireless) and a DualSense (wired) on Linux.

@ghost
Copy link
Author

ghost commented Aug 5, 2023

Tested locally, it works as expected.

Testing project: test_pr_80259.zip

Thank you for taking a look!

Unfortunately, this PR will run into Movement doesn't work as expected when mapping keys and axes to the same action #45628 until it's fixed. Considering the main use case of this PR, I think this is a blocking issue as it breaks the point of this PR. You can see this in the testing project by changing the value of NO_MOUSE in the CharacterBody3D script, which uses a separate action without mouse axis inputs.

I wouldn't say this is the main use case. Given the sensitivities on joystick and mouse inputs are going to be quite different I would expect to not bind joystick and mouse axes to the same input. In my comment on the proposal I pointed out that I would still expect to bind one action for mouse input and a separate one for joystick input. This does mean the two are not treated uniformly, but it still is a win to me because I only have to interact with Input.get_axis in the code.

I also mentioned in that comment that I want to try and build a new input system for Godot that would hopefully be able to address many of these issues because it would be a net-new system that wouldn't need to be concerned with backward compatibility.

I find that you can't really have an ideal mouse sensitivity and gamepad sensitivity with a single value.1 In the testing project, I've set my mouse sensitivity to be acceptable (still a bit high), but the gamepad sensitivity then becomes extremely low.

Absolutely. I think for now this is going to require multiple actions and two sensitivity values in GDScript. In my future system I'd like to be able to bind per-device sensitivities and such like Unreal and Unity allow.

Most games expose mouse sensitivity and gamepad sensitivity as separate toggles, while we don't have any kind of built-in overrides for captured mouse sensitivity. As a result, you still need to identify the source of the input action to apply a dedicated sensitivity modifier. This kind of breaks the point of this PR, unfortunately.

True if you're trying to combine the actions, but again with the overall limits of the input system I wouldn't expect that to be the case with this PR. To reiterate: the point of this PR is not to enable a single action to handle both mouse and joystick input, it's to enable actions to handle mouse motion at all. In order to support actions that handle multiple device types it's going to be necessary to overhaul the existing system which is beyond the scope of what I was trying to achieve.

Dedicated editor icons for mouse axes are missing, unlike joypad axes which have separate icons.

Yep that'd be a great thing to add but I'm no artist. Happy to incorporate an icon if folks have or make one.

The class reference documentation for InputEventMouseMotion (doc/classes/InputEventMouseMotion) should mention that the action's deadzone is ignored for mouse axes. This might be obvious to some, but it's better to mention it explicitly slightly_smiling_face

Ah yes for sure I need to do a documentation pass if this feature is ultimately accepted.

@addmix
Copy link
Contributor

addmix commented Aug 5, 2023

I think the best option to alleviate the controller/mouse sensitivity issue would be to add a strength property to the InputEvent class. This may seem unconventional, but let me explain how I envision it being used.

Say you have an action called "Camera Movement Right", to which you map the following inputs:
Mouse Axis X+,
Controller Right Joystick Axis X+,
Right arrow key

You cannot apply a multiplier to the result of Input.get_action_strength("Camera Movement Right"), as each input type would need a separate sensitivity. The solution I see discussed previously was to identify the source of the input, and apply the sensitivity that way. This seems needlessly complex.

If each input can have a strength/sensitivity associated with it, this can be avoided. The current convention is that Input.get_action_strength() returns a value between 0 and 1. If strength acts as a sensitivity/multiplier at the input level, then Input.get_action_strength() would evaluate as the typical 0-1 output, multiplied by the strength of the input.

This may cause need for input functions to evaluate an input's strength before any multipliers are applied, returning the unmodified 0-1 axis strength, as is available currently.

This scheme would not break compatability, as all InputEvents would have a default strength of 1, leading to no change in the output of Input.get_action_strength() until the strength value of inputs are modified.

Another rather simple feature that I believe would solve many complexity issues with input-related code is an option for input events to "echo" every frame. This would primarily be used to simplify controller camera movement. Currently, you need to use the _input() functions for mouse, and the Input.get_action_strength() functions for controller joysticks.

If a controller axis input is beyond that axes' dead zone, and this echo functionality is enabled, then the controller axis event is fired every frame until the axis returns fo the dead zone. Then controller camera movement is handled in exactly the same way as mouse movement is handled currently. This echo functionality would also extend to key inputs, allowing easy camera control with keys/buttons.

Using my input map entry listed above, implementing camera movement for mouse, joystick, and buttons is as simple as:

Edit: I realized, after I had finalized this post, that these echoed inputs would also need to be adjusted for delta-time, ideally that would be done automatically, as there are limited use-cases for a framerate-dependent input like that.

func _process(delta):
    camera.rotation.y += Input.get_action_strength("Camera Movment Right")

Or, using proper input propagation

func _unhandled_input(event):
    if event.is_action("Camera Movement Right"):
        camera.rotation.y += event.magnitude

(event.magnitude is a placeholder, used because it is less vague than event.value, but it is supposed to represent the input's axis value, multiplied by the strength of that input mapping. The entire naming scheme is a little confusing, but I'm not looking to renamed half the functions and variables in the input area of the engine)

This echo functionality combined with the input level strength/sensitivity multiplier creates a greatly streamlined input system, removing lots of repetitive and often boilerplate code, at the cost of two rather small options.

@Zireael07
Copy link
Contributor

I agree with @Calinou that #45628 should be considered a blocker for this one

@ghost
Copy link
Author

ghost commented Aug 5, 2023

I very much respect the opinions here. There are clear issues around the input map.

That said I think the best solution for all of these is to work on a new input system. I feel the current one has enough deficiencies that continuing to build on it, while having to maintain compatibility, isn’t the wisest use of time.

I’ve been wanting to work on an improved input system for Godot so perhaps I’ll come up with a fuller proposal once I have the free time.

@addmix
Copy link
Contributor

addmix commented Oct 25, 2023

I agree with @Calinou that #45628 should be considered a blocker for this one

Looks like that PR was merged. Besides the style checks failing, anything preventing this from moving forward?

@ghost ghost closed this by deleting the head repository Nov 9, 2023
@AThousandShips AThousandShips removed this from the 4.x milestone Nov 9, 2023
@Calinou
Copy link
Member

Calinou commented Jan 24, 2024

Looks like that PR was merged. Besides the style checks failing, anything preventing this from moving forward?

I've just tested this PR rebased against master 442bbb5 and it still exhibits the issue from #45628 🙁

I'll label this as salvageable nonetheless, but I think this needs to be resolved before the PR can be considered for merging.

This pull request was closed.
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.

Add MouseMovement to InputMap
5 participants