Add support for relative and absolute input remapping #57

innovate-invent · 2024-05-31T05:05:26Z

This is my first foray into Rust so please forgive any egregious convention violations. I am happy to fix any.

I have added the ability to remap relative and absolute inputs. This is primarily useful to remap the mouse wheel of my mouse.

This PR should resolve #14

wez

Thanks for this!
I have quite a few comments about the implementation; I'm finding it difficult to follow the changes and I think that being a bit more deliberate about scoping the changes will help with that!

wez · 2024-06-08T20:36:07Z

src/mapping.rs

+
+impl PartialEq for KeyCodeWrapper {
+    fn eq(&self, other: &Self) -> bool {
+        self.code == other.code && self.scale.is_negative() == other.scale.is_negative()


Please add comments both here and on the scale field above about why only the magnitude is considered for equality and hashing, as it is not obvious from the code!

I would prefer to use the compiler-provided derive for PartialEq, Eq, Hash because it can be tricky to keep things straight, and I'm not sure why this is being special cased.

wez · 2024-06-08T20:39:13Z

src/mapping.rs

+        match s.rmatch_indices(&['+', '-']).next() {
+            None => {
+                name = &s;
+                scale = 0;


It seems surprising to me that scale could validly be 0; I would have assumed that it would always be >= 1.

wez · 2024-06-08T20:42:08Z

src/mapping.rs

+                let _scale;
+                (name, _scale) = s.split_at(m.0);
+                if _scale.len() > 1 {
+                    scale = _scale.parse::<i32>().unwrap();
+                } else if _scale == "-" {
+                    scale = -1;
+                }


Stylistically speaking, this extra _scale is "bad" for two reasons:

There is a rust convention for marking a variable binding as unused by prefixing it with _, but this variable is not unused.

It feels a bit awkward to semi-shadow the scale value in the outer scope.

Suggested change

let _scale;

(name, _scale) = s.split_at(m.0);

if _scale.len() > 1 {

scale = _scale.parse::<i32>().unwrap();

} else if _scale == "-" {

scale = -1;

}

let (name, scale_str) = s.split_at(m.0);

if scale_str.len() > 1 {

scale = scale_str.parse::<i32>().unwrap();

} else if scale_str == "-" {

scale = -1;

}

wez · 2024-06-08T20:46:54Z

src/mapping.rs

+                let _scale;
+                (name, _scale) = s.split_at(m.0);
+                if _scale.len() > 1 {
+                    scale = _scale.parse::<i32>().unwrap();


In general, bare unwrap() is to be avoided. Alternatives:

If the condition is guaranteed impossible, or is guaranteed to require immediate program termination, then substitute it with .expect("reason why it can never happen") so that there is an explanation of the assumption that can help to debug it when it actually does happen. This is not appropriate in this context, because we're parsing user-provided input.

In a function, like this, that returns Result, then it should instead be changed to propagate the error value in a meaningful way. Often you can use the ? operator for this, but with anyhow we can also provide some more useful context:

Suggested change

scale = _scale.parse::<i32>().unwrap();

scale = _scale.parse::<i32>().context("failed to parse expected scale factor as an integer")?;

wez · 2024-06-08T20:49:26Z

src/mapping.rs

+        if prefix == "KEY" && scale == 0 {
+            scale = 1;
+        }
+        match EventType::from_str(&*("EV_".to_string() + prefix)) {


This is a bit easier to grok:

Suggested change

match EventType::from_str(&*("EV_".to_string() + prefix)) {

match EventType::from_str(&format("EV_{prefix}")) {

wez · 2024-06-08T20:56:07Z

src/mapping.rs

+                }
+            },
+        };
+        let mut prefix = name.split_once("_").unwrap().0;


please refactor to avoid using unwrap.

wez · 2024-06-08T20:56:50Z

src/mapping.rs

+            },
+        };
+        let mut prefix = name.split_once("_").unwrap().0;
+        if prefix == "BTN" {


please add comments about what is happening here and why

wez · 2024-06-08T21:06:36Z

src/remapper.rs

+    let mut a_ismod = false;
+    let mut b_ismod = false;
+    if let EventCode::EV_KEY(k) = a {
+       a_ismod = is_modifier(&k);
+    }
+    if let EventCode::EV_KEY(k) = b {
+       b_ismod = is_modifier(&k);
+    }
+    if a_ismod && b_ismod {
+        if b_ismod {


this feels harder to read than it was before. Please revert is_modifier back to its former state (accepting &KeyCode) to resolve this.

wez · 2024-06-08T21:09:46Z

README.md

+Mapping relative inputs to relative inputs is also possible, you can include 
+a scaling factor to both the input value and output. The input value is divided 
+by the input scaling factor, and the output is multiplied by the output scaling 
+factor. Specify a negative scale factor on the output to invert the input. If you 
+provide an input scale, you need to remap both relative directions separately:


This is quite dense and is hard to immediately grok and relate to the examples below.
Please intersperse this explanation with the examples and explain precisely what happens for each of the examples, as it isn't immediately clear what the +4 on input does in relation to the +2 on output.

FWIW, I think the scale factor looks like addition/subtraction, which makes it a bit confusing, as does the context dependent multiply-or-divide.

How about using something like +WHEEL/4 to indicate positive relative wheel, divided by 4, or -WHEEL*2 to indicate negative relative wheel multiplied by 2?

I think switching the scale from i32 to an f32 is conceptually good for this, as the parser can more directly track the intent that was written out in the config file. I think the wrinkle would be that f32 doesn't support Eq (only PartialEq). That could be resolved by using NotNan.

With that approach, the default scale would be 1.0 and the other scaling logic is simply the product of both input and output scale factors.

wez · 2024-06-08T21:20:22Z

README.md

+[[remap]]
+input = ["REL_HWHEEL_HI_RES+4"]
+output = ["REL_Y+2"]


I'm having a hard time grokking what this PR is doing overall, by which I mean: I get the gist of what the higher level goal is, but when it comes to the implementation, the details are not at all clear to me.

I think it is due in part to overloading what was a simple keycode type with something that is conceptually richer, but which in the current implementation feels a little bit like it was "finagled in".

What I think might help is to reduce the scope of the changes around the KeyCodeWrapper and to make it explicitly handle the situations that are necessary.

For example, rather than a struct it could be an enum, which would allow tracking the scale only for the relative case:

#[derive(PartialEq, Eq, Hash, Clone, Debug)] enum EventWrapper { Key(KeyCode), Relative { code: KeyCode, scale: NotNan<f32>, }, Absolute(KeyCode), }

This sort of change should make it a bit clearer in the consuming code what's going on.

innovate-invent · 2024-06-08T21:52:36Z

You are right, I had to do some finagling to get my use case to fit this code base.
I was trying to get my horizontal scroll wheel working in Helldivers last weekend.
I actually gave up on this and went with https://github.com/sezanzeb/input-remapper as it better fit what I wanted.

wez · 2024-06-08T22:06:22Z

Ah, I can understand moving on to something else, but just FYI, I spent a significant amount of time reading through this PR and working up that review, and also for your other PRs. I feel like I just wasted a good chunk of my productive time this weekend.

Add support for relative and absolute input remapping

70ea211

innovate-invent force-pushed the master branch from 3606176 to 70ea211 Compare May 31, 2024 05:12

wez requested changes Jun 8, 2024

View reviewed changes

innovate-invent closed this Jun 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for relative and absolute input remapping #57

Add support for relative and absolute input remapping #57

innovate-invent commented May 31, 2024

wez left a comment

wez Jun 8, 2024

wez Jun 8, 2024

wez Jun 8, 2024

wez Jun 8, 2024

wez Jun 8, 2024

wez Jun 8, 2024

wez Jun 8, 2024

wez Jun 8, 2024

wez Jun 8, 2024

wez Jun 8, 2024

wez Jun 8, 2024

innovate-invent commented Jun 8, 2024 •

edited

Loading

wez commented Jun 8, 2024

	scale = _scale.parse::<i32>().unwrap();
	scale = _scale.parse::<i32>().context("failed to parse expected scale factor as an integer")?;

	match EventType::from_str(&*("EV_".to_string() + prefix)) {
	match EventType::from_str(&format("EV_{prefix}")) {

Add support for relative and absolute input remapping #57

Add support for relative and absolute input remapping #57

Conversation

innovate-invent commented May 31, 2024

wez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

innovate-invent commented Jun 8, 2024 • edited Loading

wez commented Jun 8, 2024

innovate-invent commented Jun 8, 2024 •

edited

Loading