Material system does not have full internationalization support

[3TUSK]

Synopsis

Items created by using the Material System display a name in mixed language, one of them being English, which is the material name, and the other being the part name, which respects locale setting of Minecraft.

Rationality

Some of modpack developers may use this system to add customized contents to their modpack. The most obvious example would be "SevTech: Ages". With existence of this issue, it is impossible to provide full localization support for the modpack, causing trouble for non-English-speaking players. In fact, this issue is discovered when @TartaricAcid attempted to translate contents in "SevTech: Ages".

Environment

Dev environment; HEAD is 172fccbf02adbea949647e89788f35cbdbfd240e; with mapping being changed to snapshot_20180504.

The script used here is provided below.

demo.zs

```` #loader contenttweaker import mods.contenttweaker.MaterialSystem; import mods.contenttweaker.Material; var copper = MaterialSystem.getMaterialBuilder().setName("Copper").setColor(15766817).build(); var tin = MaterialSystem.getMaterialBuilder().setName("Tin").setColor(10275286).build(); var silver = MaterialSystem.getMaterialBuilder().setName("Silver").setColor(15592941).build(); var lead = MaterialSystem.getMaterialBuilder().setName("Lead").setColor(5658219).build(); var cobalt = MaterialSystem.getMaterialBuilder().setName("Cobalt").setColor(18347).build(); var metal_list = [copper, tin, silver, lead, cobalt] as Material[]; var part_names = ["dust", "gear", "plate", "nugget", "ingot", "beam", "bolt"] as string[]; for i, metal in metal_list { metal.registerParts(part_names); } ````

Screenshot

Analysis

The localized name is provided by MaterialPart::getLocalizedName from B.A.S.E.:

public String getLocalizedName() {
    //noinspection deprecation
    return I18n.translateToLocalFormatted(part.getUnlocalizedName(), material.getName());
}

Notice that the parameter is simply Material::getName, which discards the existence of Material::getUnlocalizedName. Within ContentTweaker, the property name is set by CTMaterialBuilder::setName, which is called in the script. Thus, it is impossible to display a fully localized name.

Proposed fix

As the property name in Material is also used as internal name for registration purpose, changing return value of Material::getName is definitely not feasible.
The most obvious solution is to change MaterialPart::getLocalizedName to something equivalent to this, which would fall under B.A.S.E., instead of ContentTweaker:

public String getLocalizedName() {
    //noinspection deprecation
    String materialDisplayName = I18n.translateToLocalFormatted(material.getUnlocalizedName());
    return I18n.translateToLocalFormatted(part.getUnlocalizedName(), materialDisplayName);
}

Alternatively, it is possible to utilize MaterialPart::getUnlocalizedName.
The underlying issue with the proposed fix is the potential translation key conflict with other mods, as return value of Material::getUnlocalizedName is initialized as following in constructor of Material:

this.unlocalizedName = TextUtils.toSnakeCase(name);

That said, if there is such Material instance:

var copper = MaterialSystem.getMaterialBuilder().setName("Copper").setColor(15766817).build();

Then its unlocalizedName would simply be copper, which, though most unlikely, may duplicate entries from other mod(s).
To address that, it is possible to change Material::getUnlocalizedName to something equivalent to the following:

public String getUnlocalizedName() {
    return "base.material." + unlocalizedName;
}

which also falls under B.A.S.E., given the fact that CTMaterial is simply a thin wrapper of Material.

Final thoughts

If possible, I can make a pull request to address this issue.

[SkySom]

Honestly the biggest issue here, isn't necessarily the difficulty of switching to such a system. It's more the fact that I have to figure out how best to support the current way of letting it happen. Switching entirely over to unlocalized names would break any packs that don't have the localization.

[3TUSK]

Maybe change to the following:

public String getLocalizedName() {
    //noinspection deprecation
    if (I18n.canTranslate(material.getUnlocalizedName()) { // func_94522_b <=> canTranslate
        return I18n.translateToLocalFormatted(part.getUnlocalizedName(), I18n.translateToLocalFormatted(material.getUnlocalizedName()));
    }
    return I18n.translateToLocalFormatted(part.getUnlocalizedName(), material.getName());
}

Thoughts?

[SkySom]

That would be workable, as long as I18n.canTranslate isn't an expensive call.

[3TUSK]

I18n.canTranslate is essentially a HashMap::containsKey. (func_94522_b delegates to func_94520_b (LanguageMap::isKeyTranslated), func_94520_b only has a HashMap::containsKey call). Cost should be ignorable (O(1) time complexity).