Revamp input handling

[Godnoken]

As shown in #36 (comment), I have found a way to block all input in most games by studying OSS Reshade and a bunch of forums.

Current issues;

• When should_block_messages is set to true for longer periods of time (?), game input stops being received altogether in some games. Seems to be of a greater issue in games that use Raw input. ALT + TAB may fix it.
• Some games still catch mouse hover actions - no clue why as of right now.
• The in-game cursor will still follow the IMGUI cursor in a lot of newer games. I'm not confident that this is something we can solve.
• We are currently not blocking games that use DirectInput, but this should be easily solved by hooking into it.
• All hooks, especially all new hooks that aren't SetWindowsHookEx need to be heavily tested & some may be completely unnecessary (most likely Post/Get/Peek-Message).
• Make my code idiomatic! 🥇

Any questions, help or thoughts on this is very much appreciated. I'm just a noob brute-forcing my way to success.

[Godnoken]

• When should_block_messages is set to true for longer periods of time (?), game input stops being received altogether in some games. Seems to be of a greater issue in games that use Raw input. ALT + TAB may fix it.

I have with great confidence tracked this down to being relevant only to games that use Raw input now. Should be able to fix this soon. Meanwhile you can just comment out WM_INPUT.

[veeenu]

Thank you for doing this work! I feel like this is going in the right direction, though I would address a few architectural shortcomings:

• having many instances of OnceCell and std::mem::transmute -- there are probably better patterns to achieve the same results, like putting everything into a single struct and working better with types; transmute should in general be the absolute last resort and heavily scrutinized and contained when used.
• passing pointers into functions: I'd rather have one single spot for each pointer where it gets validated and turned into a reference with .as_ref(), this improves dramatically code readability, does not require unsafe for functions, naturally guides you into better structuring and subdividing your code.
• I'd also like to reduce hooking to the absolute minimum -- we should investigate whether we can just find other HWNDs of the game and set our wnd proc there instead of the current solution of hooking into the existing wnd proc. Hooking into an existing function works, of course, but it's more undefined behavior we have to support that I'd avoid like the plague unless we're absolutely, positively, 100% sure there is no better way possible at all. The maintenance burden with the current hooks, which are absolutely necessary for the library to function, is already pretty bad due to voluntarily stepping into undefined behavior; and it is bound to grow exponentially with every single hook added. So I'd like to do the absolute best possible to avoid ever adding a new hook again. Honestly, I'd happily choose to go with fewer features rather than adding more hooks if those were the only two options possible.

Overall, the input handling code looks good and way more nuanced than what we have currently, so I'd gladly integrate this once it gets in a good state.

Thank you again!

[Godnoken]

So.

The input freeze seems to be a fairly documented issue, granted that it is the same one that we are suffering currently.

Some threads about the problem(s);
WH_MOUSE_LL
WH_KEYBOARD_LL
WH_MOUSE
WH_MOUSE_LL

The ones with _LL at the end are desktop global hooks, however, they seem to be suffering from the same issues nonetheless.

• Solutions seem to have ranged from extending the timeout on a registry key called LowLevelHooksTimeout;

MSDN

• Create a new thread, hook from there and then create a message loop to keep pumping out messages and keep thread alive;

{
    MSG msg;

    _hMouseLLHook = SetWindowsHookEx( WH_MOUSE_LL, .....);

    while(GetMessage(&msg, NULL, 0, 0) != FALSE)
    {
        TranslateMessage(&msg);
        DispatchMessage(&msg);
    }

    return 0;
}

More on that from MSDN (only specifies _LL here) and StackOverflow

• Rehooking every so often to not get timed out;

StackOverflow

• Seems irrelevant to Rust but here's someone talking about garbage collection;

StackOverflow

---

I tried all solutions above and there's likely more that I have forgotten, but it seems that using SetWindowsHookEx is less than ideal in some cases..

I also tried;

• Only hooking into the thread that uses raw input
• Use CallNextHookEx in various ways
• Hooking from DLL_PROCESS_ATTACH with and without a new thread

