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

Add support for MT 5 game translation (rebasing #2368) #2466

Merged
merged 18 commits into from Sep 10, 2019

Conversation

@yquemener
Copy link
Contributor

commented Sep 7, 2019

I am making a game that needs an internationalized version of minetest_game. I saw #2368 needed some rebasing so here it is. I think making a new PR is the correct way of doing this but please tell me if I should do things differently.

This is mostly Zweihorn's commits, I just added textdomain strings matching mod names to calls to minetest.get_translator(...)

Zweihorn and others added 18 commits May 13, 2019
M  mods/beds/beds.lua
M  mods/beds/init.lua
M  mods/carts/cart_entity.lua
M  mods/carts/init.lua
M  mods/carts/rails.lua
M  mods/creative/init.lua
M  mods/creative/inventory.lua
M  mods/default/chests.lua
M  mods/default/craftitems.lua
M  mods/default/furnace.lua
M  mods/default/init.lua
M  mods/default/nodes.lua
M  mods/default/tools.lua
M  mods/default/torch.lua
M  mods/default/trees.lua
M  mods/farming/api.lua
M  mods/farming/hoes.lua
M  mods/farming/init.lua
M  mods/farming/nodes.lua
M  mods/default/craftitems.lua
M  mods/default/nodes.lua
M  mods/doors/init.lua
M  mods/flowers/init.lua

My related review message was:
I am utterly sorry but appears as if I mixed lines 224-230 with lines 239-245 instead of an appropriate depollute. Please clarify.
M  mods/default/craftitems.lua
@yquemener

This comment has been minimized.

Copy link
Contributor Author

commented Sep 8, 2019

By the way, reading the translation advice from the wiki I suspect this is bad practice to just make a PR to add translations files but if someone wants to test the modifications with actual translation files, there is a repo that adds /locale/[MOD].[LANG].tr files : https://git.minetest.land/yvanhoe/minetest_game

@MoNTE48

This comment has been minimized.

@yquemener

This comment has been minimized.

Copy link
Contributor Author

commented Sep 8, 2019

@MoNTE48 Sorry I don't understand what you mean?

@MoNTE48

This comment has been minimized.

Copy link

commented Sep 8, 2019

@yquemener Look at the code, I gave a link to the line. This will reduce PR by 10 times. This should probably change the engine first. But there are other solutions for overriding functions in mtg.
By the way.
Why open an infinite number of PRs? You can't rebase the old one?

@paramat

This comment has been minimized.

Copy link
Member

commented Sep 8, 2019

MoNTE48 thanks, that is worth considering.

@yquemener

This comment has been minimized.

Copy link
Contributor Author

commented Sep 8, 2019

I am not the author of the old ones, if there is a way to rebase and update a PR from a different author then I am sorry, I was not aware of that. If there is a way to merge both PRs please do or tell me. I am not sure why Zweihorn opened two.

All I saw was that the last activity on either PR was to ask for a rebase and to ask that the textdomain string be entered correctly, which I did.

Your approach is nice and that's how I would have done most things, but Zweihorn's way is actually the recommended approach in the lua_api.txt doc, and was already done. As it is, the function you propose would fail in case someone passes a string that has already be localized: S(S("apple"))gives bad results but that's also solvable and I can implement it if it is preferred.

Anyway, this approach or another, I'd like to do whatever is deemed necessary to get the default game translatable. Is there any other things to fix I should be aware of?

@paramat

This comment has been minimized.

Copy link
Member

commented Sep 8, 2019

MoNTE48's suggestion was disapproved on IRC http://irc.minetest.net/minetest-dev/2019-09-08#i_5587844

16:01 paramat    is what monte48 suggests here
https://github.com/minetest/minetest_game/pull/2466#issuecomment-529196737 a good idea?
16:01 paramat    auto-set translations to item descriptions
16:03 sfan5      no
16:03 sfan5      mods know best how to translate stuff themselves
16:05 paramat    ok, that makes sense

Rebasing someone else's PR is not easy so don't worry about not doing that. Personally i would prefer to open a new PR too.

I suggest we attend to this large PR before merging other PRs to not create conflicts.
I'll need help as i am not good with translation code.
@Wuzzy2 your thoughts will be appreciated.

@paramat paramat referenced this pull request Sep 8, 2019
@MoNTE48

This comment has been minimized.

Copy link

commented Sep 8, 2019

  1. S(S("apple")) no problem for me.
    If the implementation of translations in 5.0 is worse than in intllib, then the one who did it is not the best programmer
  2. "MoNTE48's suggestion was disapproved on IRC"
    How glad I am that I am not taking part in this mess. I translated my game in 1 evening. Good luck with your work, perhaps in 2021 you will do it!
    "Translating minetest_game #1932
    Fixer-007 opened this issue on 31 Oct 2017" - 2 years! 😂 🤣
@rubenwardy

This comment has been minimized.

Copy link
Member

commented Sep 8, 2019

The S() is needed to be able to identify strings in mods that should be translated when using translation scrapers. I suppose you could boot the game up and also collect all the strings after loading

Copy link
Member

left a comment

👍 apart from this

@@ -243,8 +247,9 @@ local function furnace_node_timer(pos, elapsed)
minetest.get_node_timer(pos):stop()
end

local infotext = "Furnace " .. active .. "\n(Item: " .. item_state ..
"; Fuel: " .. fuel_state .. ")"
-- local infotext = "Furnace " .. active .. "\n(Item: " .. item_state ..

This comment has been minimized.

Copy link
@rubenwardy

rubenwardy Sep 8, 2019

Member

this probably shouldn't be left

@MoNTE48

This comment has been minimized.

Copy link

commented Sep 8, 2019

@rubenwardy I don’t understand what you mean. Give at least 1 scenario when the name of the item will be the same as another line in this mod, but will it have a different meaning?
Furnace is a Furnace regardless of context. Iron shovel is always an Iron shovel. An Apple cannot become a Pear as a result of a translating in my way.
75% of the files do not require a single change when translating in my way.

Translation MultiCraft_game:
MultiCraft/MultiCraft@3fd064f#diff-cbf292a59bbca341e9df102076b7bd01
539 additions and 91 deletions.
"Dye" mod was the most difficult to translate, ~110 lines (the Englishman can not understand me)
Also, the potion mod was quite specific.

@rubenwardy

This comment has been minimized.

Copy link
Member

commented Sep 8, 2019

You should be able to collect translations using a script to create template files for translations. It's why gettext() is always used in full in the engine, for example

@Wuzzy2

This comment has been minimized.

Copy link
Contributor

commented Sep 8, 2019

It's REVIEW TIME! Here I will complain about everything. OK, not really, but almost!

S("Binoculars\nUse with 'Zoom' key")

  •   description = S("Mapping Kit\nUse with 'Minimap' key"),
    

I recommend to split the translation for each line, as long each part is a complete unit. For example:

S("Binoculars").."\n"..S("Use with 'Zoom' key")

Why? Because some mods like show_wielded_item like to cut item descriptions at newlines. show_wielded_item only displays the first line. If you do it like this, then such mods will still work. I see it as an informal “rule” for item description that the first line of it is considered the “primary” part of the item description that is critical information, with the rest containing additional, non-essential info.

S("@1 Butterfly", desc),
S("Hidden @1 Butterfly", desc),

  • description = S("@1 Sign", desc),
  •           description = S("@1 Dye", description),
    
  •           description = S("Inner @1", description),
    
  •           description = S("Outer @1", description),
    
  •           description = S("@1 Wool", desc),
    

Please don't do this. Why? Because you cannot trust the grammar. The translation on the word “butterfly” might change depending on the word before it because of grammar reasons. It's just a coincidence that it's always the same in English. Therefore, each butterfly description must be translatable in full, individually. The proper way to do this is to always fully write out “Red Butterfly”, “Violet Butterfly” and “White Butterfly” as translatable strings. Yes, I know, this seems like a mouthful, but this is the safest way to ensure that translators can translate these properly. As a general rule of thumb: NEVER make assumptions about the grammar of other languages.

tooltip[creative_search;S("Search")]

Server crash in 3 ... 2 ... 1 ... Boom! You must enclose the S() into minetest.formspec_escape because it contains variable text you don't control. I recommend this for all texts in formspecs. Hint: You can write local F = minetest.formspec_escape at the top of the file to make it shorter. I religiously add the formspec escape for all translated/variable strings in MCL2, and you should, too.
There are many cases of translated formspec texts and I will not point you to each case individually.

creative.register_tab("all", S("All"), minetest.registered_items)

This sounds like another potential formspec crasher. But in this case, the minetest.formspec_escape should be put into the register_tab function, but I did not see it in your PR anywhere. PLZ fix. THX.

minetest.chat_send_player(self.driver, S("[boats] Cruise on"))
minetest.chat_send_player(self.driver, S("[boats] Cruise off"))

Why is “[boats]” being sent to the player as well? I thought this “[foobar] style” was reserved to debug logs only. Note that part of making any software translatable is to first clean up the source strings as well, so it is legitimate to also discuss the source strings, thus my comment is on-topic.

  •           meta:set_string("infotext", S("@1's fresh bones.", player_name))
    
  •           meta:set_string("infotext", S("@1's bones.", player_name))
    

Why did you add the period? It wasn't there before. And this is not a complete sentence, just an infotext. Please remove the period.

  • return secret, S("a locked chest"), owner
  • return secret, S("a locked trapdoor"), owner

Wait, what? This doesn't look right. Is this supposed to be translated or is this for internal use only? Can someone please check if this string is supposed to be translated? IIRC, this is part of the key API, and that the secret is an INTERNAL string or something. This might introduce a bug. If this string is never exposed to the user, you can and should strip the S().

  •   description = S("Book With Text"),
    

Turn it to “Book with Text”. Why? Consistency! Prepositions are not capitalized elsewhere. Prepositions are not capitalized in other items, like “Dirt with Grass and Footsteps”. Besides, Every Time I See Someone Capitalizing Every Word, My Eyes Bleed. THAT'S EVEN WORSE THAN ALL-CAPS! xD

  •   local infotext = S("Furnace @1 \n(Item: @2; Fuel: @3)", active, item_state, fuel_state)
    

Remove the space before the newline. Also: Shouldn't the newline be @n? I strongly recommend to test this string in-game. (Actually, ALL strings should be tested, ideally, but whatever :P).

Now you probably wonder: Why am I this time to keep the newline in the translation? Because it's an infotext, not an item description. Actually, in this case it doesn't really matter if you put it into one big string or split it into 2 strings. Both approaches would be perfectly valid here.

  •   description = "Dry Dirt",
    
  •   description = "Dry Dirt with Dry Grass",
    
  •   description = "Dry Soil",
    
  •   description = "Wet Dry Soil",
    

Empathic oopsie!

S("Screwdriver (left-click rotates face, right-click rotates axis)")

  •   description = S("Cart (Sneak+Click to pick up)"),
    

I recommend to put the item help into a separate line and remove the brackets, so the style is consistent with items like binoculars and mapping kit.

  •   description = S("Teleport you to your home point"),
    

Sounds a bit awkward. Maybe “Teleport yourself to your home point”?

In my next post I will reply about that Monte thing.

@Wuzzy2

This comment has been minimized.

Copy link
Contributor

commented Sep 8, 2019

OK, in this post I will review that suggestion from @MoNTE48.

"MoNTE48's suggestion was disapproved on IRC"
How glad I am that I am not taking part in this mess. I translated my game in 1 evening. Good luck > with your work, perhaps in 2021 you will do it!

Bwahahahaha! You have good humour. :)

