Skip to content

Conversation

@kianzarrin
Copy link
Collaborator

#730 (comment) : if TMPE is already loaded and another mod is hot reloaded, CS calls OnCreate() again.

For this reason I made the following changes:

  • moved Hot Reload logic to TrafficManagerMod.Enabled()
  • OnCreated() is skipped if LoadingExtension.IsGameLoaded

These logs prove that my changes are successful:
output_log.txt
TMPE.log


internal static AppMode currentMode => SimulationManager.instance.m_ManagersWrapper.loading.currentMode;

internal static bool InGame() {
Copy link
Member

Choose a reason for hiding this comment

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

Would it not be better to use SceneManager.GetActiveScene().name == "Game"?

I've been testing that as part of #699 and it's proved extremely reliable. (Note: PR #699 is currently non-functional as the UI stuff is a big mess - I'm waiting to see if kvakvs new Form Builder stuff can be used rather than spending ages messing with raw CO UI)

Copy link
Member

Choose a reason for hiding this comment

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

Not to mention that this will throw exception when used in menu (mentioned about that earlier)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it was not possible from OnCreated()
Now that I moved my stuff into Enable() ... yes

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have catched the exception so its fine :). I am going to change it to GetActiveScene() because its just cleaner that way

Copy link
Member

Choose a reason for hiding this comment

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

@aubergine10 I suspect that SceneManager.GetActiveScene().name == "Game" is not as reliable as we thought... Problem with it is that you will get true inside LoadingExtension.OnCreated()...
It seems that LoadingManager.instance.m_loadingComplete works way better for that case.

}
}

Instance = this;
Copy link
Member

Choose a reason for hiding this comment

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

Where is this getting used exaclty? When I look at the code in VS2019, it's not showing any references to it.

I can see down on line 110 (in updated TrafficManagerMod.cs) that it gets set to null in the OnDisabled() method, but other than that it's not getting used.

Was it supposed to be referenced in line 105 in the OnDisabled() method?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

see
TLM/TLM/State/SerializableDataExtension.cs :29
VersionLabel.cs :14

Copy link
Member

@originalfoo originalfoo left a comment

Choose a reason for hiding this comment

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

See comments above.

@kianzarrin
Copy link
Collaborator Author

I rebuild TMPE and HideCrossings multiple time. No problem. no exception. only two path find objects created.

Copy link
Member

@krzychu124 krzychu124 left a comment

Choose a reason for hiding this comment

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

I'm getting various exceptions in that scenario:

  • load savegame,
  • hot-reload,
  • back to main menu,
  • load save again (you should see null pointer after loading)
  • if you click back to desktop you will see another nullpointer because of missing main ui (not instantiated because of previous exception)

1:

NullReferenceException: Object reference not set to an instance of an object
  at TrafficManager.UI.ModUI..ctor () [0x00000] in <filename unknown>:0 
UnityEngine.GameObject:Internal_AddComponentWithType(Type)
UnityEngine.GameObject:AddComponent(Type) (at C:\buildslave\unity\build\artifacts\generated\common\runtime\GameObjectBindings.gen.cs:382)
UnityEngine.GameObject:AddComponent() (at C:\buildslave\unity\build\artifacts\generated\common\runtime\GameObjectBindings.gen.cs:387)
TrafficManager.LoadingExtension:OnLevelLoaded(LoadMode)
TrafficManager.State.SerializableDataExtension:OnCreated(ISerializableData)
SerializableDataWrapper:OnSerializableDataExtensionsCreated()
SerializableDataWrapper:GetImplementations()
SerializableDataWrapper:.ctor(SimulationManager)
SimulationManager:CreateRelay()
LoadingManager:MetaDataLoaded()
System.Reflection.MonoMethod:InternalInvoke(Object, Object[], Exception&)
System.Reflection.MonoMethod:Invoke(Object, BindingFlags, Binder, Object[], CultureInfo)
System.Reflection.MethodBase:Invoke(Object, Object[])
LoadingScreenMod.Util:InvokeVoid(Object, String)
LoadingScreenMod.<LoadLevelCoroutine>d__23:MoveNext()
UnityEngine.SetupCoroutine:InvokeMoveNext(IEnumerator, IntPtr) (at C:\buildslave\unity\build\Runtime\Export\Coroutines.cs:17)

and then:

NullReferenceException: Object reference not set to an instance of an object
  at TrafficManager.LoadingExtension.OnLevelLoaded (LoadMode mode) [0x00000] in <filename unknown>:0 
  at TrafficManager.State.SerializableDataExtension.OnCreated (ISerializableData serializableData) [0x00000] in <filename unknown>:0 
  at SerializableDataWrapper.OnSerializableDataExtensionsCreated () [0x00000] in <filename unknown>:0 
Rethrow as ModException: The Mod C:\Users\krzychu\AppData\Local\Colossal Order\Cities_Skylines\Addons\Mods\TrafficManager [0Harmony.dll, CSUtil.CameraControl.dll, CSUtil.Commons.dll, TMPE.GenericGameBridge.dll, TMPE.API.dll, TMPE.CitiesGameBridge.dll, TrafficManager.dll] has caused an error

