Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SCI: Add support for Korean fan translation #2604

Merged
merged 9 commits into from Nov 8, 2020

Conversation

@wonst719
Copy link
Contributor

@wonst719 wonst719 commented Nov 6, 2020

This PR is part of scummvm-kor merge project.

The patch adds ability to load external Korean text resource file similar to Japanese version, and to display korean text with 2x multiplied resolution (also similar to Japanese version).
Unfortunately this is not enough for supporting all fan-translated games, as some translated games require more hack.
Once this PR is merged, I intend to work on more game support.

Tested with:

  • Castle of Dr. Brain (Korean patched)

scummvm-castlebrain-kr-00000

@sev-

This comment has been minimized.

Copy link

@sev- sev- commented on engines/sci/graphics/frameout.cpp in f39567e Nov 3, 2020

Why is this commented out?

This comment has been minimized.

Copy link
Owner Author

@wonst719 wonst719 replied Nov 3, 2020

I think this is some leftover code. It's safe to delete them.

@sev-

This comment has been minimized.

Copy link

@sev- sev- commented on engines/sci/graphics/text32.cpp in f39567e Nov 3, 2020

Why did you drop const?

@sev-

This comment has been minimized.

Copy link

@sev- sev- commented on engines/sci/graphics/text32.cpp in f39567e Nov 3, 2020

Same here, I think, you could keep const

This comment has been minimized.

Copy link
Owner Author

@wonst719 wonst719 replied Nov 3, 2020

I think I should keep const, too.

@sev-

This comment has been minimized.

Copy link

@sev- sev- commented on graphics/korfont.cpp in f39567e Nov 3, 2020

Wrong code formatting in these functions.

This comment has been minimized.

Copy link
Owner Author

@wonst719 wonst719 replied Nov 3, 2020

Besides code formatting, the license of these functions (and cp949m.h) is not certain.
I'll try to replace these with some "safe" code.

@sev-

This comment has been minimized.

Copy link

@sev- sev- commented on graphics/korfont.cpp in f39567e Nov 3, 2020

Are you sure you want to include scumm/scumm.h here?

This comment has been minimized.

Copy link
Owner Author

@wonst719 wonst719 replied Nov 3, 2020

No. It should be removed. I'll fix.

@sev-

This comment has been minimized.

Copy link

@sev- sev- commented on f39567e Nov 3, 2020

This is pretty solid besides a few nitpicks, the biggest one is that #include. Good to go

@sev-
Copy link
Member

@sev- sev- commented Nov 6, 2020

Looks good to me, thanks!

@sev- sev- requested a review from bluegr Nov 6, 2020
@sev-
sev- approved these changes Nov 6, 2020
@sluicebox
Copy link
Member

@sluicebox sluicebox commented Nov 8, 2020

This looks great and is very exciting! I had no idea there were so many Korean SCI translations.

Since GK1 Korean is high-res only, GAMEOPTION_HIGH_RESOLUTION_GRAPHICS should be removed from its DOS detection table entry, as it is for Windows CD and Mac. That prevents giving users an option that doesn't do anything and it means you don't need the GK1 exception you've added to GfxFrameout::detectHiRes(). High-res is the default with that option removed. The existing exceptions in that function are for known low-res versions.

I'd like to see the repeated message-map tests in resource.cpp moved into a function like isKoreanMessageMap() with a comment explaining why that needs to be handled differently. I spend a lot of time in resource.cpp, and as I'm sure you've noticed, it's understandably built up a lot of cryptic exceptions. =)

engines/sci/resource.cpp Outdated Show resolved Hide resolved
// korean and then switch to font 1001
bool GfxText32::SwitchToFont1001OnKorean(const char *text) {
//byte firstChar = (*(const byte *)text++);
if (1/*(firstChar >= 0xA1) && (firstChar <= 0xFE)*/) {

This comment has been minimized.

@bluegr

bluegr Nov 8, 2020
Member

If this is always true, then the checking should be removed

engines/sci/detection_tables.h Outdated Show resolved Hide resolved
engines/sci/detection_tables.h Outdated Show resolved Hide resolved
engines/sci/graphics/cache.cpp Outdated Show resolved Hide resolved
engines/sci/graphics/text32.cpp Outdated Show resolved Hide resolved
engines/sci/graphics/text16.cpp Outdated Show resolved Hide resolved
engines/sci/graphics/text32.cpp Outdated Show resolved Hide resolved
engines/sci/graphics/text32.cpp Outdated Show resolved Hide resolved
engines/sci/graphics/text32.cpp Outdated Show resolved Hide resolved
graphics/korfont.cpp Outdated Show resolved Hide resolved
graphics/korfont.h Outdated Show resolved Hide resolved
@bluegr bluegr self-requested a review Nov 8, 2020
Copy link
Member

@bluegr bluegr left a comment

Overall, very nice work!
Some small changes are needed, and once the issues reported by @sluicebox and me are addressed, this can be merged :)

wonst719 added 3 commits Nov 8, 2020
- Move repeated message.map tests into ResourceManager::isKoreanMessageMap()
- Fix comments
- Fix formatting
- Remove unused codes
- Remove GK1 exception in GfxFrameout::detectHiRes()
@wonst719 wonst719 requested a review from bluegr Nov 8, 2020
@wonst719
Copy link
Contributor Author

@wonst719 wonst719 commented Nov 8, 2020

I did some cleanup and applied requested changes.

@bluegr
Copy link
Member

@bluegr bluegr commented Nov 8, 2020

Excellent, thanks!
The only leftover is the if check in SwitchToFont1001OnKorean(), but that can be fixed in-tree.

Many thanks for your work, merging

@bluegr
bluegr approved these changes Nov 8, 2020
@bluegr bluegr merged commit e2415b5 into scummvm:master Nov 8, 2020
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deepcode-ci-bot Well done, no issues found!
Details
@wonst719 wonst719 deleted the wonst719:scummvm-kor-feature-sci branch Nov 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.