However, you failed to actually defend your suggestion. This is not convincing. 👎

So, let's talk about this suggestion. You know. The one by you, MoNTE48, that was disapproved on IRC. It seems to basically revolves around this code here:

if itemdef.description and itemdef.description ~= "" then
    itemdef.description = Sl(itemdef.description)
end

Source: https://github.com/MultiCraft/MultiCraft/blob/7a75aa937c9510ebf547d25a59ebf7b962a8c92c/builtin/game/register.lua#L158

So, if I understand correctly, you try force-collect all item descriptions.

No, just no. This is not good. Why? Because it assumes the mod always wants the entire item desciption be translated. But this assumption is absurd. It's not always reasonable to just dump the entire string of the description field into S(). What if the item description has many newlines? It would make sense to break it into multiple S()s, like in many examples above (e.g. mapping kit, binoculars). It's generally considered good practice to keep translatable strings to be well-digestible chunks of text. Making a software translatable is NOT just mindlessly any string you see into S(). You need to keep the translator in mind, who will see the text and must understand it. If the translator needs to look into the sources to understand it, that's bad.
But this suggestion would force a S() into all item descriptions, mods can no longer break a particularily complex and long description into multiple digestible pieces. Forcing a S() into the description at builtin or even engine level would deprive mods of this freedom. And what's the justification for all that? Basically just because to make a diff smaller. And that's just a shitty argument.
Note that for most item description this actually wouldn't make any differences. But it DOES make a difference for those special cases with very long descriptions like those with the newlines.

