From 0ce830b20c4f6e76198fa7a9cc5c9b279fbf4a31 Mon Sep 17 00:00:00 2001 From: sug44 Date: Thu, 30 Jan 2025 13:20:01 +0300 Subject: [PATCH 1/2] Fix #3119. Don't expose ListValue lists to users Use Collections.Generic.List instead of ListValue. Return a ListValue copy of these lists to user code. Changes: FileContent:binary Lexicon:keys Lexicon:values Part:children Stage:resources Stage:resourceslex String:split Structure:suffixnames Timewarp:ratelist Timewarp:railsratelist Timewarp:physicsratelist Vessel:parts Vessel:dockingports Vessel:decouplers (separators) Vessel:resources allnodes bound variable allwaypoints function Stage:resources, Stage:resourceslex, Vessel:parts, Vessel:dockingports, Vessel:decouplers now return copies instead of references to internal lists. All lists returned by these suffixes don't get modified over time so the only way to tell that they are copies now is to use the equality operator. Stage:resources and Stage:resourceslex returning a reference was a bug. --- src/kOS.Safe/Encapsulation/Lexicon.cs | 14 ++++++------- src/kOS.Safe/Encapsulation/StringValue.cs | 8 ++++---- src/kOS.Safe/Encapsulation/Structure.cs | 6 +++--- src/kOS.Safe/Persistence/FileContent.cs | 7 +------ src/kOS/Binding/FlightStats.cs | 9 ++++---- src/kOS/Function/Suffixed.cs | 2 +- src/kOS/Suffixed/AggregateResourceValue.cs | 4 ++-- src/kOS/Suffixed/Part/PartValue.cs | 6 +++--- src/kOS/Suffixed/Part/PartValueFactory.cs | 4 ++-- src/kOS/Suffixed/ResourceTransferValue.cs | 12 ++++++----- src/kOS/Suffixed/StageValues.cs | 10 ++++----- src/kOS/Suffixed/TimeWarpValue.cs | 24 +++------------------- src/kOS/Suffixed/VesselTarget.Parts.cs | 23 ++++++++------------- src/kOS/Suffixed/VesselTarget.cs | 8 ++++---- 14 files changed, 56 insertions(+), 81 deletions(-) diff --git a/src/kOS.Safe/Encapsulation/Lexicon.cs b/src/kOS.Safe/Encapsulation/Lexicon.cs index e621281051..766d170e6e 100644 --- a/src/kOS.Safe/Encapsulation/Lexicon.cs +++ b/src/kOS.Safe/Encapsulation/Lexicon.cs @@ -115,10 +115,10 @@ private void FillWithEnumerableValues(IEnumerable values) private void InitalizeSuffixes() { AddSuffix("CLEAR", new NoArgsVoidSuffix(Clear, "Removes all items from Lexicon")); - AddSuffix("KEYS", new Suffix>(GetKeys, "Returns the lexicon keys")); + AddSuffix("KEYS", new Suffix(GetKeys, "Returns the lexicon keys")); AddSuffix("HASKEY", new OneArgsSuffix(HasKey, "Returns true if a key is in the Lexicon")); AddSuffix("HASVALUE", new OneArgsSuffix(HasValue, "Returns true if value is in the Lexicon")); - AddSuffix("VALUES", new Suffix>(GetValues, "Returns the lexicon values")); + AddSuffix("VALUES", new Suffix(GetValues, "Returns the lexicon values")); AddSuffix("COPY", new NoArgsSuffix(() => new Lexicon(this), "Returns a copy of Lexicon")); AddSuffix("LENGTH", new NoArgsSuffix(() => internalDictionary.Count, "Returns the number of elements in the collection")); AddSuffix("REMOVE", new OneArgsSuffix(one => Remove(one), "Removes the value at the given key")); @@ -158,12 +158,12 @@ private BooleanValue HasKey(Structure key) return internalDictionary.ContainsKey(key); } - public ListValue GetValues() + public ListValue GetValues() { return ListValue.CreateList(Values); } - public ListValue GetKeys() + public ListValue GetKeys() { return ListValue.CreateList(Keys); } @@ -380,9 +380,9 @@ public override BooleanValue HasSuffix(StringValue suffixName) /// to the list. /// /// - public override ListValue GetSuffixNames() + public override List GetSuffixNames() { - ListValue theList = base.GetSuffixNames(); + List theList = base.GetSuffixNames(); foreach (Structure key in internalDictionary.Keys) { @@ -392,7 +392,7 @@ public override ListValue GetSuffixNames() theList.Add(keyStr); } } - return new ListValue(theList.OrderBy(item => item.ToString())); + return new List(theList.OrderBy(item => item.ToString())); } public override Dump Dump() { diff --git a/src/kOS.Safe/Encapsulation/StringValue.cs b/src/kOS.Safe/Encapsulation/StringValue.cs index 24f6056eff..04bc7451f4 100644 --- a/src/kOS.Safe/Encapsulation/StringValue.cs +++ b/src/kOS.Safe/Encapsulation/StringValue.cs @@ -238,11 +238,11 @@ System.Collections.IEnumerator IEnumerable.GetEnumerator() return GetEnumerator(); } - // As the regular Split, except returning a ListValue rather than an array. - public ListValue SplitToList(string separator) + // As the regular Split, except returning a List rather than an array. + public List SplitToList(string separator) { string[] split = Regex.Split(internalString, Regex.Escape(separator), RegexOptions.IgnoreCase); - ListValue returnList = new ListValue(); + List returnList = new List(); foreach (string s in split) returnList.Add(new StringValue(s)); return returnList; @@ -273,7 +273,7 @@ private void StringInitializeSuffixes() AddSuffix("PADRIGHT", new OneArgsSuffix( one => PadRight(one))); AddSuffix("REMOVE", new TwoArgsSuffix( (one, two) => Remove(one, two))); AddSuffix("REPLACE", new TwoArgsSuffix( (one, two) => Replace(one, two))); - AddSuffix("SPLIT", new OneArgsSuffix, StringValue>( one => SplitToList(one))); + AddSuffix("SPLIT", new OneArgsSuffix( one => new ListValue(SplitToList(one)))); AddSuffix("STARTSWITH", new OneArgsSuffix( one => StartsWith(one))); AddSuffix("TOLOWER", new NoArgsSuffix(() => ToLower())); AddSuffix("TOUPPER", new NoArgsSuffix(() => ToUpper())); diff --git a/src/kOS.Safe/Encapsulation/Structure.cs b/src/kOS.Safe/Encapsulation/Structure.cs index 3027491585..f8e440772e 100644 --- a/src/kOS.Safe/Encapsulation/Structure.cs +++ b/src/kOS.Safe/Encapsulation/Structure.cs @@ -63,7 +63,7 @@ private void InitializeInstanceSuffixes() AddSuffix("TOSTRING", new NoArgsSuffix(() => ToString())); AddSuffix("HASSUFFIX", new OneArgsSuffix(HasSuffix)); - AddSuffix("SUFFIXNAMES", new NoArgsSuffix>(GetSuffixNames)); + AddSuffix("SUFFIXNAMES", new NoArgsSuffix(() => new ListValue(GetSuffixNames()))); AddSuffix("ISSERIALIZABLE", new NoArgsSuffix(() => this is SerializableStructure)); AddSuffix("TYPENAME", new NoArgsSuffix(() => new StringValue(KOSName))); AddSuffix("ISTYPE", new OneArgsSuffix(GetKOSIsType)); @@ -208,7 +208,7 @@ public virtual BooleanValue HasSuffix(StringValue suffixName) return false; } - public virtual ListValue GetSuffixNames() + public virtual List GetSuffixNames() { callInitializeSuffixes(); List names = new List(); @@ -218,7 +218,7 @@ public virtual ListValue GetSuffixNames() // Return the list alphabetized by suffix name. The key lookups above, since they're coming // from a hashed dictionary, won't be in any predictable ordering: - return new ListValue(names.OrderBy(item => item.ToString())); + return names.OrderBy(item => item.ToString()).ToList(); } public virtual BooleanValue GetKOSIsType(StringValue queryTypeName) diff --git a/src/kOS.Safe/Persistence/FileContent.cs b/src/kOS.Safe/Persistence/FileContent.cs index 7cff1169fd..4682984c3a 100644 --- a/src/kOS.Safe/Persistence/FileContent.cs +++ b/src/kOS.Safe/Persistence/FileContent.cs @@ -61,15 +61,10 @@ private void InitializeSuffixes() AddSuffix("EMPTY", new Suffix(() => Size == 0)); AddSuffix("TYPE", new Suffix(() => Category.ToString())); AddSuffix("STRING", new Suffix(() => String)); - AddSuffix("BINARY", new Suffix>(() => ContentAsIntList())); + AddSuffix("BINARY", new Suffix(() => new ListValue(Bytes.Select(x => new ScalarIntValue(x))))); AddSuffix("ITERATOR", new Suffix(() => new Enumerator(GetEnumerator()))); } - private ListValue ContentAsIntList() - { - return new ListValue(Bytes.Select(x => new ScalarIntValue(x))); - } - public override Dump Dump() { return new Dump { { DumpContent, PersistenceUtilities.EncodeBase64(Bytes) } }; diff --git a/src/kOS/Binding/FlightStats.cs b/src/kOS/Binding/FlightStats.cs index 8c30ed6bb7..d30f495dcf 100644 --- a/src/kOS/Binding/FlightStats.cs +++ b/src/kOS/Binding/FlightStats.cs @@ -1,3 +1,4 @@ +using System.Collections.Generic; using kOS.Module; using kOS.Control; using kOS.Safe.Binding; @@ -56,15 +57,15 @@ public override void AddTo(SharedObjects shared) return false; // Since there is no solver, there can be no node. return vessel.patchedConicSolver.maneuverNodes.Count > 0; }); - shared.BindingMgr.AddGetter("ALLNODES", () => GetAllNodes(shared)); + shared.BindingMgr.AddGetter("ALLNODES", () => new ListValue(GetAllNodes(shared))); } - public ListValue GetAllNodes(SharedObjects shared) + public List GetAllNodes(SharedObjects shared) { var vessel = shared.Vessel; if (vessel.patchedConicSolver == null || vessel.patchedConicSolver.maneuverNodes.Count == 0) - return new ListValue(); - var ret = new ListValue(vessel.patchedConicSolver.maneuverNodes.Select(e => Node.FromExisting(vessel, e, shared))); + return new List(); + var ret = new List(vessel.patchedConicSolver.maneuverNodes.Select(e => Node.FromExisting(vessel, e, shared))); return ret; } } diff --git a/src/kOS/Function/Suffixed.cs b/src/kOS/Function/Suffixed.cs index 3ffb2ab308..abf577fe28 100644 --- a/src/kOS/Function/Suffixed.cs +++ b/src/kOS/Function/Suffixed.cs @@ -732,7 +732,7 @@ public override void Execute(SharedObjects shared) AssertArgBottomAndConsume(shared); // no args // ReSharper disable SuggestUseVarKeywordEvident - ListValue returnList = new ListValue(); + ListValue returnList = new ListValue(); // ReSharper enable SuggestUseVarKeywordEvident WaypointManager wpm = WaypointManager.Instance(); diff --git a/src/kOS/Suffixed/AggregateResourceValue.cs b/src/kOS/Suffixed/AggregateResourceValue.cs index 1f565e1a43..9ca5a5d77b 100644 --- a/src/kOS/Suffixed/AggregateResourceValue.cs +++ b/src/kOS/Suffixed/AggregateResourceValue.cs @@ -99,10 +99,10 @@ public static ListValue PartsToList(IEnumerable parts, SharedObjec return list; } - public static ListValue FromVessel(Vessel vessel, SharedObjects shared) + public static List FromVessel(Vessel vessel, SharedObjects shared) { var resources = ProspectResources(vessel.parts, shared); - return ListValue.CreateList(resources.Values); + return resources.Values.ToList(); } } } \ No newline at end of file diff --git a/src/kOS/Suffixed/Part/PartValue.cs b/src/kOS/Suffixed/Part/PartValue.cs index 40c06d5e17..dc182f59d2 100644 --- a/src/kOS/Suffixed/Part/PartValue.cs +++ b/src/kOS/Suffixed/Part/PartValue.cs @@ -22,7 +22,7 @@ public class PartValue : Structure, IKOSTargetable public global::Part Part { get; private set; } public PartValue Parent { get; private set; } public DecouplerValue Decoupler { get; private set; } - public ListValue Children { get; private set; } + public List Children { get; private set; } public Structure ParentValue { get { return (Structure)Parent ?? StringValue.None; } } public Structure DecouplerValue { get { return (Structure)Decoupler ?? StringValue.None; } } public int DecoupledIn { get { return (Decoupler != null) ? Decoupler.Part.inverseStage : -1; } } @@ -37,7 +37,7 @@ internal PartValue(SharedObjects shared, global::Part part, PartValue parent, De Parent = parent; Decoupler = decoupler; RegisterInitializer(PartInitializeSuffixes); - Children = new ListValue(); + Children = new List(); } private void PartInitializeSuffixes() @@ -66,7 +66,7 @@ private void PartInitializeSuffixes() AddSuffix(new[] { "DECOUPLER", "SEPARATOR" }, new Suffix(() => DecouplerValue, "The part that will decouple/separate this part when activated")); AddSuffix(new[] { "DECOUPLEDIN", "SEPARATEDIN" }, new Suffix(() => DecoupledIn)); AddSuffix("HASPARENT", new Suffix(() => Part.parent != null, "Tells you if this part has a parent, is used to avoid null exception from PARENT")); - AddSuffix("CHILDREN", new Suffix>(() => PartValueFactory.ConstructGeneric(Part.children, Shared), "A LIST() of the children parts of this part")); + AddSuffix("CHILDREN", new Suffix(() => new ListValue(PartValueFactory.ConstructGeneric(Part.children, Shared)), "A LIST() of the children parts of this part")); AddSuffix("DRYMASS", new Suffix(() => Part.GetDryMass(), "The Part's mass when empty")); AddSuffix("MASS", new Suffix(() => Part.CalculateCurrentMass(), "The Part's current mass")); AddSuffix("WETMASS", new Suffix(() => Part.GetWetMass(), "The Part's mass when full")); diff --git a/src/kOS/Suffixed/Part/PartValueFactory.cs b/src/kOS/Suffixed/Part/PartValueFactory.cs index e202e18f5e..d0ade7288b 100644 --- a/src/kOS/Suffixed/Part/PartValueFactory.cs +++ b/src/kOS/Suffixed/Part/PartValueFactory.cs @@ -12,8 +12,8 @@ public static ListValue Construct(IEnumerable parts, SharedObjects return ListValue.CreateList(parts.Select(part => Construct(part, shared)).Where(p => p != null)); } - public static ListValue ConstructGeneric(IEnumerable parts, SharedObjects shared) { - return ListValue.CreateList(parts.Select(part => Construct(part, shared)).Where(p => p != null)); + public static List ConstructGeneric(IEnumerable parts, SharedObjects shared) { + return parts.Select(part => Construct(part, shared)).Where(p => p != null).ToList(); } public static PartValue Construct(global::Part part, SharedObjects shared) { diff --git a/src/kOS/Suffixed/ResourceTransferValue.cs b/src/kOS/Suffixed/ResourceTransferValue.cs index c044843fb0..282300c583 100644 --- a/src/kOS/Suffixed/ResourceTransferValue.cs +++ b/src/kOS/Suffixed/ResourceTransferValue.cs @@ -116,11 +116,7 @@ private TransferPartType DetermineType(object toTest) { return TransferPartType.Part; } - if (toTest is ListValue) - { - return TransferPartType.Parts; - } - if (toTest is ListValue) + if (toTest is ListValue || toTest is ListValue || toTest is List) { return TransferPartType.Parts; } @@ -350,6 +346,12 @@ private double CalculateAvailableResource(IEnumerable fromParts) parts.AddRange(partListValue.Select(pv => pv.Part)); break; } + var partList= obj as List; + if (partList!= null) + { + parts.AddRange(partList.Select(pv => pv.Part)); + break; + } break; case TransferPartType.Element: var element = obj as ElementValue; diff --git a/src/kOS/Suffixed/StageValues.cs b/src/kOS/Suffixed/StageValues.cs index 3f7f3c595a..5964603e83 100644 --- a/src/kOS/Suffixed/StageValues.cs +++ b/src/kOS/Suffixed/StageValues.cs @@ -18,7 +18,7 @@ public class StageValues : Structure private readonly SharedObjects shared; private HashSet partHash = new HashSet(); private PartSet partSet; - private ListValue resList; + private List resList; private Lexicon resLex; // Set by VesselTarget (from InvalidateParts) @@ -47,16 +47,16 @@ private void InitializeSuffixes() AddSuffix("NUMBER", new Suffix(() => StageManager.CurrentStage)); AddSuffix("READY", new Suffix(() => shared.Vessel.isActiveVessel && StageManager.CanSeparate)); - AddSuffix("RESOURCES", new Suffix>(GetResourceManifest)); - AddSuffix("RESOURCESLEX", new Suffix(GetResourceDictionary)); + AddSuffix("RESOURCES", new Suffix(() => new ListValue(GetResourceManifest()))); + AddSuffix("RESOURCESLEX", new Suffix(() => new Lexicon(GetResourceDictionary()))); AddSuffix(new string[] { "NEXTDECOUPLER", "NEXTSEPARATOR" }, new Suffix(() => shared.VesselTarget.NextDecoupler ?? (Structure)StringValue.None)); AddSuffix("DELTAV", new Suffix(() => new DeltaVCalc(shared, shared.Vessel.VesselDeltaV.GetStage(shared.Vessel.currentStage)))); } - private ListValue GetResourceManifest() + private List GetResourceManifest() { if (resList != null) return resList; - resList = new ListValue(); + resList = new List(); CreatePartSet(); var defs = PartResourceLibrary.Instance.resourceDefinitions; foreach (var def in defs) diff --git a/src/kOS/Suffixed/TimeWarpValue.cs b/src/kOS/Suffixed/TimeWarpValue.cs index 29d0505dd7..07fb33b98d 100644 --- a/src/kOS/Suffixed/TimeWarpValue.cs +++ b/src/kOS/Suffixed/TimeWarpValue.cs @@ -33,9 +33,9 @@ public static TimeWarpValue Instance private void InitializeSuffixes() { AddSuffix("RATE", new SetSuffix(GetRate, SetRate)); - AddSuffix("RATELIST", new Suffix>(GetRatesList)); - AddSuffix("RAILSRATELIST", new Suffix>(() => GetRatesList(TimeWarp.Modes.HIGH))); - AddSuffix("PHYSICSRATELIST", new Suffix>(() => GetRatesList(TimeWarp.Modes.LOW))); + AddSuffix("RATELIST", new Suffix(() => ListValue.CreateList(GetRateArrayForMode(TimeWarp.WarpMode)))); + AddSuffix("RAILSRATELIST", new Suffix(() => ListValue.CreateList(GetRateArrayForMode(TimeWarp.Modes.HIGH)))); + AddSuffix("PHYSICSRATELIST", new Suffix(() => ListValue.CreateList(GetRateArrayForMode(TimeWarp.Modes.LOW)))); AddSuffix("MODE", new SetSuffix(GetModeAsString, SetModeAsString)); AddSuffix("WARP", new SetSuffix(GetWarp, SetWarp)); AddSuffix("WARPTO", new OneArgsSuffix(WarpTo)); @@ -134,24 +134,6 @@ public ScalarValue GetDeltaT() return TimeWarp.fixedDeltaTime; } - public ListValue GetRatesList() - { - return GetRatesList(TimeWarp.WarpMode); - } - - public ListValue GetRatesList(TimeWarp.Modes warpMode) - { - float[] ratesArray = GetRateArrayForMode(warpMode); - - ListValue ratesKOSList = new ListValue(); - - // Have to convert the elements one at a time from (float) to (ScalarDoubleValue): - foreach (float val in ratesArray) - ratesKOSList.Add(val); - - return ratesKOSList; - } - public BooleanValue IsWarpSettled() { float expectedRate = GetRateArrayForMode(TimeWarp.WarpMode)[TimeWarp.CurrentRateIndex]; diff --git a/src/kOS/Suffixed/VesselTarget.Parts.cs b/src/kOS/Suffixed/VesselTarget.Parts.cs index 771ffc9795..a354a4ceaa 100644 --- a/src/kOS/Suffixed/VesselTarget.Parts.cs +++ b/src/kOS/Suffixed/VesselTarget.Parts.cs @@ -19,9 +19,9 @@ partial class VesselTarget //..... [root, child1, child2, ..., part1-1, part1-2, ..., part2-1, ... heap ;) private PartValue rootPart; private DecouplerValue nextDecoupler; - private ListValue allParts; - private ListValue dockingPorts; - private ListValue decouplers; + private List allParts; + private List dockingPorts; + private List decouplers; private Dictionary partCache; private void InvalidateParts() @@ -53,7 +53,7 @@ public DecouplerValue NextDecoupler return nextDecoupler; } } - public ListValue Parts + public List Parts { get { @@ -62,7 +62,7 @@ public ListValue Parts return allParts; } } - public ListValue DockingPorts + public List DockingPorts { get { @@ -71,7 +71,7 @@ public ListValue DockingPorts return dockingPorts; } } - public ListValue Decouplers + public List Decouplers { get { @@ -98,16 +98,12 @@ private void ConstructParts() { rootPart = null; nextDecoupler = null; - allParts = new ListValue(); - dockingPorts = new ListValue(); - decouplers = new ListValue(); + allParts = new List(); + dockingPorts = new List(); + decouplers = new List(); partCache = new Dictionary(); ConstructPart(Vessel.rootPart, null, null); - - allParts.IsReadOnly = true; - dockingPorts.IsReadOnly = true; - decouplers.IsReadOnly = true; } private void ConstructPart(global::Part part, PartValue parent, DecouplerValue decoupler) { @@ -193,7 +189,6 @@ private void ConstructPart(global::Part part, PartValue parent, DecouplerValue d allParts.Add(self); foreach (var child in part.children) ConstructPart(child, self, decoupler); - self.Children.IsReadOnly = true; } private ListValue GetPartsDubbed(StringValue searchTerm) diff --git a/src/kOS/Suffixed/VesselTarget.cs b/src/kOS/Suffixed/VesselTarget.cs index 943c9ebdbc..d981f8323b 100644 --- a/src/kOS/Suffixed/VesselTarget.cs +++ b/src/kOS/Suffixed/VesselTarget.cs @@ -255,9 +255,9 @@ private void InitializeSuffixes() AddSuffix("PARTSTAGGED", new OneArgsSuffix(GetPartsTagged)); AddSuffix("PARTSTAGGEDPATTERN", new OneArgsSuffix(GetPartsTaggedPattern)); AddSuffix("ALLTAGGEDPARTS", new NoArgsSuffix(GetAllTaggedParts)); - AddSuffix("PARTS", new NoArgsSuffix>(() => Parts)); - AddSuffix("DOCKINGPORTS", new NoArgsSuffix>(() => DockingPorts)); - AddSuffix(new string[] { "DECOUPLERS", "SEPARATORS" }, new NoArgsSuffix>(() => Decouplers)); + AddSuffix("PARTS", new NoArgsSuffix(() => new ListValue(Parts))); + AddSuffix("DOCKINGPORTS", new NoArgsSuffix(() => new ListValue(DockingPorts))); + AddSuffix(new string[] { "DECOUPLERS", "SEPARATORS" }, new NoArgsSuffix(() => new ListValue(Decouplers))); AddSuffix("ELEMENTS", new NoArgsSuffix(() => Vessel.PartList("elements", Shared))); AddSuffix("ENGINES", new NoArgsSuffix(() => Vessel.PartList("engines", Shared))); @@ -291,7 +291,7 @@ private void InitializeSuffixes() AddSuffix("CONTROLPART", new Suffix(() => PartValueFactory.Construct(GetControlPart(), Shared))); AddSuffix("DRYMASS", new Suffix(() => Vessel.GetDryMass(), "The Ship's mass when empty")); AddSuffix("WETMASS", new Suffix(() => Vessel.GetWetMass(), "The Ship's mass when full")); - AddSuffix("RESOURCES", new Suffix>(() => AggregateResourceValue.FromVessel(Vessel, Shared), "The Aggregate resources from every part on the craft")); + AddSuffix("RESOURCES", new Suffix(() => new ListValue(AggregateResourceValue.FromVessel(Vessel, Shared)), "The Aggregate resources from every part on the craft")); AddSuffix("LOADDISTANCE", new Suffix(() => new LoadDistanceValue(Vessel))); AddSuffix("ISDEAD", new NoArgsSuffix(() => (Vessel == null || Vessel.state == Vessel.State.DEAD))); AddSuffix("STATUS", new Suffix(() => Vessel.situation.ToString())); From 8e588c64dd60d7865b2de1a2a499b1a30c105211 Mon Sep 17 00:00:00 2001 From: sug44 Date: Sat, 1 Feb 2025 14:08:53 +0300 Subject: [PATCH 2/2] Optimize by not creating list copies where possible. Revert Vessel parts, dockingports, decouplers suffixes to return readonly references to internal lists --- src/kOS.Safe/Encapsulation/Lexicon.cs | 6 +++--- src/kOS.Safe/Encapsulation/StringValue.cs | 8 ++++---- src/kOS.Safe/Encapsulation/Structure.cs | 14 ++++++------- src/kOS/Binding/FlightStats.cs | 8 ++++---- src/kOS/Suffixed/AggregateResourceValue.cs | 4 ++-- src/kOS/Suffixed/Part/DockingPortValue.cs | 2 +- src/kOS/Suffixed/Part/PartValue.cs | 6 +++--- src/kOS/Suffixed/Part/PartValueFactory.cs | 4 ++-- src/kOS/Suffixed/StageValues.cs | 6 +++--- src/kOS/Suffixed/VesselTarget.Parts.cs | 23 +++++++++++++--------- src/kOS/Suffixed/VesselTarget.cs | 14 ++++++------- 11 files changed, 50 insertions(+), 45 deletions(-) diff --git a/src/kOS.Safe/Encapsulation/Lexicon.cs b/src/kOS.Safe/Encapsulation/Lexicon.cs index 766d170e6e..e616035670 100644 --- a/src/kOS.Safe/Encapsulation/Lexicon.cs +++ b/src/kOS.Safe/Encapsulation/Lexicon.cs @@ -380,9 +380,9 @@ public override BooleanValue HasSuffix(StringValue suffixName) /// to the list. /// /// - public override List GetSuffixNames() + public override ListValue GetSuffixNames() { - List theList = base.GetSuffixNames(); + ListValue theList = base.GetSuffixNames(); foreach (Structure key in internalDictionary.Keys) { @@ -392,7 +392,7 @@ public override List GetSuffixNames() theList.Add(keyStr); } } - return new List(theList.OrderBy(item => item.ToString())); + return new ListValue(theList.OrderBy(item => item.ToString())); } public override Dump Dump() { diff --git a/src/kOS.Safe/Encapsulation/StringValue.cs b/src/kOS.Safe/Encapsulation/StringValue.cs index 04bc7451f4..ecbe90ac3e 100644 --- a/src/kOS.Safe/Encapsulation/StringValue.cs +++ b/src/kOS.Safe/Encapsulation/StringValue.cs @@ -238,11 +238,11 @@ System.Collections.IEnumerator IEnumerable.GetEnumerator() return GetEnumerator(); } - // As the regular Split, except returning a List rather than an array. - public List SplitToList(string separator) + // As the regular Split, except returning a ListValue rather than an array. + public ListValue SplitToList(string separator) { string[] split = Regex.Split(internalString, Regex.Escape(separator), RegexOptions.IgnoreCase); - List returnList = new List(); + ListValue returnList = new ListValue(); foreach (string s in split) returnList.Add(new StringValue(s)); return returnList; @@ -273,7 +273,7 @@ private void StringInitializeSuffixes() AddSuffix("PADRIGHT", new OneArgsSuffix( one => PadRight(one))); AddSuffix("REMOVE", new TwoArgsSuffix( (one, two) => Remove(one, two))); AddSuffix("REPLACE", new TwoArgsSuffix( (one, two) => Replace(one, two))); - AddSuffix("SPLIT", new OneArgsSuffix( one => new ListValue(SplitToList(one)))); + AddSuffix("SPLIT", new OneArgsSuffix( one => SplitToList(one))); AddSuffix("STARTSWITH", new OneArgsSuffix( one => StartsWith(one))); AddSuffix("TOLOWER", new NoArgsSuffix(() => ToLower())); AddSuffix("TOUPPER", new NoArgsSuffix(() => ToUpper())); diff --git a/src/kOS.Safe/Encapsulation/Structure.cs b/src/kOS.Safe/Encapsulation/Structure.cs index f8e440772e..4d51fc18e4 100644 --- a/src/kOS.Safe/Encapsulation/Structure.cs +++ b/src/kOS.Safe/Encapsulation/Structure.cs @@ -63,7 +63,7 @@ private void InitializeInstanceSuffixes() AddSuffix("TOSTRING", new NoArgsSuffix(() => ToString())); AddSuffix("HASSUFFIX", new OneArgsSuffix(HasSuffix)); - AddSuffix("SUFFIXNAMES", new NoArgsSuffix(() => new ListValue(GetSuffixNames()))); + AddSuffix("SUFFIXNAMES", new NoArgsSuffix(GetSuffixNames)); AddSuffix("ISSERIALIZABLE", new NoArgsSuffix(() => this is SerializableStructure)); AddSuffix("TYPENAME", new NoArgsSuffix(() => new StringValue(KOSName))); AddSuffix("ISTYPE", new OneArgsSuffix(GetKOSIsType)); @@ -208,17 +208,17 @@ public virtual BooleanValue HasSuffix(StringValue suffixName) return false; } - public virtual List GetSuffixNames() + public virtual ListValue GetSuffixNames() { callInitializeSuffixes(); - List names = new List(); - - names.AddRange(instanceSuffixes.Keys.Select(item => (StringValue)item)); - names.AddRange(GetStaticSuffixesForType(GetType()).Keys.Select(item => (StringValue)item)); + List names = new List(); + + foreach (var suffix in instanceSuffixes.Keys) names.Add(suffix); + foreach (var staticSuffix in GetStaticSuffixesForType(GetType()).Keys) names.Add(staticSuffix); // Return the list alphabetized by suffix name. The key lookups above, since they're coming // from a hashed dictionary, won't be in any predictable ordering: - return names.OrderBy(item => item.ToString()).ToList(); + return new ListValue(names.OrderBy(item => item.ToString())); } public virtual BooleanValue GetKOSIsType(StringValue queryTypeName) diff --git a/src/kOS/Binding/FlightStats.cs b/src/kOS/Binding/FlightStats.cs index d30f495dcf..90f5427799 100644 --- a/src/kOS/Binding/FlightStats.cs +++ b/src/kOS/Binding/FlightStats.cs @@ -57,15 +57,15 @@ public override void AddTo(SharedObjects shared) return false; // Since there is no solver, there can be no node. return vessel.patchedConicSolver.maneuverNodes.Count > 0; }); - shared.BindingMgr.AddGetter("ALLNODES", () => new ListValue(GetAllNodes(shared))); + shared.BindingMgr.AddGetter("ALLNODES", () => GetAllNodes(shared)); } - public List GetAllNodes(SharedObjects shared) + public ListValue GetAllNodes(SharedObjects shared) { var vessel = shared.Vessel; if (vessel.patchedConicSolver == null || vessel.patchedConicSolver.maneuverNodes.Count == 0) - return new List(); - var ret = new List(vessel.patchedConicSolver.maneuverNodes.Select(e => Node.FromExisting(vessel, e, shared))); + return new ListValue(); + var ret = new ListValue(vessel.patchedConicSolver.maneuverNodes.Select(e => Node.FromExisting(vessel, e, shared))); return ret; } } diff --git a/src/kOS/Suffixed/AggregateResourceValue.cs b/src/kOS/Suffixed/AggregateResourceValue.cs index 9ca5a5d77b..b3674336ab 100644 --- a/src/kOS/Suffixed/AggregateResourceValue.cs +++ b/src/kOS/Suffixed/AggregateResourceValue.cs @@ -99,10 +99,10 @@ public static ListValue PartsToList(IEnumerable parts, SharedObjec return list; } - public static List FromVessel(Vessel vessel, SharedObjects shared) + public static ListValue FromVessel(Vessel vessel, SharedObjects shared) { var resources = ProspectResources(vessel.parts, shared); - return resources.Values.ToList(); + return new ListValue(resources.Values); } } } \ No newline at end of file diff --git a/src/kOS/Suffixed/Part/DockingPortValue.cs b/src/kOS/Suffixed/Part/DockingPortValue.cs index a58edd5311..4b34eb64d5 100644 --- a/src/kOS/Suffixed/Part/DockingPortValue.cs +++ b/src/kOS/Suffixed/Part/DockingPortValue.cs @@ -62,7 +62,7 @@ public PartValue GetPartner() } var otherVessel = VesselTarget.CreateOrGetExisting(otherNode.vessel, Shared); - foreach (var part in otherVessel.Parts) + foreach (PartValue part in otherVessel.Parts) { if (part.Part == otherNode.part) { diff --git a/src/kOS/Suffixed/Part/PartValue.cs b/src/kOS/Suffixed/Part/PartValue.cs index dc182f59d2..247d10f699 100644 --- a/src/kOS/Suffixed/Part/PartValue.cs +++ b/src/kOS/Suffixed/Part/PartValue.cs @@ -22,7 +22,7 @@ public class PartValue : Structure, IKOSTargetable public global::Part Part { get; private set; } public PartValue Parent { get; private set; } public DecouplerValue Decoupler { get; private set; } - public List Children { get; private set; } + public ListValue Children { get; private set; } public Structure ParentValue { get { return (Structure)Parent ?? StringValue.None; } } public Structure DecouplerValue { get { return (Structure)Decoupler ?? StringValue.None; } } public int DecoupledIn { get { return (Decoupler != null) ? Decoupler.Part.inverseStage : -1; } } @@ -37,7 +37,7 @@ internal PartValue(SharedObjects shared, global::Part part, PartValue parent, De Parent = parent; Decoupler = decoupler; RegisterInitializer(PartInitializeSuffixes); - Children = new List(); + Children = new ListValue(); } private void PartInitializeSuffixes() @@ -66,7 +66,7 @@ private void PartInitializeSuffixes() AddSuffix(new[] { "DECOUPLER", "SEPARATOR" }, new Suffix(() => DecouplerValue, "The part that will decouple/separate this part when activated")); AddSuffix(new[] { "DECOUPLEDIN", "SEPARATEDIN" }, new Suffix(() => DecoupledIn)); AddSuffix("HASPARENT", new Suffix(() => Part.parent != null, "Tells you if this part has a parent, is used to avoid null exception from PARENT")); - AddSuffix("CHILDREN", new Suffix(() => new ListValue(PartValueFactory.ConstructGeneric(Part.children, Shared)), "A LIST() of the children parts of this part")); + AddSuffix("CHILDREN", new Suffix(() => PartValueFactory.ConstructGeneric(Part.children, Shared), "A LIST() of the children parts of this part")); AddSuffix("DRYMASS", new Suffix(() => Part.GetDryMass(), "The Part's mass when empty")); AddSuffix("MASS", new Suffix(() => Part.CalculateCurrentMass(), "The Part's current mass")); AddSuffix("WETMASS", new Suffix(() => Part.GetWetMass(), "The Part's mass when full")); diff --git a/src/kOS/Suffixed/Part/PartValueFactory.cs b/src/kOS/Suffixed/Part/PartValueFactory.cs index d0ade7288b..5283429211 100644 --- a/src/kOS/Suffixed/Part/PartValueFactory.cs +++ b/src/kOS/Suffixed/Part/PartValueFactory.cs @@ -12,8 +12,8 @@ public static ListValue Construct(IEnumerable parts, SharedObjects return ListValue.CreateList(parts.Select(part => Construct(part, shared)).Where(p => p != null)); } - public static List ConstructGeneric(IEnumerable parts, SharedObjects shared) { - return parts.Select(part => Construct(part, shared)).Where(p => p != null).ToList(); + public static ListValue ConstructGeneric(IEnumerable parts, SharedObjects shared) { + return new ListValue(parts.Select(part => Construct(part, shared)).Where(p => p != null)); } public static PartValue Construct(global::Part part, SharedObjects shared) { diff --git a/src/kOS/Suffixed/StageValues.cs b/src/kOS/Suffixed/StageValues.cs index 5964603e83..32d885f2e2 100644 --- a/src/kOS/Suffixed/StageValues.cs +++ b/src/kOS/Suffixed/StageValues.cs @@ -18,7 +18,7 @@ public class StageValues : Structure private readonly SharedObjects shared; private HashSet partHash = new HashSet(); private PartSet partSet; - private List resList; + private ListValue resList; private Lexicon resLex; // Set by VesselTarget (from InvalidateParts) @@ -53,10 +53,10 @@ private void InitializeSuffixes() AddSuffix("DELTAV", new Suffix(() => new DeltaVCalc(shared, shared.Vessel.VesselDeltaV.GetStage(shared.Vessel.currentStage)))); } - private List GetResourceManifest() + private ListValue GetResourceManifest() { if (resList != null) return resList; - resList = new List(); + resList = new ListValue(); CreatePartSet(); var defs = PartResourceLibrary.Instance.resourceDefinitions; foreach (var def in defs) diff --git a/src/kOS/Suffixed/VesselTarget.Parts.cs b/src/kOS/Suffixed/VesselTarget.Parts.cs index a354a4ceaa..0067181d1d 100644 --- a/src/kOS/Suffixed/VesselTarget.Parts.cs +++ b/src/kOS/Suffixed/VesselTarget.Parts.cs @@ -19,9 +19,9 @@ partial class VesselTarget //..... [root, child1, child2, ..., part1-1, part1-2, ..., part2-1, ... heap ;) private PartValue rootPart; private DecouplerValue nextDecoupler; - private List allParts; - private List dockingPorts; - private List decouplers; + private ListValue allParts; + private ListValue dockingPorts; + private ListValue decouplers; private Dictionary partCache; private void InvalidateParts() @@ -53,7 +53,7 @@ public DecouplerValue NextDecoupler return nextDecoupler; } } - public List Parts + public ListValue Parts { get { @@ -62,7 +62,7 @@ public List Parts return allParts; } } - public List DockingPorts + public ListValue DockingPorts { get { @@ -71,7 +71,7 @@ public List DockingPorts return dockingPorts; } } - public List Decouplers + public ListValue Decouplers { get { @@ -98,12 +98,16 @@ private void ConstructParts() { rootPart = null; nextDecoupler = null; - allParts = new List(); - dockingPorts = new List(); - decouplers = new List(); + allParts = new ListValue(); + dockingPorts = new ListValue(); + decouplers = new ListValue(); partCache = new Dictionary(); ConstructPart(Vessel.rootPart, null, null); + + allParts.IsReadOnly = true; + dockingPorts.IsReadOnly = true; + decouplers.IsReadOnly = true; } private void ConstructPart(global::Part part, PartValue parent, DecouplerValue decoupler) { @@ -189,6 +193,7 @@ private void ConstructPart(global::Part part, PartValue parent, DecouplerValue d allParts.Add(self); foreach (var child in part.children) ConstructPart(child, self, decoupler); + self.Children.IsReadOnly = true; } private ListValue GetPartsDubbed(StringValue searchTerm) diff --git a/src/kOS/Suffixed/VesselTarget.cs b/src/kOS/Suffixed/VesselTarget.cs index d981f8323b..1ab591fd46 100644 --- a/src/kOS/Suffixed/VesselTarget.cs +++ b/src/kOS/Suffixed/VesselTarget.cs @@ -255,9 +255,9 @@ private void InitializeSuffixes() AddSuffix("PARTSTAGGED", new OneArgsSuffix(GetPartsTagged)); AddSuffix("PARTSTAGGEDPATTERN", new OneArgsSuffix(GetPartsTaggedPattern)); AddSuffix("ALLTAGGEDPARTS", new NoArgsSuffix(GetAllTaggedParts)); - AddSuffix("PARTS", new NoArgsSuffix(() => new ListValue(Parts))); - AddSuffix("DOCKINGPORTS", new NoArgsSuffix(() => new ListValue(DockingPorts))); - AddSuffix(new string[] { "DECOUPLERS", "SEPARATORS" }, new NoArgsSuffix(() => new ListValue(Decouplers))); + AddSuffix("PARTS", new NoArgsSuffix(() => Parts)); + AddSuffix("DOCKINGPORTS", new NoArgsSuffix(() => DockingPorts)); + AddSuffix(new string[] { "DECOUPLERS", "SEPARATORS" }, new NoArgsSuffix(() => Decouplers)); AddSuffix("ELEMENTS", new NoArgsSuffix(() => Vessel.PartList("elements", Shared))); AddSuffix("ENGINES", new NoArgsSuffix(() => Vessel.PartList("engines", Shared))); @@ -291,7 +291,7 @@ private void InitializeSuffixes() AddSuffix("CONTROLPART", new Suffix(() => PartValueFactory.Construct(GetControlPart(), Shared))); AddSuffix("DRYMASS", new Suffix(() => Vessel.GetDryMass(), "The Ship's mass when empty")); AddSuffix("WETMASS", new Suffix(() => Vessel.GetWetMass(), "The Ship's mass when full")); - AddSuffix("RESOURCES", new Suffix(() => new ListValue(AggregateResourceValue.FromVessel(Vessel, Shared)), "The Aggregate resources from every part on the craft")); + AddSuffix("RESOURCES", new Suffix(() => AggregateResourceValue.FromVessel(Vessel, Shared), "The Aggregate resources from every part on the craft")); AddSuffix("LOADDISTANCE", new Suffix(() => new LoadDistanceValue(Vessel))); AddSuffix("ISDEAD", new NoArgsSuffix(() => (Vessel == null || Vessel.state == Vessel.State.DEAD))); AddSuffix("STATUS", new Suffix(() => Vessel.situation.ToString())); @@ -353,11 +353,11 @@ public BoundsValue GetBoundsValue() { Direction myFacing = VesselUtils.GetFacing(Vessel); Quaternion inverseMyFacing = Quaternion.Inverse(myFacing.Rotation); - Vector rootOrigin = Parts[0].GetPosition(); + Vector rootOrigin = ((PartValue)Parts[0]).GetPosition(); Bounds unionBounds = new Bounds(); for (int pNum = 0; pNum < Parts.Count; ++pNum) { - PartValue p = Parts[pNum]; + PartValue p = (PartValue)Parts[pNum]; Vector partOriginOffsetInVesselBounds = p.GetPosition() - rootOrigin; Bounds b = p.GetBoundsValue().GetUnityBounds(); Vector partCenter = new Vector(b.center); @@ -382,7 +382,7 @@ public BoundsValue GetBoundsValue() // The above operation is expensive and should force the CPU to do a WAIT 0: Shared.Cpu.YieldProgram(new YieldFinishedNextTick()); - return new BoundsValue(min, max, delegate { return Parts[0].GetPosition(); }, delegate { return VesselUtils.GetFacing(Vessel); }, Shared); + return new BoundsValue(min, max, delegate { return ((PartValue)Parts[0]).GetPosition(); }, delegate { return VesselUtils.GetFacing(Vessel); }, Shared); } public void ThrowIfNotCPUVessel()