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

Add roundmultiple function #1170

Merged
merged 3 commits into from
Oct 9, 2024
Merged

Conversation

seanmcleod
Copy link
Member

Based on the request and discussions in issue - #1159

I'm in 2 minds whether to rather add a separate round(x) function in addition to roundmultiple(x, m) where currently if no second argument is provided to roundmultiple then it assumes a value of 1.

@seanmcleod seanmcleod mentioned this pull request Oct 3, 2024
1 task
Copy link

codecov bot commented Oct 3, 2024

Codecov Report

Attention: Patch coverage is 0% with 7 lines in your changes missing coverage. Please review.

Project coverage is 25.12%. Comparing base (57361d0) to head (0dccfd7).
Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
src/math/FGFunction.cpp 0.00% 7 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1170      +/-   ##
==========================================
- Coverage   25.13%   25.12%   -0.01%     
==========================================
  Files         170      170              
  Lines       18330    18337       +7     
==========================================
  Hits         4608     4608              
- Misses      13722    13729       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Octal450
Copy link

Octal450 commented Oct 3, 2024

Many thanks. My personal option is that unless there's some reason, having the shortcut tag would be nice.

Kind Regards,
Josh

Copy link
Member

@bcoconni bcoconni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR @seanmcleod. I suggest a couple of modifications:

  • Use plain round() when <roundmultiply> is used with a single argument.
  • Fix the parameter passed to aFunc<> so that the error message do not tell that 2 arguments are required when <roundmultiply> is used with zero or more than 2 arguments.

src/math/FGFunction.cpp Outdated Show resolved Hide resolved
src/math/FGFunction.cpp Outdated Show resolved Hide resolved
Co-authored-by: Bertrand Coconnier <bcoconni@users.noreply.github.com>
@bcoconni
Copy link
Member

bcoconni commented Oct 8, 2024

I must express that I'm not excited about this PR since <roundmultiply> is a trivial function, which could simply be replaced by <round> with a single parameter as I've explained in #1159 (comment). I don't believe adding such trivial functions to FGFunction is beneficial. However, if everyone else agrees to proceed in this direction, I won't be the one to cause issues.

Co-authored-by: Bertrand Coconnier <bcoconni@users.noreply.github.com>
@Octal450
Copy link

Octal450 commented Oct 8, 2024

I think it's really useful, so thank you gents.

@bcoconni
Copy link
Member

bcoconni commented Oct 8, 2024

I think it's really useful, so thank you gents.

I would have much preferred a solid argument about why this change is making a difference rather than having a mere opinion that you think it is useful. Anyway, whatever…

@bcoconni
Copy link
Member

bcoconni commented Oct 8, 2024

@seanmcleod, as far as I am concerned, you can push the "merge" button whenever you feel this PR ready for being pushed to master (well, I mean you mentioned you were in 2 minds).

@Octal450
Copy link

Octal450 commented Oct 8, 2024

I think it's really useful, so thank you gents.

I would have much preferred a solid argument about why this change is making a difference rather than having a mere opinion that you think it is useful. Anyway, whatever…

I didn't think you were asking for one, so I didn't want to argue :)

But fair enough.

When creating instrumentation, JSBSim systems are the best way to go. They are much more efficient and fast than FlightGear's Nasal and in some cases, more flexible. However, instrumentation almost always displays nice formatted numbers to the pilot, rather than 3.5308928409832904 or whatever.

Rounding is the quick solution, hence the nice round tag... But -

Often times, displaying things to the 1th decimal place can be as unhelpful at a glance, for numbers that are very large (Ie, fuel flow on jetliners). So, these instruments are rounded to the 10th, or sometiimes the 50th.

Second case, I use JSBSim to do all my performance computations when simulating aircraft flight computers and I need to be able to round the numbers to various places there.

There is just 2 use-cases where a flexible rounding operation is useful, I can list more.

I think that just like mod allows you to specify the value, and isn't just fixed to a value like 2, I think rounding should be equally flexible, as a basic function.

Absolutely you right that doing more complex things with it makes no sense to include in the code here, but I think, round to an nth is a nice standard operation.

Bests,
Josh

@agodemar agodemar merged commit 5979050 into JSBSim-Team:master Oct 9, 2024
30 checks passed
@seanmcleod
Copy link
Member Author

@agodemar you jumped the gun a bit on merging the PR into master 😉

Based on @bcoconni's review comments, I had incorporated them last night, but wanted to tidy up the code a bit more, as per below, and was about to push them when I got the message that you had merged the PR into master 😉 Anyway, I'll push them to master as a separate commit just now.

image

In follow-up to @bcoconni's question above I was going to say I assume this sort of rounding would mostly be used for instrument display purposes, as opposed to for FDM calculations. I've never written any Nasal code for FlightGear, but I was going to ask whether Nasal as a more general purpose language wouldn't be the more typical place to implement some of these sorts of utility functions. I see in the meantime that @Octal450 has made some comments on some of the pros/cons from his perspective/experience in terms of Nasal etc.

@agodemar
Copy link
Contributor

agodemar commented Oct 9, 2024

@seanmcleod I'm sorry for the inconvenience. I assumed that the motivations given by @Octal450 are acceptable.
Please, go ahead with your work on this refinement.

@seanmcleod
Copy link
Member Author

@agodemar no problem, not a big deal 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants