Persisted modules are losing their customisations when switched to derived/base instances.

[BrettRyland]

Describe your problem with KSPCF :
If a craft is built with the Waterfall mod installed and then loaded on an install without the Waterfall mod (or vice-versa), then the ModuleEnginesFX module is replaced by a default ModuleEngines (or vice-versa) module. E.g.,

[WRN 23:26:54.357] [KSPCF:ModuleIndexingMismatch] Persisted module "ModuleEnginesFX" at index [0] has been removed, no matching module in the part config
[WRN 23:26:54.358] [KSPCF:ModuleIndexingMismatch] Module "ModuleEngines" at index [0] doesn't exist in the persisted part, a new instance will be created

The effect of this is that any customisations of the engine, such as activating it on an action group, are lost.
Since ModuleEngine is a base class of ModuleEnginesFX, it would be better to build the new module by casting it from the old module whenever such a cast is valid (i.e., both modules classes are known and one is derived from the other).
This is likely also an issue for any other mods that change modules to derived versions of a base class.

KSP version : 1.12.5
Link to your KSP.log file : KSP log files
Example craft files where the bobcat engine is activated on AG1 if-and-only-if the module is not replaced.

[BrettRyland]

I know it's not particularly good etiquette to bump an issue, but is anyone currently working on this issue?
I feel that this is rather important because it is craft-breaking when sharing craft that have customised their engine settings between users with and without the Waterfall mod.

[gotmachine]

Well, in theory, there is no guarantee that data issued by the derived class will be treated correctly by the base class, and even less so in the reverse case.

This being said, it does make sense from a practical PoV, and doing this would likely result in less broken things for the vast majority of cases.

I will give a shoot at implementing that.

[gotmachine]

So, fix implemented in f5690bf

Note that the modified patch will only load the persisted/saved data if the module indexes are still matching. Handling cases where the module is involved in an index reordering would be a lot more hazardous, as we can't exclude the possibility of multiple modules sharing the same inheritance tree being on the same part.

[BrettRyland]

Thanks! Building it locally, it appears to be working.

Incidentally, the ..\packages\Krafs.Publicizer.2.2.1\build\Krafs.Publicizer.props references in the KSPCommunityFixes.csproj file break the build process on Linux as those packages get installed in ~/.nuget/packages/ instead of in the project root.

[BrettRyland]

In the case where the module is involved in an index reordering, would it be possible to check for there being any other modules that share the base/derived module type as part of the inheritance tree and still apply the patch if it's the only one? That would cover most cases where such a replacement makes sense, I think.

[BrettRyland]

Something like the following perhaps?

diff --git a/KSPCommunityFixes/BugFixes/ModuleIndexingMismatch.cs b/KSPCommunityFixes/BugFixes/ModuleIndexingMismatch.cs
index 2014491..3d391fe 100644
--- a/KSPCommunityFixes/BugFixes/ModuleIndexingMismatch.cs
+++ b/KSPCommunityFixes/BugFixes/ModuleIndexingMismatch.cs
@@ -353,6 +353,26 @@ namespace KSPCommunityFixes
                     }
                     else
                     {
+                        int altModuleIdxCount = 0;
+                        int altModuleIdx = 0;
+                        if (protoModuleIdx < partModuleCount && allModuleTypes.TryGetValue(protoModule.moduleName, out moduleType))
+                        {
+                            for (int moduleIdx = 0; moduleIdx < partModuleCount; ++moduleIdx)
+                            {
+                                if (part.Modules[moduleIdx].GetType().IsAssignableFrom(moduleType) || moduleType.IsInstanceOfType(part.Modules[moduleIdx]))
+                                {
+                                    ++altModuleIdxCount;
+                                    altModuleIdx = moduleIdx;
+                                }
+                            }
+                        }
+                        if (altModuleIdxCount == 1)
+                        {
+                            LoadProtoPartSnapshotModule(protoPart.modules[protoModuleIdx], part.modules[altModuleIdx]);
+                            foundModules[altModuleIdx] = protoModule;
+                            Debug.LogWarning($"[KSPCF:ModuleIndexingMismatch] Persisted module \"{protoModule.moduleName}\" at index [{protoModuleIdx}] has been moved to index [{altModuleIdx}] and loaded as a \"{part.Modules[altModuleIdx].moduleName}\"");
+                        }
+                        else
                             Debug.LogWarning($"[KSPCF:ModuleIndexingMismatch] Persisted module \"{protoModule.moduleName}\" at index [{protoModuleIdx}] has been removed, no matching module in the part config");
                     }
                 }
