[BUG] Mod not working on entering non initial world

[DoctorVanGogh]

Describe the Bug: If you start the client, enter a world, leave that world and then enter a world again, the mod wont work.

To Reproduce:

1. Start game
2. Enter singleplayer world (any)
3. Leave world
4. Enter singleplayer world (any)

Expected Behaviour: The mod should actually work on non initial worlds ;)

Additional Context I poked around a tiny bit. Game log gives

[Error] [campaigncartographer] An exception was thrown when trying to start the mod:
System.NullReferenceException: Object reference not set to an instance of an object.
   at ApacheTech.Common.DependencyInjection.Abstractions.ServiceDescriptor..ctor(Type serviceType, Object implementation, ServiceLifetime lifetime)
   at Gantry.Core.DependencyInjection.Extensions.HostExtensions.AddModSystems(IServiceCollection services)
   at Gantry.Core.DependencyInjection.ModHost.BuildClientHost(ICoreClientAPI capi)
   at Gantry.Core.DependencyInjection.ModHost.StartPreUniversal(ICoreAPI api)
   at Gantry.Core.ModSystems.Generic.ModSystemBase.StartPre(ICoreAPI api)

as the first exception, then several other errors for the mod.

Debugger suggests something amiss in the game mod systems records. ApacheTech.VintageMods.CampaignCartographer.Services.ModMenu.ModMenu seems to have gone missing here. Predictably, ServiceDescriptor constructor then barfs and things start to tumble from there.

This may be a cause for #32. Everything works fine if you just launch a world, then quit the client. Things dont work if you leave the world, but dont close the client.

[ApacheTech]

I'm assuming this was with CC version 3.0.3, on game version 1.17.9?

It's odd, because this bug was initially a problem with the vanilla game. They set a static value at first launch, and didn't unset it on log-out, so subsequent logins weren't synchronised properly. But, I spent a long time with the devs hunting this bug down, and fixing it.

That then led to a bug being uncovered within Gantry, that was masked by the bug in vanilla, which had a similar effect on my mods. However, that has now been fixed as well.

I've tweaked the Execute Order of the mods systems slightly, which should help with the synchronisation. It seems like the features are trying to pull the ModMenu class from the IOC before it's been instantiated. And it's the game that instantiates the ModSystems; that much, I have no control over.

Try with v3.1.0, and let me know if you get the same results.

[DoctorVanGogh]

CC version 3.0.3, on game version 1.17.9

Yes, sorry for not giving the version numbers. My bad. Sadly, no change with 3.10.

But I think I found the issue:

• ApacheTech.VintageMods.CampaignCartographer.Services.ModMenu.ModMenu is not started for the server side - I did not look into what the system actually does, but if the name is appropriate, then that is quite reasonable.
• ModLoader.RunModPhase(ref List<ModSystem> enabledSystems, ModRunPhase phase) will replace its enabledSystems list with the list of all "started" systems: Note how ModMenu is missing :
• ApiEx.Current.ModLoader will have a sticky reference to the server side ModLoader from the first loaded world. Which... after entering the world will no longer have ModMenuas enabled.
• Now the second load comes along and Gantry.Core.DependencyInjection.Extensions.HostExtensions.AddModSystems tries to look up the ModMenu system using the old ModLoader from the first world load. Where that system is now disabled. And won't be found 😉

Execution sequence:

World load 1

Gantry.Core.DependencyInjection.Extensions.HostExtensions.AddModSystems

ModLoader.RunModPhase(ref List<ModSystem> enabledSystems, ModRunPhase phase)

Note ModLoaderis 0x01DBF9A6. And the ModMenu system is now disabled 😆

World load 2

Gantry.Core.DependencyInjection.Extensions.HostExtensions.AddModSystems

barfs here

ModLoader.RunModPhase(ref List<ModSystem> enabledSystems, ModRunPhase phase)

Note how the ModLoader for every world entry is different, but you keep reusing the first one. Which has things flagged as disabled.

[ApacheTech]

ModMenu is purely UX, so doesn't need to be loaded on the Server side at all.

