dkavolis / Ferram-Aerospace-Research

Aerodynamics model for Kerbal Space Program
Other
81 stars 32 forks source link

Rodhern's update 3. #5

Closed Rodhern closed 5 years ago

Rodhern commented 5 years ago

A possible merge as per #4. I do not use KSP 1.5.1 myself, so I can't even know if this will compile. The changes are:

dkavolis commented 5 years ago

Thanks for your PR. I just have a few questions and suggestions.

Rodhern commented 5 years ago
* You replaced some spaces in localization files with explicit unicode, any reason a simple space is not sufficient? You have also removed some spaces such that values following them will appear without one.

In my KSP 1.3.1 version only the explicit unicode spaces show up in the GUI. That is the reason they are made explicit. The ones that are removed are to show clearer that the characters will not appear.

* Instead of `Loop` in export it might be better to use `Sweep`.

That makes sense; thanks.

* The input values of altitude and Mach number are taken from a file deep in KSP folder, I think it would be better to have a separate tab open where the derivatives are for inputting the values manually (which could be saved in config.xml). I was thinking of having an input list for each, something like in Paraview:
  ![image](https://user-images.githubusercontent.com/12998363/48317339-41911280-e5e8-11e8-9b06-b842f01e9c75.png)
  where the user could input values one by one or as a lin/logscale.

Sounds good to me, but I am not able to code that myself.

* The export format is inefficient (outputs too much data in terms of definitions) and will probably be difficult to read with existing tools (I'm thinking Python). What do you think about outputting the results in csv format? Body name, aircraft properties and inertia could be left in the header and the rest in a table format. It would make importing the file that much easier into existing tools.

Sound good too. Again I am not sure I have the skill and/or time to make it so. Feel free to take what can be reused and create some better export functionality.

* The exported files are stored in FerramAerospaceResearh/Plugins/PluginData which can be confusing. It might be better to store all the files in FerramAerospaceResearch/StabilityDerivatives or something similar, the default file names should also be changed so they don't overwrite each other, maybe "{CraftName}_{datetime}" such that they are unique.

:-)

* Running the export at multiple points freezes the game for a short time, this should be offloaded to a different thread.

:-)

* What are the benefits of your `SillySearchMethod` over `BrentsMethod`? The latter is robust, fast and popular root finding algorithm.

The benefit is that BrentsMethod won't (as easily) find the maximum CL, which in turns produces an inability to solve the AoA for slow flight. The SillySearchMethod did better in my examples. [Edit: You can compile two versions, one with BrentsMethod and one with SillySearchMethod, then see how slow you can go and still get an AoA solution in the SPH with the same plane. It would be interesting to see how the methods compare for your plane designs.]

dkavolis commented 5 years ago

Sounds good to me, but I am not able to code that myself.

I will probably give it a try then when I have more time.

Sound good too. Again I am not sure I have the skill and/or time to make it so. Feel free to take what can be reused and create some better export functionality.

This shouldn't be too difficult to implement, once everything is done a contour plot might be a nice additional feature.

The benefit is that BrentsMethod won't (as easily) find the maximum CL, which in turns produces an inability to solve the AoA for slow flight. The SillySearchMethod did better in my examples.

Then a switch based on Mach number could be used.

Rodhern commented 5 years ago

This shouldn't be too difficult to implement, once everything is done a contour plot might be a nice additional feature.

Agreed, that would be pretty cool.

Then a switch based on Mach number could be used.

Excellent idea. So it would kind of go: First use Brent, if that fails then go Silly ?

dkavolis commented 5 years ago

Excellent idea. So it would kind of go: First use Brent, if that fails then go Silly ?

Simply if mach number is below, say, 0.3, use SillySearchMethod, otherwise use BrentsMethod. The best switch value should be slightly larger than the largest BrentsMethod fails on.

Rodhern commented 5 years ago

Simply if mach number is below, say, 0.3, use SillySearchMethod, otherwise use BrentsMethod. The best switch value should be slightly larger than the lowest BrentsMethod fails on.

Sounds fine. My only concern was that the threshold speed might be quite craft dependent.

dkavolis commented 5 years ago

You can then add a check for failure but a fixed Mach switch should be faster on average.

Rodhern commented 5 years ago

You can then add a check for failure but a fixed Mach switch should be faster on average.

Sounds good. I will see when I get time to look at the code again, and get it fixed, maybe next weekend, maybe later. If you feel like tinkering be my guest and go ahead before then.

dkavolis commented 5 years ago

Also, the sweep should use a new CalculateStabilityDerivs method which does not recompute vehicle properties every time.

dkavolis commented 5 years ago

Do you mind submitting separate PRs for each of the changes? It would make keeping track of changes easier in case ferram ever returns and some of them can be merged already.

Rodhern commented 5 years ago

Do you mind submitting separate PRs for each of the changes? It would make keeping track of changes easier in case ferram ever returns and some of them can be merged already.

I guess I can do that. I will plan to probably do that some time. (I am already a bit surprised at how Github does PRs; that the branch tips just keep chugging along).

dkavolis commented 5 years ago

Do you mind submitting the changes without sweep/export of the stability derivatives (can be a single PR) so I can merge them in?

Rodhern commented 5 years ago

Do you mind submitting the changes without sweep/export of the stability derivatives (can be a single PR) so I can merge them in?

That sounds like a plan to me. I will try to find some time in the new year to make an updated pull request to one of your newer branches.

I will do it as a single new untested (because I do not play any of the newest KSP versions and cannot check to see if the merged source compiles) PR, once I have removed / commented out the stability derivatives export buttons in the GUI, and added (some or most of) the code readability enhancing normalizations and tweaks discussed in the comments above.

Rodhern commented 5 years ago

Closing this PR; it is to be replaced by a newer PR.