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 Hebrew translation of Torin #2215

Merged
merged 3 commits into from May 19, 2020
Merged

Conversation

@ZvikaZ
Copy link
Contributor

ZvikaZ commented May 1, 2020

  • Detect fan made Hebrew translation of Torin's Passage
  • Use BiDi algorithm in SCI engine
@ZvikaZ ZvikaZ force-pushed the ZvikaZ:z_torin_hebrew branch 3 times, most recently from 8f2ce87 to 54d96ab May 1, 2020
Common::String textString;
const char *text;
if (g_sci->getLanguage() == Common::HE_ISR) {
#ifdef USE_TRANSLATION

This comment has been minimized.

Copy link
@sev-

sev- May 1, 2020

Member

That is incorrect and will not work when translation is enabled but BiDi is not. You have to wrap it in both conditions.

This comment has been minimized.

Copy link
@ZvikaZ

ZvikaZ May 1, 2020

Author Contributor

But see in common\translation.cpp:

#ifdef USE_FRIBIDI
String TranslationManager::convertBiDiString(const String &input, const Common::Language lang) {

...

}
#else
String TranslationManager::convertBiDiString(const String &input, const Common::Language lang) {
	return input;
}
#endif

Therefore, in case that translation is enabled but BiDi is not, it should just return the original input.

If you're worried about the (g_sci->getLanguage() == Common::HE_ISR) - it's OK, because SciEngine::getLanguage() is always defined, and not dependent on any ifdef.

Anyway, I have just verified it without USE_TRANSLATION, and it worked well.

This comment has been minimized.

Copy link
@criezy

criezy May 1, 2020

Member

Maybe the convertBiDiString() that takes a language could be moved out of TranslationManager so that it is available also when USE_TRANSLATION is not defined?

I am also wondering if assuming iso8859-8 is a good idea, and maybe we might want to pass the charset to the function as well (e.g. maybe we support some Hebrew games that use Unicode encoding and would benefit from this, or we might want to change TranslationManager to use Unicode in the future but without breaking BiDi in SCI).

This comment has been minimized.

Copy link
@ZvikaZ

ZvikaZ May 2, 2020

Author Contributor

@criezy , I agree.
Let's wait with this PR until #2217 is approved, and then I'll update the code here.

This comment has been minimized.

Copy link
@Kawa-oneechan

Kawa-oneechan May 3, 2020

Contributor

For a second there I misread that as "SCI games that use Unicode encoding" and thought someone had beaten me to it.

@ZvikaZ ZvikaZ requested a review from sev- May 1, 2020
_drawPosition.x += (textRectWidth - textWidth) / 2;
} else if (_alignment == kTextAlignRight) {
_drawPosition.x += textRectWidth - textWidth;
if (g_sci->getLanguage() != Common::HE_ISR) { // i.e. regular Left To Right direction

This comment has been minimized.

Copy link
@bluegr

bluegr May 9, 2020

Member

It would be better if this check was wrapped into a separate function, e.g. isRTLLanguage(), as we might support other RTL languages in the future

This comment has been minimized.

Copy link
@ZvikaZ

ZvikaZ May 9, 2020

Author Contributor

Agreed. I have even already implemented isLanguageLTR (I prefer positive checks rather than if not something.
I will update his after PR #2217 will be accepted, along with other required changes.

This comment has been minimized.

Copy link
@sev-

sev- May 12, 2020

Member

Now #2217 is accepted. Please, update this PR, so we could merge it.

This comment has been minimized.

Copy link
@ZvikaZ

ZvikaZ May 17, 2020

Author Contributor

@sev- , @bluegr , @criezy , a reminder to check my updates (see detailed comment below).
@sluicebox , you might want to review this.
Thanks.

@ZvikaZ ZvikaZ force-pushed the ZvikaZ:z_torin_hebrew branch from 54d96ab to eeafd41 May 12, 2020
@ZvikaZ
Copy link
Contributor Author

ZvikaZ commented May 12, 2020

Continuing the work at PR #2236 , which moved convertBiDiString(..page) to unicode-bidi.h, now moving to there also the (..lang) flavour.

Thus, translation.h has only the SVM-GUI related function, and the two util functions page+code are
in unicode-bidi.

Also fixed a bug reported in Discord.
Copying here it as well:

Problem:

In this code:

char c[] = {0xee, 0xe5, 0xe3, 0x21, 0x20, 0xee, 0xe9, 0x20, 0xe6, 0xe4, 0x20, 0xf9, 0xed, 0x3f, 0x22, 0};
String s_in = c;
Common::U32String us = s_in.decode(Common::kWindows1255);
String s_out = us.encode(Common::kWindows1255);

I expect s_in and s_out to be equal.
But s_out is just a bunch of question marks.
Somehow, the .encode failed, but I don't understand why.
These chars are perfect Windows 1255 encoding, as can be seen at https://en.wikipedia.org/wiki/Windows-1255

Solution:

In the file backends\platform\sdl\win32\win32.cpp, line 441:
string = Common::convertUtf32ToUtf8(UTF32Str).c_str();
There is a problem.
convertUtf32ToUtf8 returns a String, and .c_str() returns a pointer to its internal storage, and that's assigned to string.
But this returned String, along with its storage, is later deallocated, and now string (note the S and s in my explanations...) is pointing to garbage.

Fixed it by declaring:
Common::String tempString; outside of the scoped if, and changing that line to:

    tempString = Common::convertUtf32ToUtf8(UTF32Str);
    string = tempString.c_str();
@ZvikaZ ZvikaZ requested review from bluegr and criezy May 12, 2020
@ZvikaZ
Copy link
Contributor Author

ZvikaZ commented May 12, 2020

Note: the builds aren't really failed.
2 passed, and the Mac one also went well, and finished without errors, but then it timed out after 50 minutes (what is it doing there so long, after finishing?...)

@ZvikaZ ZvikaZ force-pushed the ZvikaZ:z_torin_hebrew branch 2 times, most recently from a63d5d8 to eeafd41 May 17, 2020
ZvikaZ added 3 commits May 12, 2020
Continuing the work at #2236,
which moved `convertBiDiString(..page)` to unicode-bidi.h,
now moving to there also the `(..lang)` flavour.

Thus, translation.h has only the SVM-GUI related function, and the
two util functions page+code are in unicode-bidi.
- Detect fan made Hebrew translation of Torin's Passage
- Use BiDi algorithm in SCI engine
@ZvikaZ ZvikaZ force-pushed the ZvikaZ:z_torin_hebrew branch from eeafd41 to 5d404ae May 19, 2020
@sev-
Copy link
Member

sev- commented May 19, 2020

Now it is clean and tidy. Thank you!

@sev- sev- merged commit b4d40ed into scummvm:master May 19, 2020
0 of 2 checks passed
0 of 2 checks passed
Codacy/PR Quality Review Hang in there, Codacy is reviewing your Pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
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

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