这是indexloc提供的服务,不要输入任何密码
Skip to content

Conversation

@amurzeau
Copy link
Contributor

@amurzeau amurzeau commented Aug 22, 2022

Hi,

Here is the PR to add translations as discussed in #152.

I've used the C++11 version as there is no significant performance impact (I've checked with valgrind that the initialization doesn't use much cpu time).

I've also added a when to change the font, this is required to display characters that the default font doesn't support.
For that, I use the current language to construct the font atlas with only needed characters.
When using a non-default font, text is not as good, but I can't make it better. I think subpixel rendering is what is needed, but ImGui doesn't support it (yet).

Fixes: #152

@k4zmu2a
Copy link
Owner

k4zmu2a commented Aug 23, 2022

Hello.

This translations step 1 looks pretty good to me.
C++11 was the preferred choice, nice that it worked out.

Here are my usual nitpicks:
• The game should not crash when FontFileName cannot be loaded. Either remove the option or make it fallback to built-in font. Currently it locks out the game, that is unacceptable.
• Don’t use macros, they are only to be used as last resort.
• Language list in options should use human readable language names, as in v1. There should be no NUMBER in user language list.
• Do not use strings for language id, instead use enum directly. I would prefer if you used v1 language enum names, such as English vs en.
• I would like translation_id_e name shortened, to something like Msg or Ids.
• Forward declare translation stuff in the header. translated_strings should be private static?
• Check translations.h includes, some could be unused or provided by pch.

That is all for this review.
I am going to bring in more FT stuff; it should not conflict with your things.

@amurzeau
Copy link
Contributor Author

About the macro, I think I can use InitializedArray to have the display name associated with the language enums.
To avoid the NUMBER item in enums, I can use a std::vector instead of a fixed array inside InitializedArray, I don't think there is a performance penalty, this will just do one more allocation.
But there won't be any compile time check of the array size then, so do you really think its better without NUMBER enums ? unless there is a way to handle that somehow

translated_strings is in an anonymous namespace, this is the same as static, but allowing a declaration so I can keep it's definition at the end of the file (as the definition is very long).

@k4zmu2a
Copy link
Owner

k4zmu2a commented Aug 29, 2022

I believe there is a miscommunication happening here.
I will take a more hand on approach with my suggestions by applying them directly to add-translations2.

@k4zmu2a
Copy link
Owner

k4zmu2a commented Aug 29, 2022

This PR is now aligned with my vision™))

One last thing for you to do before the merge:
Remove placeholder English texts from missing translations (for example in Menu1_UseMaxResolution_640x480).
This is misleading, makes it hard to see actual translation state of the game.
Translation array initialization should be sparce, with missing texts set to null by default.

@amurzeau
Copy link
Contributor Author

Thanks for your changes, I've done the removal of English texts for non-English languages.

@k4zmu2a k4zmu2a merged commit 66a8680 into k4zmu2a:master Aug 31, 2022
@k4zmu2a
Copy link
Owner

k4zmu2a commented Aug 31, 2022

Alright, this PR is now good to go.
Thank you for taking this brave first step into the world of cross-platform localization support)

Up next - my plans for localization support v1:
• Embedded font - stripped pan-Unicode, spliced two fonts, something else.
• Auto system font support, per platform folders, known good font names, language specific fonts (CJK), font selection from list.
• ImGui font handling changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Adding Translations in v2

2 participants