From 538a10a16077dac3414e52584aad52f1ecbd2405 Mon Sep 17 00:00:00 2001 From: pleroy Date: Sun, 19 Nov 2023 11:44:26 +0100 Subject: [PATCH 1/6] Use absl containers for data structures that are keyed by parts. This matters for vessels that have hundreds of parts. --- ksp_plugin/identification.hpp | 11 +++++------ ksp_plugin/plugin.cpp | 20 ++++++++++---------- ksp_plugin/plugin.hpp | 5 +++-- 3 files changed, 18 insertions(+), 18 deletions(-) diff --git a/ksp_plugin/identification.hpp b/ksp_plugin/identification.hpp index 82c312a8d0..a8589a6349 100644 --- a/ksp_plugin/identification.hpp +++ b/ksp_plugin/identification.hpp @@ -1,10 +1,9 @@ #pragma once -#include -#include #include #include +#include "absl/container/btree_map.h" #include "base/macros.hpp" // 🧙 For forward declarations. #include "base/not_null.hpp" @@ -20,7 +19,7 @@ namespace internal { using namespace principia::base::_not_null; // The GUID of a vessel, obtained by |v.id.ToString()| in C#. We use this as a -// key in an |std::map|. +// key in a map. using GUID = std::string; // Corresponds to KSP's |Part.flightID|, *not* to |Part.uid|. C#'s |uint| @@ -44,9 +43,9 @@ struct VesselByGUIDComparator { }; template -using PartTo = std::map, - T, - PartByPartIdComparator>; +using PartTo = absl::btree_map, + T, + PartByPartIdComparator>; using VesselSet = std::set, VesselByGUIDComparator>; using VesselConstSet = std::set, diff --git a/ksp_plugin/plugin.cpp b/ksp_plugin/plugin.cpp index e89bcf0be6..8b9ae19bc8 100644 --- a/ksp_plugin/plugin.cpp +++ b/ksp_plugin/plugin.cpp @@ -360,9 +360,9 @@ void Plugin::InsertOrKeepVessel(GUID const& vessel_guid, if (vit == vessels_.end()) { // Restore the zombie parameters if we have some, otherwise use the default. auto prediction_parameters = DefaultPredictionParameters(); - auto const pit = - zombie_prediction_adaptive_step_parameters_.find(vessel_guid); - if (pit != zombie_prediction_adaptive_step_parameters_.end()) { + if (auto const pit = + zombie_prediction_adaptive_step_parameters_.find(vessel_guid); + pit != zombie_prediction_adaptive_step_parameters_.end()) { prediction_parameters = pit->second; zombie_prediction_adaptive_step_parameters_.erase(pit); } @@ -447,9 +447,8 @@ void Plugin::InsertOrKeepLoadedPart( main_body_frame.FromThisFrameAtTime(previous_time) * world_to_main_body_centred; - auto it = part_id_to_vessel_.find(part_id); - bool const part_found = it != part_id_to_vessel_.end(); - if (part_found) { + if (auto const it = part_id_to_vessel_.find(part_id); + it != part_id_to_vessel_.end()) { not_null& associated_vessel = it->second; not_null const current_vessel = associated_vessel; if (vessel == current_vessel) { @@ -509,11 +508,12 @@ void Plugin::ApplyPartIntrinsicTorque( } bool Plugin::PartIsTruthful(PartId const part_id) const { - auto const it = part_id_to_vessel_.find(part_id); - if (it == part_id_to_vessel_.end()) { + if (auto const it = part_id_to_vessel_.find(part_id); + it == part_id_to_vessel_.end()) { return false; + } else { + return it->second->part(part_id)->truthful(); } - return it->second->part(part_id)->truthful(); } void Plugin::PrepareToReportCollisions() { @@ -662,7 +662,7 @@ void Plugin::SetPartApparentRigidMotion( Identity()(angular_velocity_of_world_), Apparent::unmoving}; - not_null vessel = FindOrDie(part_id_to_vessel_, part_id); + not_null const vessel = FindOrDie(part_id_to_vessel_, part_id); CHECK(is_loaded(vessel)); not_null const part = vessel->part(part_id); CHECK(part->is_piled_up()); diff --git a/ksp_plugin/plugin.hpp b/ksp_plugin/plugin.hpp index ca40755880..f7cc912f00 100644 --- a/ksp_plugin/plugin.hpp +++ b/ksp_plugin/plugin.hpp @@ -11,6 +11,7 @@ #include #include +#include "absl/container/flat_hash_map.h" #include "absl/status/status.h" #include "base/disjoint_sets.hpp" #include "base/monostable.hpp" @@ -99,7 +100,7 @@ using namespace principia::quantities::_quantities; using namespace principia::quantities::_si; // The index of a body in |FlightGlobals.Bodies|, obtained by -// |b.flightGlobalsIndex| in C#. We use this as a key in an |std::map|. +// |b.flightGlobalsIndex| in C#. We use this as a key in a map. using Index = int; class Plugin { @@ -536,7 +537,7 @@ class Plugin { GUIDToOwnedVessel vessels_; // For each part, the vessel that this part belongs to. The part is guaranteed // to be in the parts() map of the vessel, and owned by it. - std::map> part_id_to_vessel_; + absl::flat_hash_map> part_id_to_vessel_; IndexToOwnedCelestial celestials_; // Not null after initialization. From eb2fd3f19eed9d625d049ebd0c8ab187ab667a4b Mon Sep 17 00:00:00 2001 From: pleroy Date: Sun, 19 Nov 2023 13:38:46 +0100 Subject: [PATCH 2/6] Cache the inverse of a RigidMotion. --- physics/rigid_motion.hpp | 9 ++++++++- physics/rigid_motion_body.hpp | 13 ++++++++----- 2 files changed, 16 insertions(+), 6 deletions(-) diff --git a/physics/rigid_motion.hpp b/physics/rigid_motion.hpp index ce55c0484e..2880cb1a70 100644 --- a/physics/rigid_motion.hpp +++ b/physics/rigid_motion.hpp @@ -68,7 +68,7 @@ class RigidMotion final { DegreesOfFreedom operator()( DegreesOfFreedom const& degrees_of_freedom) const; - RigidMotion Inverse() const; + RigidMotion const& Inverse() const; template typename SimilarMotion> SimilarMotion Forget() const; @@ -103,6 +103,13 @@ class RigidMotion final { // d/dt rigid_transformation⁻¹(ToFrame::origin). Velocity velocity_of_to_frame_origin_; + // Cache the inverse as it is used often and is fairly expensive to compute. + // We must use a pointer because the class |RigidMotion| is not completely + // defined at this place. We want |RigidMotion| to be copyable, hence the use + // of a |shared_ptr| which has the right semantics (the inverse is computed + // once and shared among copies). + mutable std::shared_ptr> inverse_; + template friend class RigidMotion; diff --git a/physics/rigid_motion_body.hpp b/physics/rigid_motion_body.hpp index 10edf92b23..c57dfc5de4 100644 --- a/physics/rigid_motion_body.hpp +++ b/physics/rigid_motion_body.hpp @@ -92,12 +92,15 @@ DegreesOfFreedom RigidMotion::operator()( } template -RigidMotion +RigidMotion const& RigidMotion::Inverse() const { - return RigidMotion( - rigid_transformation_.Inverse(), - -orthogonal_map()(angular_velocity_of_to_frame_), - (*this)({FromFrame::origin, FromFrame::unmoving}).velocity()); + if (inverse_ == nullptr) { + inverse_ = std::make_shared>( + rigid_transformation_.Inverse(), + -orthogonal_map()(angular_velocity_of_to_frame_), + (*this)({FromFrame::origin, FromFrame::unmoving}).velocity()); + } + return *inverse_; } template From 33a210b38edc632ab18d770734571acbfb99dc7a Mon Sep 17 00:00:00 2001 From: pleroy Date: Mon, 20 Nov 2023 23:50:04 +0100 Subject: [PATCH 3/6] Absl containers in the vessel. --- ksp_plugin/vessel.hpp | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/ksp_plugin/vessel.hpp b/ksp_plugin/vessel.hpp index ffc9c42b2f..40cf029b62 100644 --- a/ksp_plugin/vessel.hpp +++ b/ksp_plugin/vessel.hpp @@ -1,14 +1,13 @@ #pragma once -#include -#include #include #include -#include #include #include #include +#include "absl/container/btree_map.h" +#include "absl/container/flat_hash_set.h" #include "absl/status/status.h" #include "absl/synchronization/mutex.h" #include "base/not_null.hpp" @@ -405,8 +404,10 @@ class Vessel { // non-collapsible as we don't know anything about it. bool is_collapsible_ = false; - std::map>> parts_; - std::set kept_parts_; + // Use an ordered map for consistent order of iteration (e.g., when computing + // the barycentre). + absl::btree_map>> parts_; + absl::flat_hash_set kept_parts_; // The vessel trajectory is made of a number of history segments ending at the // backstory and (most of the time) the psychohistory and prediction. The From 3baadfdddbdebdc4c718d5208dd96b90ae2692b5 Mon Sep 17 00:00:00 2001 From: pleroy Date: Wed, 22 Nov 2023 19:16:08 +0100 Subject: [PATCH 4/6] Bad iterators. --- ksp_plugin/plugin.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/ksp_plugin/plugin.cpp b/ksp_plugin/plugin.cpp index 8b9ae19bc8..dd8bedbd61 100644 --- a/ksp_plugin/plugin.cpp +++ b/ksp_plugin/plugin.cpp @@ -1693,10 +1693,11 @@ void Plugin::AddPart(not_null const vessel, PartId const part_id, std::string const& name, Args... args) { - auto const [it, inserted] = part_id_to_vessel_.emplace(part_id, vessel); + auto const [_, inserted] = part_id_to_vessel_.emplace(part_id, vessel); CHECK(inserted) << NAMED(part_id); - auto deletion_callback = [it = it, &map = part_id_to_vessel_] { - map.erase(it); + auto deletion_callback = [part_id, &map = part_id_to_vessel_] { + // This entails a lookup, but iterators are not stable in |flat_hash_map|. + map.erase(part_id); }; auto part = make_not_null_unique(part_id, name, From ecee34ac52609521d973d0cabed791b3f5ec1777 Mon Sep 17 00:00:00 2001 From: pleroy Date: Wed, 22 Nov 2023 22:30:29 +0100 Subject: [PATCH 5/6] C# microoptimizations. --- ksp_plugin_adapter/ksp_plugin_adapter.cs | 76 +++++++++++++----------- 1 file changed, 42 insertions(+), 34 deletions(-) diff --git a/ksp_plugin_adapter/ksp_plugin_adapter.cs b/ksp_plugin_adapter/ksp_plugin_adapter.cs index ae46eeaab0..c7f9168e0a 100644 --- a/ksp_plugin_adapter/ksp_plugin_adapter.cs +++ b/ksp_plugin_adapter/ksp_plugin_adapter.cs @@ -537,9 +537,10 @@ private void UpdateVessel(Vessel vessel, double universal_time) { var part_rotation = (UnityEngine.QuaternionD)part_actual_motion.r; var part_angular_velocity = (Vector3d)part_actual_motion.w; part_rb.position = root_part_rb.position + part_position; - part_rb.transform.position = root_part_rb.position + part_position; part_rb.rotation = part_rotation; - part_rb.transform.rotation = part_rotation; + part_rb.transform.SetPositionAndRotation( + root_part_rb.position + part_position, + part_rotation); part_rb.velocity = root_part_rb.velocity + part_velocity; part_rb.angularVelocity = part_angular_velocity; } @@ -587,10 +588,16 @@ private bool is_manageable(Vessel vessel) { return UnmanageabilityReasons(vessel) == null; } - public static bool is_lit_solid_booster(ModuleEngines module) { - return module != null && - module.EngineIgnited && - module.engineType == EngineType.SolidBooster; + public static bool is_lit_solid_booster(Part part) { + foreach (PartModule module in part.Modules) { + var module_engines = module as ModuleEngines; + if (module_engines != null && + module_engines.EngineIgnited && + module_engines.engineType == EngineType.SolidBooster) { + return true; + } + } + return false; } private string UnmanageabilityReasons(Vessel vessel) { @@ -1152,8 +1159,8 @@ private System.Collections.IEnumerator WaitedForFixedUpdate() { yield break; } - // We are going to touch plenty of |Transform|s, so we will prevent - // Unity from syncing with the physics system all the time. + // We are not changing the |Transform|s, but if we don't disable auto- + // sync the profiles show |SyncColliderTransform|. UnityEngine.Physics.autoSyncTransforms = false; double Δt = Planetarium.TimeScale * Planetarium.fetch.fixedDeltaTime; @@ -1169,6 +1176,7 @@ private System.Collections.IEnumerator WaitedForFixedUpdate() { // |WaitForFixedUpdate|, since some may be destroyed (by collisions) during // the physics step. See also #1281. foreach (Vessel vessel in FlightGlobals.Vessels) { + int main_body_index = vessel.mainBody.flightGlobalsIndex; string vessel_guid = vessel.id.ToString(); string unmanageability_reasons = UnmanageabilityReasons(vessel); if (unmanageability_reasons != null) { @@ -1181,7 +1189,7 @@ private System.Collections.IEnumerator WaitedForFixedUpdate() { plugin_.InsertOrKeepVessel(vessel_guid, vessel.vesselName, - vessel.mainBody.flightGlobalsIndex, + main_body_index, !vessel.packed, out bool inserted); if (!vessel.packed) { @@ -1189,11 +1197,9 @@ private System.Collections.IEnumerator WaitedForFixedUpdate() { foreach (Part part in vessel.parts.Where(PartIsFaithful)) { UnityEngine.Rigidbody part_rb = part.rb; uint part_id = part.flightID; - QP degrees_of_freedom; - if (part_id_to_degrees_of_freedom_.ContainsKey(part_id)) { - degrees_of_freedom = - part_id_to_degrees_of_freedom_[part_id]; - } else { + if (!part_id_to_degrees_of_freedom_.TryGetValue( + part_id, + out QP degrees_of_freedom)) { // Assumptions about the invariants of KSP will invariably fail. // This is a graceful fallback. Log.Error("Unpacked part " + part.name + " (" + @@ -1219,35 +1225,33 @@ private System.Collections.IEnumerator WaitedForFixedUpdate() { (XYZ)part_rb.centerOfMass, (XYZ)part_rb.inertiaTensor, (WXYZ)part_rb.inertiaTensorRotation, - (from PartModule module in part.Modules - select module as ModuleEngines).Any(is_lit_solid_booster), + is_lit_solid_booster(part), vessel_guid, - vessel.mainBody.flightGlobalsIndex, + main_body_index, main_body_degrees_of_freedom, degrees_of_freedom, (WXYZ)part_rb.rotation, (XYZ)part_rb.angularVelocity, Δt); - if (part_id_to_intrinsic_torque_.ContainsKey(part_id)) { - plugin_.PartApplyIntrinsicTorque(part_id, - (XYZ)part_id_to_intrinsic_torque_ - [part_id]); + if (part_id_to_intrinsic_torque_.TryGetValue( + part_id, + out Vector3d intrinsic_torque)) { + plugin_.PartApplyIntrinsicTorque(part_id, (XYZ)intrinsic_torque); } - if (part_id_to_intrinsic_force_.ContainsKey(part_id)) { + if (part_id_to_intrinsic_force_.TryGetValue(part_id, + out Vector3d intrinsic_force)) { // When a Kerbal is doing an EVA and holding on to a ladder, the // ladder imbues them with their weight at the location of the // vessel to which the ladder is attached. This leads to fantastic // effects where doing an EVA accelerates the vessel, see #1415. // Just say no to stupidity. if (!(vessel.isEVA && vessel.evaController.OnALadder)) { - plugin_.PartApplyIntrinsicForce( - part_id, - (XYZ)part_id_to_intrinsic_force_[part_id]); + plugin_.PartApplyIntrinsicForce(part_id, (XYZ)intrinsic_force); } } - if (part_id_to_intrinsic_forces_.ContainsKey(part_id)) { - foreach (var part_centred_force in part_id_to_intrinsic_forces_[ - part_id]) { + if (part_id_to_intrinsic_forces_.TryGetValue(part_id, + out PartCentredForceHolder[] intrinsic_forces)) { + foreach (var part_centred_force in intrinsic_forces) { plugin_.PartApplyIntrinsicForceAtPosition( part_id, (XYZ)part_centred_force.force, @@ -1437,6 +1441,10 @@ private System.Collections.IEnumerator WaitedForFixedUpdate() { if (has_active_manageable_vessel() && !FlightGlobals.ActiveVessel.packed && plugin_.HasVessel(FlightGlobals.ActiveVessel.id.ToString())) { + // We are going to touch plenty of |Transform|s, so we will prevent + // Unity from syncing with the physics system all the time. + UnityEngine.Physics.autoSyncTransforms = false; + Vector3d q_correction_at_root_part = Vector3d.zero; Vector3d v_correction_at_root_part = Vector3d.zero; CelestialBody main_body = FlightGlobals.ActiveVessel.mainBody; @@ -1480,9 +1488,10 @@ private System.Collections.IEnumerator WaitedForFixedUpdate() { // See https://github.com/mockingbirdnest/Principia/pull/1427, // https://github.com/mockingbirdnest/Principia/issues/1307#issuecomment-478337241. part_rb.position = part_position; - part_rb.transform.position = part_position; part_rb.rotation = part_rotation; - part_rb.transform.rotation = part_rotation; + part_rb.transform.SetPositionAndRotation( + part_position, + part_rotation); part_rb.velocity = part_velocity; part_rb.angularVelocity = part_angular_velocity; @@ -1704,10 +1713,9 @@ private void JaiFailliAttendre() { Part physical_parent = closest_physical_parent(part); if (part.bodyLiftLocalVector != UnityEngine.Vector3.zero || part.dragVector != UnityEngine.Vector3.zero) { - if (part_id_to_intrinsic_forces_.ContainsKey( - physical_parent.flightID)) { - var previous_holder = - part_id_to_intrinsic_forces_[physical_parent.flightID]; + if (part_id_to_intrinsic_forces_.TryGetValue( + physical_parent.flightID, + out PartCentredForceHolder[] previous_holder)) { part_id_to_intrinsic_forces_[physical_parent.flightID] = new PartCentredForceHolder[previous_holder.Length + 2]; previous_holder.CopyTo( From ffc19828cef6d9489f01f0e35fc865820adbb035 Mon Sep 17 00:00:00 2001 From: pleroy Date: Wed, 22 Nov 2023 22:31:51 +0100 Subject: [PATCH 6/6] Revert. --- ksp_plugin_adapter/ksp_plugin_adapter.cs | 8 ++------ physics/rigid_motion.hpp | 9 +-------- physics/rigid_motion_body.hpp | 13 +++++-------- 3 files changed, 8 insertions(+), 22 deletions(-) diff --git a/ksp_plugin_adapter/ksp_plugin_adapter.cs b/ksp_plugin_adapter/ksp_plugin_adapter.cs index c7f9168e0a..98f6417e8f 100644 --- a/ksp_plugin_adapter/ksp_plugin_adapter.cs +++ b/ksp_plugin_adapter/ksp_plugin_adapter.cs @@ -1159,8 +1159,8 @@ private System.Collections.IEnumerator WaitedForFixedUpdate() { yield break; } - // We are not changing the |Transform|s, but if we don't disable auto- - // sync the profiles show |SyncColliderTransform|. + // We are going to touch plenty of |Transform|s, so we will prevent + // Unity from syncing with the physics system all the time. UnityEngine.Physics.autoSyncTransforms = false; double Δt = Planetarium.TimeScale * Planetarium.fetch.fixedDeltaTime; @@ -1441,10 +1441,6 @@ private System.Collections.IEnumerator WaitedForFixedUpdate() { if (has_active_manageable_vessel() && !FlightGlobals.ActiveVessel.packed && plugin_.HasVessel(FlightGlobals.ActiveVessel.id.ToString())) { - // We are going to touch plenty of |Transform|s, so we will prevent - // Unity from syncing with the physics system all the time. - UnityEngine.Physics.autoSyncTransforms = false; - Vector3d q_correction_at_root_part = Vector3d.zero; Vector3d v_correction_at_root_part = Vector3d.zero; CelestialBody main_body = FlightGlobals.ActiveVessel.mainBody; diff --git a/physics/rigid_motion.hpp b/physics/rigid_motion.hpp index 2880cb1a70..ce55c0484e 100644 --- a/physics/rigid_motion.hpp +++ b/physics/rigid_motion.hpp @@ -68,7 +68,7 @@ class RigidMotion final { DegreesOfFreedom operator()( DegreesOfFreedom const& degrees_of_freedom) const; - RigidMotion const& Inverse() const; + RigidMotion Inverse() const; template typename SimilarMotion> SimilarMotion Forget() const; @@ -103,13 +103,6 @@ class RigidMotion final { // d/dt rigid_transformation⁻¹(ToFrame::origin). Velocity velocity_of_to_frame_origin_; - // Cache the inverse as it is used often and is fairly expensive to compute. - // We must use a pointer because the class |RigidMotion| is not completely - // defined at this place. We want |RigidMotion| to be copyable, hence the use - // of a |shared_ptr| which has the right semantics (the inverse is computed - // once and shared among copies). - mutable std::shared_ptr> inverse_; - template friend class RigidMotion; diff --git a/physics/rigid_motion_body.hpp b/physics/rigid_motion_body.hpp index c57dfc5de4..10edf92b23 100644 --- a/physics/rigid_motion_body.hpp +++ b/physics/rigid_motion_body.hpp @@ -92,15 +92,12 @@ DegreesOfFreedom RigidMotion::operator()( } template -RigidMotion const& +RigidMotion RigidMotion::Inverse() const { - if (inverse_ == nullptr) { - inverse_ = std::make_shared>( - rigid_transformation_.Inverse(), - -orthogonal_map()(angular_velocity_of_to_frame_), - (*this)({FromFrame::origin, FromFrame::unmoving}).velocity()); - } - return *inverse_; + return RigidMotion( + rigid_transformation_.Inverse(), + -orthogonal_map()(angular_velocity_of_to_frame_), + (*this)({FromFrame::origin, FromFrame::unmoving}).velocity()); } template