Fix delay command

[n1LS]

Fixes #592 by making sure that commands are not triggered until the delay is up and makes sure they are triggered once the delay expires regardless of the groove.

[n1LS]

Not sure why the other commits are stuck in there. Will clean that up if necessary.

[democloid]

@n1LS Can you explain a bit the root cause of the problem, how is it failing and how we are trying to fix it? It's not immediately obvious to me what's going on and I'm a bit weary of touching this codepath and worry of breaking other stuff.

[n1LS]

Sure thing, I should have done that right away for this kinda fix...

Root issue: The delay sets timeToStart for the relevant channels and then calls Start() a few ticks later. ProcessCommands on the other hand is still called each tick, initializing the command instantly. The command then gets reset when timeToStart hits 0 and Start() is called.

1. The added check in Player.cpp:635 in ProcessCommands()

if (timeToStart_[i]) {
  continue;
}

makes sure nothing happens on delayed channels before the note is started.

2. When the delay expires and Start() is called, the command needs to instantly trigger. The groove-check

if (gs->TriggerChannel(i)) { // If groove says it is time to play

prevents that from happening (because we are already mid note and the next time it would pass we're already on the next step), so it needs to be skipped if the delay just ended this cycle. That's why in Player::Update() the part decrementing the delays stores expirations in bool delayExpired[SONG_CHANNEL_COUNT]and passes that to ProcessCommands(). The check is then expanded to

if (gs->TriggerChannel(i) || (delayExpired && delayExpired[i])) {
   // process the commands here

Now, the command gets initialized in the same tick as the note actually starts.

Since ProcessCommands() is also called from Player::Start, the changes are implemented as to not affect that route: the extra groove-check-condition resolves to false since no delayExpired array gets passed into ProcessCommands().

[democloid]

Other than the comment I made, which I think should be fixed, but may be understanding this wrong.

Now one thing that hasn't been explicitly discussed and I want to clarify just in case so that we are all on the same page:
What this does (and I agree it's the intended behaviour) is that it applies a delay of a certain amount of ticks after moment the ROW should trigger. when the row triggers is dictated by the groove.

For example if the groove is:

01 03
02 09

Step 2 is starting 3 ticks before than the usual 06 06 groove. If in turn in step 2 we have a DLY command of 03, that would delay the row by 3 ticks, and step 2 would essentially trigger on tick 6, cancelling the groove (for that step).

BTW, the manual says that DLY delays the NOTE, we should modify as part of this to say "entire row".

Another question: are tables being calculated correctly as part of this or also needs reviewing?

[n1LS]

Another question: are tables being calculated correctly as part of this or also needs reviewing?

@democloid:

Old answer: I have a whole bunch of Traces in there atm to follow the entire playback flow anyways, so I'll do some testing tomorrow.

Edit: Each table column runs in its own time so naturally a delay command in a table doesn't do anything as there is nothing else it can trigger after the delay.
Tables do get started correctly so that a KIL in row 0 kills the voice before it plays.

[n1LS]

Step 2 is starting 3 ticks before than the usual 06 06 groove. If in turn in step 2 we have a DLY command of 03, that would delay the row by 3 ticks, and step 2 would essentially trigger on tick 6, cancelling the groove (for that step).

If by "cancelling" you mean cancelling out the effect of the groove and playing at regular 6:6 time, then yes.

If not:
The way the groove is progressing is not affected by the delay. Each row operates entirely within its groove time window. A delay simply starts a note N ticks after the groove dictates. If N is it greater than the current groove step length, the row will be entirely skipped (except for the delay command, of course).