Add support for Atelier Yumia EBM format#105
Add support for Atelier Yumia EBM format#105ekafjr2802 wants to merge 4 commits intoVitaSmith:masterfrom
Conversation
|
I appreciate the effort, but you are basically splitting gust_ebm into 2 completely independent paths depending on whether we have Yumia or not, instead of reusing the common code. So all the stuff we do, such as checking the header type, gets discarded when Yumia is detected. And since you are not reusing code that could be factorized, you are introducing new IDs ( This may look like minor issues, but in the long run, it makes it harder to maintain the application if each new format just does its whole separate thing instead of reusing what can be reused. I also don't get why you chose to replace the perfectly fine If we are not going to factorize the parts of code that can also apply to Yumia, then I'd rather introduce a separate gust_ebm that only handles Yumia, because it doesn't make much sense to me to apply the patch as it stands right now. That's not to say I may not use your patch as a base, but, in its present form, I just can't see myself applying it to gust_ebm, sorry. |
|
Thanks for the feedback! I totally get your point about the code structure and naming consistency. I've updated the code to address your suggestions: 1. Dropped the new ID names: It now uses the standard voice_id and name_id keys for both formats. The tool simply checks the version (JSON_VERSION_YUMIA) to decide whether to read them as strings or integers. 2. Restored legacy logic: I reverted the changes in the legacy path, so backward compatibility should be 100% safe now. I kept everything in gust_ebm.c for now with a cleaner separation, but if you'd still prefer a completely separate file for Yumia, just let me know and I can split it up! |
|
Still not exactly what I have in mind... Please look at the comment that describe the structure at the top of the source: /* An ebm structure is as follows:
uint32_t type; // always seems to be set to 2
uint32_t voice_id; // id of the voice for the speaking character
uint32_t unknown1; // ???
uint32_t name_id; // id of the name to use for the speaking character
uint32_t extra_id; // seems to be -1 for system messages
uint32_t expr_id; // serious = 0x09, surprise = 0x0a, happy = 0x0c, etc.
uint32_t duration1[2]; // [OPTIONAL] Probably duration values. Used by Nelke, Sophie 2, etc
uint32_t msg_id; // sequential id of the message
uint32_t unknown2; // ???
uint32_t msg_length; // length of msg_string
char msg_string[]; // text message to display
uint32_t duration2[] // [OPTIONAL] Probably duration values. Used by NOA2, Ryza 2, Sophie 2
*/The first thing you should do is document how the Yumia ebm differs from that (by adding whatever extra field Yumia uses). As you can see we already have Then, once we have that, what should happens is that either a first pass should be run (possibly on a small section of the file) to detect whether we have a Yumia EBM or not. This would set an is_yumia_format boolean. See for instance what we do after // Detect the length of the structure we will work withwhere we detect the optional Then you can simply reuse the existing code and just add the extra field, within that code, with a mere 2 lines: if (is_yumia_format)
ebm_header[++j] = json_object_get_uint32(json_message, "some_new_yumia_id");You would then save is_yumia_format as part of the JSON (rather than use As long as you have two completely separate paths of code to deal with the same Also, factorizing the existing code would have led you to use the same much cleaner means of setting up your EBM struct with: At this stage, you are not only adding a different path to handle a lot of data that could be factorized, but in this path, you are not even reusing the same logic as the one that was used before for the same elements, which I'm afraid is not something I can accept in a PR. |
|
Thank you for the detailed feedback and guidance. I understand your point about maintainability and code factorization. However, implementing the changes to fit the exact architectural requirements is a bit beyond my current capacity/time constraints right now. I will close this PR for now so it doesn't clutter the repository. Feel free to use any part of my logic/findings if you decide to implement Yumia support in the future. Thanks again for your time! |
|
Ah, I didn't realize that the Yumia EBM was that different from previous versions. It looked to me like there were only a few new I'll see what I can do to use your patch, as I don't really want your work to go to waste, but it might be some time before I do that. |
Hi, this PR adds support for the EBM format used in Atelier Yumia.
Changes made:
params1andparams2) and strings correctly.I have tested it by unpacking and repacking .ebm files from the game, and the output matches expected binary structures.