I don't understand how it's using the "old" ModLoader. Is this a vanilla bug? My code-flow seems to be accurate enough.

I've removed the reflection aspect completely, and I'm now using the list publicly available within the Mod instance. I recently exposed this within my ModEx helper class, so I have it available within the static context now. It should be refreshed each launch.

ApacheTech.VintageMods.CampaignCartographer_v3.1.1.zip

Before I publish this, give it a try, and see if it helps.

Thank you for all this, it's been a great help.

[DoctorVanGogh]

First things first:

Before I publish this, give it a try, and see if it helps.

Kinda... but also ... not. The exceptions are gone. But things now "just dont work" silently on non-initial game world entry :rofl: Waypoint editing dialog is the vanilla one, missing lots of colors. .wpcommand just doesnt work.

So 👍 on the not barfing, 👎 on the actually solving the issue front.

I don't understand how it's using the "old" ModLoader. Is this a vanilla bug? My code-flow seems to be accurate enough.

Well, it looks like a bug in Gantry to me. So might have to move this issue over there.

https://github.com/ApacheTech-Nuget-Packages/VintageStory.Gantry/blob/aacf0e4722ebd7ee963afbdb926b89cb6b81c392/src/Gantry.Core/ApiEx.cs#L30-L44

This seems to be called from StartPre. And it will only ever initialize ApiEx's Server & Client static properties to the first values it gets. Which is wrong. Object lifetime issues strike again. :wink:

There can be many many ICoreServerAPI & ICoreClientAPI instances during the lifetime of the application. So can't use static.

A lot of subtle issues may be hiding under that static reference. In theory every single use of that property is using potentially outdated objects on non first worlds.

This looks like a nasty issue to fix to me. Since moving away from a static property here would be quite the breaking change. And solving this while keeping that static sounds like an "interesting problem".

---

Just some random ideas for that problem:

• Might take a look at how Microsoft solved their async task states with AsyncLocal & ExecutionContext. They solve a structurally somewhat similar problem.
• Might child containers per server/client lifetime and resolving everything from there make sense? You've already lifted an entire DI framework in there. Might as well make use of it.

Sounds fun 😆

[ApacheTech]

Ok... I've spent the last couple of days working through all of this, piece by piece. There was a lot to unpack. But, I think I'm there, now.

I cannot move away from a static context with this. Harmony only works in a static context, with no way of injecting instanced members. That's why ApiEx exists. There is no way to make mods like this, without being able to access the API within a static context. The game doesn't make it easy to work with though.

But there is only one instance of ClientMain, and ServerMain within the lifetime of a client being logged into a world. These are the instances that are used. Both of these have an instance of CoreClientAPI, and CoreServerAPI, respectively. On the client, the singleton ICoreClientAPI is instantiated within the constructor of ClientMain. On the server, the ICoreServerAPI is instantiated within ServerSystemModHandler, for some reason. But, in both cases, disposing the API will result in shutting down the game, on that app-side. Disposing early would crash the client/server. They are both captured dependencies of their owner classes.

So, to ensure we're grabbing the captured dependency, I should populate ApiEx.ClientMain and ApiEx.ServerMain within the Initialise method, and then take ApiEx.Client and ApiEx.Server from the captured dependencies. They can only ever be singleton for the lifetime of the server/client. They can't ever be transient.

        internal static void Initialise(ICoreAPI api)
        {
            switch (api.Side)
            {
                case EnumAppSide.Server:
                    ServerMain = api.World as ServerMain;
                    break;
                case EnumAppSide.Client:
                    ClientMain = api.World as ClientMain;
                    break;
                case EnumAppSide.Universal:
                default:
                    throw new ArgumentOutOfRangeException(nameof(api), api, "App-side cannot be determined.");
            }
        }

        public static ICoreClientAPI Client => ClientMain.Api as ICoreClientAPI;

        public static ICoreServerAPI Server => ServerMain.Api as ICoreServerAPI;

I've also removed the null-coalescing assignment operator. It was there, for the sake of performance, more than anything. You can have multiple classes inherit from ModSystemBase, so I didn't want to populate the properties for each mod system in the mod. However, this brings around a whole other issue with needing to nullify the orphans, once you log out.

There was also a major flaw within my file handling service, which I have now fixed. However, for this bug, the biggest problem was down to a really, really, really stupid rookie error. Within Campaign Cartographer, I override the Dispose() method for my ModHost, so I can dispose of my custom chat commands. However, I didn't then add in the base.Dispose(); to bubble down to the host. This, in turn, meant that none of the Gantry stuff was disposed of; including everything within the IOC container; all the paths for that specific world; settings; everything. I need to write an analyser for that, really.

https://github.com/ApacheTech-VintageStory-Mods/ApacheTech.VintageMods.CampaignCartographer/blob/master/src/ApacheTech.VintageMods.CampaignCartographer/Program.cs#L84

So, I've released new versions of everything, from the common nuget packages, through to the whole of Gantry. Just to make sure I'm starting off now on a version that has been finely picked over.

Thank you for all your hard work, and diligence with this. Such a simple fix, for a crazy problem. But, a lot of the things I've put in place will help in future. I've gone wildly over-cautious. A lot of this will hopefully be fixed when the game is ported to .NET7. We'll be using an AssemblyLoadContext to load mods, so when a user logs out, the libraries will be fully freed, and unloaded. Until then, we have to jury rig stuff like this. Not fun.

Once again, if I can ask you to check this version out. It's working as intended now, for me. I've tried rapidly changing between different worlds; running multiple processes in different worlds; local LAN games from single-player, and from VintagestoryServer. It all shows the right settings for the right worlds, and the mod works as intended in each world.

ApacheTech.VintageMods.CampaignCartographer_v3.1.2.zip

[NominalHero0]

Hi Apache, I'm a bit of a layman compared to DoctorVanGogh but, I was following along on this bug and, having read your last post decided to give 3.1.2 a try. Initial testing looks good for expected functionality; I've swapped between SP and MP worlds a few times without any of the previous issues appearing.

Hopefully, as a data point, this is helpful to you.

I appreciate your dedication to this mod - its hands down one of the most useful mods for VS. Let me know if I can help test for anything specific on this version.

[DoctorVanGogh]

Glad you could sort things out.

Using a non-coalescing assignment is functionally identical to my stack+top approach. So it should work fine 🤞
Important point was that not just Current was fixed, but also other references like Client & Server. Which they now are. So things should work.

But I'll have to ask, what do you mean by

I cannot move away from a static context with this. Harmony only works in a static context, with no way of injecting instanced members.

I've been around, making use of Harmony since 1.0. It has always been able to muck around in both static & instance methods. After all, an instance method is just a static method with a hidden, implicit class-typed parameter called this :wink: (Ignoring the whole virtual aspect, which just means you need to patch the right vtable). So maybe I'm misunderstanding you here.

[ApacheTech]

Harmony patches are written within static methods, like this:

https://github.com/ApacheTech-VintageStory-Mods/ApacheTech.VintageMods.CampaignCartographer/blob/master/src/ApacheTech.VintageMods.CampaignCartographer/Features/AutoWaypoints/Patches/AutoWaypointsPatches.GuiDialogTrader.cs

But, there's no way to guarantee that the class you're patching will have access to the API. So, you're in a position where you need an API, but you don't even know which app-side your code is currently running on. It could be either. I'm running on the opinion that in most cases, having separate logic for client and server, would break DRY. This isn't always the case, but as a rule of thumb. So, I made ApiEx, based on Novocain's original AppSideAnywhere script.

https://github.com/Novocain1/MiscMods/blob/1.15/VSHUD/Utility/CheckAppSideAnywhere.cs

But, with that, it caused huge amounts of lag when performing actions such as chopping down trees; where the API is called multiple times per leaf, and per log that is affected by the action. The game uses a non-mementoised recursive algorithm to determine which blocks should be affected. In earlier versions of CC, the client would hang for upwards of 10 seconds, when you cut a Redwood tree down.