Therefore, this suggestion was rightfully rejected. 👍

By the way, you all should read the Gettext manual. Many of my principles I applied here in my review from the previous post come from the Gettext manual. The Gettext manual is not just a manual on Gettext, it also contains great justified advice on how to make a software properly translatable. Sure, mods don't use Gettext, but most of the suggested practices can be applied here, too.

@MoNTE48

This comment has been minimized.

Copy link

commented Sep 8, 2019

I agree with the @Wuzzy2 words above.

However, you failed to actually defend your suggestion.

Well, I didn’t try. I periodically check for updates for some of the mods that I use. I partially use default from MTG. I saw this PR and said, “Oh God! They do it again! I wondered how to avoid it all day. Then I translated the game in one evening. I made a minimal diff to make it easier to compare my version with MTG. Everything was pointless!”

Again, I do not agree with your statement. If you use my method, all "Apple" items will be translated as "Яблоко". But the item "Apple Wood" will remain untranslated.

Your goal is to create an internalization game, right? You would like the maximum amount of content translated into the maximum number of languages ​​to expand your audience. Most mods are satisfied with the registration of new items and do not require the translation of additional lines.
My way would be to create a community-supported translation base for ALL mods.
Create the Online Translate API to in-loading-time translations the game into non-popular languages, or languages ​​that no one in the community speaks using Online services! Imagine the potential of my idea.
So many old mods that barely get an API fix. They would get translation support without changing the code at all.
Too much text, yep? 🤐 All players know English, how else would they download the game from the English site? 😉
Forget my words.

