Codecov Report

❌ Patch coverage is 72.72727% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 24.91%. Comparing base (83e3df2) to head (a714722).
⚠️ Report is 2 commits behind head on master.

@@            Coverage Diff             @@
##           master    #1327      +/-   ##
==========================================
+ Coverage   24.89%   24.91%   +0.02%     
==========================================
  Files         169      169              
  Lines       19669    19675       +6     
==========================================
+ Hits         4896     4902       +6     
  Misses      14773    14773              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Just some food for thought: we might want to compute the NEU position only when needed rather than forcing it being updated every time step.

Yep, I actually thought of it at the time I first implemented them. Especially given it's not something that other systems like the EOM etc. need on each time step, plus they're not going to be popular properties in general. So, adding some additional conditional logic will more than likely be better performance wise as opposed to calculating it on every time step.

However, I've run into an issue with our recent friend, FGPropertyManager::Tie 😉

So these are the changes I made, note the change in const.

-  double GetNEUPositionFromStart(int idx) const { return vNEUFromStart(idx); }
-  const FGColumnVector3& GetNEUPositionFromStart() const { return vNEUFromStart; }

+  double GetNEUPositionFromStart(int idx) { return (GetNEUPositionFromStart())(idx); }
+  const FGColumnVector3& GetNEUPositionFromStart();

const FGColumnVector3& FGAuxiliary::GetNEUPositionFromStart()
{ 
  if (!NEUValid) {
    // Position tracking in local frame with local frame origin at lat, lon of initial condition
    // and at 0 altitude relative to the reference ellipsoid. Position is NEU (North, East, UP) in feet.
    vNEUFromStart = NEUStartLocation.LocationToLocal(in.vLocation);
    vNEUFromStart(3) *= -1.0;  // Flip sign for Up, so + for altitude above reference ellipsoid
    NEUValid = true;
  }

  return vNEUFromStart; 
}

bool FGAuxiliary::Run(bool Holding)
{
............
  // New timestep so vNEUFromStart is no longer valid since we only calculate it on demand
  NEUValid = false;
...........  
}

Which results in this, doesn't appear we can currently have getters that aren't const for binding via FGPropertyManager::Tie?

1>FGAuxiliary.cpp
1>C:\source\jsbsim\src\models\FGAuxiliary.cpp(469,20): error C2672: 'JSBSim::FGPropertyManager::Tie': no matching overloaded function found
1>    C:\source\jsbsim\src\input_output\FGPropertyManager.h(437,7):

One workaround for the const issue, make the methods const again and declare vNEUFromStart and NEUValid as mutable.

+  double GetNEUPositionFromStart(int idx) const { return (GetNEUPositionFromStart())(idx); }
+  const FGColumnVector3& GetNEUPositionFromStart() const;

  FGLocation NEUStartLocation;
  mutable FGColumnVector3 vNEUFromStart;
  mutable bool NEUValid;

One workaround for the const issue, make the methods const again and declare vNEUFromStart and NEUValid as mutable.

+  double GetNEUPositionFromStart(int idx) const { return (GetNEUPositionFromStart())(idx); }
+  const FGColumnVector3& GetNEUPositionFromStart() const;

  FGLocation NEUStartLocation;
  mutable FGColumnVector3 vNEUFromStart;
  mutable bool NEUValid;

Yep ! Seems reasonable to me. Especially since the mutable members are "byproducts" of the class. The method GetNEUPositionFromStart is not actually changing the class state so we can still claim it is const during the call to the getter.

Yep, I came across the following when I was looking into the options available.

https://isocpp.org/wiki/faq/const-correctness

What do I do if I want a const member function to make an “invisible” change to a data member?

Use mutable (or, as a last resort, use const_cast).

A small percentage of inspectors need to make changes to an object’s physical state that cannot be observed by external users — changes to the physical but not logical state.

For example, the collection-object discussed earlier cached its last lookup in hopes of improving the performance of its next lookup. Since the cache, in this example, cannot be directly observed by any part of the collection-object’s public interface (other than timing), its existence and state is not part of the object’s logical state, so changes to it are invisible to external users. The lookup method is an inspector since it never changes the object’s logical state, irrespective of the fact that, at least for the present implementation, it changes the object’s physical state.

When methods change the physical but not logical state, the method should generally be marked as const since it really is an inspector-method. That creates a problem: when the compiler sees your const method changing the physical state of the this object, it will complain — it will give your code an error message.

The C++ compiler language uses the mutable keyword to help you embrace this logical const notion. In this case, you would mark the cache with the mutable keyword, that way the compiler knows it is allowed to change inside a const method or via any other const pointer or reference. In our lingo, the mutable keyword marks those portions of the object’s physical state which are not part of the logical state.

Looks good to me 👍