feat: add mouse wheel support #445
Conversation
for both horizontal and vertical movement.
|
I worked on this with @ShadowApex . There are a lot of existing stubs for Mouse::Button that are unused. This first PR adds a functional Mouse::Wheel ability aside it. In a follow-up, we can deprecate and eventually remove the old Mouse::Button functionality. This was tested and confirmed working with a Xbox 360 controller and the reference |
ShadowApex
left a comment
ShadowApex
left a comment
There was a problem hiding this comment.
Looks great! Thanks for this!
| - mouse: | ||
| button: WheelUp | ||
| wheel: | ||
| name: Vertical |
There was a problem hiding this comment.
What value is the name attribute providing? I.e. I can't have a horizontal up or vertical left? Plane is essentially assumed.
There was a problem hiding this comment.
What do you mean? Are you saying there would be issues with trying to use both at the same time? The normal scroll up and down function and pushing left or right feel very different (at least on my mice). I think it would be clunky to try and and go horizontal up or vertical left.
There was a problem hiding this comment.
I think the name is redundant. Up and down will always be vertical, so it's not adding value. I agree that vertical left would be weird/clunky and doesn't make sense.
There was a problem hiding this comment.
Ah, I understand now. The target_events.mouse.wheel.name and target_events.mouse.wheel.direct attributes are somewhat redundant. So maybe I can consolidate into something like this.
Before:
- mouse:
wheel:
name: Vertical
direction: up
After:
- mouse:
wheel: Up
to align with similar existing target events. For example:
- mouse:
button: Right
There was a problem hiding this comment.
@pastaq The name is the name of the capability, and the direction is used for mapping a particular direction of the wheel if the user doesn't want to map the whole axis. Relying only on the direction kind of breaks convention with how we define mapping every other type of event.
E.g. axes are mapped with this convention:
- gamepad:
axis:
name: LeftStick
direction: leftThere was a problem hiding this comment.
@pastaq The
nameis the name of the capability, and the direction is used for mapping a particular direction of the wheel if the user doesn't want to map the whole axis. Relying only on the direction kind of breaks convention with how we define mapping every other type of event.E.g. axes are mapped with this convention:
- gamepad: axis: name: LeftStick direction: left
This isn't an analogous situation. Vertical and horizontal aren't Capabilities, they are directions. Mouse:Wheel is a Capability that can do up down left right, just like Gamepad:LeftStick. If your analogy was accurate it would be Gamepad:LeftStick: Vertical:Up, which would be equally silly. There's also not a conflict with the button click as that should be handled by Mouse:Button:Middle
There was a problem hiding this comment.
So you would rather see Mouse:Wheel changed into a Vector2 value? That will require some minor refactoring.
| )); | ||
| }; | ||
| match direction.as_str() { | ||
| "up" => Ok(InputValue::Float(1.0)), |
There was a problem hiding this comment.
Cases with the same return value can be combined.
"up" | "right => Ok(InputValue::Float(1.0)),
"down" | "left" => Ok(InputValue::Float(-1.0)),
There was a problem hiding this comment.
Nice, I didn't know about that Rust functionality. Thanks! I'll update the code.
|
@LukeShortCloud have you had a chance to look at my nits? |
3 participants
for both horizontal and vertical movement.