KSPModdingLibs / KSPCommunityFixes

Community patches for bugs in the KSP codebase.
https://github.com/KSPModdingLibs/KSPCommunityFixes/releases/latest
57 stars 18 forks source link

`ForceSyncSceneSwitch` patch incompatibility with Universal Storage 2 #273

Open gotmachine opened 1 week ago

gotmachine commented 1 week ago

The ForceSyncSceneSwitch patch is causing US2 to break the PAW UI controls prefab. Steps to reproduce :

The following exception happens :

[EXC 20:21:43.454] NullReferenceException: Object reference not set to an instance of an object
    UIPartActionController.SetupItemControls () (at <39c0323fb6b449a4aaf3465c00ed3c8d>:0)
    UIPartActionController.Start () (at <39c0323fb6b449a4aaf3465c00ed3c8d>:0)
    UnityEngine.DebugLogHandler:LogException(Exception, Object)
    ModuleManager.UnityLogHandle.InterceptLogHandler:LogException(Exception, Object)
    UnityEngine.Debug:CallOverridenDebugHandler(Exception, Object)

Then opening the PAW will trigger a flood of ItemPrefab for control type 'UI_xxx' not found. errors.

This is caused by US2 adding a new entry to the fieldPrefabs list in the UIPartActionController prefab every time the editor scene is loaded. That entry is a GameObject/Component lying around in the editor scene root.

Upon leaving the editor scene, that object is destroyed, leaving that destroyed entry in the prefab. On exiting the editor, stock will do a double scene loading : first the loadingBuffer scene, then the requested scene. Somehow, this result in the destroyed object being cleaned up from the prefab list, I guess because the prefab is truly unloaded during the time the loadingBuffer scene is the only one loaded.

But with the ForceSyncSceneSwitch patch, the loadingBuffer scene isn't loaded at all (to avoid the redundant asset cleanup call). This result in the prefab not being unloaded/reloaded (I guess because it is also used in the flight scene), the null entry persisting, and finally in the nullref exception. Doing an editor > KSC > flight scene transition does prevent the issue, but trying to call Resources.UnloadUnusedAssets() after the scene transition request doesn't.

What US2 is doing is a bit sketchy to begin with, but I it could be possible to have plugins doing similar stuff and relying on the prefabs being unloaded between the scene transitions using the buffered load path.

The only way I found to reliably reproduce stock behavior and avoid the US2 issue is to keep loading the loadingBuffer scene in between the requested scene. The extra Resources.UnloadUnusedAssets() and the async loading don't seem necessary, but that still leave us with a double asset cleanup (due to the double scene load), and I would guess a large part of the gains were actually in avoiding to unload assets that are shared between the flight and editor scenes.

My position for now would be to put together a MM patch disabling the ForceSyncSceneSwitch patch when US2 is installed.

On a side note, adding custom PAW UI controls is cumbersome and messy, I won't really blame US2 for doing it that way. Maybe we should look into providing an API in KSPCF for this.

gotmachine commented 1 week ago

For reference, this is what I came with as a "safer" version of the patch :

static void HighLogic_LoadScene_Override(GameScenes sceneToLoad)
{
    if (HighLogic.LoadedSceneIsGame)
    {
        if (sceneToLoad == GameScenes.MAINMENU && PSystemSetup.Instance != null)
        {
            PSystemSetup.Instance.RemoveNonStockLaunchSites();
        }
        if (sceneToLoad == GameScenes.MAINMENU)
        {
            MissionSystem.RemoveMissionObjects(removeAll: true);
            if (HighLogic.CurrentGame != null && HighLogic.CurrentGame.missionToStart != null)
            {
                MissionSystem.RemoveMissonObject(HighLogic.CurrentGame.missionToStart.MissionInfo);
            }
        }
    }

    GameScenes currentScene = HighLogic.LoadedScene;
    bool transitionValue = HighLogic.fetch.sceneBufferTransitionMatrix.GetTransitionValue(currentScene, sceneToLoad);
    HighLogic.SetLoadSceneEventsAndFlags(sceneToLoad, false);

    if (transitionValue)
    {
        SceneManager.LoadScene("loadingBuffer");
        SceneManager.LoadScene((int)sceneToLoad);
    }
    else
    {
        SceneManager.LoadScene((int)sceneToLoad);
    }
}

Unfortunately, this mean a double asset cleanup is performed due to the double scene load, and that assets shared between the previous and next scene will be unloaded / reloaded, because they don't exist in the loadingBuffer scene (and which was likely a large part of why the current patch improves the VAB <> flight scene transitions significantly).

JonnyOThan commented 1 week ago

On a side note, adding custom PAW UI controls is cumbersome and messy, I won't really blame US2 for doing it that way. Maybe we should look into providing an API in KSPCF for this.

TweakScale Rescaled does something like this: https://github.com/JonnyOThan/TweakScale/blob/4719b9ff96521463b8b31b8488c1c6e61b891845/Source/HarmonyPatching/Squad.cs#L99