implementation of more file load/save routines for the loaders #9
Conversation
vampirefrog
left a comment
vampirefrog
left a comment
There was a problem hiding this comment.
I'll assume that you based your changes on your own research and that the code mostly works. We'll know with more certainty when we add some kind of automatic testing.
dmp_file.c
Outdated
| int dmp_file_save(struct dmp_file *f, int (*write_fn)(void *, size_t, void *), void *data_ptr) { | ||
| uint8_t buf[50] = { 0 }; | ||
| int bc = 0; | ||
| buf[bc++] = 0x0a; // version 10 |
There was a problem hiding this comment.
please use an incrementing pointer
dmp_file.c
Outdated
| #include "tools.h" | ||
|
|
||
| int dmp_file_load(struct dmp_file *f, uint8_t *data, size_t data_len, int system) { | ||
| #define SYSTEM_YM2612_OPN 0x02 |
There was a problem hiding this comment.
I am not sure if these constants belong in this file or even in this position (I like to place them at the top of the file). But since they are used in a field that's defined in the struct, and the struct is defined in the header file, it's better if we move these inside the header file.
There was a problem hiding this comment.
The DMP format changed with each iteration, making it hard to keep track to write proper importer/exporter libraries as you have to consider each and every version if you want to make it properly. The mentioned constants are for version 11 or above that now also support OPM patches in addition to OPN, so a new header field indicating the target chip/system was added with the two possible values 2 and 8 relevant for us. Before that, the system was always OPN.
I agree with the placement of the defines though, I moved them to the header now.
dmp_file.c
Outdated
| f.lfo = opn_voice_get_ams(voice); | ||
| f.lfo2 = opn_voice_get_pms(voice); | ||
| for(int i = 0; i < 4; i++) { | ||
| struct opn_voice_operator *op = &voice->operators[i]; |
There was a problem hiding this comment.
I don't think this variable is used.
dmp_file.c
Outdated
| fop->sl = opn_voice_get_operator_sl(voice, i); | ||
| fop->rr = opn_voice_get_operator_rr(voice, i); | ||
| fop->ssg = opn_voice_get_operator_ssg_eg(voice, i); | ||
|
|
| }; | ||
|
|
||
| void dmp_file_init(struct dmp_file *f); | ||
| int dmp_file_load(struct dmp_file *f, uint8_t *data, size_t data_len, int system); |
There was a problem hiding this comment.
Could you explain why you removed this field?
There was a problem hiding this comment.
As written above, the older versions of the DMP file format supported only YM2612 systems anyway, and for newer versions, the system information is part of the file itself, so it is not needed as a parameter.
op3_file.c
Outdated
| uint8_t buf[8192] = { 0 }; | ||
| strcpy((char*)&buf[0], "Junglevision Patch File\x1A"); | ||
| int bc = 32; | ||
| buf[bc++] = f->count_melodic & 0xff; |
There was a problem hiding this comment.
please use incrementing pointer
tfi_file.c
Outdated
| */ | ||
|
|
||
| // get signed DT from unsigned DT | ||
| static int DT_u2s(int dt_unsigned) { |
There was a problem hiding this comment.
please use lowercase function names
sbi_file.c
Outdated
| int sbi_file_save(struct sbi_file *f, int (*write_fn)(void *, size_t, void *), void *data_ptr) { | ||
| uint8_t buf[52] = { 0 }; | ||
| int bc = 0; | ||
| buf[bc++] = 'S'; |
There was a problem hiding this comment.
can u use pointer increment here too, please?
2 participants
Not all of them fully tested yet, but better than returning -1 I think.