Implement a libbn function for supporting angle inputs in rads and degs
Open Source Solid Modeling CAD
Brought to you by:
brlcad
Menu
▾
▴
1 Attachments
#481 Implement a libbn function for supporting angle inputs in rads and degs
According to /TODO line 826, added support to rt tools and gtools for specifying az/el in radians or degrees with an explicit 'rad' suffix and an implicit 'deg' suffix.
I will submit a patch refractoring the remaining libged commands that takes angles as well soon, that's why not removed entry from TODO file.
Not bad, but I see some issues:
You don't follow our indentation rules as stated in the HACKING file.
You don't evaluate the return of sscanf().
* You let sscanf() write a string of arbitrary length to an array of size 4.
And, how did you tested your patch?
Thank you for the feedback,
I have fixed the indentation issues according to the rules.
Now using a different approach with evaluation of ret value.
Restricted sscanf() by width specifier.
I tested the patch by running rt command in mged, testing all possible cases and checking the output on command window where it specifies the azimuth and elevation after every run.
For glint, I ran it via terminal from bin directory. Again tested all possible cases.
For gqa, after testing I found out that azimuth and elevation input is not yet implemented, but my code works and reported for any unexpected input.
This looks better. I've however two remarks:
- According to the documentation I consulted, the maximum field with specifier for the string of characters should be 3, i.e. "%3s"? See e.g. the example in http://www.cplusplus.com/reference/cstdio/scanf/.
- Why did you removed a blank line right before the implementation of bn_decode_hvect() in src/libbn/str.c?
I made the maximum width 4 for error checking in case the user inputs something like 'rada' or 'degb'. What's you opinion on this ?
The space removal was totally unintentional my bad, I didn't notice that minus sign. I have fixed more such issues where I forgot to add newline. Thank you for the catch.
That's a good reason for reading up to 4 chars, but in this case the unit variable should have size 5 to carry the terminating '\0' too.
Yeah, that's right. That got me thinking why it worked anyway, turns out strcmp compares until either a null character is reached or the strings differ.
http://www.cplusplus.com/reference/cstring/strcmp/
Since my second input to strcmp were null-terminated strings ("rad" , "deg"). It stopped comparing for inputs like "rada", and returned non zero.
But it's right to keep it to size 5 as you mentioned.
Last edit: Sharan 2018-04-07
Sscanf() probably wrote the 0 behind the array, maybe in val or ret, where it wasn't noticed. "man sscanf" says "String input conversions store a terminating null byte ('\0') to mark the end of the input; the maximum field width does not include this terminator."
Your patch doesn't include all reads of angles, you already mentioned here in the preamble. One othes is in src/rt/do.c, for example.
However, well done!
r70872