Page MenuHomeWildfire Games

Introduce translatable attack names
ClosedPublic

Authored by bb on Sep 5 2020, 12:43 AM.

Details

Summary

Since rP22866 the attack types are not properly internationalized in the tooltips. This patch fixes that commit by providing names to every attacktype is the template (similar to statuseffects).

Not entirely sure whether I used the correct nomenclature for all entities (especially fauna, elephants, siege and ships), maybe @Nescio has some opinion on this?

This patch contains most of the template changes required for D368.

Test Plan

Open unit_demo to see the front doesn't fall
Break your head around whether it is actually translated, or hope the best
Check every template, whether the correct nomenclature is used

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
bb added a comment.Sep 23 2020, 11:26 PM

This is actually easy to fix, we can add a context tag in the template, which will be extracted already. So I just added the code to pipe the context through and use it for translations. In example: one can put <AttackName context="Double-edged">Sword</AttackName> in the template.

How does this work, i.e. what's the result on transifex?

Below the translation field, there is a "more info" box displayed. The third item says "Context", the context string added here will be displayed there.

On this specific topic: I also added context for all sword wielding units.

No you didn't: you set it for the generic template_unit_*_swordsman.xml shared parents, which is wrong for about half their children.

And then it is replaced by the correct term, when wanted. If I counted correctly there are 20 different sword wielding units, of which 4 use a single-edged one. I believe avoiding duplication legitimizes settting the double-edged variant in the parent.

Then another concern: if I understand correctly, the attack name is taken from the template and translated separately. However, in some languages e.g. “spear” and “attack” will give a different translation than “spear-attack”.

Again context, I will add a context for all the attackNames now (a general one in messages.json), which will be visible for all in transifex for all strings which don't have a context defined in the template.

binaries/data/mods/public/simulation/templates/gaia/fauna_walrus.xml
5 ↗(On Diff #13413)

The different tooltips serve fundamentally different purposes, hence if one ever considers adding a general context, one is likely in trouble.

binaries/data/mods/public/simulation/templates/template_structure_defensive_tower_artillery.xml
5 ↗(On Diff #13487)

Lack of knowing something better. The particles are stones, so I assumed it must be some kind of stonethrower inside. Better suggestions are welcome

This is looking better and better :) Just one thought, for entities like animals, which have no clear weapon they use, it may be clearer to just use "melee"? E.g. the elephant uses both trunk and tusks.

Below the translation field, there is a "more info" box displayed. The third item says "Context", the context string added here will be displayed there.

If I understand correctly, strings are translated on transifex, not occurrences, i.e. if a particular string is translated, all occurrences that are exactly the same have the same translation, which is typically fine for full sentences or longer texts. Although context is displayed on transifex, it's essentially meta-data, and since it does not alter the string, I fear both "Sword" (with double-edged context) and "Sword" (with single-edged context) will have the same translation. That said, I don't know the inner workings of transifex and I didn't test for this particular case.

And then it is replaced by the correct term, when wanted. If I counted correctly there are 20 different sword wielding units, of which 4 use a single-edged one. I believe avoiding duplication legitimizes settting the double-edged variant in the parent.

Did you check champions too? Those usually don't have swordsman in their file name. Anyway, if single-edged blades are indeed a small minority, I agree it's better to set Sword in the generic template parent and a different string in the relevant children. Perhaps ‘Saber’? It's a bit anachronistic, but probably clearer than e.g. ‘Backsword’.

bb added a comment.EditedSep 26 2020, 12:10 AM

Below the translation field, there is a "more info" box displayed. The third item says "Context", the context string added here will be displayed there.

If I understand correctly, strings are translated on transifex, not occurrences, i.e. if a particular string is translated, all occurrences that are exactly the same have the same translation, which is typically fine for full sentences or longer texts. Although context is displayed on transifex, it's essentially meta-data, and since it does not alter the string, I fear both "Sword" (with double-edged context) and "Sword" (with single-edged context) will have the same translation. That said, I don't know the inner workings of transifex and I didn't test for this particular case

A very valid question, but transifex is slightly smarter:

  1. It translates strings per module (see the grouping of strings in transifex). This ofcourse does not help in this case, since all strings will be in Sim. Templates.
  2. Transifex cares about the context. Strings with different context will appear as different items in the list (e.g look at the "Range:" string in Misc. GUI, one without context, one with "aura" context). Also as you can see in the code the context is passed to the translate call (translateWithContext(context, string) and that will choose the translation of the tuple (context, string) so context should help us in this case!!. Different context == different translation

And then it is replaced by the correct term, when wanted. If I counted correctly there are 20 different sword wielding units, of which 4 use a single-edged one. I believe avoiding duplication legitimizes settting the double-edged variant in the parent.

Did you check champions too? Those usually don't have swordsman in their file name. Anyway, if single-edged blades are indeed a small minority, I agree it's better to set Sword in the generic template parent and a different string in the relevant children. Perhaps ‘Saber’? It's a bit anachronistic, but probably clearer than e.g. ‘Backsword’.

You are right, also founda new type: kush_champ_amum has a Sickle. Also some heroes. 9 Singles on 51 swords now
EDIT: somehow feel Sickle is the wrong word, but can't find the correct one...

bb updated this revision to Diff 13543.Sep 26 2020, 12:10 AM

Successful build - Chance fights ever on the side of the prudent.

builderr-debug-macos.txt
../../../source/simulation2/scripting/JSInterface_Simulation.cpp:155:4: warning: suggest braces around initialization of subobject [-Wmissing-braces]
                        CFixedVector2D(-halfSize.X, -halfSize.Y),
                        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.
../../../source/third_party/fmt/format.cpp:145:7: warning: '_POSIX_C_SOURCE' is not defined, evaluates to 0 [-Wundef]
#if ((_POSIX_C_SOURCE >= 200112L || _XOPEN_SOURCE >= 600) && !_GNU_SOURCE) || (defined(__ANDROID__) && __ANDROID__)
      ^
../../../source/third_party/fmt/format.cpp:145:37: warning: '_XOPEN_SOURCE' is not defined, evaluates to 0 [-Wundef]
#if ((_POSIX_C_SOURCE >= 200112L || _XOPEN_SOURCE >= 600) && !_GNU_SOURCE) || (defined(__ANDROID__) && __ANDROID__)
                                    ^
2 warnings generated.
In file included from ../../../source/graphics/tests/test_Camera.cpp:17:
/Users/wfg/Jenkins/workspace/macos-differential/source/graphics/tests/test_Camera.h:168:4: warning: suggest braces around initialization of subobject [-Wmissing-braces]
                        CVector3D(-101.0f, -101.0f, 101.0f),
                        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.
In file included from ../../../source/simulation2/tests/test_SerializeTemplates.cpp:17:
/Users/wfg/Jenkins/workspace/macos-differential/source/simulation2/tests/test_SerializeTemplates.h:39:4: warning: suggest braces around initialization of subobject [-Wmissing-braces]
                        3, 0, 1, 4, 1, 5
                        ^~~~~~~~~~~~~~~~
                        {               }
1 warning generated.
builderr-release-macos.txt
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libnetwork.a(precompiled.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libtinygettext.a(precompiled.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libtinygettext.a(tinygettext.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/liblobby.a(precompiled.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libglooxwrapper.a(precompiled.o) has no symbols
../../../source/simulation2/scripting/JSInterface_Simulation.cpp:155:4: warning: suggest braces around initialization of subobject [-Wmissing-braces]
                        CFixedVector2D(-halfSize.X, -halfSize.Y),
                        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libsimulation2.a(precompiled.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libscriptinterface.a(precompiled.o) has no symbols
../../../source/third_party/fmt/format.cpp:145:7: warning: '_POSIX_C_SOURCE' is not defined, evaluates to 0 [-Wundef]
#if ((_POSIX_C_SOURCE >= 200112L || _XOPEN_SOURCE >= 600) && !_GNU_SOURCE) || (defined(__ANDROID__) && __ANDROID__)
      ^
../../../source/third_party/fmt/format.cpp:145:37: warning: '_XOPEN_SOURCE' is not defined, evaluates to 0 [-Wundef]
#if ((_POSIX_C_SOURCE >= 200112L || _XOPEN_SOURCE >= 600) && !_GNU_SOURCE) || (defined(__ANDROID__) && __ANDROID__)
                                    ^
2 warnings generated.
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libengine.a(precompiled.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libgraphics.a(precompiled.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libatlas.a(precompiled.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libgui.a(precompiled.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/liblowlevel.a(dbghelp.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/liblowlevel.a(file_stats.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/liblowlevel.a(vfs_path.o) has no symbols
In file included from ../../../source/graphics/tests/test_Camera.cpp:17:
/Users/wfg/Jenkins/workspace/macos-differential/source/graphics/tests/test_Camera.h:168:4: warning: suggest braces around initialization of subobject [-Wmissing-braces]
                        CVector3D(-101.0f, -101.0f, 101.0f),
                        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.
In file included from ../../../source/simulation2/tests/test_SerializeTemplates.cpp:17:
/Users/wfg/Jenkins/workspace/macos-differential/source/simulation2/tests/test_SerializeTemplates.h:39:4: warning: suggest braces around initialization of subobject [-Wmissing-braces]
                        3, 0, 1, 4, 1, 5
                        ^~~~~~~~~~~~~~~~
                        {               }
1 warning generated.

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/1584/display/redirect

Different context == different translation

Good to know! I assume that also applies to no context? By the way, could you also add context to the <GenericName> of those templates, to allow consistency with <AttackName>?

You are right, also founda new type: kush_champ_amum has a Sickle. Also some heroes. 9 Singles on 51 swords now
EDIT: somehow feel Sickle is the wrong word, but can't find the correct one...

It's a khopesh, which is not a sickle-sword (i.e. pointing forward and sharp on the inside of the curve), but an axe-sword (i.e. pointing backwards and sharp on the outside of the curve). It's probably better to stick with ‘single-edged sword’, though.

bb added a comment.Sep 26 2020, 11:36 AM

Different context == different translation

Good to know! I assume that also applies to no context? By the way, could you also add context to the <GenericName> of those templates, to allow consistency with <AttackName>?

no context != some context, also usually strings without context ought to be translated with a translate call, not a translateWithContext call. And those are pulled from a different part of the dictionary (the without context part).
In theory we can add context in a similar way for any template node (including GenericName). The GenericName translation code flow is however slightly different than we have for AttackName, so it isn't completely straightforward. In any case this should be the subject of another revision.

bb updated this revision to Diff 13546.Sep 26 2020, 11:37 AM

No sickle

Freagarach added inline comments.Oct 16 2020, 1:54 PM
binaries/data/mods/public/simulation/components/Attack.js
443 ↗(On Diff #13546)

?

bb updated this revision to Diff 13639.Oct 16 2020, 7:57 PM
bb edited the test plan for this revision. (Show Details)

split object definition over several lines

  • Code looks good.
  • AttackName preferable over Weapon*.
  • Translations seem to be done properly.
  • (I would still prefer something like melee for animals, who tend to use a plethora of attack techniques, but certainly not something I would get annoyed over.)
binaries/data/mods/public/globalscripts/Templates.js
210 ↗(On Diff #13639)

This can be split over multiple lines as well. (Sorry that I didn't note that earlier.)

binaries/data/mods/public/l10n/messages.json
404 ↗(On Diff #13639)

Surroundings suggest using (four spaces).

binaries/data/mods/public/simulation/templates/template_unit_infantry_ranged_archer.xml
5 ↗(On Diff #13639)

(One _could_ use a bow in melee ;) )

bb updated this revision to Diff 13673.Oct 28 2020, 10:40 PM
bb marked 2 inline comments as done.
bb added inline comments.
binaries/data/mods/public/l10n/messages.json
404 ↗(On Diff #13639)

No idea why that is, would never do it myself, also the file is not entirely consistent....

binaries/data/mods/public/simulation/templates/template_unit_infantry_ranged_archer.xml
5 ↗(On Diff #13639)

In case we implement that, we always have the option to rename the strings, which looks like to be in the scope of that hypothetical patch

Successful build - Chance fights ever on the side of the prudent.

builderr-release-macos.txt
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libsimulation2.a(precompiled.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libgraphics.a(precompiled.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libatlas.a(precompiled.o) has no symbols

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/1683/display/redirect

Freagarach accepted this revision.Nov 5 2020, 1:11 PM

See above. If @Nescio doens't have any improvements I would say "merge this".

This revision is now accepted and ready to land.Nov 5 2020, 1:11 PM
Nescio added a comment.Nov 5 2020, 6:50 PM

First of all, I approve of the idea of this patch: defining attack names in the templates instead of hardcoding them in a gui js file is indeed an improvement.
However, I'm not exactly happy with the new strings. Although the old situation (capture, melee, ranged, slaughter) may not have been perfect, it was clear and consistent: a one-on-one correspondence between the attack type defined in the templates and the name displayed by the gui.
This patch proposes a plethora of particular terms which are essentially arbitrary. I fail to see why using tusks, teeth, claws, horns, axe, spear, etc. is intrinsically better than just melee. For each of them one could ask why exactly this particular string and not something else.
In my opinion it would be better to define the attack name in the template where the attack node is defined, and match the names with the types, i.e. stick with Melee and Ranged, at least for now.

binaries/data/mods/public/simulation/templates/gaia/fauna_rhinoceros_white.xml
5 ↗(On Diff #13546)

Shouldn't this be singular?

binaries/data/mods/public/simulation/templates/gaia/fauna_tiger.xml
5 ↗(On Diff #13673)

Or paws?

binaries/data/mods/public/simulation/templates/units/kush_infantry_clubman_b.xml
5 ↗(On Diff #13543)

Mace

One of the points of this patch is working towards D368. After that patch e.g. a spearman can have both a spear and a sword for attack and melee won't do then. I agree on the animal-terms being a bit weird (hence my proposition for melee there), but the rest _is_ an improvement IMHO.

My 2 cents on this:

  • I think this is actually somewhat moot, because we ought not put the name of the attack in the tooltip, but rather an icon(/something) for 'instant' or 'uses a projectile', since that's effectively the only difference. The name of the attack is trivia/lore/noise.
  • I don't mind the fancy names, in the DnD spirit of "I attack with my horn and deal 5 damage" being more fun than "I melee-attack and deal 5 damage".

There is inconsistent context for double-edged swords, it seems.

One of the points of this patch is working towards D368. After that patch e.g. a spearman can have both a spear and a sword for attack and melee won't do then.

Yes, I understand, but the crucial words are “after that patch” and “then”; different attack names can be introduced then; right now distinguishing between sword and spear is still meaningless.

I think this is actually somewhat moot, because we ought not put the name of the attack in the tooltip, but rather an icon(/something)

No objections, but when the cursor hovers over the icon, a string out to pop up, right?

I don't mind the fancy names

But what do they really add right now, apart from more work for translators?

Two other things:

  • If I understand correctly, what is displayed as <AttackName> + "Attack", which makes me wonder about pluralization; "Machine Gun Attack" or "Machine Guns Attack"?
  • As pointed out earlier, <AttackName>Melee</AttackName> ought to be defined in template_unit_infantry_melee.xml, not in skirmish/units/default_infantry_melee_b.xml; mutatis mutandis for Ranged.
  • If I understand correctly, what is displayed as <AttackName> + "Attack", which makes me wonder about pluralization; "Machine Gun Attack" or "Machine Guns Attack"?

Nay currently it is:

Attack:
  Capture <values>
  Machine Guns <values>

But what do they really add right now, apart from more work for translators?

I definitely agree with you on that. I'm willing to let this go, but imo right now the obvious solution is to remove the attack name from the tooltip, since it's hardly useful.

Why require an attack name string at all? (Is that what you meant, @wraitii?) Can't it be made optional? “Attack” is already displayed on the line above:

bb updated this revision to Diff 13782.Nov 7 2020, 9:31 PM
bb marked an inline comment as done.

In my opinion it would be better to define the attack name in the template where the attack node is defined, and match the names with the types, i.e. stick with Melee and Ranged, at least for now.

We can't use the node key directly, since that cannot be translated.

Yes, I understand, but the crucial words are “after that patch” and “then”; different attack names can be introduced then; right now distinguishing between sword and spear is still meaningless.

This patch is meant to shrink the size of the more complicated patch. Such that we don't have lots of template changes floating around while doing that. So yes this patch introduces some template behavior which is not completely reflected in the underlying code. Splitting patches almost always results in this.

As pointed out earlier, <AttackName>Melee</AttackName> ought to be defined in template_unit_infantry_melee.xml, not in skirmish/units/default_infantry_melee_b.xml; mutatis mutandis for Ranged.

There is no meaning in adding it in the parent, it will be replaced by all but one children and that child being the skirmish replace template. I did rather remove the attack node entirely from the parent. If you wish we can change the name Melee to Default Melee, if that is better.

In my opinion it would be better to define the attack name in the template where the attack node is defined, and match the names with the types, i.e. stick with Melee and Ranged, at least for now.

They add nice fancy names, which I would call a feature on its own

I definitely agree with you on that. I'm willing to let this go, but imo right now the obvious solution is to remove the attack name from the tooltip, since it's hardly useful.

Removing the name will leave two lines with some values, noone will understand what they mean.

Removing the name will leave two lines with some values, noone will understand what they mean.

One or two lines directly preceded by a line that says Attack:, therefore my guess is most people will correctly understand what is meant.

We can't use the node key directly, since that cannot be translated.

I meant inserting <AttackName>Melee</AttackName> in template_*_melee.xml and <AttackName>Ranged</AttackName> in template_*_ranged.xml

There is no meaning in adding it in the parent, it will be replaced by all but one children and that child being the skirmish replace template. I did rather remove the attack node entirely from the parent. If you wish we can change the name Melee to Default Melee, if that is better.

Those skirmish files are placeholders, they're not supposed to do anything, and they should not define any strings etc.
Having <AttackName> is now mandatory, causing errors if files lack them, the sensible thing is to insert it in the template_* that introduces the <Attack> node as a fallback. That they're replaced by practically all children is unimportant: most attack values defined in the template_*_melee.xml and template_*_ranged.xml are replaced by their children. Likewise, the template_* files have <GenericName> nodes, despite them being replaced too.
Another solution would be to simply deprecate that template step, since it doesn't do very much, and isn't present in the champion and hero templates either. D2959 does that for cavalry; a similar patch could be made for infantry.

They add nice fancy names, which I would call a feature on its own

While the new names work for soldiers, the strings chosen for structures, ships, and siege engines are at least odd, if not wrong.

binaries/data/mods/public/simulation/templates/template_unit_cavalry_melee_swordsman.xml
5 ↗(On Diff #13782)

The context is a proper sentence, therefore should end in a full stop.
Also applies to the other sword templates.

binaries/data/mods/public/simulation/templates/template_unit_cavalry_ranged_archer.xml
5 ↗(On Diff #13782)

Don't forget to insert Crossbow in mace_champion_infantry_crossbowman.xml (unless D2886 is committed first).

binaries/data/mods/public/simulation/templates/template_unit_infantry_ranged_slinger.xml
5 ↗(On Diff #13782)

Sling
A slingshot is a quite different weapon.

binaries/data/mods/public/simulation/templates/units/mace_champion_infantry_swordsman.xml
5 ↗(On Diff #13782)

Maybe add context here too? I doubt everyone knows what a rhomphaia is.

bb updated this revision to Diff 13863.Nov 11 2020, 11:26 PM
bb marked 3 inline comments as done.
bb added inline comments.
binaries/data/mods/public/simulation/templates/units/mace_champion_infantry_swordsman.xml
5 ↗(On Diff #13782)

Happy to add context wherever needed. However if a translator doesn't know an english word, it isn't the job of the template changer to fix that, rather the translator needing to look up the meaning. Also note that all entries without a custom context (one in the template), gets name of an attack as context.Do you think there still is a need for some more context? If so do you have some proposal? (Adding something like Attack using a rhomphaia. doesn't seem to add any information)

Successful build - Chance fights ever on the side of the prudent.

builderr-release-macos.txt
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libgraphics.a(precompiled.o) has no symbols

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/1841/display/redirect

Siege engine attack names are still problematic.
Also, slaughter attack is not actually displayed in the interface; does it still need an attack name? If so, “slaughter knife” seems unnecessary long; I'd prefer “slaughter” without knife, but could accept “knife” without slaughter. (Let's hope the prop actor gets replaced by an actual knife at some point: https://wildfiregames.com/forum/topic/29654-request-sacrificial-knife/ )

binaries/data/mods/public/simulation/templates/template_unit_ship_bireme.xml
5 ↗(On Diff #13417)

Hand-held crossbows weren't used in naval warfare in ancient times. Large crossbows (i.e. siege engines) were, but only on large Hellenistic wargalleys (i.e. quinquereme equivalents).
Bows and arrows were probably used by all; javelins, spears, pikes, and axes by many.
However, “bow” also means the front end of a ship.

binaries/data/mods/public/simulation/templates/template_unit_ship_trireme.xml
5 ↗(On Diff #13863)

See bireme.

binaries/data/mods/public/simulation/templates/units/mace_champion_infantry_swordsman.xml
5 ↗(On Diff #13782)

This particular weapon fell out of use before the English language existed. The point is “rhomphaia” is a technical term; you won't find it in an English dictionary. Therefore a bit of context could be helpful.
On the other hand, we don't provide context for specific names either. (Is that actually possible?)

bb added a comment.Nov 12 2020, 11:03 PM

Maybe I remove the slaughter thingy, since it is adding dead code here. I find it somewhat bad that it is not displayed. IMO it should just be displayed along side the other types (I actually do this in #252). Changing one template there won't hurt too much (especially since we need to change that template then anyway).

binaries/data/mods/public/simulation/templates/template_unit_ship_bireme.xml
5 ↗(On Diff #13417)

I guess the main weapons of ships would be rams, but we don't have them (yet). So we kindoff have to do with bolt shooting type of attacks (which probably are not entirely accurate, fixing this needs #4000 and #252). Can we get away with calling all these kind of weapons (so also some of the siege) ballista?

binaries/data/mods/public/simulation/templates/units/mace_champion_infantry_swordsman.xml
5 ↗(On Diff #13782)

Do you have some string we can use here? A rhomphaia is a a sword like weapon.?

Context on specific names can added very easily in a generic form (i.e, the same context for all templates): just add it in messages.json. Having the custom context like here, needs some code changes (similar to the ones here). In principle not too hard too do. Maybe make a ticket?

Maybe I remove the slaughter thingy, since it is adding dead code here. I find it somewhat bad that it is not displayed. IMO it should just be displayed along side the other types (I actually do this in #252).

No, slaughter attack shouldn't be displayed: it's applicable to only a handful of animals; moreover, the values are chosen such that it would result in an instant kill, thus the exact numbers are unimportant.

Can we get away with calling all these kind of weapons (so also some of the siege) ballista?

A ballist(r)a is a large torsion engine capable of hurling stones. A scorpio(n) is a small bolt-shooter (suitable for the 0 A.D.'s bolt tower). Machines firing larger bolts are called katapeltēs (Greek) or catapulta (Latin) without qualifier, though that word is also used as the generic term for all kinds of artillery; English “catapult” has quite a meaning nowadays, though, and thus ought to be avoided.
In my opinion it's best if artillery would simply used their projectile (i.e. Bolt or Stone) as the attack name. Maybe that's not entirely consistent with archers (it is with javelineers), but it's probably better than the other options raised so far.

Context on specific names can added very easily in a generic form (i.e, the same context for all templates): just add it in messages.json. Having the custom context like here, needs some code changes (similar to the ones here). In principle not too hard too do.

It would be quite useful to have that option for specific names; adding the polytonic Greek (e.g. θυρεοφόρος) could be a great help for translators, since there are various transliteration conventions. For instance, ῥομφαία (rhomphaia) is written rumpia in Latin, romfaia in Italian, and ромфея in Russian, to name just a few.

binaries/data/mods/public/simulation/templates/template_unit_ship_bireme.xml
5 ↗(On Diff #13417)

I guess the main weapons of ships would be rams, but we don't have them (yet).

Yes, ramming would be great to have, but that's not possible right now; it might be in the distant future, but that's irrelevant for this patch.

So we kindoff have to do with bolt shooting type of attacks (which probably are not entirely accurate,

Bolt-shooting is entirely wrong for most ships and civs; it's only justifiable for the quinqueremes, not for the triremes or biremes.
For instance, Athenian triremes and penteconters (“bireme”) had a fighting crew of ten Athenian hoplites (for boarding) and four Cretan archers each.
Since archery was a skill present throughout the world, and in 0 A.D. warships fire arrows, the bow is probably the most appropiate.

binaries/data/mods/public/simulation/templates/units/mace_champion_infantry_swordsman.xml
5 ↗(On Diff #13782)

A rhomphaia is actually a long blade on a short pole. One could say it's both a sword and a spear, or neither.
How about the following? A Thracian two-handed weapon capable of both thrusting and slashing.

the values are chosen such that it would result in an instant kill

(Annoyingly, that is quite wrong.)

bb added a comment.Nov 13 2020, 10:47 PM

Actually can't easily remove the slaughter thingy, since it is processed through the code. On the topic on whether it will be shown: #252 will abstract the notion of attacktype, so there won't be any difference between the attacktypes. Don't worry slaughter won't be able to instakill everything, we have restricted classes for that. Showing the restricted classes might be something to consider.

Rhomphaia judging from the argument for specificnames, we should show the greek original in the context.

bb updated this revision to Diff 13895.Nov 13 2020, 10:50 PM

comments

Successful build - Chance fights ever on the side of the prudent.

builderr-debug-macos.txt
../../../source/simulation2/scripting/JSInterface_Simulation.cpp:152:4: warning: suggest braces around initialization of subobject [-Wmissing-braces]
                        CFixedVector2D(-halfSize.X, -halfSize.Y),
                        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.
In file included from ../../../source/graphics/tests/test_Camera.cpp:17:
/Users/wfg/Jenkins/workspace/macos-differential/source/graphics/tests/test_Camera.h:168:4: warning: suggest braces around initialization of subobject [-Wmissing-braces]
                        CVector3D(-101.0f, -101.0f, 101.0f),
                        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.
In file included from ../../../source/simulation2/tests/test_SerializeTemplates.cpp:17:
/Users/wfg/Jenkins/workspace/macos-differential/source/simulation2/tests/test_SerializeTemplates.h:39:4: warning: suggest braces around initialization of subobject [-Wmissing-braces]
                        3, 0, 1, 4, 1, 5
                        ^~~~~~~~~~~~~~~~
                        {               }
1 warning generated.
builderr-release-macos.txt
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libnetwork.a(precompiled.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/liblobby.a(precompiled.o) has no symbols
../../../source/simulation2/scripting/JSInterface_Simulation.cpp:152:4: warning: suggest braces around initialization of subobject [-Wmissing-braces]
                        CFixedVector2D(-halfSize.X, -halfSize.Y),
                        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libsimulation2.a(precompiled.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libscriptinterface.a(precompiled.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libengine.a(precompiled.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libgraphics.a(precompiled.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libatlas.a(precompiled.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libgui.a(precompiled.o) has no symbols
In file included from ../../../source/graphics/tests/test_Camera.cpp:17:
/Users/wfg/Jenkins/workspace/macos-differential/source/graphics/tests/test_Camera.h:168:4: warning: suggest braces around initialization of subobject [-Wmissing-braces]
                        CVector3D(-101.0f, -101.0f, 101.0f),
                        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.
In file included from ../../../source/simulation2/tests/test_SerializeTemplates.cpp:17:
/Users/wfg/Jenkins/workspace/macos-differential/source/simulation2/tests/test_SerializeTemplates.h:39:4: warning: suggest braces around initialization of subobject [-Wmissing-braces]
                        3, 0, 1, 4, 1, 5
                        ^~~~~~~~~~~~~~~~
                        {               }
1 warning generated.

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/1865/display/redirect

The proposed attack name contexts are a bit verbose; wouldn't just “A single-edged sword.” or “Attack using a single-edged sword.” instead of “Name of an attack using a single-edged sword.” be sufficiently clear?

Rhomphaia judging from the argument for specificnames, we should show the greek original in the context.

Though context isn't (yet) shown for specific names.

binaries/data/mods/public/simulation/templates/skirmish/units/default_infantry_melee_b.xml
3–7 ↗(On Diff #13413)

I'm still not happy about this, but I suppose I can't convince you.

binaries/data/mods/public/simulation/templates/template_structure_defensive_tower_artillery.xml
5 ↗(On Diff #13487)

Stone without Ball would be correct too.

binaries/data/mods/public/simulation/templates/units/mace_champion_infantry_swordsman.xml
5 ↗(On Diff #13895)

Maybe place the Greek inside parentheses? ... a rhomphaia (ῥομφαία), a Thracian ...

bb updated this revision to Diff 13929.Nov 14 2020, 11:20 PM
bb marked 2 inline comments as done.

Comments

bb added a comment.Nov 14 2020, 11:20 PM

The proposed attack name contexts are a bit verbose; wouldn't just “A single-edged sword.” or “Attack using a single-edged sword.” instead of “Name of an attack using a single-edged sword.” be sufficiently clear?

I rather be too much verbose than to less in this case. We actually want some identifier for the way of attacking, not necessarily the weapon (“A single-edged sword.”) or the act of attacking (“Attack using a single-edged sword.”) and I wouldn't be surprised if there were languages who would make a difference between this.

Successful build - Chance fights ever on the side of the prudent.

builderr-release-macos.txt
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libengine.a(precompiled.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libatlas.a(precompiled.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libgui.a(precompiled.o) has no symbols

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/1893/display/redirect

Another question: does a string have to be retranslated if the context is changed?

Nescio retitled this revision from Properly internationalize attack names/Generlize to arbitrary names in the GUI to Introduce translatable attack names.Nov 15 2020, 9:27 AM
bb added a comment.Nov 15 2020, 2:25 PM

Another question: does a string have to be retranslated if the context is changed?

Yes. Transifex does offer the feature to suggest translations of similar already translated strings, which will help the translator in this case.

That's a bit annoying. Also, if I understand correctly, those attack names without an explicitly defined context get a default context defined in the tooltip.js file. So if that string or line is edited, do all those attack names have to be retranslated then too?

bb added a comment.Nov 16 2020, 12:54 PM

Annoying indeed, but it is how getText works...
To be more precise: the default context is defined in the messages.json file, the tooltip.js string indeed needs to match this string (that way we can retrieve the correct string). Therefore changing the tooltip.js line doesn't cause any retranslations. Changing the string only in tooltips.js, will result in non translated strings. Changing the string in the messages.json, WILL cause retranslations (and to see the translation ingame one also needs to change the tooltips.js string). I hope this is somehow clear...

Thank you for the clarification.
I suppose there is no alternative approach possible that would not force such retranslations?

bb added a comment.Nov 17 2020, 11:05 AM

Indeed, especially since a different context can mean a different translations.

Nescio accepted this revision.Nov 18 2020, 7:24 PM

Concerns I have:

  • the names are essentially arbitrary, more based on the appearance of an actor prop and animation than anything else
  • <AttackName> is mandatory, not optional; omitting it causes errors
  • strings are added in the default skirmish placeholders
  • changing context forces retranslation

Nonetheless, this patch (allowing attack names to be translated) is an improvement over the status quo (hard coded attack names that are not translated). Furthermore, committing it would probably make it easier to have entities with an optional number of arbitrary attacks at some point in the future.
The patch is complete and correct, applying the patch and going through entity lists in Atlas or the structure tree does not trigger any errors or warnings. Doing a grep in the templates folder also confirms its complete. As for the proposed strings, I went through the changes a few more times and did not find any that needs immediate replacement. While not perfect, they're good enough for now, and if someone has a better suggestion, they could be altered individually, of course. Moreover, I went through the unit list in Atlas again and can confirm that all swordsman exceptions are encountered for.
To summarize, the patch is an improvement, works, is correct, and is complete, therefore I'm accepting it.

bb added a comment.Nov 18 2020, 9:41 PM

Big thank you, @Nescio, your comments and questions have improved the code and strings tremendously.

This revision was automatically updated to reflect the committed changes.