kichikuou / xsystem35-sdl2

Multi-platform implementation of AliceSoft's System3.x game engine.
https://kichikuou.github.io/web/
GNU General Public License v2.0
61 stars 8 forks source link

Fix infinite loop in MP command #3

Closed nunuhara closed 5 years ago

nunuhara commented 5 years ago

Fix an infinite loop that can when occur when printing messages that contain 1-byte characters (i.e. half-width latin characters).

When 1-byte characters are used, 'blen' is not necessarily even. This code calculates the number of padding characters needed using sjis_count_char rather than assuming all characters are 2 bytes.

This fixes the English translations for various System 3.x games. I tested Rance IV and Kichikuou Rance, and they both seem to be fully working after this change.

kichikuou commented 5 years ago

https://github.com/kichikuou/xsystem35-sdl2/commit/fa790cdcbd9bfea4395215496602efca7c0874b3 fixes this with another bug where strncpy() may truncate the string in the middle of a multibyte character.

Again, thanks for reporting the bugs!

nunuhara commented 5 years ago

Good catch.

This code doesn't quite work right though. It seems like the MP command actually pads/truncates to to a number of full-width character cells. So num2 should be treated as a width (measured in full-width character cells) rather than a number of characters. Otherwise, ASCII strings get cut in half: 3 (At this menu, each entry is truncated with MP <str>,8. The third line should read "Alice's Mansion".)

It's also worth noting that several other functions in cmdm.c assume their input is all 2-byte characters. Rance IV and KR don't seem to use these functions at all in my testing so I haven't been able to check if they break anything.

kichikuou commented 5 years ago

In fact, the translated version of system3.9 seems to behave differently from the original.

Scenario:

   ZH 2:  ; Disables halfwidth - fullwidth conversion
   MS 1, "12345":
   MP 1,3: '$'R
   MP 1,8: '$'R
   MS 2, "12345":
   MP 2,3: '$'R
   MP 2,8: '$'A

Original System3.9 Ver5.49:

sys39

System3.9 Ver5.49 bundled with the Rance IV translation:

sys39en

Looks like the translated version just appends num2 white spaces to the string, instead of truncating/padding. I guess the translator didn't bother to fix all the occurrences of the MP command but just patched the executable.

How should we fix this? Options:

  1. Treat num as a width as you suggested. Probably this will work well for both Japanese and English versions, but I'm not 100% sure.

  2. Add a command-line flag which controls the behavior of the MP command. Note that this solution doesn't scale: if two translated games had different sets of patches, single --english-version flag would not suffice.

  3. Switch the behavior by the game title. This technique is already used in the codebase; see commandMT() in cmdm.c. We need a list of game titles for which this patch should be applied.

nunuhara commented 5 years ago

I'm not completely sure, but by looking at the MP commands and eyeballing the size of the text boxes, it looks like the Kichikuou translation doesn't have the same patch as IV. So that rules out 2.

It may turn out that IV is a special case, in which case 3 is naturally the best option.

What tools are you using to generate these test programs? I can test all of the translations to see which ones are affected but I'm not sure how to go about it.

kichikuou commented 5 years ago

I'm using the System39 SDK included in AliceCD, which is not redistributable unfortunately.

Here is the test scenario file I used in the above. To try this, replace *SA.ALD file in the game directory with this and simply run the game.

nunuhara commented 5 years ago

I've tested this with all of the translated games that I know of, and here what I've found:

Games with patched MP command:

It seems likely that everything translated by 2.0 Translations is patched. That potentially puts all of the unconfirmed games into the patched category, but maybe not considering those games use different executables (system3.exe and system35.exe). I'm assuming the test ALD is not compatible with those executables as I was not able to run the test program with them.

I think (3) is the best option. (1) is a bit hacky and might introduce subtle bugs in the unpatched games. I can start working on a patch for this, unless you want to do it yourself.

By the way, how do you run Rance 4.1 and 4.2 in xsystem35? I've noticed they work on https://kichikuou.github.io/web/ but I can't figure out how to run them locally since there are no ALD files in the game directory.

kichikuou commented 5 years ago

Thanks. I agree that (3) is the best option. Please go ahead and create a patch.

Rance 4.1 and 4.2 are using the System3 system, which is incompatible with xsystem35. To run them locally, system3-sdl2 can be used. KichikuouWeb uses an emscripten-build version of this when a System3 game is detected.

nunuhara commented 5 years ago

Okay, this latest patch should solve the issue for Rance III/IV. For some reason the English translations use a different command to set the title, so now the game-specific hacks are split between commandMT() and command2F28().

kichikuou commented 5 years ago

Cool, thanks!