Upd. https://github.com/taikedz-mt/babelfish
This guy is a genius. But he did not think that he could automatically translate the game into all the languages that Yandex Translate supports. The translation will not be perfect, but it is better than nothing. Adding a few filters and debugging would be "almost" perfect.

@paramat

This comment has been minimized.

Copy link
Member

commented Sep 8, 2019

Why is “[boats]” being sent to the player as well? I thought this “[foobar] style” was reserved to debug logs only.

I agree, that was my mess, something like "Boat cruise mode on/off" is good.

Empathic oopsie!

Is this for 'Wet Dry ...'? Weird i know but it's actually logical in a way. Perhaps 'Irrigated Dry Soil' would be better.

@paramat

This comment has been minimized.

Copy link
Member

commented Sep 8, 2019

Due to the size of this PR and the need to merge soon, i think improving the original strings, and perhaps some of Wuzzy2's other suggestions, could be left to a follow up PR. I don't want to delay this PR too much and be perfectionist about it.

@Wuzzy2

This comment has been minimized.

Copy link
Contributor

commented Sep 8, 2019

If you use my method, all "Apple" items will be translated as "Яблоко". But the item "Apple Wood" will remain untranslated.

This … this doesn't make any sense. Why would Apple Wood be not translated? As far I understand the code, it just dumps all item descriptions into S(). So both “Apple” and “Apple Wood”. So both strings will be collected and thus become translatable. But this example is irrelevant, I was thinking about item descriptions like “Map\nUse minimap key to use” where it makes sense for the mod author to split it into S("Map").."\n"..S("Use minimap key to use"). But your system would force the entire string into S("Map\nUse minimap key to use"), and there is absolutely no chance for the mod author to undo this. This takes away an important choice from the mod author.

So many old mods that barely get an API fix. They would get translation support without changing the code at all.

This is the wrong way to make games translatable. Legacy mods and games have problems that are much worse than lack of translation support. The game/mod author must have full control over what exact portions of the software are translated, everything else just leads to silliness. You try to automate it all away, which is laudable, but the hard truth is, it just doesn't work that way.

My way would be to create a community-supported translation base for ALL mods.
Create the Online Translate API to in-loading-time translations the game into non-popular languages, or languages ​​that no one in the community speaks using Online services! Imagine the potential of my idea.

This sounds great on paper, but it just doesn't work in practice. Depriving the game/mod authors of the choice of what exact portions to make translatable is not acceptable and creates more problems than it solves.

What we actually need to do is to make adding the translation system as simple as possible, but not simpler. There are other, real, problems in the current system, that make adoption difficult, such as the usage of a hand-written file format instead of a well-established one, and the lack of an official (!) toolchain to actually extract the strings and generating a template.txt, and an official (!) script to update the locale files.

I will no longer comment on your suggestion, as I feel this goes way too off-topic now. Better open a new issue or PR instead. Thanks.