As for injected instances; I really meant that you can only inject into a patch, things that are included within the class you are patching. If the class doesn't have an API field or property, then you can't inject your own. On top of that, the game has no internal IOC, and I have no control over the instantiation of ModSystem classes, so I can't inject into them. So, I'm having to rely on what would usually be a code smell; a Global Service Locator. IOC.Services.Resolve<T>(). This can be used in both instanced, and static contexts, but I know it's not an ideal solution. I'd love the game to work towards a more CUPID/GRASP based architecture. Hopefully, one day. (But I doubt it!). For now, and for the foreseeable future, what would normally be seen as an egregious coding violation, is instead a vital, and ubiquitous component.

I know that you can manually patch methods, and use anonymous functions to drag Harmony kicking and screaming into an instanced environment; but the hoops you have to jump through makes it impractical. You have to scan the class for the compiler generated display class of the local function, for each and every method you want to patch. But even then, it still needs a static method to run as a patch, even if it's just forwarding the method call to the anonymous function, otherwise it throws a runtime error, saying it needs to be within a static context, and can't inject instance methods as patches. So, you're back at square one.

Campaign Cartographer is relatively Harmony patch light; but if you look at a mod like Accessibility Tweaks, I use them extensively. Because of this, I'd tried to streamline the process as much as possible, allowing for rapid rollout of new features. This is an example of one of the more involved transpiler patches:

https://github.com/ApacheTech-VintageStory-Mods/ApacheTech.VintageMods.AccessibilityTweaks/blob/master/src/ApacheTech.VintageMods.AccessibilityTweaks/Features/CameraMovement/Patches/CameraMovementPatches.ShaderProgramBase.cs

But, other times, it's as simple, and as elegant as this:

https://github.com/ApacheTech-VintageStory-Mods/ApacheTech.VintageMods.AccessibilityTweaks/blob/master/src/ApacheTech.VintageMods.AccessibilityTweaks/Features/WeatherEffects/Patches/WeatherEffectsPatches.WeatherSimulationParticles.cs

But, that's only after a very long time, tweaking the underlying WorldSettingsConsumer<T> class, to make it all work.

https://github.com/ApacheTech-Nuget-Packages/VintageStory.Gantry/blob/master/src/Gantry.Services.FileSystem/Configuration/Consumers/WorldSettingsConsumer.cs

Having said all of this, this game is the only game I've ever modded, and the first time I've ever used Harmony. So, if there is a ridiculously large hole in my understanding, and I've just been making life very hard for myself over the last couple of years, I'd love to learn more, and discuss it in more detail.

[DoctorVanGogh]

First of: I have near zero knowledge about the internals of VintageStory. So I can't really get into the nitty gritty details of performance implications etc. What I do know is that I like their structured API approach for modding. One of - if not the - best approaches I've seen.

But I've done one or two things with Harmony. So I really can't see the issue with patching instanced methods. Here's an old patch for another game I've done:

    [HarmonyPatch(typeof(StorageSettings), "set_" + nameof(StorageSettings.Priority))]
    [UsedImplicitly]
    [SuppressMessage("ReSharper", "InconsistentNaming")]
    internal class StorageSettings_set_Priority
    {
        public static void Postfix(StorageSettings __instance) {
            (__instance as UserSettings)?.NotifyOwnerSettingsChanged();
        }
    }

If I remember correctly I had replaced instantiations of StorageSettings with UserSettings but could not override the Priority property setter since it wasn't virtual. Oh well, one Harmony patch later (with a parameter with the "magic" __instance name) and I get my frankenpatcheded overrides.

---

As for "things that have no inherent API", I'd go with double patches per type?

• One postfixed on the constructor, where you grab whatever API you need (although in your GUIDialog example the class actually has the api injected via parameter :grin:). Then stick that into a static ConditionalWeakTable. Use object instance for the key, your (collection of) API as the value.
• Then in whatever instance method patches you have, you simply grab your "stored" APIs via that ConditionalWeakTables TryGetValue.