@@ -595,6 +615,26 @@ namespace KSPCommunityFixes
                     }
                     else
                     {
+                        int altModuleIdxCount = 0;
+                        int altModuleIdx = 0;
+                        if (moduleNodeIdx < partModuleCount && allModuleTypes.TryGetValue(nodeModuleName, out moduleType))
+                        {
+                            for (int moduleIdx = 0; moduleIdx < partModuleCount; ++moduleIdx)
+                            {
+                                if (part.Modules[moduleIdx].GetType().IsAssignableFrom(moduleType) || moduleType.IsInstanceOfType(part.Modules[moduleIdx]))
+                                {
+                                    ++altModuleIdxCount;
+                                    altModuleIdx = moduleIdx;
+                                }
+                            }
+                        }
+                        if (altModuleIdxCount == 1)
+                        {
+                            part.Modules[altModuleIdx].Load(moduleNode);
+                            foundModules[altModuleIdx] = moduleNode;
+                            Debug.LogWarning($"[KSPCF:ModuleIndexingMismatch] Persisted module \"{nodeModuleName}\" at index [{moduleNodeIdx}] has been moved to index [{altModuleIdx}] and loaded as a \"{part.Modules[altModuleIdx].moduleName}\"");
+                        }
+                        else
                         Debug.LogWarning($"[KSPCF:ModuleIndexingMismatch] Persisted module \"{nodeModuleName}\" at index [{moduleNodeIdx}] has been removed, no matching module in the part config");
                     }
                 }

[gotmachine]

In the case where the module is involved in an index reordering, would it be possible to check for there being any other modules that share the base/derived module type as part of the inheritance tree and still apply the patch if it's the only one? That would cover most cases where such a replacement makes sense, I think.

As I said, the problem is that we can't guarantee that there won't be multiple modules sharing the same inheritance tree on the same part. With your changes, in such a case, and when a reordering is involved, we would end up putting the data in the wrong module.

If we want to support that, it should be done in the second loop where we have already resynchronized everything, but that also require keeping track of what which saved module index was synchronized to which module instance index, so we only check the "not found at all" modules.

In the end, I kinda doubt there is much practical cases where doing this would be useful as in the WF case, as well as in any other case I can think of, the modules involved would be in the base part config, not added through a MM patch, so their indices are unlikely to change. I guess it can still happen if the part config itself is modified, or in a yet to been seen case of a mod swapping a module itself coming from another mod, but well...

If I have some motivation at some point, I will look into it, for now the current patch state should cover the issue this was about.

[BrettRyland]

Ah, I see I misread some of the code. Yes, it would necessitate tracking potential matches during the earlier loop and only applying the match when the mapping is 1-to-1.

For reference, I have seen at least one craft (from a different user) where the order of the modules in a part (the LiquidEngineRE-J10 from the Making History expansion) were not in the same order as in the base part config (the ModuleEngines and ModuleGimbal were switched from 0,1 to 1,0), but I don't know how that happened and as you say, that's probably a rare enough occurence as to not warrant automation in KSPCF.
I'll continue to monitor my logs for such cases during the next season of the Runway Project competition on Scott Manley's discord (I monitor them anyway for BDA dev stuff.)

Thanks for your work so far! The latest pre-release gives a noticeable improvement in framerate for the sort of multi-vessel behaviour one sees in BDArmory tournaments (typically 4-8 vessels each with 50-150 parts).