The issue has so far only popped up for me if we modify raw mouse movement input to WM_NULL i.e. blocking it for 7-10 seconds while moving the cursor fast continuously (moving slow seems fine...)
As far as I know, WH_GETMESSAGE, WH_MOUSE etc get called before the message is sent to GetMessage and PeekMessage. Hence I believe that when we modify the raw mouse movement to WM_NULL we are somehow overloading or "destroying" the game's message loop. However, that theory falls flat since hooking into GetMessage/PeekMessage essentially would be no different, apart from the issues listed above.

I'm getting really tired of troubleshooting this. If no one knows something that could help, I suggest that we drop raw mouse input blocking until a solution is found - OR I delve into hooking the functions specifically and hope that the problem lies with the SetWindowsHookEx hooks.

[Godnoken]

• having many instances of OnceCell -- there are probably better patterns to achieve the same results, like putting everything into a single struct

Fixed this to the best of my ability.

I assume Atomics inside of the Input struct isn't necessary at all, but I just stuck with them because the syntax is so nice. Not sure if it has any performance impact?

[Godnoken]

I tried hooking into Peek/Get/Post-Message properly without SetWindowsHookEx & WH_GETMESSAGE and there have so far been zero issues. Blocking input seems to work flawlessly and there is no longer any game input freezing happening.

SetWindowsHookEx just doesn't seem to quite cut it. It's 100% plausible that I've done something wrong when using it, but until we figure out what it is.. either we ignore raw input for hudhook or we go down the route of hooking to these functions individually. Either is fine by me, it won't add much work, if even any. If we go with the first option I'll just have a different local fork for the latter since it is absolutely essential for my app to work the way I want it to.

[veeenu]

Man, that's a treasure trove of information 😄

Okay so I read a bit more about SetWindowsHookEx and it seems it's doing global hooking? Plus it also looks like the timeout behavior is deliberate, and designed to protect from hooks running too long, which makes sense. I'm not liking SetWindowsHookEx much either, having considered this.

