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

SCUMM: Multi-font for Korean fan translated games #2606

Merged
merged 3 commits into from Nov 6, 2020

Conversation

@wonst719
Copy link
Contributor

@wonst719 wonst719 commented Nov 6, 2020

This PR is part of scummvm-kor merge project.

It adds the ability to load and use multiple external Korean fonts to match the original look.

On a separate note, I intend to refactor CJK text rendering to use Graphics::Font*.

Tested with:

  • Loom (DOS/CD/Korean patched)
  • Monkey Island 2 (DOS/Korean patched)
  • Indy 4 (DOS/CD/Korean patched)
  • DOTT (DOS/CD/Korean patched)
    scummvm-monkey2-kr-00002
    scummvm-monkey2-kr-00004
@sev-

This comment has been minimized.

Copy link

@sev- sev- commented on engines/scumm/string.cpp in 73bca81 Nov 3, 2020

Are you sure this doesn't break Japanese?

This comment has been minimized.

Copy link
Owner Author

@wonst719 wonst719 replied Nov 3, 2020

Yep, it's essentially the same code if _language is set to JA_JPN.

@sev-

This comment has been minimized.

Copy link

@sev- sev- commented on 956e701 Nov 3, 2020

Looks fine

@deepcode-ci-bot
Copy link

@deepcode-ci-bot deepcode-ci-bot bot commented Nov 6, 2020

DeepCode's analysis on #956e70 found:

  • ⚠️ 1 warning 👇

Top issues

Description Example fixes
Using sprintf can lead to severe buffer overflow vulnerabilities. Use the safe alternative snprintf instead. Occurrences: 🔧 Example fixes

👉 View analysis in DeepCode’s Dashboard | Configure the bot

- use snprintf() instead of sprintf()
@athrxx
Copy link
Member

@athrxx athrxx commented Nov 6, 2020

The new code seems to be well separated from the existing code. I don't see any issues.

Rendering through the CharsetRendererTownsV3 is a bit ugly. And half of the methods get redirected to CharsetRendererV3 anyway. And the actual MI2 FM-Towns version doesn't even use CharsetRendererTownsV3 (since it isn't Scumm3; it uses CharsetRendererClassic). But that's just nitpicking. We can still check after the merge whether it makes sense to come up with a new CharsetRenderer subclass...

@sev-
Copy link
Member

@sev- sev- commented Nov 6, 2020

Thanks, all is nice and clean. I'll do the occasional formatting fixes in-tree.

@sev- sev- merged commit 0818564 into scummvm:master Nov 6, 2020
1 of 2 checks passed
1 of 2 checks passed
deepcode-ci-bot DeepCode is checking for issues.
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@wonst719 wonst719 deleted the wonst719:scummvm-kor-feature-scumm-multifont branch Nov 7, 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

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