In a way ConditionalWeakTable was designed to allow functionally "injecting" new fields & properties into objects. It does not keep strong references on the keys so they can be garbage collected as needed, and keeps the values alive as long as the key exists. No ServiceLocator needed. No costly Api creation on every method call. Just a simple dictionary lookup.

Here's a nice article on ConditionalWeakTable https://www.red-gate.com/simple-talk/blogs/conditionalweaktable-and-dynamic-properties-in-net-4/

[ApacheTech]

Oh no, you can patch instanced methods. The point is that the patches themselves, are static methods, and so cannot access the instance methods of their owning class.

        [HarmonyPrefix]
        [HarmonyPatch(typeof(WeatherSimulationParticles), "SpawnHailParticles")]
        public static bool Patch_WeatherSimulationParticles_SpawnHailParticles_Prefix()
        {
            return Settings.HailstonesEnabled;
        }

You can't have:

        [HarmonyPrefix]
        [HarmonyPatch(typeof(WeatherSimulationParticles), "SpawnHailParticles")]
        public bool Patch_WeatherSimulationParticles_SpawnHailParticles_Prefix()
        {
            return Settings.HailstonesEnabled;
        }

Ideally, I'd love to make a ModSystem with patches in it, and those patches use the API given to the ModSystem. However, there are a lot of synchronisation issues with this; and also issues with reloading mods, when a player is already in the world.

I have used ConditionalWeakTable before, and I've got an extension method within my extensions NuGet package that allows easy access to dynamic properties added to classes.

https://github.com/ApacheTech-Nuget-Packages/ApacheTech.Common.Extensions/blob/master/ApacheTech.Common.Extensions/System/ObjectExtensions.cs

However, this is a giant hoop to jump through. Adding patches to every single class you might want to patch, just so you can patch the class. The way I have it now, I cache everything once, at start-up, and then use those cached instances where they are needed. There's no setup required, to be able to patch any class. I just decorate the owning class, and write a patch method.

Even with your approach, I still need an externalised, static cache to place the pointers in; it just means I have one pointer for each instance, that lasts for the lifetime of the instance; instead of one single, globalised pointer. Most of the classes that I patch are singletons; or at least captured dependencies of singletons. What you effectively end up with is an array of pointers to the same object on the Heap; which if you converted to a Set, would have a count of 1. ApiEx.Client is just that; the singular pointer that can be referenced anywhere, without needing to first add that pointer to a table, for the lifetime of the object you're patching. I'm not cloning the API at all, so there's no cost to it; I'm literally just storing a pointer in a static property.

One really cool use of Harmony, has been to implement INotifyPropertyChanged, on any arbitrary POCO class. I use this for all the settings classes, so whenever anyone changes one of the settings, it automatically saves those settings to file. It's been a game-changer in how I'm able to very rapidly introduce new features, and settings into existing mods.

https://github.com/ApacheTech-Nuget-Packages/VintageStory.Gantry/tree/master/src/Gantry.Services.FileSystem/Configuration/ObservableFeatures

I then bind the settings class, within an observable wrapper, to a section within a settings file.

https://github.com/ApacheTech-Nuget-Packages/VintageStory.Gantry/blob/master/src/Gantry.Services.FileSystem/Configuration/JsonSettingsFile.cs#L53-L92

[DoctorVanGogh]

Ah I see - this is getting philosophical 😀

You are absolutely correct that there is no implicit benefit between lots of small references or a single large one. You're just shifting your captured dependency storage locations around. It was you who lamented the code smell of the service locator :wink:

Let's agree that the implementation of a harmony patch can never be an instance. I just figured you had misunderstood something about Harmony and instanced methods. Turns out, you hadn't. 👍

[ApacheTech]

Yeah, it just means that anything you put into a patch, from outside of the scope of the original patched instance, needs to also be in a static context. :)

Anyway, thank you for all your help with this. I'm glad a lot of the underlying problems have now been fixed. Feel free to poke around all my repos, and see if you spot any glaring errors, haha. I've now released v3.1.2, so I'll close this report. I think you'll still be able to reply though; if it needs to be re-opened at all.