(about wet dry soil)

Haha, yes, the wet dry soil is super-silly as well. But I actually meant the missing S(). But if you change the word “wet”, you need to change it for all other soils, of course. Or maybe rename the “dry” to “coarse” or something else (but remember to rename all related nodes).

Upd. https://github.com/taikedz-mt/babelfish

This is completely off-topic and has nothing to do with this PR.

@Wuzzy2

This comment has been minimized.

Copy link
Contributor

commented Sep 8, 2019

@paramat: Please look again at:

return secret, S("a locked chest"), owner
return secret, S("a locked trapdoor"), owner

(scroll up for the full text …)

I suspect this will introduce a sneaky and potentially nasty-to-debug bug. It should be checked if these strings are really user-facing, or only for internal use. So at least this thing should get a closer look.

The other stuff, yeah, makes sense for a separate PR.

@Wuzzy2

This comment has been minimized.

Copy link
Contributor

commented Sep 8, 2019

So. To make it simple for you:

If these 2 strings are really user-facing, and not for internal use: Keep the S(). You're done. Nothing more to do.
And if these 2 strings are NOT user-facing, and only used internally, then they must NOT be translated. Translating internally-used strings can lead to hilarious bugs. In this case, the fix is to simply remove the S() for both. And then you're done.

@paramat

This comment has been minimized.

Copy link
Member

commented Sep 8, 2019

Ok thanks, input much appreciated.

@paramat

This comment has been minimized.

Copy link
Member

commented Sep 8, 2019

Wuzzy2:

return secret, "a locked chest", owner

return secret, "a locked trapdoor", owner

minetest_game/game_api.txt

Lines 1022 to 1032 in b701e50

`on_skeleton_key_use(pos, player, newsecret)`
* Is called when a player right-clicks (uses) a skeleton key on your
* node.
* `pos` - position of the node
* `player` - PlayerRef
* `newsecret` - a secret value(string)
* return values:
* `secret` - `nil` or the secret value that unlocks the door
* `name` - a string description of the node ("a locked chest")
* `owner` - name of the node owner

Looks like the strings are node descriptions, so need translation.
So, nothing to do.

@paramat

This comment has been minimized.

Copy link
Member

commented Sep 8, 2019

Hmm but then:

minetest_game/game_api.txt

Lines 1043 to 1046 in b701e50

Aside from the secret value, the function should retun a descriptive
name for the node and the owner name. The return values are all
encoded in the key that will be given to the player in replacement
for the wielded skeleton key.

Which suggests these strings are internally used for encoding.
@sofar

@sofar

This comment has been minimized.

Copy link
Member

commented Sep 8, 2019

It is user facing.

The game_api.txt text is correct. The secret value is encoded as a meta property (not shown to user, obviously), the description is encoded as item description which is shown to the user in a tooltip.

@paramat

This comment has been minimized.

Copy link
Member

commented Sep 8, 2019

Thanks.

@yquemener

This comment has been minimized.

Copy link
Contributor Author

commented Sep 9, 2019

Thanks everyone for the great input. I think I'll have to deal a lot with internationalization issues so I'll look into the gettext manual. I wanted to be as reactive as possible with that PR but please give me 24h, we got directly hit by a typhoon this night and got emergencies to deal with as well as power outage. Will try to fix all the issues mentioned asap, wether in another PR if this one gets merged (which would be great to avoid doing too much rebasing) or in this one if it has to be fixed.

@paramat

This comment has been minimized.

Copy link
Member

commented Sep 9, 2019

Since those strings are user facing, they ok translated, and according to Wuzzy2 all else could be left to a later PR, so i'll remove the 'action ...' label.

So this seems mergeable now, seems best to merge it quickly and do the rest in a follow-up.

@sfan5
sfan5 approved these changes Sep 10, 2019
Copy link
Member

left a comment

Looks fine. Confirmed functionality of a few translation strings.

@sfan5 sfan5 added >= Two approvals and removed One approval labels Sep 10, 2019
@sfan5 sfan5 merged commit bb9279c into minetest:master Sep 10, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@lhofhansl

This comment has been minimized.

Copy link
Contributor

commented Sep 14, 2019

This accidentally removed the waving = 3 labels from water, which is needed for the waving water effect. Must've happening upon merge. Check git directly, this commit removed them.

MoNTE48 added a commit to MoNTE48/minetest_game that referenced this pull request Oct 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.