boformer / CitiesHarmony

Harmony 2.x assembly provider mod for Cities: Skylines
MIT License
73 stars 17 forks source link

Make sure that EnsureHarmonyInstalled/DoOnHarmonyReady are not called from ILoadingExtension, only from OnEnabled #5

Closed boformer closed 1 year ago

drok commented 3 years ago

I would like to know why. What can happen if they are called from ILoadingExtension.

I have a mod where I use proper locks between threads and install patches as the game is running, so that game data is not corrupted. It works very well, and the mod can be added and removed to the game live, during running simulation.

I would like to know what the concern is.

boformer commented 3 years ago

The original reason was that auto-subscribing to mods while a city is loading is scary. There is no guarantee how long it takes, and some mods might require their Harmony patches to be in place before certain methods of the game are called (e.g. InitializePrefab).

Now that we moved away from auto-subscribing CitiesHarmony and showing a Yes/No prompt to the user instead, it is even more important that it is done in main menu.

You can still apply your patches whenever you want, even ingame. Just use HarmonyHelper.IsHarmonyInstalled to check if Harmony is available before calling any methods that reference HarmonyLib stuff.

I will close this issue. It is clearly mentioned in the README when developers should call what.

We are not restricting developers to call methods from LoadingExtension if they need to, but it's not recommended.

drok commented 3 years ago

Thanks for the explanation.

I strongly disagree that Harmony should be called at all from OnEnable(). In fact calling it there is what's causing the loading order problems you're having today.

The game calls OnEnable() for each enumerated mod in turn, ie, it is guaranteed, except for the last mod, that all precedent mods get OnEnable() before all mods have been OnEnable(), so it's the wrong place to check for compatibility with other mods, or to check if a pre-requisite mod is installed and enabled. Pre-requisites cannot be ascertained at OnEnable() time.

Also, OnEnable() is called at the main menu typically, or whenever the mod is loaded, which can be mid-game if the user subscribes/unsubscribes in the middle of a game. At the main menu, you obviously cannot tell what AppMode the user will select (Game, AssetEditor, etc), so it's the wrong place to install hooks.

Typically OnEnable() should be used to check the app's version, and to override its own enable state as required, but it's too early to interact with the game or other mods.

On the other hand, when the user subscribes to a mod mid game, the chance to install patches before InitializePrefab() is long gone, a mod should not assume that OnEnable() means it can hook InitializePrefab().

Furthermore, ILoadingExtension.OnCreated() is called after all mods have been enumerated, their enable/disable state is known, and their IUserMod classes have been instantiated. That is the first opportunity the mod has to check if it's compatible with the other mods, or if some other pre-requisite mod is present (and connect with it its plugin instance reference). This is also the first opportunity to know what AppMode the app is in (game, asset editor, etc), and thus, whether the mod should activate or not.

ILoadingExtension.OnCreated() is typically the earliest time a well designed mod can sanely call Harmony to install patches. If all mods did this, there would not be a 0Harmony.dll loading order problem.

Of course, depending on needs, some mods may install patches even later, at 'ThreadingExtension.OnCreated()' time, or even later, at 'ILoadingExtension.OnLevelLoaded()', or even later yet, for mods that replace a SimulationManager, possibly in the Start() Monobehaviour method of their replacement manager.

I must say, I believe the advice and examples to install patches in OnEnable() is misguided, and should be reversed.

EDIT: There is no guarantee how long auto-subscribing will take no matter where you call it. The internet connection may be down when it's called and it could take a really long time, up to forever. The argument that subscribing from the main menu is a good idea just because the subscription is likely to arrive in time does not hold much water. Also, a mod, or a collection, may be subscribed mid game. Popping many "would you like to subscribe" windows, or even one, at that time, is a poor design choice. If the player installs a mod mid game, the Harmony subscription should be silent and immediate. There is no practical scenario in which the user would answer "no" that I can imagine.