MC7000 add Fixed Loop + improvements + experimental beatcount #3738
Conversation
| // Todo: check for latency and use it to normalize the jog factor so jog wont be | ||
| // depending on audio latency anymore. | ||
|
|
There was a problem hiding this comment.
I haven't worked with the jog wheel scratching code in a while but I never noticed this was an issue. Is there are Zulip or forum thread where this issue is discussed?
There was a problem hiding this comment.
Yes - this is very obvious when you change from standard 23 to 5 ms the Jog is not usable - or it takes like 10 rounds to finally do something. TBH - I don't know if that has been discussed in chat already. I didn't
|
Sorry, didn't realize this was still a Draft PR before I reviewed it. |
Don't be sorry - at some point one needs to look into that. Thanks a lot for suggestions. Some are useful. Gonna change the variables definition to put |
to get the actual track position needs a lot of calculations, especially at low latency. They are easy enough but many. So I finally made the calculation only once and decide which LEDs are to be triggered for the Platter and PADs in Slicer Mode. I am using and tested this for a while already and it runs very well. |
toszlanyi
force-pushed
the
Denon_MC7000_mapping
branch
from
ec904de
to
5cf61eb
Compare
toszlanyi
changed the title
|
I think the modifications are ready for a final review. Thanks a lot for the useful discussions so far. If you see any other point that is critical then please let me know. Cheers! |
|
Thanks for your patience enduring my endless comments. Your code looks reasonable now. |
|
PR #369 contains the manual change accordingly |
| }; | ||
|
|
||
| // Previous Rate range toggle | ||
| MC7000.prevRateRange = function(midichan, control, value, status, group) { | ||
| if (value === 0) { | ||
| return; // don't respond to note off messages | ||
| } | ||
| var deckNumber = script.deckFromGroup(group); | ||
| var deckOffset = script.deckFromGroup(group) - 1; |
There was a problem hiding this comment.
script.deckFromGroup(group) - 1 is used very often. You might consider to extract a tiny helper function deckOffsetFromGroup() to not forget about the - 1.
There was a problem hiding this comment.
Thanks Uwe - it gets strange if 3 people comment to the code review. I followed more or less and changed all the variables regarding deck or offset number as suggested here and mainly I used deckNumber - 1 but if i didn't need deckNumber then only deckOffset is used.
At the end it is the same and 3 people had 3 different opinions which gets a little frustrating.
@uklotzde , @Swiftb0y , @ronso0 ... Can you please clarify internally and then let me know which way you wanna go and I possibly change all over again.
Thanks you.
There was a problem hiding this comment.
Using deckOffset is fine. I am just suggesting to move the duplicated code into a small helper function and then use that instead of the script.deckFromGroup() followed by the subtraction. The code will get more readable, no functional change.
There was a problem hiding this comment.
Just some final polishing. Actually I wanted to ask if this PR is ready for merge before I stumbled upon this simple and obvious improvement. One of the basic refactoring patterns.
There was a problem hiding this comment.
I agree, the change is not really conflicting with everything we discussed so far. Its just an addition to extract the script.deckFromGroup(group) - 1 part into its own function. this way its less likely you may forget the - 1part in the future.
There was a problem hiding this comment.
I'd say we should merge now, the change can go in a separate PR.
What is updated:
Documentation gonna be changed after we are clear to merge those changes for 2.3
Thanks a lot for comments.