Support for input remapping (mouse-buttons/modifiers-keys).

[JaapSuter]

Hello,

First of; great library. Thank you so much for writing it, and sharing it with the community. It's already incredibly useful for us.

Second; I don't expect this pull-request to be accepted in its current form. While my changes currently serve our needs, consider it a rough sketch at best. I hope it can start a discussion on whether ImPlot even needs support for input remapping, and if so, how to best do that.

Some background: we integrated ImPlot in an already complex application that has existing input-bindings for things like select, zoom, and pan. We use the same input patterns across a variety of different 2D and 3D panels, and our users have certain expectations as to what Left/Right/Middle mouse buttons do, as well as Ctrl/Alt/Shift modifier keys.

To be more specific, we use middle-click for panning in 2D panels, and left-click for boxes select and zoom. This happens to be different from the existing ImPlot defaults.

Rather than changing the (currently) hardcoded key-bindings, I figure it'd add a little configurable struct that specifies the bindings, in the hope the official version of ImPlot can add such a feature at some point. I imagine there might be other ImPlot users who'd like to configure the input mechanism.

The diff should be pretty self explanatory, but it essentially boils down to this one struct:

Like I said, I'm not attached to my solution. Change the names of structs and functions, or come up with a different system all together. I'm open to suggestions, and more than happy to do the work and submit a new pull request.

For now, a couple of thoughts/questions to start the conversation...

Currently, ImPlot has no global state that applies across different BeginPlot/EndPlot pairs. That's great, as each plot is independent. However, because (imho) input-mapping is preferably consistent across an entire application, it seemed most useful to me to have a single global ImPlot::SetInputMap that you simply call once (probably right after initializing ImGui), and never touch again.

The alternative would be to have make it an extra argument to BeginPlot, or have a SetNextPlotInputMap function. The current input mapping would then become a part of the gp state variable, instead of being saved in a global gi as I currently do. If we did this though, you'd have to pass the same input map in every location where you do plotting in a program.

To support the odd scenario where one wants to change the input map just locally in a specific location, I made it so SetInputMap returns the previous input mapping, so you can save it and then restore it when you're done.

Unrelated, I should point out that we haven't yet had a need for the query-drag system in our software. We only use box-select and panning. While I tried to get it right in adding support for input remapping, I did only a bare minimum of testing to make sure query-drags still work, and still interact properly with switching between select/query, and cancelling of queries. I suspect I may have gotten a few corner cases wrong, input-code is always tricky like that.

Once again, thank you for this great library. And to be clear, I'm open to the argument that a custom input feature is not worth the added complexity. In which case, just let me know and close this pull request. Thank you!

[epezent]

Hi @JaapSuter, thanks for the feedback! Input remapping has crossed my mind, so thanks for taking a first pass. This approach looks pretty good to me. Here are a few comments:

However, because (imho) input-mapping is preferably consistent across an entire application, it seemed most useful to me to have a single global ImPlot::SetInputMap that you simply call once (probably right after initializing ImGui), and never touch again.

If it's not likely to change at runtime, what about using defines in imconfig.h? I have no preference, but it's another idea to consider.

The alternative would be to have make it an extra argument to BeginPlot, or have a SetNextPlotInputMap function. The current input mapping would then become a part of the gp state variable, instead of being saved in a global gi as I currently do. If we did this though, you'd have to pass the same input map in every location where you do plotting in a program.

No, can't say I'm a big fan of this. I like what you have better!

To support the odd scenario where one wants to change the input map just locally in a specific location, I made it so SetInputMap returns the previous input mapping, so you can save it and then restore it when you're done.

Ah, in this case defines wouldn't be so helpful and your approach would be better. I can't quite imagine a scenario where the input mapping needs to change in the same application, but I guess it's possible. I suppose it's also possible that users would want to expose input mapping options to their application users.

Unrelated, I should point out that we haven't yet had a need for the query-drag system in our software. We only use box-select and panning. While I tried to get it right in adding support for input remapping, I did only a bare minimum of testing to make sure query-drags still work, and still interact properly with switching between select/query, and cancelling of queries. I suspect I may have gotten a few corner cases wrong, input-code is always tricky like that.

Yea, the query system seemed like a useful thing at first, but in hindsight it hasn't really done much for me either, and you're not the first to make this comment. I will double check that everything continues to work before merging, though.

Overall, I would suggest a slight reworking:

Model this after ImPlotStyle and GetStyle(). In that case, you could have a persistent ImPlotInputMap instance in ImPlotContext. GetInputMap() would then return a reference to that instance, where the user can change the member variables or entire struct as desired. You can eliminate the SetInutMap method.

[JaapSuter]

Wonderful. Thank you for the quick and informative response Evan. Really appreciate it.

I totally agree with your suggested rework of modeling ImPlotInputMap to model ImPlotStyle and mirro the GetStyle function. I simply wasn't aware of that function yet, but it makes total sense.

Unfortunately I won't be able to address this until this coming Sunday, but I'll get on it then right away and submit you an updated pull request for your consideration.

Thank you!

[epezent]

Sounds good!

[JaapSuter]

Hi Evan.

I've updated my pull request. ImPlot::SetInputMap is gone, and instead there's now a ImPlot::GetInputMap function that returns a mutable reference, per your suggestion.

I've removed the private gi reference to the input map, and the input expressions now use gp.InputMap everywhere. This makes the input expressions a bit longer to read, but it's a matter of taste, and I defer to your opinion on how you'd prefer to handle that. I was tempted to introduce a few static utility functions to combine some of the long-boolean-combined "if-this-mouse-button-and-this-modifier-or-not-this-mouse-button" code. But, it's up to you.

Lastly, both ImGui and ImPlot appear to avoid in-class initializers (defined in the header file, where a structure/class is defined as well). I think this might be to support older C++ compilers. That's why in the original pull request I had a private GetDefaultInputMap function, to initialize the defaults properly. One thing that bugged me was that it meant I had to keep the comments describing the default mapping (in the header file) and the actual defaults (in the .cpp file) in sync.

Personally, I feel C++ compiler support for in-class initializers has reached a point where just assigning the defaults in the header is the way to go. That also makes it self documenting, and removes the need for a GetDefaultInputMapping or special initialization step in the .cpp file.

However, I'm aware it breaks with ImGui convention of having no initializers in the header file. So let me know if you want me to undo this.

Anyway, once again, I don't expect you to just drop in this pull request verbatim. Let me know how you want to proceed, or if I can change anything else.

Thank you!

[JaapSuter]

Oh, I should also mention. My git-fu and familiarity with rebase versus merge isn't that great, so I apologize this ended up being 3 commits instead of just a single clean commit on top of the latest master in your original repo. Let me know if you want me to submit a new pull request from a new branch so it has just a single squashed commit.

[epezent]

Jaap, looks good! Although I agree with your assessment, I do think we should conform to ImGui's standard by moving initialization to a constructor, with comments indicating the default values in the header. If you can make this change, I'll be happy to test and merge this. Thanks for your contribution!

Don't worry about git, I'm pretty terrible myself.

[JaapSuter]

Done.

Mirroring the ImGui style, instead of using an initializer list in the constructor, I set the default values in the body of the constructor.

[epezent]

I tidied this up a bit, and fixed a few issues:

1. I couldn't quite follow what you had done with the Query controls. I've reverted them back to as they were. Based on your description of what you need, I don't think this will change anything for you. Please confirm.
2. The modifiers were not working because you were testing them with == and not bitwise AND (I replaced these with our HasFlag function). Also, do we (you) actually need modifier keys for these buttons?.
3. Mappings for fit and context menu added.
4. Slight renaming and additional documentation to header.

Can you test my commit and ensure that it will enable the style of control you wish to achieve?

Also, I'm wondering if we need to support more than just the keys in ImGuiKeyModFlags. For example, someone might want to use QWERTY keys for particular modifiers. I also recognize this could become really complicated if we let it, so perhaps this is good enough especially considering this wasn't even an option before. Thoughts?

[JaapSuter]

Thank you so much for making these changes. I saw you put in quite a bit of effort still, which I really appreciate it. I hope my pull request was able to at least save you a few minutes of work. Well, it got the conversation started, and that's great!

Comments:

1. We don't use the query system, so I defer to your expertise on how to do 'm properly.

2. Actually, testing them with == instead of a bitwise HasFlag was intentional. My reasoning was that a user should hold precise modifier keys that the input mapping desires. Meaning, if the input mapping is set such that shift needs to be held down, then if the user suddenly adds control or alt, it's no longer the desired input mapping, and the condition no longer holds. That said, I realize that for some scenarios that might be a bit shortsighted. There are many input scenarios where you want the more forgiving HasFlag, and you do want conservatively test if a modifier key is held, irrespective of others also being held down. Again, I defer to your judgement. It honestly doesn't really affect what we need from input mapping support.

3. Thank you.

4. Looks great. Again, thanks for the work you did on this.

I just tested it with your latest commits, and it totally helps us accomplish our goals. As far as we're concerned this can go into your master.

With regards to your final question about QWERTY keys. For now, I'd suggest to go with a KISS approach, and consider what you have as "good enough". But that's probably just because I don't have any input mapping needs beyond what was in my original pull request... :-)

Thank you!

[epezent]

Actually, testing them with == instead of a bitwise HasFlag was intentional. My reasoning was that a user should hold precise modifier keys that the input mapping desires. Meaning, if the input mapping is set such that shift needs to be held down, then if the user suddenly adds control or alt, it's no longer the desired input mapping, and the condition no longer holds. That said, I realize that for some scenarios that might be a bit shortsighted. There are many input scenarios where you want the more forgiving HasFlag, and you do want conservatively test if a modifier key is held, irrespective of others also being held down. Again, I defer to your judgement. It honestly doesn't really affect what we need from input mapping support.

I see. That actually makes sense. Where I noticed the issue was when holding Ctrl to toggle between Selection/Query, and then also using the Horizontal/Vertical modifiers. In that case, the modifiers did need to be forgiving. The others could follow your approach, but I think I'll just leave them as is for the time being.

Thanks again for the work! Merging...

[epezent]

I meant to suggest that you create a new Issue and share your input mapping for others, if you don't mind :)

[epezent]

#170 has removed ImPlotInputMap from the public API. The struct and function for retrieving it can now be found in implot_internal.h unmodified. Upon reinspecting the way input is currently handled, I realized there are far to many edge cases that would manifest under certain configurations of ImPlotInputMap. Eventually we may try to tighten up input handling and reintroduce this to the public API. Until then, consider it an experimental feature.