`CitizenManager` extension desync when Manager gets patched by other mods

[krzychu124]

• removed citizen extensions due to issues with de-sync when other mod replace instance of array after caching the ref,
• optimized citizen access by ID by adding ref param where possible and feasible
• replaced const MAX_CITIZEN_COUNT and MAX_INSTANCE_COUNT in loops with a call for current array size (I've heard about some mod(s), still early, WIP, which could lift the game limits, so fixes possible compatibility issues ahead of time)

[krzychu124]

by removing the extension method we should save some time due to avoid the method call overhead.

In CitizenManager.instance the instance is property which is a method under the hood + it has an if condition that runs each call + array buffer is a field of field of the Array32 wrapper instance. Extension method was using arrayref directly instead of generic<T>->instance(with if)->arrayWrapperField->arrayref IMO It's worse than before but whatever.

[kianzarrin]

@krzychu124 this is a big change. We have similar code for NetNode, NetSegment, Vehicle, ... . if we want to change all of them its going to be uncomfortable!

why not to do this?

private static CitizenUnit[] _citizenUnitBuffer => Singleton<CitizenManager>.instance.m_units.m_buffer;

One downside is when it is used inside a loop then accessing Singleton<CitizenManager>.instance.m_units.m_buffer would have performance penalty. but we could simply avoid using the extensions inside performance critical loops.instead of changing all of TMPE.

Another solution is to update the static cache before every simulation step. that way we can maintain fast code and we wouldn't be passing ref around (not sure how fast ref arguments are).

A third solution is to ask other mods to update Mangers buffers when level is preloaded. then TMPE can simple update the static cache when level starts.

I was aware of this potential issue (of someone expanding the arrays) when I introduced the strategy of caching NetSegment/NetNode buffers but I though if it becomes a problem then we can do one of the above so I went ahead with it anyways.

[krzychu124]

We have similar code for NetNode, NetSegment, Vehicle, ... . if we want to change all of them its going to be uncomfortable!

I don't plan changing any of those since there are unlikely to change and for Vehicles/ParkedVehicles More Vehicles mod replaces the ref when mod is enabled.

why not to do this?

It's the worst possible option because it hides the "expensive" call behind the method and in most cases you can just put array ref in a variable and reuse it.

Another solution is to update the static cache before every simulation step. that way we can maintain fast code and we wouldn't be passing ref around (not sure how fast ref arguments are).

No idea what are you talking about here. Objects are passed by ref. Updating cache every step means doing stuff you don't have to do if you would just use ref when needed.

A third solution is to ask other mods to update Mangers buffers when level is preloaded. then TMPE can simple update the static cache when level starts.

They can't use level preload because they need to work with deserialized data or read it manually so not available at that moment. Also depends on subscription order of the event. If you think about updating before OnLevelLoaded then too late -> mods can try to access data in Serializable extension to check if mod settings are still valid compared to loaded savegame which is called before. The best would be to write prefix for data deserializer and update same as MoreVehicles does (in OnEnabled)

Singleton<>.instance is most expensive call (result from the mono profiler I've managed to setup yesterday)

Time(ms) Count   P/call(ms) Method name
########################
 14938.000  159775    0.093   ColossalFramework.Singleton`1::get_instance()
 ...

[kianzarrin]

Another solution is to update the static cache before every simulation step. that way we can maintain fast code and we wouldn't be passing ref around (not sure how fast ref arguments are).

No idea what are you talking about here. Objects are passed by ref. Updating cache every step means doing stuff you don't have to do if you would just use ref when needed.

I mean like this:

namespace TrafficManager.Util.Extensions
{
    using ColossalFramework;

    public static class CitizenUnitExtensions {
        internal static OnBeforeSimulationStep() =>  _citizenUnitBuffer = Singleton<CitizenManager>.instance.m_units.m_buffer;

        private static CitizenUnit[] _citizenUnitBuffer = Singleton<CitizenManager>.instance.m_units.m_buffer;
        internal static ref CitizenUnit ToCitizenUnit(this uint citizenUnit) => ref _citizenUnitBuffer[citizenUnit];
    }
}

They can't use level preload because they need to work with deserialized data or read it manually so not available at that moment.

Does deserialization release CitizenManager.m_units buffer and then allocate a new one? I was under the impression that it keeps the buffer but modifies its content. and if that is the case, they can simply expand the array before deserialization.

[krzychu124]

I'm aware what you meant and I don't like it/recommend anyone doing this.

they can simply expand the array before deserialization.

They could but they don't. Case-closed. I'm not going to teach anyone how to write good/better code. That takes a way too much time which could be spend on other things.

[krzychu124]

I need two reviews 😉