Add a per-model execution-enable property - #1481
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1481 +/- ##
==========================================
- Coverage 25.10% 25.03% -0.08%
==========================================
Files 171 171
Lines 18843 18827 -16
==========================================
- Hits 4731 4713 -18
- Misses 14112 14114 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Adds Setadot, Setbdot, SetNx, SetNy and SetNz to FGAuxiliary so a host that owns the state integration can inject the alpha/beta rates and body-axis load factors that Auxiliary would otherwise derive from the propagation it is no longer running. The host-owned domain is propagation together with ground reactions. The two are tightly coupled through the 6DoF integration and are externalised as one unit, so when a host takes them over these are among the few derived quantities Auxiliary cannot recompute and must be supplied. Together with the per-model enable property (JSBSim-Team#1481), this is the main JSBSim-core change needed to host the engine inside an externally hosted simulation such as DCS World. The rest reuses existing mechanisms: atmosphere override, FGWinds, and the existing velocity setters. A large capability for a small, additive surface. It follows the state-propagation review that Agostino De Marco raised in JSBSim-Team#1390 and JSBSim-Team#1391.
Adds Setadot, Setbdot, SetNx, SetNy and SetNz to FGAuxiliary so a host that owns the state integration can inject the alpha/beta rates and body-axis load factors that Auxiliary would otherwise derive from the propagation it is no longer running. The host-owned domain is propagation together with ground reactions. The two are tightly coupled through the 6DoF integration and are externalised as one unit, so when a host takes them over these are among the few derived quantities Auxiliary cannot recompute and must be supplied. Together with the per-model enable property (JSBSim-Team#1481), this is the main JSBSim-core change needed to host the engine inside an externally hosted simulation such as DCS World. The rest reuses existing mechanisms: atmosphere override, FGWinds, and the existing velocity setters. A large capability for a small, additive surface. It follows the state-propagation review that Agostino De Marco raised in JSBSim-Team#1390 and JSBSim-Team#1391.
Each scheduled model gains a simulation/models/<name>/enabled property (default true), bound by FGFDMExec using a stable canonical name per eModels. When the property is set false, FGModel::Run() skips the model's body while its last state and property bindings are preserved. This is a property-tree mechanism by which an external system can replace a model, for example an external propagator owning FGPropagate, or a host physics engine owning ground reactions, without removing it from the schedule. Behaviour is unchanged unless a model is explicitly disabled. The canonical names are deliberately independent of FGModel::Name, since some models rename themselves when their config is loaded. A static_assert keeps the name table in step with the eModels enum.
a0aa665 to
1399a6b
Compare
|
I took a look at #1390 to refresh my memory of some of the earlier discussions and came across my comment at the time towards the end of the thread, see - #1390 (comment) Which it doesn't look like it was answered at the time, so my questions still stand. |
|
I agree with you @seanmcleod70 and in addition to that, I am not too happy with the implementation. I am really struggling about why the enable/disable feature should imply modifying the
This does not look like a reason to me for centralizing the feature in |
|
Taking the general question about this and #1482 - I agree that a single MR is better.
@seanmcleod70 the points you raised in #1390 about So this change together with #1482 are what I've ended up with because the better design is actually to run most of the models except the one that absolutely must be replaced which in this case is Propagate. I did all of the models because that is what I understood that @bcoconni wanted even though all that actually is needed for this case is to be able to disable the Propagate model There is a good argument for disabling ground reactions completely but that isn't necessary for my work because keeping the gear retracted achieves the same, albeit with a small performance hit. @bcoconni asked a good question about centralizing the feature and original reason was to follow the way that exec is already managing allocating all of the models. In hindsight this isn't really adding much and instead I'd change the FGModel constructor to accept an optional |
My point was that if we want to be able to disable any model then IMHO the best option is to add a property to I mean we may target a change with a reduced scope for a start and then after having cumulated some experience over the usage of that feature, we may be able to extend the concept to all instances of
I do not follow your point here. @seanmcleod70 made some good points about the extent of the setters that would have to be added but also about the properties that would no longer be updated. How your PR is addressing that particular issue? Because jsbsim/src/models/FGPropagate.cpp Lines 829 to 921 in d371f38 |
Move the enable-name binding out of FGFDMExec entirely: the FGModel constructor takes an optional stable canonical name and binds simulation/models/<name>/enabled itself, and each model passes its own name at construction. This removes the central name array, the static_assert that guarded it, and the binding loop; FGFDMExec no longer references the feature. BindModelEnabled becomes a protected helper. Response to review on JSBSim-Team#1481 (bcoconni): the feature now lives wholly in the FGModel hierarchy.
|
See the update on #1482 - basically I don't need this code change any more. However being able to disable modules is something that may still have some value; certainly being able to freeze (disable) systems it's something that is a standard feature for the systems on the level D simulators as it permits isolation of issues and there are times when I could see that it might be useful. One specific example is a project I worked on last year where run time performance was critical so there was a JSBSim patch script as part of the CI to to remove ground reactions, external reactions and some other systems (I can't remember the exact list). Before I close this and delete the branch I'd like to have the discussion about if there is any need for this code. |
Integration is suppressed by setting the Propagate integrators all to eNone at init. We then inject the state from set_current_state_body_axis which will no longer be overwritten by Propagate. This removes the need to disable modules (JSBSim-Team/jsbsim#1481). GroundReactions now runs but we keep it effectively disabled by never extending the gear and by having no contact points. EarthPositionAngle is forced to zero every frame because the DCS World's world is static and does not rotate and not doing this would probably cause longitude to drift west. In set_current_state_body_axis the attitude is now set before the body velocity. SetUVW converts body velocity to inertial using the transforms in force and then Propagate::Run's CalculateUVW inverts that using the transforms. Setting the attitude first makes both directions use the same frame. Because this now looks like finished, validated code I have taken the leap to strip all the commented-out FlightGear interface code carried in JSBSim_interface since the port.
Integration is suppressed by setting the Propagate integrators all to eNone at init. We then inject the state from set_current_state_body_axis which will no longer be overwritten by Propagate. This removes the need for new setters (JSBSim-Team/jsbsim#1482) and the need to disable modules (JSBSim-Team/jsbsim#1481) and also . GroundReactions now runs but we keep it effectively disabled by never extending the gear and by having no contact points. There is still an argument for keeping the ability to disable modules but we will wait and see what the JSBSim-Team decides. EarthPositionAngle is forced to zero every frame because the DCS World's world is static and does not rotate and not doing this would probably cause longitude to drift west. In set_current_state_body_axis the attitude is now set before the body velocity. SetUVW converts body velocity to inertial using the transforms in force and then Propagate::Run's CalculateUVW inverts that using the transforms. Setting the attitude first makes both directions use the same frame. Because this now looks like finished, validated code I have taken the leap to strip all the commented-out FlightGear interface code carried in JSBSim_interface since the port.
There was a problem hiding this comment.
I think the code in the constructor of FGModel could be simpler using a tied property instead of managing explicitly an SGPropertyNode instance with all the if's that that drains.
Also I don't see much point in using 2 different names, one for the enable property and another one for the Name member? I would merge them into a single one. And the model classes would still be able to rename themselves without affecting the name of the enabled property.
Finally, I am not a huge fan of Uncle Bob's clean code principle about using a myriad of small trivial functions. So I'd rather not have the new method BindModelEnabled, especially since, when using a tied property, its code is 2 lines long.
|
Just for the record, I have just came across this old feature request on Sourceforge #86 Turn off models individually from @jonsberndt and dating back to October 10, 2011. Your PR will allow to close an almost 15-year-old request @Zaretto 😆 |
…d require Name in the constructor Bind simulation/models/<Name>/enabled by tying it to a bool member constructed from the model name in the constructor when the model name isn't empty. FGInput, FGInputType, FGOutput and FGOutputType already had an enabled property so remove that and use the new one from the base class.
|
Thanks @bcoconni for the guidance, and it makes it a lot nicer. All applied and pushed ready for your input. One thing I'd like your guidance on and maybe @seanmcleod70 was my change to use |
|
@Zaretto @bcoconni @seanmcleod70 this is a great outcome! |
|
@Zaretto I took the liberty to modify your code to fix the failure of the test case
I do not have strong opinions about that. The thing is that IMHO the class FGInertial::FGInertial(FGFDMExec* fgex)
: FGModel(fgex, "FGInertial")
{
Name = "Earth";That would allow having the property named jsbsim/src/models/FGInertial.cpp Lines 297 to 302 in 0b688c8 @agodemar My understanding is that this PR is reduced in scope with respect to the discussions we had in #1390 and #1391. The PR is now only focusing on individually disabling the |
|
@bcoconni thank you for fixing the test case and the dual naming works for me. @agodemar this PR isn't really related to the discussion about propagation we had in #1390, #1391 and #1482 - in effect disabling the integration in the propagation gives me enough to make it work for my current case. However I do think that all of the discussions are pointing us towards the bigger question of how we can extend JSBSim at the class level but that is probably a separate discussion. |
bcoconni
left a comment
bcoconni
left a comment
There was a problem hiding this comment.
I have pushed a commit for the dual naming of the FGInertial instances.
Looks good to me now.
4 participants
Implements per-model enable proposed by @bcoconni in #1390.
The suggested solution (#1390 (comment)):
This branch:
Behaviour is unchanged unless a model is explicitly disabled. A disabled model's body does not run, but its last state and property bindings are preserved, so an external system can take over that model in place, for example an external propagator owning FGPropagate.
Two notes: