-
Notifications
You must be signed in to change notification settings - Fork 464
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
Allow output precision to be specified for socket output #579
Conversation
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 guess a safeguard should be added to the precision setting in order to avoid the user requesting a precision higher than the format width.
@@ -121,6 +121,12 @@ bool FGOutputSocket::Load(Element* el) | |||
el->GetAttributeValue("protocol") + "/" + | |||
el->GetAttributeValue("port")); | |||
|
|||
// Check if output precision for doubles has been specified, default to 7 if not | |||
if(el->HasAttribute("precision")) | |||
precision = (int)el->GetAttributeValueAsNumber("precision"); |
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.
Since the format width setw
is still 12 characters wide, I guess we should make sure that precision
is no higher than, say 10 (reserving storage for 1 digit and the decimal point) ?
If precision
is beyond this value then either it is ignored or it is capped. A warning message should be issued as well when JSBSim alters the user input value.
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.
When I was writing the code I half wondered about letting the user specify the width as well as the precision to give them full control.
I must say reading the documentation on setw
and setprecision
it wasn't entirely clear what the exact output would be especially given various combinations, and with comments like this in the C++ documentation.
The exact effects this modifier has on the input and output vary between the individual I/O functions and are described at each operator<< and operator>> overload page individually.
I assumed setprecision
would set the number of places after the decimal point, but it doesn't, e.g. from the C++ documentation - https://en.cppreference.com/w/cpp/io/manip/setprecision
default precision (6): 3.14159
std::setprecision(10): 3.141592654
So I ran a couple of tests with various combinations:
double test = 123456789.12345678912345;
setw setprecision output strlen
12 10 " 123456789.1" 12
12 12 "123456789.123" 13
12 14 "123456789.12346" 15
double test = 56789123456789.12345678912345;
setw setprecision output strlen
12 10 "5.678912346e+13" 15
12 12 "5.67891234568e+13" 17
12 14 "56789123456789" 14
I'm happy to add some code to apply a capping on the precision relative to the width, just not sure on the best logic to apply though 😉
Capping it at 10 is a safe option, users will need to realise that 10 doesn't mean 10 places after the decimal point, and in the first example there will be what they may think is 'wasted' padding. And for the second example it doesn't guarantee that the overall width will be 12.
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.
Thanks for reading the C++ docs and testing, I wasn't aware of all those subtleties 👍
Your tests clearly show that if we want the string length to stay below 12 characters then a number of places must be allocated in the string for:
- the leading minus sign
-
- the decimal point
.
- the exponent
e
- the exponent sign
+
or-
- the exponent value: 2 figures max ?
That sums up to 6 characters. So keeping the string length below 12 characters in all cases means that precision
must be lower than or equal to 6 characters meaning that, even with the current code, numbers can be more than 12 characters long.
So the question is about knowing if issuing to a socket connection numbers that are more than 12 characters long is a problem ? I have not inspected the code but I guess that it's the socket receivers that might make such assumptions, not JSBSim as an emitter.
To stay on the safe side, I would suggest to disable this feature for FlightGear sockets (i.e. to force precision
to 7 in FGOutputFG
and to issue a warning message whenever the user specifies a different value) in case FlightGear makes some assumptions about the numbers length.
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.
Oh, and I agree that checking precision
against 10 is irrelevant to the general case FGOutputSocket
. But that opens the question about whether or not the user should be allowed to modify the width setw
?
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.
To stay on the safe side, I would suggest to disable this feature for FlightGear sockets (i.e. to force
precision
to 7 inFGOutputFG
Hmm, I'm a bit confused about the FlightGear comment with regards to precision
since when I took a quick look it looks like the FlightGear socket outputs a binary data structure containing binary floats and doubles.
Whereas the precision
we're dealing with here is for a text representation with a certain amount of precision.
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.
To stay on the safe side, I would suggest to disable this feature for FlightGear sockets (i.e. to force
precision
to 7 inFGOutputFG
Hmm, I'm a bit confused about the FlightGear comment with regards to
precision
since when I took a quick look it looks like the FlightGear socket outputs a binary data structure containing binary floats and doubles.Whereas the
precision
we're dealing with here is for a text representation with a certain amount of precision.
Err.. well, it was indeed nonsense. Sorry !
PR merged despite all the rubbish I wrote 😉 |
Allow a user to optionally specify the precision for the output of double properties on an output socket.