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.
Zaretto
force-pushed
the
feature/model-enabled-property
branch
from
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.
| %%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%*/ | ||
|
|
||
| FGModel::FGModel(FGFDMExec* fdmex) | ||
| FGModel::FGModel(FGFDMExec* fdmex, const std::string& enableName) |
There was a problem hiding this comment.
| FGModel::FGModel(FGFDMExec* fdmex, const std::string& enableName) | |
| FGModel::FGModel(FGFDMExec* fdmex, const std::string& name) | |
| : Name(name) |
I would take opportunity of passing a name to the constructor to initialize the member Name. That would save setting Name in each inherited class.
| if (!enableName.empty()) BindModelEnabled(enableName); | ||
|
|
There was a problem hiding this comment.
Not sure why a separate method is needed for this trivial task? The constructor of FGModel is a dozen lines long, the content of BindModelEnabled could be copied inside the constructor and its code would still be much shorter than any screen height.
| const string path = "simulation/models/" + name + "/enabled"; | ||
| const bool existed = PropertyManager->HasNode(path); | ||
| ModelEnabled = PropertyManager->GetNode(path, true); | ||
| // Default to enabled; a freshly created node would otherwise read false. | ||
| if (!existed && ModelEnabled) ModelEnabled->setBoolValue(true); |
There was a problem hiding this comment.
| const string path = "simulation/models/" + name + "/enabled"; | |
| const bool existed = PropertyManager->HasNode(path); | |
| ModelEnabled = PropertyManager->GetNode(path, true); | |
| // Default to enabled; a freshly created node would otherwise read false. | |
| if (!existed && ModelEnabled) ModelEnabled->setBoolValue(true); | |
| const string path = "simulation/models/" + Name + "/enabled"; | |
| Property->Tie(path, &enabled); |
Tying the property to a member bool enabled makes the code much shorter and saves the trouble of all the ifs you have used to guard the code from potential errors.
This code should be moved directly inside the constructor.
| // entirely: its Run() body does not execute, but its last state and property | ||
| // bindings are preserved so an external system can replace it, for example | ||
| // external propagation or host-owned ground reactions. | ||
| if (ModelEnabled && !ModelEnabled->getBoolValue()) return true; |
There was a problem hiding this comment.
| if (ModelEnabled && !ModelEnabled->getBoolValue()) return true; | |
| if (!enabled) return true; |
Using a tied property makes the test much simpler and more legible.
| class SGPropertyNode; | ||
|
|
There was a problem hiding this comment.
| class SGPropertyNode; |
You don't need that when using tied properties.
| /** Constructor. | ||
| @param enableName optional stable canonical name for this model. When | ||
| non-empty it binds simulation/models/<enableName>/enabled | ||
| (default true); setting that property false skips this | ||
| model's Run(). Empty means the model has no enable flag. */ | ||
| explicit FGModel(FGFDMExec*, const std::string& enableName = ""); |
There was a problem hiding this comment.
| /** Constructor. | |
| @param enableName optional stable canonical name for this model. When | |
| non-empty it binds simulation/models/<enableName>/enabled | |
| (default true); setting that property false skips this | |
| model's Run(). Empty means the model has no enable flag. */ | |
| explicit FGModel(FGFDMExec*, const std::string& enableName = ""); | |
| /** Constructor. | |
| @param name optional stable canonical name for this model. When | |
| non-empty it binds simulation/models/<enableName>/enabled | |
| (default true); setting that property false skips this | |
| model's Run(). Empty means the model has no enable flag. */ | |
| FGModel(FGFDMExec*, const std::string& name); |
- You no longer need the
explicitqualifier for a constructor with more than one argument. - I would rather use
namefor the name argument since the point is to use that value to populate the memberNamein the process. - I would not provide a default value to
name, thus forcing all classes inheriting fromFGModelto pick a name.
| protected: | ||
| unsigned int exe_ctr; | ||
| unsigned int rate; | ||
| SGPropertyNode* ModelEnabled = nullptr; |
There was a problem hiding this comment.
| SGPropertyNode* ModelEnabled = nullptr; | |
| bool enable = true; |
Using a tied property allows to define the default value here and saves using a lot of ifs to check for the existence of the pointer and property.
| /** Bind this model's execution-enable flag to | ||
| simulation/models/<name>/enabled (default true), where name is the stable | ||
| canonical name passed to the constructor. When that property is set false, | ||
| Run() is skipped while the model's last state and property bindings are | ||
| preserved, the property-tree mechanism by which an external system can | ||
| replace a model, for example external propagation owning FGPropagate or a | ||
| host physics engine owning FGGroundReactions. */ | ||
| void BindModelEnabled(const std::string& name); |
There was a problem hiding this comment.
This method is no longer needed but the docs should be kept and moved in the class documentation (lines 57-63)
| FGOutput::FGOutput(FGFDMExec* fdmex) : FGModel(fdmex, "output") | ||
| { | ||
| Name = "FGOutput"; |
There was a problem hiding this comment.
| FGOutput::FGOutput(FGFDMExec* fdmex) : FGModel(fdmex, "output") | |
| { | |
| Name = "FGOutput"; | |
| FGOutput::FGOutput(FGFDMExec* fdmex) : FGModel(fdmex, "FGOutput") | |
| { |
Since the FGModel constructor populates Name itself, you no longer need to initialize it in the FGOutput constructor.
There was a problem hiding this comment.
And the same should be applied to the other model classes FGAcceleration, FGAerodynamics, etc.
|
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! |
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: