Trigger Warning - richardjwild/arctracker GitHub Wiki
The code for dealing with audio output was in pretty good shape by now, and probably past the point of diminishing returns where the effort of further refactoring is not worth the benefit it yields. It was time to turn my attention to other parts of the codebase. But first, I removed the SIGINT handler code because it was making things worse, not better: sometimes interrupting the program would leave it still running, and you would be forced to send SIGKILL instead. I didn't understand the bug but it was clearly better before I put the code in, so I took it out again.
I decided to tackle the get_new_note function next because I could see some duplicated code in there, although it needed a bit of rearrangement to make the duplication apparent. First I inverted the sense of an if statement because nowadays I prefer conditions to test whether something is true rather than whether it is not true, I find this aids comprehension. Then I removed some more absurd pointer arithmetic and replaced it with an array lookup. This yielded the benefit that the code was dealing with an actual sample_details struct instead of a pointer to a sample_details struct. Next I did the same thing with a pointer to the array of sample periods.
There was some code that really confused me:
p_current_voice->note_currently_playing += (13 - sample.note);
For the life of me I couldn't figure out what that was supposed to do. I had to consult the Desktop Tracker manual which, thankfully, is still preserved on Jason Tribbeck's home page, to discover that sample.note there is a transpose value that can be applied to the samples, to be used as a correction if they weren't all sampled at the same pitch. (Tracker did not have this feature). I rearranged the logic so that I could rename the note field to transpose and simply add the value to note being triggered by the event. The transpose field also gave me to opportunity to simplify the code that calculated sample periods (I should really have split that change up into more than one commit).
Once again I inverted the sense of an if statement with an eye to exposing yet more code duplication, and I noticed the conversion of a Desktop Tracker gain to a Tracker one could be simplified. This refactoring made it possible for me to group together all the Desktop Tracker and Tracker-related code in one if statement rather than several. I also extracted a function to determine whether a sample repeats or not, which reduced the amount of code inside the Tracker/Desktop Tracker if block.
Now, the number of if blocks in set_new_note was a clear indication that this function had too many responsibilities, so I extracted a set_portamento_target function, because a tone portamento command was really a different thing to a note-on event, and it made little sense to handle it in the same function. The code that determines which of these two things was happening thus kicked out to the calling function. Having done so, I immediately noticed that the event parameter did not need to be a pointer and the periods pointer argument could be made const as well. I then did a very similar clean-up of the get_new_note function signature, and renamed the parameters because I hated all these p_ prefixes on the names.
I then renamed get_new_note set set_new_note although this change was not going to last long, because in the very next commit I renamed it again to trigger_new_note. I was groping my way towards what the real purpose of this function ought to be.
There remained one more bit of curious logic where it would silence the channel if the note-on event referred to an empty sample slot. It's likely this was some undocumented behaviour of one of the old SoundTrackers that someone repurposed as a note-off command in one of their songs. (Why they didn't use a C00 command to set the volume to zero like everybody else does, I have no idea). Now we all have to implement the same behaviour in order to stay compatible. (Obviously, the only reason I know about this is because I have one modfile in my possession that does this). Anyway, I kicked this logic up to the calling function as well.
I then cleaned up the trigger_new_note function. By the time I was done with it, it looked like this:
void trigger_new_note(
current_event event,
sample_details sample,
channel_info *voice,
unsigned int *periods,
module_type p_module_type)
{
voice->channel_playing = true;
voice->sample_pointer = sample.sample_data;
voice->phase_accumulator = 0.0;
voice->repeat_offset = sample.repeat_offset;
voice->repeat_length = sample.repeat_length;
voice->arpeggio_counter = 0;
voice->note_currently_playing = event.note + sample.transpose;
voice->period = periods[voice->note_currently_playing];
voice->target_period = voice->period;
voice->sample_repeats = sample_repeats(sample, p_module_type);
voice->sample_length = voice->sample_repeats
? sample.repeat_offset + sample.repeat_length
: sample.sample_length;
voice->gain = p_module_type == TRACKER
? sample.volume
: (sample.volume * 2) + 1;
}
While I was doing this I noticed that the repeat_offset field in the voice_info struct was not actually used for anything, so I deleted it. I also renamed sample_length to sample_end to disambiguate it from the similarly named field in the sample_info struct.
I felt the time was right now for a grand renaming of types which I had been itching to do for a while. Another thing I had been itching to do was to remove that stupid yn type so I did just that. Also, the return status was no longer ever set so I removed it from the play_module function.