Skip to content
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

Always set useDefault to false when calling SGPropertyNode::tie() #363

Closed
bcoconni opened this issue Dec 23, 2020 · 1 comment
Closed

Always set useDefault to false when calling SGPropertyNode::tie() #363

bcoconni opened this issue Dec 23, 2020 · 1 comment
Assignees
Labels

Comments

@bcoconni
Copy link
Member

The bug SF2480 has been recently reported in FlightGear which brought to my attention a feature from SGPropertyNode::tie that I think is mostly misused in JSBSim. This feature is the parameter useDefault which allows to keep the existing value from a property when JSBSim ties to the property in question. This feature is used by FGPropertyManager::Tie as well:

* @param useDefault true if any existing property value should be
* copied to the variable; false if the variable should not
* be modified; defaults to true.
*/
void
Tie (const std::string &name, double *pointer, bool useDefault = true);

We have now encountered 2 bugs linked to the usage of that feature (see commits fc9fbda and 9827225) which are basically triggered by a reset in FlightGear and properties tied with useDefault=true.

The reset sequence of FlightGear does not delete the property manager when the instance of JSBSim is destroyed. Then when a property is tied, JSBSim uses the current value of the property whatever it is. I guess the idea is that the value held by the property has been intentionally set to something meaningful prior to the call to tie() and as such should be kept ...which is kind of a leap of faith. On the other hand, when useDefault is set to false, the property value is overwritten by tie() and its previous value is lost which is kind of a hard reset.

Bluntly speaking, I think using this feature is mostly (if not always) a bad idea and is actually a path to disaster: the value held by the property is never validated before calling Tie() so the code is quite trustful to the fact that the value has been set on purpose to something meaningful. But to me, this feature is similar to using uninitialized variables i.e. it may work in most cases - by chance - but it is actually a ticking bomb waiting to blow up to our faces at any random time.

I'll go even farther: I cannot imagine a practical usage to that feature (at least as long as JSBSim is concerned) since FlightGear kills the FDM and reinitializes it from the ground up at each reset. Even worse, some values kept from the previous run might become the seed of future NaNs, who knows ?

My proposal: always set useDefault to false for every single call to SGPropertyNode::tie() that JSBSim makes. What does that mean ? If someone has initialized some properties before JSBSim is initialized then these values will be ignored and overwritten.

I have submitted this idea to the FlightGear mailing list to get some feedback from aircraft modelers.

@bcoconni bcoconni added the bug label Dec 23, 2020
bcoconni added a commit that referenced this issue Feb 6, 2021
See issue #363: setting `useDefault` to true is equivalent to have uninitialized variables.
@bcoconni
Copy link
Member Author

bcoconni commented Feb 6, 2021

Just pushed the commit 193d28c that sets useDefault to false.
We may need to reconsider this position for initial conditions (ic/ properties) in which case this issue shall be re-opened.

@bcoconni bcoconni closed this as completed Feb 6, 2021
@bcoconni bcoconni self-assigned this Feb 6, 2021
bcoconni added a commit that referenced this issue Apr 30, 2021
See issue #363: setting `useDefault` to true is equivalent to have uninitialized variables.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant