Multi-touch MIDI keyboard developments

[ijijn]

As coded, the MIDI keyboard component assumes that each MouseInputSource index is distinct, but it is easy enough to have multiple sources all with e.g. index 0 yet different types, such as the default mouse input and the first finger of touch. This leads to conflicts and unusual behaviour whenever different modes of input are used together.

This fix keeps tabs on each input source separately so there are no accidental collisions.

[julianstorer]

I can't make sense of these diffs, can you do a clean rebase against the tip of develop so I can see what you're actually trying to change?

But TBH it sounds like this isn't really a problem with the keyboard component, but rather the native mouse behaviour, because those MouseSource indexes definitely should all be unique!

[ijijn]

Hmm, this should be a clean rebase: just a single commit from the latest develop tip, in both this case and the other one.

The diff tools I use (GitKraken's built-in diff and Beyond Compare) show much fewer, cleaner changes (especially when options to ignore trivial white space differences are turned on) compared to the crazy stuff GitHub is doing here. (To be clear, I did work quite hard trying to maintain the existing white space style.)

I'm fairly sure that those early returns and reduced indentation are causing most of this nonsense. Is there a convenient way to share what I'm seeing with you, or are you able to get this sort of view of the diffs fairly easily? I hope we can get over this hurdle!

As you probably suspect, most of the changes are trivial auto, const or early return tweaks. There's just a small amount of actual fundamental difference in the code.

Regarding your other point about indexes: great, that makes total sense to me. In my experience, the indexes tend to start from 0 for every separate type, so fixing this at a deeper level to give them all unique "index IDs" would really help. It would remove the need to compare both the index and type, which I'm currently doing.

The other basic change I made was to use an adaptive collection rather than a "fixed"-sized array of size 32 potential inputs with dummy values. This kind of mirrors what I did in the Slider class to keep track of current touches-in-progress. In addition to greater flexibility, often less work is required, just iterating over those input sources that currently exist. How do you feel about this particular detail?

[julianstorer]

I'd find it much easier to say how good these changes are if I could see them. It's not just white-space, it looks like you've reverted a bunch of other commits along with your changes. Maybe just do another PR based on the tip if you can't get this one into a decent state?

[julianstorer]

...oh, unless it was you that deliberately added all those autos and consts? That seems likely as they mostly break our style guide! Please don't attempt to refactor when you're suggesting changes - it just creates extra work for us to see what the important changes are..

[ijijn]

Yeah, that's pretty much what happened. This time I've kept the refactoring minimal and local to the changes.

I'm interested: which species of these changes break your style guide? I did my best to comply with every detail on your coding standards page. Was it the const parameters for regular types? That seems the most likely candidate, in which case it would be good to call that out specifically; in any case, I couldn't find any mention of it.

[julianstorer]

To be fair, the coding standards doc is way out-of-date and our new one hasn't made it to the website yet.. Yes, the auto and const stuff was just different in small ways to the way we'd do things, and it just creates a lot of noise in the diff. Even in this current version you've also done some unnecessary conversion of if-blocks into early return statements which also makes it hard to see what's going on.

But after digging through all this, would it actually end up being changes that wouldn't be needed if the sources had the right indexes?

[ijijn]

I see.

Argh, I thought I had caught all of those early returns. That should be the last of them gone now.

I would still like to encourage the adoption of these transformations even if there is upcoming work on index fixes at the source. Here are just some of the points that immediately spring to mind.

First of all, I'm wondering about how these indexes are calculated: are they to be allocated sequentially based on time started and independent of type, or is there a scheme based on type?

Sequential

If they are allocated sequentially, are these numbers to be recycled for expired sources while others are still active? If so, it is potentially useful to know the incoming order of these events, and this information would basically be lost. Or perhaps the available numbers would not be reset until all input events have gone, but then this could lead to the serious problem of running out of indexes under a "fixed asset" system.

Scheme

e.g.
mouse = 3x, touch = 3x + 1, pen = 3x + 2, or
mouse = x, touch = 10 + x, pen = 20 + x

I imagine a scheme wouldn't be especially future-proof as there would be many ways to mess this up, including adding input types or increasing the maximum number of inputs: in short, unnecessary coupling. Plus this would result in an even sparser representation than we have now, with many more checks to perform, increased memory usage and reduced cache coherency. I've observed a fairly dramatic improvement in performance during "hot slides" using my vector-based approach, even with many active touches.

In short, all of these problems are solved by keeping a simple register of active inputs, which is at the heart of my proposals.

Furthermore, the changes required to adopt the unique indexing system when it lands would be trivial and take just a few seconds: essentially, struct => int.