Skip to content

CitizenManager extension desync when Manager gets patched by other mods #1599

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Oct 3, 2022
Merged

CitizenManager extension desync when Manager gets patched by other mods #1599

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
62ad838
Removed citizen extensions due to issues with de-sync, optimized citi…
krzychu124 Jun 9, 2022
2dd7d59
Merge branch 'master' of https://github.com/krzychu124/Cities-Skyline…
krzychu124 Jun 12, 2022
69f82e3
Merge branch 'master' into bugfix/citizen-manager-array-desync
krzychu124 Jul 11, 2022
c3715d2
Merge branch 'master' into bugfix/citizen-manager-array-desync
krzychu124 Jul 21, 2022
329f695
Merge branch 'master' of https://github.com/CitiesSkylinesMods/TMPE i…
krzychu124 Oct 3, 2022
44e3639
Typo fix
krzychu124 Oct 3, 2022
52f51b6
Merge branch 'master' into bugfix/citizen-manager-array-desync
krzychu124 Oct 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 32 additions & 19 deletions TLM/TLM/Manager/Impl/AdvancedParkingManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,16 +39,20 @@ public AdvancedParkingManager(Spiral spiral) {
}

protected override void OnDisableFeatureInternal() {
for (uint citizenInstanceId = 0; citizenInstanceId < ExtCitizenInstanceManager.Instance.ExtInstances.Length; ++citizenInstanceId) {
ExtPathMode pathMode = ExtCitizenInstanceManager.Instance.ExtInstances[citizenInstanceId].pathMode;
CitizenManager citizenManager = CitizenManager.instance;
CitizenInstance[] instancesBuffer = citizenManager.m_instances.m_buffer;
ExtCitizenInstance[] extCitizenInstances = ExtCitizenInstanceManager.Instance.ExtInstances;

for (uint citizenInstanceId = 0; citizenInstanceId < extCitizenInstances.Length; ++citizenInstanceId) {
ExtPathMode pathMode = extCitizenInstances[citizenInstanceId].pathMode;
switch (pathMode) {
case ExtPathMode.RequiresWalkingPathToParkedCar:
case ExtPathMode.CalculatingWalkingPathToParkedCar:
case ExtPathMode.WalkingToParkedCar:
case ExtPathMode.ApproachingParkedCar: {
// citizen requires a path to their parked car: release instance to prevent
// it from floating
Singleton<CitizenManager>.instance.ReleaseCitizenInstance((ushort)citizenInstanceId);
citizenManager.ReleaseCitizenInstance((ushort)citizenInstanceId);
break;
}

Expand All @@ -60,9 +64,9 @@ protected override void OnDisableFeatureInternal() {
case ExtPathMode.DrivingToTarget: {
// citizen instance requires a car but is walking: release instance to
// prevent it from floating
ref CitizenInstance citizenInstance = ref citizenInstanceId.ToCitizenInstance();
ref CitizenInstance citizenInstance = ref instancesBuffer[citizenInstanceId];
if (citizenInstance.IsCharacter()) {
Singleton<CitizenManager>.instance.ReleaseCitizenInstance((ushort)citizenInstanceId);
citizenManager.ReleaseCitizenInstance((ushort)citizenInstanceId);
}

break;
Expand All @@ -79,6 +83,7 @@ protected override void OnEnableFeatureInternal() {

public bool EnterParkedCar(ushort instanceId,
ref CitizenInstance instanceData,
ref Citizen citizen,
ushort parkedVehicleId,
out ushort vehicleId) {
#if DEBUG
Expand Down Expand Up @@ -190,7 +195,6 @@ public bool EnterParkedCar(ushort instanceId,

// set vehicle id for citizen instance
instanceData.m_path = 0u;
ref Citizen citizen = ref instanceData.m_citizen.ToCitizen();

citizen.SetParkedVehicle(instanceData.m_citizen, 0);
citizen.SetVehicle(instanceData.m_citizen, vehicleId, 0u);
Expand Down Expand Up @@ -287,6 +291,7 @@ public ExtSoftPathState UpdateCitizenPathState(ushort citizenInstanceId,
citizenInstanceId,
ref citizenInstance,
ref extInstance,
ref citizen,
ref extCitizen);
}

Expand Down Expand Up @@ -361,6 +366,7 @@ public ExtSoftPathState UpdateCitizenPathState(ushort citizenInstanceId,
citizenInstanceId,
ref citizenInstance,
ref extInstance,
ref citizen,
ref extCitizen);
}

Expand Down Expand Up @@ -988,6 +994,7 @@ protected ExtSoftPathState OnCitizenPathFindSuccess(ushort instanceId,
return OnCitizenPathFindSuccess_Default(
instanceId,
instanceData,
ref citizenData,
ref extInstance,
ref extCitizen,
logParkingAi,
Expand All @@ -1013,6 +1020,7 @@ protected ExtSoftPathState OnCitizenPathFindSuccess(ushort instanceId,
instanceId,
ref instanceData,
ref extInstance,
ref citizenData,
ref extCitizen,
usesCar,
logParkingAi,
Expand Down Expand Up @@ -1114,6 +1122,7 @@ private ExtSoftPathState
OnCitizenPathFindSuccess_CarPath(ushort instanceId,
ref CitizenInstance instanceData,
ref ExtCitizenInstance extInstance,
ref Citizen citizen,
ref ExtCitizen extCitizen,
bool usesCar,
bool logParkingAi,
Expand Down Expand Up @@ -1206,6 +1215,7 @@ private ExtSoftPathState
if (EnterParkedCar(
instanceId,
ref instanceData,
ref citizen,
parkedVehicleId,
out ushort vehicleId))
{
Expand Down Expand Up @@ -1299,6 +1309,7 @@ private ExtSoftPathState
private ExtSoftPathState
OnCitizenPathFindSuccess_Default(ushort instanceId,
CitizenInstance instanceData,
ref Citizen citizen,
ref ExtCitizenInstance extInstance,
ref ExtCitizen extCitizen,
bool logParkingAi,
Expand All @@ -1324,7 +1335,6 @@ private ExtSoftPathState
"section. Ensuring that citizen is allowed to use their car.");

ushort sourceBuildingId = instanceData.m_sourceBuilding;
ref Citizen citizen = ref instanceData.m_citizen.ToCitizen();
ushort homeId = citizen.m_homeBuilding;

if (parkedVehicleId == 0) {
Expand Down Expand Up @@ -1398,8 +1408,9 @@ private ExtSoftPathState
}

// spawn a passenger car near the current position
if (AdvancedParkingManager.Instance.TrySpawnParkedPassengerCar(
if (TrySpawnParkedPassengerCar(
citizenId: instanceData.m_citizen,
citizen: ref citizen,
homeId: homeId,
refPos: currentPos,
vehicleInfo: vehicleInfo,
Expand Down Expand Up @@ -1502,11 +1513,13 @@ private ExtSoftPathState
/// <param name="instanceId">Citizen instance id</param>
/// <param name="instanceData">Citizen instance data</param>
/// <param name="extInstance">extended citizen instance information</param>
/// <param name="citizen">citizen information</param>
/// <param name="extCitizen">extended citizen information</param>
/// <returns>if true path-finding may be repeated (path mode has been updated), false otherwise</returns>
protected ExtSoftPathState OnCitizenPathFindFailure(ushort instanceId,
ref CitizenInstance instanceData,
ref ExtCitizenInstance extInstance,
ref Citizen citizen,
ref ExtCitizen extCitizen) {
IExtCitizenInstanceManager extCitInstMan = Constants.ManagerFactory.ExtCitizenInstanceManager;
IExtBuildingManager extBuildingMan = Constants.ManagerFactory.ExtBuildingManager;
Expand Down Expand Up @@ -1568,7 +1581,6 @@ protected ExtSoftPathState OnCitizenPathFindFailure(ushort instanceId,
// relocate parked car if abandoned
if (extInstance.pathMode == ExtPathMode.CalculatingWalkingPathToParkedCar) {
// parked car is unreachable
ref Citizen citizen = ref instanceData.m_citizen.ToCitizen();
ushort parkedVehicleId = citizen.m_parkedVehicle;

if (parkedVehicleId != 0) {
Expand Down Expand Up @@ -1802,6 +1814,7 @@ public bool TryMoveParkedVehicle(ushort parkedVehicleId,

public bool FindParkingSpaceForCitizen(Vector3 endPos,
VehicleInfo vehicleInfo,
ref CitizenInstance driverInstance,
ref ExtCitizenInstance extDriverInstance,
ushort homeId,
bool goingHome,
Expand All @@ -1810,16 +1823,13 @@ public bool FindParkingSpaceForCitizen(Vector3 endPos,
out Vector3 parkPos,
ref PathUnit.Position endPathPos,
out bool calculateEndPos) {
IExtCitizenInstanceManager extCitInstMan = Constants.ManagerFactory.ExtCitizenInstanceManager;
ref CitizenInstance citizenInstance = ref extDriverInstance.instanceId.ToCitizenInstance();

#if DEBUG
bool citizenDebug =
(DebugSettings.VehicleId == 0 || DebugSettings.VehicleId == vehicleId)
&& (DebugSettings.CitizenInstanceId == 0 || DebugSettings.CitizenInstanceId == extDriverInstance.instanceId)
&& (DebugSettings.CitizenId == 0 || DebugSettings.CitizenId == extDriverInstance.instanceId.ToCitizenInstance().m_citizen)
&& (DebugSettings.SourceBuildingId == 0 || DebugSettings.SourceBuildingId == citizenInstance.m_sourceBuilding)
&& (DebugSettings.TargetBuildingId == 0 || DebugSettings.TargetBuildingId == citizenInstance.m_targetBuilding);
&& (DebugSettings.CitizenId == 0 || DebugSettings.CitizenId == driverInstance.m_citizen)
&& (DebugSettings.SourceBuildingId == 0 || DebugSettings.SourceBuildingId == driverInstance.m_sourceBuilding)
&& (DebugSettings.TargetBuildingId == 0 || DebugSettings.TargetBuildingId == driverInstance.m_targetBuilding);

bool logParkingAi = DebugSwitch.BasicParkingAILog.Get() && citizenDebug;
bool extendedLogParkingAi = DebugSwitch.ExtendedParkingAILog.Get() && citizenDebug;
Expand All @@ -1833,10 +1843,10 @@ public bool FindParkingSpaceForCitizen(Vector3 endPos,

if (!allowTourists) {
// TODO remove this from this method
uint citizenId = citizenInstance.m_citizen;
uint citizenId = driverInstance.m_citizen;

if (citizenId == 0 ||
(citizenId.ToCitizen().m_flags & Citizen.Flags.Tourist) != Citizen.Flags.None) {
(CitizenManager.instance.m_citizens.m_buffer[citizenId].m_flags & Citizen.Flags.Tourist) != Citizen.Flags.None) {
return false;
}
}
Expand Down Expand Up @@ -1961,6 +1971,7 @@ public bool FindParkingSpaceForCitizen(Vector3 endPos,
}

public bool TrySpawnParkedPassengerCar(uint citizenId,
ref Citizen citizen,
ushort homeId,
Vector3 refPos,
VehicleInfo vehicleInfo,
Expand Down Expand Up @@ -1988,6 +1999,7 @@ public bool TrySpawnParkedPassengerCar(uint citizenId,

bool buildingParkSuccess = TrySpawnParkedPassengerCarBuilding(
citizenId,
ref citizen,
homeId,
refPos,
vehicleInfo,
Expand Down Expand Up @@ -2059,7 +2071,7 @@ ref Singleton<SimulationManager>
parkRot,
citizenId))
{
citizenId.ToCitizen().SetParkedVehicle(citizenId, parkedVehicleId);
CitizenManager.instance.m_citizens.m_buffer[citizenId].SetParkedVehicle(citizenId, parkedVehicleId);
Copy link
Collaborator

@kvakvs kvakvs Jul 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this better? Thought we upgraded most/all direct uses of m_ buffers to helpers?
Also in several other locations, m_ buffers are used

I get it this is to always get the latest ref which may be modified by some mods, but we can update the ref ourselves too.

Copy link
Member Author

@krzychu124 krzychu124 Jul 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes we could update ref ourselves but I see a few problems.

  1. When? Between user hit "load" on a savegame to the frame you see in-game screen is a ton of frames where other mods could change ref and ref swap cannot be reliably detected - working on desynced refs will cause weird bugs, assuming we will notice that something is wrong.
  2. There is no need to update ref once game loads everything and show the in-game UI.
  3. It's hard to benchmark what is the real cost and potentially using cached ref can cause more performance issues than accessing it when needed (in range of method call)

The other managers (Net/Building) are really hard to mod so I doubt anyone try that in near future. Still, there are certain ways how you can do ref swap without breaking anything, although much harder and limits how some things can be handled (e.g. deserialization).


parkedVehicleId.ToParkedVehicle().m_flags &= (ushort)(VehicleParked.Flags.All & ~VehicleParked.Flags.Parking);

Expand All @@ -2085,6 +2097,7 @@ ref Singleton<SimulationManager>
}

public bool TrySpawnParkedPassengerCarBuilding(uint citizenId,
ref Citizen citizen,
ushort homeId,
Vector3 refPos,
VehicleInfo vehicleInfo,
Expand Down Expand Up @@ -2127,7 +2140,7 @@ public bool TrySpawnParkedPassengerCarBuilding(uint citizenId,
parkRot,
citizenId))
{
citizenId.ToCitizen().SetParkedVehicle(citizenId, parkedVehicleId);
citizen.SetParkedVehicle(citizenId, parkedVehicleId);

parkedVehicleId.ToParkedVehicle().m_flags &= (ushort)(VehicleParked.Flags.All & ~VehicleParked.Flags.Parking);

Expand Down
Loading