Separating the Formats I - richardjwild/arctracker GitHub Wiki
Previously I had been doing a lot of work in read_mod.c and during that I had decided the next thing I wanted to do was to move all the code related to reading a Tracker module into one compilation unit and all that related to reading a Desktop Tracker module into another. This job began simply enough, I created a new file tracker_module.c with associated header and copied all the code into it that dealt with Tracker modfiles. As ever, I then renamed the parameters because I heartily disliked this practice of prefixing all the parameter names with p_.
Next a created another file desktop_tracker_module.c and header file, and copied all the code related to Desktop Tracker modules into that. That turned out to be a single monolithic function, so I resolved to do something about that later.
While doing this I hatched the idea that it would be cool to use runtime polymorphism to encapsulate the module formats behind a common interface, just as I had previously done with the different audio APIs. So I created an interface:
typedef struct {
bool (*is_this_format)(mapped_file_t);
module_t (*read_module)(mapped_file_t);
} module_format;
The idea is quite simple. I intended to pass an array of all the supported formats to the read_mod function, and it would loop through them all and call is_this_format for each in turn on the mapped modfile until one of them returns true. The one that says "yes!" would then have its read_module function called to return the module_t struct. In this way, read_mod.c became completely agnostic of the modfile format: it no longer had any dependency on either tracker_module.h or desktop_tracker_module.h. Instead, the main function in arctracker.c assembled all the supported formats:
int main(int argc, char *argv[])
{
read_configuration(argc, argv);
module_format formats[] = {tracker_format(), desktop_tracker_format()};
module_t module = read_file(formats, 2);
audio_api_t audio_api = initialise_audio_api();
play_module(&module, audio_api);
audio_api.finish();
exit(EXIT_SUCCESS);
}
This meant that the read_file function now looked like this, which I thought was particularly clean:
module_t read_file(module_format *formats, int no_formats)
{
mapped_file_t file = load_file(configuration().mod_filename);
for (int i = 0; i < no_formats; i++)
{
module_format format = formats[i];
if (format.is_this_format(file))
{
module_t module = format.read_module(file);
printf("File is %s format.\n", module.format_name);
printf("Module name: %s\nAuthor: %s\n", module.name, module.author);
return module;
}
}
error("File type not recognised");
}
As was the case when I applied the same treatment to the audio APIs, I was surprised to discover that once the code compiled, it worked first time. Implementing runtime polymorphism in C is easier than I would have guessed.
Unrelated to this work, but while doing it I noticed that arctracker segfaulted when loading one particular modfile, so I found the problem and fixed it.
I had successfully removed all knowledge of the file formats from the read_mod.c module, but knowledge of the difference remained in play_mod.c. Mainly it was to do with handling effects. The Tracker effect commands were largely a subset of the Desktop Tracker ones, and the common ones were all handled the same, so there was no great difficulty here. The other main difference is that Desktop Tracker modules can have up to four effects per event while Tracker modules can have only one. The first thing I did was create a superset of effects commands that could cover both types of module format, and add functions in tracker_module.c and desktop_tracker_module.c for mapping between the format-specific effect codes to my new enumeration. Initially I thought that I wanted a polymorphic abstraction of these functions in the module_t struct, although I subsequently changed my mind about that.
Next I turned my attention to play_mod.c and the first thing I did was delete support for big-endian architectures. The time to add support for another architecture is when you have an example to run and test your code on: I do not and I had no idea whether this code worked, so it went. I then realised that the encoding of an event was different for a Tracker and a Desktop Tracker module, so what was needed was an an abstract function that would decode an event from either of the supported module types. That was the real abstraction, not the one for mapping effect codes, that was required on the module_t struct.
Another difference between the module formats was when printing out the pianola roll, but I figured there was no good reason to treat them differently so I simply deleted the if statement. Job done. I then renamed the 'command' and 'data' members of the channel_event_t struct to command0 and data0 to be consistent with the other members. After that, I refactored the bit mask and shifting in the read_tracker_module and read_desktop_tracker_module functions, and then wrote macros for the mask and shift operations to be more explicit about what is happening.
I was then ready to replace the command codes with the enumeration in the process_tracker_command and process_desktop_tracker_command functions of read_mod.c, which was the precursor to unifying both functions into a single one. Now there was no longer any knowledge of the different file formats outside of tracker_module.c and desktop_tracker_module.c, which was my main objective achieved. All that was left now was to clean things up.
The channel_event_t structure was quite ugly:
typedef struct {
__uint8_t note;
__uint8_t sample;
__uint8_t command0;
command_t command0_decoded;
__uint8_t data0;
__uint8_t command1;
command_t command1_decoded;
__uint8_t data1;
__uint8_t command2;
command_t command2_decoded;
__uint8_t data2;
__uint8_t command3;
command_t command3_decoded;
__uint8_t data3;
} channel_event_t;
so I created a separate structure to hold an effect and then modified the other struct to hold an array of effects, like this:
typedef struct {
__uint8_t code;
command_t command;
__uint8_t data;
} effect_t;
typedef struct {
__uint8_t note;
__uint8_t sample;
effect_t effects[4];
} channel_event_t;
This change also cleaned up some of the code in play_mod.c quite a bit too. I was now able to delete the module type enum because no part of the codebase needed to know any longer, and renamed the format_name member to simply format.
On a whim I went on a renaming tear and moved the macro definitions for the Tracker module chunk names to tracker_module.h. With that in mind, I went and moved the effect code macro defintions into the appropriate header files, and went on to clean up the main header file and finished by reformatting it.