-
Notifications
You must be signed in to change notification settings - Fork 540
Add NEU (North East Up) properties for distance from IC start #1321
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1321 +/- ##
==========================================
+ Coverage 24.87% 24.89% +0.01%
==========================================
Files 169 169
Lines 19655 19669 +14
==========================================
+ Hits 4889 4896 +7
- Misses 14766 14773 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
In terms of a naming convention for these properties I did notice that @agodemar had in some code he referenced in the issue used the naming convention of NEA - North East Altitude, so Altitude instead of Up. May be a better option in terms of naming? |
|
This is looking redundant with the properties I have seen from #1318 that the properties that I have listed above have been chosen to display the 3D trajectory so not sure what the point of this PR is ? |
|
In terms of some more context, often textbooks on flight dynamics include linearized models like https://colab.research.google.com/drive/1qv5Hll_v4UWcC6YVJDv2KYifhyz7vU5Q?usp=sharing Most of these linearized models assume a flat earth and generate trajectory output relative to a starting point which is then plotted in 2D or 3D to show the trajectory. So the idea was to provide the same trajectory coordinates from a JSBSim run that could be used in the same plot or a comparison plot etc. @agodemar mentions the following in https://github.com/agodemar/WiReS/blob/master/calc/02_Tutorial02_ComplexManeuver-UnitConversion-Trajectory.ipynb
The So after @yuki-2000 asked about 3D cartesian coordinates to use, and I noticed that the Now, I guess it may be the case that given the small distances for these sorts of trajectories that there isn't really much difference versus the geodesic based properties. You mentioned:
Correct, but anyone using a local tangential frame to the earth should know this 😉 The trajectories typically rendered using a flat local frame aren't long distance and definitely not intercontinental trajectories from say Paris to Tokyo. They're typically used to render short term maneuvers. |
|
@agodemar I meant to ask, was there a particular reason you didn't make use of the
One quick test, using the 737 cruise script, after 100s, note I converted the position/neu-n-ft = 64761.025060
position/distance-from-start-lat-mt = 64667.549213
position/neu-e-ft = 37390.086006
position/distance-from-start-lon-mt = 37336.438030 |
|
@seanmcleod70 |
|
@yuki-2000 no problem with you jumping in and commenting. It was your original question/discussion that started this. So, just so that we're on the same plane/page.
So, this PR adds properties so that a trajectory relative to a local NEU plane like the local NED plane in green shown above can be easily retrieved from JSBSim. Now there are existing properties, i.e.
So In the Using the 737 cruise script as a basis, here are some figures on what the differences look like. So I've started the 737 off at a (lat, lon) of (0,0) and have it flying due north. Note, all values are in feet. Time: 0
position/neu-n-ft = 0.000000
position/neu-e-ft = 0.000000
position/neu-u-ft = 30000.000000
position/distance-from-start-lat-mt = 0.000000
position/distance-from-start-lon-mt = 0.000000
position/geod-alt-ft = 30000.000000
position/h-sl-ft = 30000.000000
Time: 5
position/neu-n-ft = 3674.827387
position/neu-e-ft = -0.000005
position/neu-u-ft = 30000.032050
position/distance-from-start-lat-mt = 3669.531074
position/distance-from-start-lon-mt = 0.000005
position/geod-alt-ft = 30000.356431
position/h-sl-ft = 30000.356431
Time 10:
position/neu-n-ft = 7424.477390
position/neu-e-ft = -0.000095
position/neu-u-ft = 30000.291574
position/distance-from-start-lat-mt = 7413.776597
position/distance-from-start-lon-mt = 0.000095
position/geod-alt-ft = 30001.615652
position/h-sl-ft = 30001.615653
Time 50:
position/neu-n-ft = 37392.207156
position/neu-e-ft = -0.018114
position/neu-u-ft = 30010.842574
position/distance-from-start-lat-mt = 37338.256797
position/distance-from-start-lon-mt = 0.018088
position/geod-alt-ft = 30044.427430
position/h-sl-ft = 30044.427432
Time 100:
position/neu-n-ft = 74779.571828
position/neu-e-ft = 0.064755
position/neu-u-ft = 29944.511870
position/distance-from-start-lat-mt = 74671.675133
position/distance-from-start-lon-mt = 0.064662
position/geod-alt-ft = 30078.833996
position/h-sl-ft = 30078.834005Note that there is no autopilot engaged, the aircraft is simply trimmed at 30,000ft and then left to fly for 100s. If there was an autopilot engaged to maintain an altitude, then |
|
The flat Earth assumption is not limited to using a fixed coordinate frame, it is also assuming constant gravity (i.e. g = 9,81 m/s²), no Coriolis effect, no curvature to the ground (i.e. the ground is an infinite plane), and some others I may be omitting. So I am a bit concerned that this feature may be advertised as such.
I am not sure we can reasonably expect that from anyone using this feature. Not that I am against this PR but I think we must be wary about what this feature is doing and what it can be used for. IMHO the following points should be properly documented:
|
Yep, I guess my initial assumption was that the sorts of users making use of these coordinates would be aerospace student's or professors like @agodemar 😉 I've pushed some commits to update and expand the documentation. In terms of right handed versus left handed coordinate system for NED versus NEU, I happened to go with NEU and then later on noticed that @agodemar had used the same convention in his utility function, although he named it NEA (North East Altitude). However, there is another option, i.e. ENU which is right handed like NED. https://pages.hmc.edu/spjut/AdvRoc/OrientPos.md.html So any particular preferences/suggestions between NEU versus ENU? |
|
Why has this PR been merged ? I did not have the impression we were in a rush ? But I may be wrong. I have made some comments which could very well be discarded but at least I would have expected to have the opportunity to confirm that the changes from @seanmcleod70 are addressing my concerns… or not. Also there was an open question "So any particular preferences/suggestions between NEU versus ENU?" which has not been answered. If one finds that I am taking too much time to respond, he/she can drop a comment "Do everyone agree with the merging ?" or something similar that would act as a "last call before closing the doors". And even if I did not mention that in my previous comment, I find that the property names could be more explicit. But well, that will be for another time. |
|
P.S. And please do not revert the commit. That would add a new commit to the commit history for no added value. If one is wanting to continue the discussion, better opening a new PR. |
|
@bcoconni no rush from my side. Happy to create another PR to address your comments/suggestions. |
|
@bcoconni sorry, I did not pay attention to your question. My fault |
@bcoconni so something like the following? Which makes it clear what the origin of the NEU coordinates is, i.e. to prevent confusion where someone could think that the NEU frame is attached the aircraft?
So encoding the origin, Roughly matching the existing property - |
Yeah, that was my idea although I am not sure |
bcoconni
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way NEUStartLocation is initialized is not idiomatic to JSBSim.
And I do not understand why the conversion from feet to meters has been removed while the results are used by properties which are returning their value in meters ?
| // 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. | ||
| if (!NEUFromStartInitialized) { | ||
| NEUStartLocation = FDMExec->GetIC()->GetPosition(); | ||
| NEUStartLocation.SetPositionGeodetic(NEUStartLocation.GetLongitude(), NEUStartLocation.GetGeodLatitudeRad(), 0.0); | ||
| NEUFromStartInitialized = true; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather initialize NEUStartLocation in a method that would be called by FGFDMExec::Initialize:
Lines 676 to 681 in 83e3df2
| void FGFDMExec::Initialize(const FGInitialCondition* FGIC) | |
| { | |
| Propagate->SetInitialState(FGIC); | |
| Winds->SetWindNED(FGIC->GetWindNEDFpsIC()); | |
| Run(); | |
| } |
Since
FGFDMExec::Initialize is called itself by FGFDMExec::RunIC that would ensure that NEUStartLocation would be initialized at the same time than the rest of the simulation. And that would save te usage of the flag NEUFromStartInitialized.
| { | ||
| return in.vLocation.GetDistanceTo(FDMExec->GetIC()->GetLongitudeRadIC(), | ||
| in.vLocation.GetGeodLatitudeRad())* fttom; | ||
| in.vLocation.GetGeodLatitudeRad()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not clear why the unit conversion from feet to meters is removed ? This method is used to compute the property position/distance-from-start-lon-mt which is expressed in meters.
jsbsim/src/models/FGAuxiliary.cpp
Line 446 in 83e3df2
| PropertyManager->Tie("position/distance-from-start-lon-mt", this, &FGAuxiliary::GetLongitudeRelativePosition); |
| { | ||
| return in.vLocation.GetDistanceTo(in.vLocation.GetLongitude(), | ||
| FDMExec->GetIC()->GetGeodLatitudeRadIC())* fttom; | ||
| FDMExec->GetIC()->GetGeodLatitudeRadIC()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above. This method is used to compute the property position/distance-from-start-lat-mt which is expressed in meters.
jsbsim/src/models/FGAuxiliary.cpp
Line 447 in 83e3df2
| PropertyManager->Tie("position/distance-from-start-lat-mt", this, &FGAuxiliary::GetLatitudeRelativePosition); |
| auto ic = FDMExec->GetIC(); | ||
| return in.vLocation.GetDistanceTo(ic->GetLongitudeRadIC(), | ||
| ic->GetGeodLatitudeRadIC())* fttom; | ||
| ic->GetGeodLatitudeRadIC()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above. This method is used to compute the property position/distance-from-start-mag-mt which is expressed in meters.
jsbsim/src/models/FGAuxiliary.cpp
Line 448 in 83e3df2
| PropertyManager->Tie("position/distance-from-start-mag-mt", this, &FGAuxiliary::GetDistanceRelativePosition); |
Oops, that was inadvertent. I'd changed that locally purely for testing/comparison purposes, the changes weren't meant to be included in the commit. I've submitted a new PR - #1327, which fixes that, updates the naming of the NEU properties and implements your suggestion regarding NEU initialization. |
…-Team#1321) Co-authored-by: Sean McLeod <[email protected]>


Based on discussion #1318 in terms of a request to be able to easily plot the coordinates of an aircraft in the local frame to show a 2D or 3D plot of the aircraft's trajectory I've added 3 new properties in
FGAuxiliary.They output the current offset of the aircraft in a local frame with an origin based on the initial condition's lat, lon and 0 alt relative to the geoid ellipsoid.