Eliminate all static variables and methods

[VictorPhilipp]

As a developer I want my application to be hot-swappable to allow for a faster development time.

Assumptions/Preconditions

• Static variables and methods prevent the mod from being modified and updated while the game is running: C:S cannot fully unload old instances.
• Other features relevant for hot-swappability, like loading/saving custom data or deployment/removal of method detours are triggered at specific OnLoad/OnUnload events.

Tasks

• Remove as many static fields/methods as possible. Convert them into instance-level members.
• If not everything can be converted, collect remaining members and put them at a central place.
• Extend OnLoad/OnUnload event handlers such they take care that any remaining static members are unset/set.
• Check if the mod can be hot-swapped. If any further problems arise that were not foreseen create an issue on GitHub.

Acceptance criteria

• Static members are gone, except for a limited amount
  • Remaining static members are instantiated/invalidated at mod loading/unloading time
• Mod functionality remains unchanged
• Mod loading/unloading still works

[originalfoo]

As a novice learning C#, may I ask why? (it will be useful knowledge of a mod I'm writing).

[originalfoo]

Also, could we get a linter (or whatever) set up to help us with this task?

[FireController1847]

I don't see the purpose in this. There's always a need every once in a while for a static variable and/or method. Just look in Colossal Order's code; They use them often for things that don't require an instance.

[originalfoo]

Ah, I think I get it - if user has 2 copies of TMPE installed (eg. Labs and Stable, or one of those and local build, or one of those and pcfantasy mods that replicate chunks of TMPE), it's all using the same namespaces so the static stuff from one is overwriting that of the other?

[VictorPhilipp]

@aubergine10 Exactly, issue #210 proofs that C:S is unable to handle multiple mod instances correctly regarding static variables/methods. The second instance of TMPE (the one from the AJR mod) tries to access a static variable (Options.turnOnRed) that is present in AJR's TMPE but not in the other instance.

By removing all static members (except for a very limited & controlled set, in case we need it) we also can make the mod 100% hot-swappable. Then, we would not have to restart the game on every code change.

[VictorPhilipp]

Also, could we get a linter (or whatever) set up to help us with this task?

Could you add an issue for this? Maybe also add a linter / rule scheme that we can use.

[FireController1847]

I have never had to restart the game on a code change, I've only had to exit and re-enter the world. If you want I could start work on this while you guys work on Harmony integration, and then once that's done I can work on EVE

[originalfoo]

Remaining static members are instantiated/invalidated at mod loading/unloading time

I assume instantiation occurs automatically due to classes / members being defined as static?

How would such things be invalidated when unloading?

[FireController1847]

@aubergine10 Probably setting them as null in the classes, giving them a value at load, and setting them back to null and unload

[VictorPhilipp]

Another reason: I would like to use a mocking framework (probably Moq since it's free) for unit testing. However, it does not support mocking static methods.

[dymanoid]

Note that 'true' hot swapping is impossible.

The game uses a single app domain both for the game itself and for all mods. Unloading an assembly from an app domain is not supported on Mono (it will be supported only on .NET Core 3.0 and .NET 5).

Even if there will be no static references anymore, the types of the 'old' assembly will still be in the app domain and thus can be instantiated. Having 10 'hot swaps` will result in 10 copies of an assembly loaded into the same app domain (provided that every new assembly copy gets a new version number; otherwise, the game's logic won't load such an assembly).

But even that won't help much, because the game uses assembly resolution based on assembly names. See the CurrentDomain_AssemblyResolve method in the BuildConfig class. So if some code uses Reflection to obtain an instance of a type, the game might provide a wrong type from a 'zombie' assembly that is not a part of the 'hot-swapped' mod instance, thus causing weird exceptions like "Cannot cast object of type MyType to MyType" and so on.

[kvakvs]

What is wrong with static methods?
They are just functions without global/static state.
Static method calls are 10 times faster than instance virtual method calls.
OK with instance calls though.
Some (most of them really) static methods have no instance to bind to by design.

[FireController1847]

@kvakvs Not to challenge your thoughts, but I'd love to see some references for the claims that they're 10 times faster.

[kvakvs]

@FireController1847 We were discussing terrible GameObject.SendMessage performance with @krzychu124 and here's what he referred to https://jacksondunstan.com/articles/3605
Ok virtual calls are super expensive, instance calls are not too much, just an extra argument passed.

[originalfoo]

With the next couple of updates to mod checker (#434 and #439) issues with zombie TM:PE assemblies will be significantly reduced. That being the case, and factoring in that hot-swappability can't reliably be achieved (as per Dymanoid's comment above), should we be rethinking our stance on static methods, classes, etc?

[dymanoid]

There is nothing wrong with static methods, especially when they are pure functions (read the definition of that if you are not sure about this term). However, they don't allow polymorphism.

If you are running into performance troubles because of virtual method calls only, please let me see such an application. In 99.999% of the cases the reason is somewhere else. The measurements in the referenced blog post are crap, they measure everything but the method call performance. Just tell me if you want more info.

The static fields, properties, and methods were suspected in the original post to prevent the hot swapping, but the real reason has nothing to do with static members and methods. I explained it in my previous comment above.

However, static fields and properties are evil from design point of view. They should be avoided. They make the code less testable, they reduce the code maintainability and break many software design principles, like e. g. SOLID.

[originalfoo]

As of v11-alpha3, the mod checker will always report duplicate instances of TM:PE even if the mod checker has been disabled by user.

[krzychu124]

Static variables usage has been reduced. Hot-reload has been implemented some time ago and works without issues(the satisfying degree of usability). Closing.