Btw, I've gone back and checked the code a bit. I'm still very worried about the hooks -- mostly because they run on very fundamental bits of the windows event loop which are going to run a bazillion times per second... Ngl, the thought of adding overhead to every single one of those calls (even if it's just a call + ret) makes me uncomfortable. The render loop has a cozy 16.6ms to play with at every turn, whereas those might be fired super fast and delay in there is most likely going to mess with the games in unexpected manners -- see the above reason wrt SetWindowsHookEx.

Due to its usage in the practice tools, one of the core needs that I had while designing hudhook was to mess with the game as little as possible in order to not influence the underlying execution in any manner that could be unpredictable. This is already super hard to juggle as is. Integrating this PR in its current state would mean forgoing that design principle altogether.

Don't get me wrong, I think this is super useful and I'd be glad to integrate it in some capacity, but I think it would be worth it to change its design so that it is opt-in -- i.e. an extra set of hooks that you can add at the beginning if you so wish, and that leaves everything else the same.

It would be even better if we found a totally different way that didn't require hooking, or that drastically limited hooking; but I can work with the opt-in thing until we understand the problem space better and find something better.

This might tie in with the idea to expose some internals of the library in order to build external plugins. I want to allocate some cycles to designing that, but lately it's been quite hard for me to do any progress at all due to real life commitments (and the need to unwind a bit).

(as always, thank you so much for all this work!)

I assume Atomics inside of the Input struct isn't necessary at all, but I just stuck with them because the syntax is so nice. Not sure if it has any performance impact?

IIRC loads and stores should compile down to a single operation anyway on x86/x64, and their only usage is instruction reordering. Should be completely fine 😅

[veeenu]

I tried hooking into Peek/Get/Post-Message properly without SetWindowsHookEx & WH_GETMESSAGE and there have so far been zero issues. Blocking input seems to work flawlessly and there is no longer any game input freezing happening.

Is there a chance that hooking only those could be enough to block the input?

[veeenu]

Went and checked Reshade out of curiosity... Found this: https://github.com/crosire/reshade/blob/5ad47a818e919e25db08af94f31abe3744f0bbcc/res/exports.def#L452

So what I did overlook about Reshade is that it's actually a proxy DLL: hooking in that case might be a bit different because it happens at startup time. The address that the host application sees in the first place is the hooked function, so there's no altering of the control flow instructions iiuc. Just find the original function and call it. That's a bit different than what we (are forced to) have. I would still be a bit scared of going that way, honestly, but I can see how that might do the trick (then again maybe the design goals of Reshade are different).

[Godnoken]

Okay so I read a bit more about SetWindowsHookEx and it seems it's doing global hooking? Plus it also looks like the timeout behavior is deliberate, and designed to protect from hooks running too long, which makes sense. I'm not liking SetWindowsHookEx much either, having considered this.

It only does global hooking if it is specified to do so. The current implementation only hooks into the specified threads of the process. If we'd use WH_MOUSE_LL it would indeed be global.

Btw, I've gone back and checked the code a bit. I'm still very worried about the hooks -- mostly because they run on very fundamental bits of the windows event loop which are going to run a bazillion times per second... Ngl, the thought of adding overhead to every single one of those calls (even if it's just a call + ret) makes me uncomfortable. The render loop has a cozy 16.6ms to play with at every turn, whereas those might be fired super fast and delay in there is most likely going to mess with the games in unexpected manners -- see the above reason wrt SetWindowsHookEx.

Yeah, I thought about that too. Not sure if it would be a problem though, granted that nothing gets stuck on the message handler. Although - I don't know if there's even much overhead added? Would this implementation really be that different from hooking the WNDPROC? Besides, Reshade is doing this and a lot more and it seems stable enough.

Due to its usage in the practice tools, one of the core needs that I had while designing hudhook was to mess with the game as little as possible in order to not influence the underlying execution in any manner that could be unpredictable. This is already super hard to juggle as is. Integrating this PR in its current state would mean forgoing that design principle altogether.

Completely understandable. I do not know how important blocking game input is for those use cases and I have little understanding of how much these hooks can affect a game. Definitely needs a lot of testing.

Don't get me wrong, I think this is super useful and I'd be glad to integrate it in some capacity, but I think it would be worth it to change its design so that it is opt-in -- i.e. an extra set of hooks that you can add at the beginning if you so wish, and that leaves everything else the same.

Absolutely, I agree - maybe we could even make it an opt-in build command? Not sure what's preferable.
As you can tell I do not have a great sense of how to architecture things well yet, so examples of how things could be done is always very welcome. I'm happy to learn.

It would be even better if we found a totally different way that didn't require hooking, or that drastically limited hooking; but I can work with the opt-in thing until we understand the problem space better and find something better.

Unfortunately, if I understand you correctly, I'm pretty sure this is impossible. I'm of course not all-knowing in the matter but nothing so far has told me that there is any other way other than hooking in some shape or form.

This might tie in with the idea to expose some internals of the library in order to build external plugins. I want to allocate some cycles to designing that

I like the sound of that. Reshade has been a huge source of inspiration for me so far in many different cases and the way he structured it for plugins is quite nice.

but lately it's been quite hard for me to do any progress at all due to real life commitments (and the need to unwind a bit).

Don't worry about it mate, I'm super content that you're still around to give feedback.

IIRC loads and stores should compile down to a single operation anyway on x86/x64, and their only usage is instruction reordering. Should be completely fine

That sounds alien to me, but I get the jist. Awesome.

(as always, thank you so much for all this work!)

I'm just glad I can give back! Cheers!

I tried hooking into Peek/Get/Post-Message properly without SetWindowsHookEx & WH_GETMESSAGE and there have so far been zero issues. Blocking input seems to work flawlessly and there is no longer any game input freezing happening.

Is there a chance that hooking only those could be enough to block the input?

Absolutely - for most games. If it uses DirectInput then we're all outta gum.

Hooking SetCursorPos and GetCursorPos would be preferable too in some cases if you want the mouse to stay still, but as always, many games use other ways to move the mouse..

So what I did overlook about Reshade is that it's actually a proxy DLL: hooking in that case might be a bit different because it happens at startup time. The address that the host application sees in the first place is the hooked function, so there's no altering of the control flow instructions iiuc. Just find the original function and call it. That's a bit different than what we (are forced to) have. I would still be a bit scared of going that way, honestly, but I can see how that might do the trick (then again maybe the design goals of Reshade are different).

Yep, perhaps I never said so. I don't know enough about what differences it makes, but I have noticed that some things get easier when you use a proxy DLL. Quite hard to hook into something like RegisterClass when you don't use it.

There's also an implementation for Reshade to be used for injection according to dll_main.cpp

And as you can see in the hook_manager.cpp there's export hooks, function hooks and vtable hooks implemented.

[Godnoken]

Hmmm. I was thinking about doing something like this for an opt-in.

On ImguiRenderer init;

  if render_loop.should_hook_windows_messages() {
    // Hook Peek/Get/post
  } else {
    // SetWindowLongPtr WNDPROC etc
  }

and then they would need their own msg_procs;

fn imgui_wnd_proc_impl(umsg, ..params) -> LRESULT {
  handle_input(umsg);

  return CallWindowProc(...)
}

fn windows_message_proc(lpmsg) -> bool {
  handle_input(lpmsg.message);

  return is_blocking_input()
}

I'm not sure if there's a better way to do it, but this seems ok to me. It'd be a nice way to opt-in for other hooks as well, such as SetCursorPos or DirectInput.

[Godnoken]

• having many instances of OnceCell and std::mem::transmute -- there are probably better patterns to achieve the same results, like putting everything into a single struct and working better with types; transmute should in general be the absolute last resort and heavily scrutinized and contained when used.

I tried alleviating this in the latest commit. However, I don't know if there is any safe way to do the same as transmute for function pointers at the moment?

[veeenu]

I tried alleviating this in the latest commit. However, I don't know if there is any safe way to do the same as transmute for function pointers at the moment?

This looks already significantly better, thank you!

There are things from this PR that I'd like to integrate in isolation, because I think they can stand on their own. In particular, the handle_window_message function.

What I'd like to do is start a fresh branch with only that, make a few changes to it (e.g. pass a &mut Io to it, retain the wnd proc mechanism via SetWindowLong, have the window procs lock the renderers and invoke handle_window_message over the Io stored in the imgui context) and merge that in once we're satisfied.

Then, we can rebase this PR on main, which will reduce the size of the diff significantly and could pave the way to new ideas and approaches.

Let me know what you think! Would you like me to help with that, or would you prefer taking a crack at it yourself?

[Godnoken]

What I'd like to do is start a fresh branch with only that, make a few changes to it (e.g. pass a &mut Io to it, retain the wnd proc mechanism via SetWindowLong, have the window procs lock the renderers and invoke handle_window_message over the Io stored in the imgui context) and merge that in once we're satisfied.

Hmm, but wouldn't this approach make it really clunky to integrate the other hooks? Or do you mean that we would make two different functions like I suggested above, but only integrate the wnd_proc one to begin with?

Let me know what you think! Would you like me to help with that, or would you prefer taking a crack at it yourself?

I can do it when I understand 😆

[veeenu]

Hmm, but wouldn't this approach make it really clunky to integrate the other hooks? Or do you mean that we would make two different functions like I suggested above, but only integrate the wnd_proc one to begin with?

I mean to integrate things gradually and have as much decoupling as possible so that both approaches can eventually coexist. I wouldn't want to use the multiple hooks approach myself, but I also think the work you have done on them is really valuable; figuring out a way of having them both would furthermore have the advantage of forcing us to rethink a cleaner and more modular API that plays well into the long term plan of extensibility.

This is not trivial, but I think it's the best course of action.

I'll get to that myself as soon as I can and send over a review request!

[Godnoken]

I mean to integrate things gradually and have as much decoupling as possible so that both approaches can eventually coexist. I wouldn't want to use the multiple hooks approach myself, but I also think the work you have done on them is really valuable; figuring out a way of having them both would furthermore have the advantage of forcing us to rethink a cleaner and more modular API that plays well into the long term plan of extensibility.

Understandable, however, I do not see the need to couple the current handle_window_message with wnd_proc. I believe it would be a lot easier and better for modularity to keep it separate and only pass umsg to it. Unless I'm missing something?

I'll get to that myself as soon as I can and send over a review request!

Cool!

[veeenu]

Understandable, however, I do not see the need to couple the current handle_window_message with wnd_proc. I believe it would be a lot easier and better for modularity to keep it separate and only pass umsg to it. Unless I'm missing something?

Oh, I didn't mean to do that. It's a good idea that the function stands on its own, I'm just going to keep using the current window proc approach (subclassing via SetWindowLong) with the new implementation. That function will be wrapped in a wnd proc so we can avoid straying too far from the current implementation all in one go, and test the new input functionality in isolation to make sure it doesn't break clients. Once we're confident of that, we can move on to figure out a solution for the multihooks thing, and simply invoke handle_window_message from the hook instead of from the wnd proc wrapper.

[Godnoken]

Yeah I definitely understand the caution, just not sure I understood how you wanted to implement it fully. I will just have to wait and see 😄

[veeenu]

I finally had the time to test this branch against Dark Souls III and Elden Ring. I used the branch as-is. Unfortunately, the inputs that I receive are duplicated in both games for some reason, and two cursor are actually shown. The underlying input is indeed blocked, though, which is a great result.

I wish there was a way of doing this that didn't involve all these hooks -- I have a poor understanding of them and I'm afraid rerouting core Windows events functionality might result in unintended side effects like these.

I just want to thank you again and reiterate that the research and effort you put into this is amazing. Unfortunately we can't merge it currently, but I feel like if we push more and seek to understand the situation better, the day when we will crack will come sooner rather than later!

out.webm

[Godnoken]

I finally had the time to test this branch against Dark Souls III and Elden Ring. I used the branch as-is. Unfortunately, the inputs that I receive are duplicated in both games for some reason, and two cursor are actually shown. The underlying input is indeed blocked, though, which is a great result.

Interesting. The duplicated input is something I never experienced myself, but neither would I probably notice because it wouldn't make any difference on my app.. 😄
There are plenty of mentions of it and there are fixes for it in the Reshade codebase, but I never implemented any of it due to not noticing the issues and the fact that it seems fairly complicated.

As for the cursor, you have to put SetCursor(HCURSOR(0)) here and possibly also hook the SetCursor function, it depends on the game.

If any game you try likes to lock the cursor somewhere you can also put ClipCursor(std::ptr::null()) in there.

I just want to thank you again and reiterate that the research and effort you put into this is amazing. Unfortunately we can't merge it currently, but I feel like if we push more and seek to understand the situation better, the day when we will crack will come sooner rather than later!

Thank you very much! Don't worry about it, all the work I put in is also massively benefitting me in many ways. No shame in saying no, quite the opposite - I'm glad to be able to discuss this and get viewpoints from someone with far more experience. You've been very patient with me so far and I appreciate that tremendously.

[Godnoken]

Oh, and by the way - I found a way to fix this.

It works in all games but one that I have tried so far. It is Star Wars: Empire at War and I currently don't have the faintest idea how the game is registering where the mouse is hovering. It could be DirectInput, an internal input manager or something else. Don't know.

[veeenu]

Closing this PR as the viable parts of it were gradually upstreamed over time and the underlying code changed so much that the conflicts are probably unsolvable. Hoping to work around this via a transparent overlay HWND that consumes the messages.