UnityEngine.DebugLogHandler:Internal_LogException(Exception, Object)
UnityEngine.DebugLogHandler:LogException(Exception, Object)
UnityEngine.Logger:LogException(Exception, Object)
UnityEngine.Debug:LogException(Exception)
SerializableDataWrapper:OnSerializableDataExtensionsCreated()
SerializableDataWrapper:GetImplementations()
SerializableDataWrapper:.ctor(SimulationManager)
SimulationManager:CreateRelay()
LoadingManager:MetaDataLoaded()
System.Reflection.MonoMethod:InternalInvoke(Object, Object[], Exception&)
System.Reflection.MonoMethod:Invoke(Object, BindingFlags, Binder, Object[], CultureInfo)
System.Reflection.MethodBase:Invoke(Object, Object[])
LoadingScreenMod.Util:InvokeVoid(Object, String)
LoadingScreenMod.<LoadLevelCoroutine>d__23:MoveNext()
UnityEngine.SetupCoroutine:InvokeMoveNext(IEnumerator, IntPtr) (at C:\buildslave\unity\build\Runtime\Export\Coroutines.cs:17)

3 (tmpe.log)- look at call time:

Info 12.6415353: OnLevelLoaded complete.
Info 12.9638589: Setting main menu button position to [118,900]
Info 12.9819596: Setting main menu position to [46,777]
Debug 13.0347918: Public transport info view panel found.
Info 13.1384836: ThreadingExtension.OnBeforeSimulationFrame: First frame detected. Checking detours.
Info 13.1495116: ThreadingExtension.OnBeforeSimulationFrame: First frame detected. Detours checked. Result: 0 missing detours
Debug 505.6433201: CustomPathManager: OnDestroy
Debug 512.9205270: ModUI destructor is called.
Debug 512.9363504: ModUI.DisableTool: called
Warning 512.9478318: ModUI.DisableTool: tool is null!
   at CSUtil.Commons.Log.LogToFile(System.String log, LogLevel level)
   at CSUtil.Commons.Log.Warning(System.String s)
   at TrafficManager.UI.ModUI.DisableTool()
   at TrafficManager.UI.ModUI.DestroyTool()
   at TrafficManager.UI.ModUI.ReleaseTool()
   at TrafficManager.UI.ModUI.Finalize()

Debug 36.8922696: Current tool: Tool Controller (DefaultTool)
Debug 37.0623220: TrafficManagerTool.ShowAdvisor(MainMenu) called.
Debug 37.0766636: ModUI.EnableTool: called
Info 37.0856128: Initializing traffic manager tool...
Debug 37.0955545: TrafficLightTool: Awake -1390331904

[edit4] This might be cause:

Info 0.3470659: Scan complete: 0 incompatible mod(s) found
Info 0.3554350: Mono version: 2.0 (Visual Studio built mono)
Debug 0.3659219: LoadingExtension.OnCreated() called
Debug 0.9809369: SerializableDataExtension.OnCreated() called
Debug 0.9894320: HOT RELOAD ...
Info 0.9983126: Loading Traffic Manager: PE Data
Info 1.0066368: Initializing flags
Info 1.0206870: OnBeforeLoadData: ExtNodeManager

@kianzarrin
Copy link
Collaborator Author

I reset InGameHotRelaod when quiting to main menu. I tested hot reloading TMPE and/or another mod multiple times each time I quite to main menu and load game again. No exception, no error.

@kianzarrin kianzarrin requested a review from krzychu124 March 1, 2020 23:32
@originalfoo originalfoo added this to the 11.2 milestone Mar 1, 2020
@originalfoo
Copy link
Member

This is looking better now.

I think at some point we should consider creating a Lifecycle.cs that contains some sort of state engine for the mod lifecycle management (enabed -> loading -> unloading -> disabled) but that's something for a later date.

@kianzarrin
Copy link
Collaborator Author

kianzarrin commented Mar 1, 2020

I think at some point we should consider creating a Lifecycle.cs that contains some sort of state engine for the mod lifecycle management (enabed -> loading -> unloading -> disabled) but that's something for a later date.

I have thought of that but I can't see how it would work.

the only thing I can think of is:

  1. move all code from OnEnabled/OnDisabled, LoadingExtension.cs, and SerializeDataExtension.cs to lifecycle.cs
  2. then all OnEnabled/OnDisabled, LoadingExtension.cs, and SerializeDataExtension.cs do is to call appropriate functions of lifecycle.cs at appropriate times.

So lifeCycle.cs acts as a wrapper for IUserMod, LoadingExtension, and SerializableDataExtension . A bit ugly if you ask me.

The best thing to do is a mod for hot reload to patch CS so that it does not do weird stuff.

@originalfoo
Copy link
Member

Regarding the lifecycle stuff, I think we should create an issue to track that idea rather than pollute this PR. I'll create one now and copy stuff in to it.

@originalfoo originalfoo mentioned this pull request Mar 2, 2020
Copy link
Member

@krzychu124 krzychu124 left a comment

Choose a reason for hiding this comment

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

LGTM, now works without noticeable issues

Copy link
Member

@originalfoo originalfoo left a comment

Choose a reason for hiding this comment

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

Tested it only once, in-game, but worked well! LGTM 👍

@kianzarrin kianzarrin merged commit d89441a into CitiesSkylinesMods:master Mar 2, 2020
@kianzarrin kianzarrin deleted the 730-custompath-loading branch March 2, 2020 09:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants