From 18cc5876fe93ccb44e962d73d28b734edb947885 Mon Sep 17 00:00:00 2001 From: Patrick Kenneally Date: Fri, 9 Dec 2022 16:07:04 -0700 Subject: [PATCH 1/5] Remove redundant return statements Former-commit-id: abf9001d6345caf89a3fe3a6d75dfba1742ad5ed Former-commit-id: 1a25a192811f5d584c42e40a9e14f4275345174c --- src/architecture/utilities/keplerianOrbit.cpp | 7 ------- 1 file changed, 7 deletions(-) diff --git a/src/architecture/utilities/keplerianOrbit.cpp b/src/architecture/utilities/keplerianOrbit.cpp index df7629e65c..7e6b2843f6 100644 --- a/src/architecture/utilities/keplerianOrbit.cpp +++ b/src/architecture/utilities/keplerianOrbit.cpp @@ -33,7 +33,6 @@ KeplerianOrbit::KeplerianOrbit() this->right_ascension = 0.0; this->mu = MU_EARTH; this->change_orbit(); - return; } /*! The constructor requires orbital elements and a planet */ @@ -47,7 +46,6 @@ KeplerianOrbit::KeplerianOrbit(classicElements oe, GravBodyData* planet){ this->right_ascension = oe.Omega; this->mu = planet->mu; this->change_orbit(); - return; } /*! The copy constructor works with python copy*/ @@ -61,13 +59,11 @@ KeplerianOrbit::KeplerianOrbit(const KeplerianOrbit &orig){ this->mu = orig.mu; this->planet = orig.planet; this->change_orbit(); - return; } /*! Generic Destructor */ KeplerianOrbit::~KeplerianOrbit() { - return; } /*! @@ -179,7 +175,6 @@ void KeplerianOrbit::change_orbit(){ this->orbital_energy = -this->mu / 2 / this->a(); this->r_apogee = this->a() * (1 + this->e()); this->r_perigee = this->a() * (1 - this->e()); - return; } /*! This method only changes the outputs dependent on true anomaly so that one * orbit may be queried at various points along the orbit*/ @@ -195,14 +190,12 @@ void KeplerianOrbit::change_f(){ this->eccentric_anomaly = safeAcos((this->e() + cos(this->f()) / (1 + this->e() * cos(this->f())))); // this->mean_anomaly = this->E() - this->e() * sin(this->E()); // this->flight_path_angle = safeAcos(sqrt((1 - pow(this->e(), 2)) / (1 - pow(this->e(), 2)*pow(cos(this->E()), 2)))); // - return; } /*! This method sets the planet being orbited */ void KeplerianOrbit::set_planet(GravBodyData *plt){ this->planet = plt; this->mu = plt->mu; - return; } From 02bc2f13239546e680e560ecb10a2984b6bf32e8 Mon Sep 17 00:00:00 2001 From: Patrick Kenneally Date: Fri, 9 Dec 2022 16:22:09 -0700 Subject: [PATCH 2/5] Remove unused imports --- .../utilitiesSelfCheck/_UnitTest/test_keplerianOrbit.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/architecture/utilitiesSelfCheck/_UnitTest/test_keplerianOrbit.py b/src/architecture/utilitiesSelfCheck/_UnitTest/test_keplerianOrbit.py index c1e5d8660c..5234d60f15 100644 --- a/src/architecture/utilitiesSelfCheck/_UnitTest/test_keplerianOrbit.py +++ b/src/architecture/utilitiesSelfCheck/_UnitTest/test_keplerianOrbit.py @@ -26,7 +26,6 @@ # Creation Date: Sept 10 2019 # -import pytest import os, inspect from Basilisk.simulation import gravityEffector from Basilisk.utilities import orbitalMotion From 47d4e1b2709e7ec892d94ce1fc54a4cd5c1517b2 Mon Sep 17 00:00:00 2001 From: Patrick Kenneally Date: Thu, 15 Dec 2022 18:47:40 -0700 Subject: [PATCH 3/5] Decouple by removing planet memeber variable --- src/architecture/utilities/keplerianOrbit.cpp | 15 ++++++--------- src/architecture/utilities/keplerianOrbit.h | 6 ++---- .../_UnitTest/test_keplerianOrbit.py | 5 +---- 3 files changed, 9 insertions(+), 17 deletions(-) diff --git a/src/architecture/utilities/keplerianOrbit.cpp b/src/architecture/utilities/keplerianOrbit.cpp index 7e6b2843f6..0c7c668550 100644 --- a/src/architecture/utilities/keplerianOrbit.cpp +++ b/src/architecture/utilities/keplerianOrbit.cpp @@ -35,16 +35,15 @@ KeplerianOrbit::KeplerianOrbit() this->change_orbit(); } -/*! The constructor requires orbital elements and a planet */ -KeplerianOrbit::KeplerianOrbit(classicElements oe, GravBodyData* planet){ - this->set_planet(planet); +/*! The constructor requires orbital elements and a gravitational constant value */ +KeplerianOrbit::KeplerianOrbit(classicElements oe, const double mu){ this->semi_major_axis = oe.a; this->eccentricity = oe.e; this->inclination = oe.i; this->true_anomaly = oe.f; this->argument_of_periapsis = oe.omega; this->right_ascension = oe.Omega; - this->mu = planet->mu; + this->mu = mu; this->change_orbit(); } @@ -57,7 +56,6 @@ KeplerianOrbit::KeplerianOrbit(const KeplerianOrbit &orig){ this->argument_of_periapsis = orig.argument_of_periapsis; this->right_ascension = orig.right_ascension; this->mu = orig.mu; - this->planet = orig.planet; this->change_orbit(); } @@ -192,10 +190,9 @@ void KeplerianOrbit::change_f(){ this->flight_path_angle = safeAcos(sqrt((1 - pow(this->e(), 2)) / (1 - pow(this->e(), 2)*pow(cos(this->E()), 2)))); // } -/*! This method sets the planet being orbited */ -void KeplerianOrbit::set_planet(GravBodyData *plt){ - this->planet = plt; - this->mu = plt->mu; +/*! This method sets the gravitational constants of the body being orbited */ +void KeplerianOrbit::set_mu(const double mu){ + this->mu = mu; } diff --git a/src/architecture/utilities/keplerianOrbit.h b/src/architecture/utilities/keplerianOrbit.h index 762ac76644..f6c02b32ae 100644 --- a/src/architecture/utilities/keplerianOrbit.h +++ b/src/architecture/utilities/keplerianOrbit.h @@ -20,7 +20,6 @@ #pragma once #include -#include "simulation/dynamics/_GeneralModuleFiles/gravityEffector.h" #include @@ -30,7 +29,7 @@ class KeplerianOrbit { public: KeplerianOrbit(); - KeplerianOrbit(classicElements oe, GravBodyData* planet); + KeplerianOrbit(classicElements oe, const double mu); KeplerianOrbit(const KeplerianOrbit &orig); ~KeplerianOrbit(); @@ -60,7 +59,7 @@ class KeplerianOrbit { double rDot(); double c3(); classicElements oe(); - void set_planet(GravBodyData* plt); + void set_mu(const double mu); void set_a(double a); void set_e(double e); void set_i(double i); @@ -69,7 +68,6 @@ class KeplerianOrbit { void set_f(double f); private: - GravBodyData* planet; double mu; double semi_major_axis; double eccentricity; diff --git a/src/architecture/utilitiesSelfCheck/_UnitTest/test_keplerianOrbit.py b/src/architecture/utilitiesSelfCheck/_UnitTest/test_keplerianOrbit.py index 5234d60f15..eaecca6c86 100644 --- a/src/architecture/utilitiesSelfCheck/_UnitTest/test_keplerianOrbit.py +++ b/src/architecture/utilitiesSelfCheck/_UnitTest/test_keplerianOrbit.py @@ -27,7 +27,6 @@ # import os, inspect -from Basilisk.simulation import gravityEffector from Basilisk.utilities import orbitalMotion from Basilisk.architecture import keplerianOrbit from copy import copy @@ -80,9 +79,7 @@ def unitKeplerianOrbit(show_plots=False): # constructor with arguments oe = orb.oe() - earth = gravityEffector.GravBodyData() - earth.mu = orbitalMotion.MU_EARTH - orb2 = keplerianOrbit.KeplerianOrbit(oe, earth) + orb2 = keplerianOrbit.KeplerianOrbit(oe, orbitalMotion.MU_EARTH) assert orb2.r_BP_P() == orb.r_BP_P() if not orb2.r_BP_P() == orb.r_BP_P(): testFailCount += 1 From 4fbb21a5a03622a152adaad8ec2d3104c4a7e050 Mon Sep 17 00:00:00 2001 From: Patrick Kenneally Date: Fri, 9 Dec 2022 18:49:42 -0700 Subject: [PATCH 4/5] Initialize values directly in the class This change seek primarily seeks to establish discipline in preventing "use-before-set" possibilities. Generally member data should be initialized in-class or in a constructor initialization list. See C++ Core Guidelines C.48 and C.49 --- src/architecture/utilities/keplerianOrbit.cpp | 56 +++++------- src/architecture/utilities/keplerianOrbit.h | 87 ++++++++++--------- 2 files changed, 68 insertions(+), 75 deletions(-) diff --git a/src/architecture/utilities/keplerianOrbit.cpp b/src/architecture/utilities/keplerianOrbit.cpp index 0c7c668550..196d2c2662 100644 --- a/src/architecture/utilities/keplerianOrbit.cpp +++ b/src/architecture/utilities/keplerianOrbit.cpp @@ -18,20 +18,12 @@ */ #include "keplerianOrbit.h" -#include "architecture/utilities/astroConstants.h" #include #include /*! This constructor initialized to an arbitrary orbit */ KeplerianOrbit::KeplerianOrbit() { - this->semi_major_axis = 1E5; - this->eccentricity = 1E-5; - this->inclination = 0.0; - this->true_anomaly = 0.0; - this->argument_of_periapsis = 0.0; - this->right_ascension = 0.0; - this->mu = MU_EARTH; this->change_orbit(); } @@ -67,14 +59,14 @@ KeplerianOrbit::~KeplerianOrbit() /*! body position vector relative to planet */ -Eigen::Vector3d KeplerianOrbit::r_BP_P(){ +Eigen::Vector3d KeplerianOrbit::r_BP_P() const { return this->position_BP_P; } /*! body velocity vector relative to planet */ -Eigen::Vector3d KeplerianOrbit::v_BP_P(){ +Eigen::Vector3d KeplerianOrbit::v_BP_P() const{ return this->velocity_BP_P; } @@ -82,52 +74,52 @@ Eigen::Vector3d KeplerianOrbit::v_BP_P(){ /*! angular momentum of body relative to planet */ -Eigen::Vector3d KeplerianOrbit::h_BP_P(){ +Eigen::Vector3d KeplerianOrbit::h_BP_P() const{ return this->orbital_angular_momentum_P; } /*! return mean anomaly angle */ -double KeplerianOrbit::M(){return this->mean_anomaly;} +double KeplerianOrbit::M() const {return this->mean_anomaly;} /*! return mean orbit rate */ -double KeplerianOrbit::n(){return this->mean_motion;}; //!< return mean orbit rate +double KeplerianOrbit::n() const {return this->mean_motion;}; //!< return mean orbit rate /*! return orbit period */ -double KeplerianOrbit::P(){return this->orbital_period;}; //!< return orbital period +double KeplerianOrbit::P() const {return this->orbital_period;}; //!< return orbital period /*! return true anomaly */ -double KeplerianOrbit::f(){return this->true_anomaly;}; //!< return true anomaly +double KeplerianOrbit::f() const {return this->true_anomaly;}; //!< return true anomaly /*! return true anomaly rate */ -double KeplerianOrbit::fDot(){return this->true_anomaly_rate;}; +double KeplerianOrbit::fDot() const {return this->true_anomaly_rate;}; /*! return right ascencion of the ascending node */ -double KeplerianOrbit::RAAN(){return this->right_ascension;}; +double KeplerianOrbit::RAAN() const {return this->right_ascension;}; /*! return argument of periapses */ -double KeplerianOrbit::omega(){return this->argument_of_periapsis;}; +double KeplerianOrbit::omega() const {return this->argument_of_periapsis;}; /*! return inclination angle */ -double KeplerianOrbit::i(){return this->inclination;}; +double KeplerianOrbit::i() const {return this->inclination;}; /*! return eccentricty */ -double KeplerianOrbit::e(){return this->eccentricity;}; +double KeplerianOrbit::e() const {return this->eccentricity;}; /*! return semi-major axis */ -double KeplerianOrbit::a(){return this->semi_major_axis;}; +double KeplerianOrbit::a() const {return this->semi_major_axis;}; /*! return orbital angular momentum magnitude */ -double KeplerianOrbit::h(){return this->h_BP_P().norm();}; +double KeplerianOrbit::h() const {return this->h_BP_P().norm();}; /*! return orbital energy */ double KeplerianOrbit::Energy(){return this->orbital_energy;}; /*! return orbit radius */ -double KeplerianOrbit::r(){return this->r_BP_P().norm();}; +double KeplerianOrbit::r() const {return this->r_BP_P().norm();}; /*! return velocity magnitude */ -double KeplerianOrbit::v(){return this->v_BP_P().norm();}; +double KeplerianOrbit::v() const {return this->v_BP_P().norm();}; /*! return radius at apoapses */ -double KeplerianOrbit::r_a(){return this->r_apogee;}; +double KeplerianOrbit::r_a() const {return this->r_apogee;}; /*! return radius at periapses */ -double KeplerianOrbit::r_p(){return this->r_perigee;}; +double KeplerianOrbit::r_p() const {return this->r_perigee;}; /*! return flight path angle */ -double KeplerianOrbit::fpa(){return this->flight_path_angle;}; +double KeplerianOrbit::fpa() const {return this->flight_path_angle;}; /*! return eccentric anomaly angle */ -double KeplerianOrbit::E(){return this->eccentric_anomaly;}; +double KeplerianOrbit::E() const {return this->eccentric_anomaly;}; /*! return semi-latus rectum or the parameter */ -double KeplerianOrbit::p(){return this->semi_parameter;}; +double KeplerianOrbit::p() const {return this->semi_parameter;}; /*! return radius rate */ -double KeplerianOrbit::rDot(){return this->radial_rate;}; +double KeplerianOrbit::rDot() const {return this->radial_rate;}; /*! return escape velocity */ -double KeplerianOrbit::c3(){return this->v_infinity;}; +double KeplerianOrbit::c3() const {return this->v_infinity;}; /*! set semi-major axis */ void KeplerianOrbit::set_a(double a){this->semi_major_axis = a; this->change_orbit();}; @@ -185,7 +177,7 @@ void KeplerianOrbit::change_f(){ this->velocity_BP_P = cArray2EigenVector3d(v); // this->true_anomaly_rate = this->n() * pow(this->a(), 2) * sqrt(1 - pow(this->e(), 2)) / pow(this->r(), 2); // this->radial_rate = this->r() * this->fDot() * this->e() * sin(this->f()) / (1 + this->e() * cos(this->f())); // - this->eccentric_anomaly = safeAcos((this->e() + cos(this->f()) / (1 + this->e() * cos(this->f())))); // + this->eccentric_anomaly = safeAcos(this->e() + cos(this->f()) / (1 + this->e() * cos(this->f()))); // this->mean_anomaly = this->E() - this->e() * sin(this->E()); // this->flight_path_angle = safeAcos(sqrt((1 - pow(this->e(), 2)) / (1 - pow(this->e(), 2)*pow(cos(this->E()), 2)))); // } diff --git a/src/architecture/utilities/keplerianOrbit.h b/src/architecture/utilities/keplerianOrbit.h index f6c02b32ae..36a3cc6f4b 100644 --- a/src/architecture/utilities/keplerianOrbit.h +++ b/src/architecture/utilities/keplerianOrbit.h @@ -21,6 +21,7 @@ #include #include +#include "architecture/utilities/astroConstants.h" //! @brief The KeplerianOrbit class represents an elliptical orbit and provides a coherent set of @@ -34,30 +35,30 @@ class KeplerianOrbit { ~KeplerianOrbit(); - Eigen::Vector3d r_BP_P(); //!< body position vector relative to planet - Eigen::Vector3d v_BP_P(); //!< body velocity vector relative to planet - Eigen::Vector3d h_BP_P(); //!< angular momentum of body relative to planet - double M(); - double n(); - double P(); - double f(); - double fDot(); - double RAAN(); - double omega(); - double i(); - double e(); - double a(); - double h(); + Eigen::Vector3d r_BP_P() const; //!< body position vector relative to planet + Eigen::Vector3d v_BP_P() const; //!< body velocity vector relative to planet + Eigen::Vector3d h_BP_P() const; //!< angular momentum of body relative to planet + double M() const; + double n() const; + double P() const; + double f() const; + double fDot() const; + double RAAN() const; + double omega() const; + double i() const; + double e() const; + double a() const; + double h() const; double Energy(); - double r(); - double v(); - double r_a(); - double r_p(); - double fpa(); - double E(); - double p(); - double rDot(); - double c3(); + double r() const; + double v() const; + double r_a() const; + double r_p() const; + double fpa() const; + double E() const; + double p() const; + double rDot() const; + double c3() const; classicElements oe(); void set_mu(const double mu); void set_a(double a); @@ -68,26 +69,26 @@ class KeplerianOrbit { void set_f(double f); private: - double mu; - double semi_major_axis; - double eccentricity; - double inclination; - double argument_of_periapsis; - double right_ascension; - double true_anomaly; - double true_anomaly_rate; - double orbital_period; - double orbital_energy; - double v_infinity; - double orbit_radius; - double radial_rate; - double r_apogee; - double r_perigee; - double semi_parameter; - double flight_path_angle; - double eccentric_anomaly; - double mean_motion; - double mean_anomaly; + double mu = MU_EARTH; + double semi_major_axis = 1E5; + double eccentricity = 1E-5; + double inclination{}; + double argument_of_periapsis{}; + double right_ascension{}; + double true_anomaly{}; + double true_anomaly_rate{}; + double orbital_period{}; + double orbital_energy{}; + double v_infinity{}; + double orbit_radius{}; + double radial_rate{}; + double r_apogee{}; + double r_perigee{}; + double semi_parameter{}; + double flight_path_angle{}; + double eccentric_anomaly{}; + double mean_motion{}; + double mean_anomaly{}; Eigen::Vector3d orbital_angular_momentum_P; Eigen::Vector3d position_BP_P; Eigen::Vector3d velocity_BP_P; From 44fbc26ba986a5e609d6227f65d2a3cb5de1d330 Mon Sep 17 00:00:00 2001 From: Patrick Kenneally Date: Fri, 9 Dec 2022 18:50:12 -0700 Subject: [PATCH 5/5] Refactor to initializer list --- src/architecture/utilities/keplerianOrbit.cpp | 31 +++++++++---------- 1 file changed, 15 insertions(+), 16 deletions(-) diff --git a/src/architecture/utilities/keplerianOrbit.cpp b/src/architecture/utilities/keplerianOrbit.cpp index 196d2c2662..c576f78616 100644 --- a/src/architecture/utilities/keplerianOrbit.cpp +++ b/src/architecture/utilities/keplerianOrbit.cpp @@ -28,26 +28,25 @@ KeplerianOrbit::KeplerianOrbit() } /*! The constructor requires orbital elements and a gravitational constant value */ -KeplerianOrbit::KeplerianOrbit(classicElements oe, const double mu){ - this->semi_major_axis = oe.a; - this->eccentricity = oe.e; - this->inclination = oe.i; - this->true_anomaly = oe.f; - this->argument_of_periapsis = oe.omega; - this->right_ascension = oe.Omega; - this->mu = mu; +KeplerianOrbit::KeplerianOrbit(classicElements oe, const double mu) : mu(mu), + semi_major_axis(oe.a), + eccentricity(oe.e), + inclination(oe.i), + argument_of_periapsis(oe.omega), + right_ascension(oe.Omega), + true_anomaly(oe.f){ this->change_orbit(); } /*! The copy constructor works with python copy*/ -KeplerianOrbit::KeplerianOrbit(const KeplerianOrbit &orig){ - this->semi_major_axis = orig.semi_major_axis; - this->eccentricity = orig.eccentricity; - this->inclination = orig.inclination; - this->true_anomaly = orig.true_anomaly; - this->argument_of_periapsis = orig.argument_of_periapsis; - this->right_ascension = orig.right_ascension; - this->mu = orig.mu; +KeplerianOrbit::KeplerianOrbit(const KeplerianOrbit &orig) : mu(orig.mu), + semi_major_axis(orig.a()), + eccentricity(orig.e()), + inclination(orig.i()), + argument_of_periapsis(orig.omega()), + right_ascension(orig.RAAN()), + true_anomaly(orig.f()) +{ this->change_orbit(); }