Closed SerTheGreat closed 6 years ago
I think this is a good proof-of-concept for the implementation, but there are a few things I'd like to see done differently before merging it (sorry for a wall of text):
RPM_IGNORE
is a global value, not a per-vessel value. Instead of instantiating it in each vessel computer, the global list should go in the RPMGlobals class (Core/RPMGlobals.cs). The loading step goes in RPMShaderLoader.LoaderRasterPropMonitorValues(). In that way, we don't have to pay for loading the table for each vessel that's instantiated. It's done once when the game loads, and every vessel can reference that common data.
ShouldIgnoreModule()
looks rather expensive - there are multiple string concatentations per call, along with two linear searches of a List. That's a lot of temporary allocations for something that is called for every module on a ship, particularly since most modules on most ships won't be in this list. I think a different approach might be called for:
1 - Instead of a single list containing both "all modules" parts and specific modules, have two containers:
in RPMGlobals:
/// List of parts where all PartModule should be ignored.
internal static List<string> ignoreAllPartModules = new List<string>();
/// List of parts where some PartModules should be ignored. Part names are the Key, list of module names is the Value.
internal static Dictionary<string, List<string>> ignorePartModules = new Dictionary<string, List<string>>();
in LoadRasterPropMonitorValues(), parse each moduleName
from the RPM_IGNORE lists. If the value field ends with ".*", add the string preceding the ".*" to the ignoreAllPartModules list. Otherwise, add the string after the last "." to the list that belongs to the part name's Key in ignorePartModules.
2 - in RPMVCPerModule, add
private readonly static List<string> emptyIgnoreList = new List<string>();
This creates a default list so there's no need to add an additional test when the part doesn't have an ignore list, without adding a temporary allocation to the garbage collector for each module in the vessel.
3 - in RPMVesselComputer.UpdateModuleLists(), inside the part loop but before the part module for-each loop, add code like
string partName = partsList[partsIdx].name;
if (!RPMGlobals.ignoreAllPartModules.Contains(partName))
{
List<string> modulesToIgnore;
if (!RPMGlobals.ignorePartModules.TryGetValue(partName, out modulesToIgnore))
{
modulesToIgnore = emptyIgnoreList;
}
... foreach loop
}
That allows us to skip all modules in a part without additional tests.
3 - ShouldIgnoreModule() can be eliminated by replacing the call to it with !modulesToIgnore.Contains(module.moduleName)
.
These changes reduce the per-module cost to a search of a single list, most likely an empty one, with a dictionary lookup and an additional list search per-part. One could sort the lists in ignorePartModules
, but I don't think a binary search would be much cheaper than a linear search since there likely won't be more than a handful of modules in each list.
I haven't tested the code above to see if it compiles exactly as written, so there may be syntax errors.
One other consideration - some part names may have a '.' in them. I don't recall if KSP converts '_' to '.', or the other way around. If the code in LoadRasterPropMonitorValues is doing a string.Split, it'll require extra processing to account for '.' in the part name. The separator token might be better as a colon (eg, "MyPartName:MyModuleName" or "MyPartName:*").
I should be able to get this merged this weekend.
Enables to specify ignored part modules as a plain config as RPM_IGNORE node with values following the syntax: