Added counter for current streak and max streak

[nbelakovski]

Hi Michael,

Been watching your youtube videos. They're super well done! I also really liked your polyrythm tool, and was impressed that it worked with my MIDI keyboard out-of-the-box. I wanted a streak counter so that I could use it to indicate when I had achieved proficiency at a certain tempo, and thereby move up the tempo. What do you think? Happy to make changes to it per your suggestions.

Thanks,
Nickolai

[michaelnew]

Hey thanks for the PR! At first glance it looks good, but I'll go through it in a day or two and make sure everything makes sense to me. I've thought about the same thing and definitely like the idea.

[nbelakovski]

Just made a couple changes.

Refactored the tempo methods to follow a get/set naming convention I've used in other projects in which there's just one function that does both. This makes it easier because you just have to remember the variable name, no need to remember get or set or if there's an underscore or whatever. The convention is that to get the value, you simply call tempo() and to set it, you call tempo(someNewTempoValue).

Also aligned all the text that was on the left so that the ':' is aligned. Not sure if it looks any better or worse than what was there before. What are your thoughts?

One more question for you: Do you think it's worth having both current and highest streak counter? Current seems like it might be a bit distracting, and it's implemented to go to 0 as soon as you hit a bad note, so the only information it gives comes from seeing it change in your peripheral vision. I dunno, maybe that's still helpful?

One sort-of issue with the use case as I proposed above is that it the counter doesn't reset on tempo changes. I figure users can get around that by reloading the page. The tempo box should probably support input to make that use case a little smoother, but I'll leave that for another PR.

[nbelakovski]

Went ahead and fixed the ability to enter stuff into the tempo box. Ideally I'd make a separate PR for it, but I think having a separate commit is fine.

[nbelakovski]

One more change! I made the highest streak box clickable, and reset to 0 on click. This should really help when changing up tempo or changing the polyrythm - no need to reload the page and reset everything.

[michaelnew]

Hey I finally got a chance to pull this code and play around with it. I definitely like it.

After using it a bit though, I don't think the streak counters should be over on the left along with the other controls. The tempo, mute, etc. are all actual controls that you interact with and change, but the streak counter is just an informational label (I do like the addition of being able to reset it, but even so, it's mostly just a passive element). Something like this might be less confusing:

I also like the idea of keeping the streak counter hidden until the user actually gets a streak of one or more beats. The whole interface is a bit confusing to someone first seeing it, not to mention the entire concept of playing polyrhythms is intimidating, so I'd like to keep it as simple as possible up front. What are your thoughts?

[nbelakovski]

I like the idea of moving them up top, that makes a lot more sense. I'll work on that sometime tonight.

Not sure about hiding it until a streak appears. Seems like it might throw someone off as soon as it shows up. But I get what you're saying about keeping the interface simple, I'm also a fan of keeping things uncluttered. How about this, I'll code up that functionality, and then you and I can play around with it and see how it feels. Maybe try different values for when it shows up. I'll try to get to that sometime tonight as well.

Thanks for the suggestions.

[michaelnew]

I was thinking the logic would be, essentially,visible = (current_streak > 0 || best_streak > 0). If you want to code it up so we can try it out then that would be great, but otherwise don’t bother with it. I’m not totally sure about it either way and it’s a very minor issue, so I don’t want to get hung up on it.

And again, thanks for the contribution. After using the feature for a while it feels like it absolutely needs to be there.

[nbelakovski]

I made the changes. Not super happy with the way I did the alignment/spacing of the streak/best boxes up top, but not sure there's a better way to do it. I saw that the initial beatChannels are not centered, and I figured it's because you wanted the initial channels to be close to center, but using keyboard keys that made sense? In any case, I couldn't have the streak/best centered because it looked weird, so I fudged them to be on the outside. You'll see once you load it.

For visibility I tried this out: The streak counter only becomes visible once you've hit a streak of at least 4, and then the best counter comes up once you've gone as far as you can on that streak. Not sure I really like it, I think it's better if they just show up on the initial load. Or maybe just the "best" label can be hidden on initial load, and come up the first time someone gets a streak of any kind. Play around with it, lemme know what you think. The code for controlling the visibility is in publishStreakData in beat_visualizer.js

[nbelakovski]

One more thing, there's a bug with the tempo now, which I'll haven't had time to fix yet but just wanted to make note of it. If I change the temp to, say, 22 with the keyboard, and then press the "uptempo" button, the tempo goes to 221. Not sure why that is, and it's not a big deal since you can just change with keyboard again, but once found it will probably be trivial to fix.

[ghost]

@nbelakovski Without looking at the code, it sounds like one or both of the two operands to the + operator is being coerced to a string, in which case JavaScript will concatenate '1' to 22 instead of adding 1.

[nbelakovski]

@errorx666 Yup, looks like that was it, thanks for the suggestion. I threw a parseInt into the setTempo method and all is well again :)

[nbelakovski]

I changed the visibility properties such that the current streak is visible on first load, and the best streak only shows up once you've managed to get some non-zero streak.

I think this is ready to go in, unless you have some other executive decisions you want to make @michaelnew ?

[michaelnew]

Sorry for the delay. Nice work though. I'm pretty happy with how this works.

I know you said you didn't like the way the labels are aligned, and I'm not a big fan of it either. I may play around with a few things as far as that goes before merging this into master, but this is at least in dev now.

Thanks again for coding this up and for being willing to tweak it.