Improving the ndmf.localization adoption experience

[enitimeago]

I went through a fair amount of trial-and-error to figure out how to adopt ndmf.localization and resolve various issues I've run into with it so far, but I'm having a hard time coming up with a concrete "issue" to describe what happened.

I'll try and summarize what happened and write out my experience, in case there might be some useful takeaways at all?

Wasn't sure if this should be filed under Issues or Discussions here, feel free to convert to an issue if that makes more sense?

Happy to try and expand on anything that's unclear here.

Long story short

In summary, I think I ran into:

• A non-NDMF related issue that AssetDatabase.LoadAssetAtPath<LocalizationAsset> can unexpectedly return null when called (too early?) due to [InitializeOnLoad]
  • This doesn't make sense to me though since Modular Avatar uses [InitializeOnLoad] and works fine.
• Subsequently passing these null localization assets inside ndmf.localization.Localizer's assetLoader callback will cause a null pointer exception inside NDMF at

  ndmf/Editor/UI/Localization/Localizer.cs

  Lines 63 to 71 in 1465ba0

  	return assetLoader().Select<
  	LocalizationAsset,
  	(string, Func<string, string>)
  	>(asset => (asset.localeIsoCode, k =>
  	{
  	var s = asset.GetLocalizedString(k);
  	if (s == k) s = null;
  	return s;
  	})).ToList();

The long story

Looking at Modular Avatar's Editor/Localization/Localization.cs, its Localization class is annotated with [InitializeOnLoad] and the comment "ensure that we register languages with NDMF on domain load".

I tried adding the same attribute when I created my Localizer class, which constructs an NDMF Localizer inside its static constructor:

    [InitializeOnLoad]
    internal static class Localization
    {
        static Localization()
        {
            _localizationAssets = Directory.GetFiles("Packages/enitimeago.non-destructive-mmd/Localization")
                .Select(AssetDatabase.LoadAssetAtPath<LocalizationAsset>)
                .Where(asset => asset != null)
                .OrderBy(asset => asset.localeIsoCode)
                .ToList();
            /* ... do other things ... */
            Localizer = new Localizer("en-US", () => _localizationAssets);
            /* ... do other things ... */
        }

This turned out to cause my package to fail to load localizations when imported into an open Unity project, until I right-clicked the project and clicked Reimport.

Not understanding the root cause, but observing Modular Avatar's, NDMF's, Avatar Optimizer's localization setups were using GUIDs instead of hardcoded package paths, I thought I'd try doing the same:

    [InitializeOnLoad]
    internal static class Localization
    {
        static Localization()
        {
            string localizationDir = AssetDatabase.GUIDToAssetPath("...removed guid...");
            string[] localizationDirFiles = Directory.GetFiles(localizationDir);
            if (localizationDirFiles.Length == 0)
            {
                Debug.LogError($"Translation files not found! Tried searching {localizationDir}");
                return;
            }
            _localizationAssets = localizationDirFiles
                .Select(AssetDatabase.LoadAssetAtPath<LocalizationAsset>)
                .Where(asset => asset != null)
                .OrderBy(asset => asset.localeIsoCode)
                .ToList();
            if (_localizationAssets.Count == 0)
            {
                Debug.LogError("Translation files failed to be loaded!");
                return;
            }
            /* ... do other things ... */
            Localizer = new Localizer("en-US", () => _localizationAssets);
            /* ... do other things ... */
        }

Having done so I observed "Translation files failed to be loaded!" in Unity's log right after importing the package.

Further fruitless rewrites landed me with this:

    [InitializeOnLoad]
    internal static class Localization
    {
        /* ... other stuff ... */
        private static Locale[] _locales = new Locale[]
        {
            new Locale { IsoCode = "en-us" },
            new Locale { IsoCode = "ja-jp" },
        };
        static Localization()
        {
            Localizer = new Localizer("en-us", () =>
                _locales.Select(locale =>
                {
                    if (locale.Asset == null)
                    {
                        string localizationDir = AssetDatabase.GUIDToAssetPath("...removed guid...");
                        locale.Asset = AssetDatabase.LoadAssetAtPath<LocalizationAsset>($"{localizationDir}/{locale.IsoCode}.po");
                        if (locale.Asset != null)
                        {
                            Debug.Log($"Successfully loaded LocalizationAsset for {locale.IsoCode}");
                        }
                        else
                        {
                            Debug.LogError($"Failed to load LocalizationAsset for {locale.IsoCode}");
                        }
                    }
                    return locale.Asset;
                })
                .Where(asset => asset != null)
                .ToList());
            LanguagePrefs.RegisterLanguageChangeCallback(typeof(Localization), _ => OnLanguageChange());
        }

And I'd see "Failed to load LocalizationAsset for en-us" "Failed to load LocalizationAsset for ja-jp" in my logs.

Accidentally leaving out .Where(asset => asset != null) caused a null pointer exception inside NDMF.

I tried some other things that didn't seem to make any effect:

• The localization files were previously in a dedicated "localization" assembly, so I tried creating an AssemblyInfo.cs inside it
• I tried merging the localization files from the dedicated localization assembly (packageRoot/Localizations/.po), to a subfolder of the "editor" assembly (packageRoot/Editor/Localizations/.po)
• My editor assembly had autoReferenced: true and I changed that to false

Finally, I tried removing [InitializeOnLoad], and that allowed localizations to be successfully loaded when the package was imported.

👉 Question: How is Modular Avatar's use of [InitializeOnLoad] different? A cursory look at Modular Avatar shows that it also seems to reference localization files in its static initializer, albeit json files not po files? https://github.com/bdunderscore/modular-avatar/blob/8e6d8302ef5d6899f152d56649c81a8db3e38046/Editor/Localization/Localization.cs#L55

👉 Question: What's the "correct" time to be loading localization files? I feel like my final rewrite here, despite the rewrite probably being unnecessary for localizations to "work", is the cleanest approach i.e. only try to load localization files when NDMF asks for them.

👉 Question: Should docs recommend searching localization files using GUIDs or hardcoded paths? I would have guessed GUIDs, but changing to GUIDs bit me when I tried moving my localizations from the dedicated localization assembly to editor assembly subfolder. Crucially I let Unity move the .meta file (and therefore GUID) of the assembly to inside the editor assembly. (In other words packageRoot/Localizations.meta got moved to packageRoot/Editor/Localizations.meta.) In-place upgrading my package caused localizations to break with "Translation files not found!", and I had to resolve this by creating a new GUID for the Localizations folder.

👉 Question: Should docs recommend a language code capitalization? i.e. "en-US" vs "en-us"

👉 Question: Should NDMF be catching null assets being returned by the asset loader? Anything else that can be implemented in NDMF that can help stop people making similar mistakes to me? :)

[bdunderscore]

Your issues with loading assets from static constructors may be caused by static constructors being run off the main thread, where loading assets isn't allowed, or running before the asset database is initialized. For this reason it can be better to use [InitializeOnLoadMethod] instead for these purposes.

1. MA's use is different as it's not loading assets, only JSON files (MA started with an independent implementation, before I discovered that Unity understands .po files). A better example is https://github.com/bdunderscore/ndmf/blob/main/Editor/UI/Localization/NDMFLocales.cs#L9 . Note that this only accesses assets from the loader callback; the callback should only be invoked in the context of UI code, so it should always run safely on the main thread.
2. Yes, loading when requested is safest in general.
3. Not sure how that broke for you... in general folder GUIDs seem more likely to be merged (e.g. I don't think importing a unitypackage will complain if you have a new GUID for an existing folder). If you're building a package there's not too much harm to using paths.
4. Unfortunately there's already some inconsistencies here, so I've just decided to make things case-insensitive. It looks like there are some bugs around that still that I need to address however (Fix language code not recognizable due to inconsistent case #178)
5. Yeah, that could use a check. It might also be good to have an API to just give it a folder and automatically enumerate translation files.

[enitimeago]

Thanks for the explanations! It sounds like I've been partially tripped up by my lack of understanding of Unity Editor's threading model.

Your issues with loading assets from static constructors may be caused by static constructors being run off the main thread, where loading assets isn't allowed

To clarify, did you mean to write that the static constructor runs off not the main thread? With some cursory searching I couldn't find anything other than https://stackoverflow.com/questions/58729409/unity-calling-resources-load-from-a-different-thread which suggests that assets are only loadable on the main thread?

Not sure how that broke for you... in general folder GUIDs seem more likely to be merged (e.g. I don't think importing a unitypackage will complain if you have a new GUID for an existing folder). If you're building a package there's not too much harm to using paths.

Yeah, I was hoping that Unity would handle the move better. I suspected something was wrong when I did the in-place upgrade and spotted the old "Localization" folder was still hanging around (but empty) in addition to the new "Editor/Localization" folder where I moved the .po files to.

Unfortunately there's already some inconsistencies here, so I've just decided to make things case-insensitive.

🥲

Yeah, that could use a check. It might also be good to have an API to just give it a folder and automatically enumerate translation files.

Sounds good. I'm trying to get my package out the door as fast as I can first, but I'm happy to send a PR for this if I end up having the spare time for this before you.