search

dkavolis / Ferram-Aerospace-Research

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

Rodhern's update 3 (without the export functionality). #23

Closed Rodhern closed 5 years ago

Rodhern commented 5 years ago

A renewed possible merge for #4.

Note: I do not use KSP 1.6.1 myself and with my merge preparations to the code I don't know if it even compiles.

Rodhern commented 5 years ago

Do you have a craft file that I could test for the new search method on or does Brent's method fail for every craft at low Mach numbers?

The new search method is not an important part of the update, so it does not have to be used if you like Brent's method better. Even if the switch statement always invokes Brent's method we can just leave in the code for the new search method in the solution - in case anyone wants to recompile their own DLL later on.

I don't have a particular craft file, but on some old modded KSP 1.3.1 version I have, if I load the (stock) Mallard and try different speeds (all at Kerbin zero altitude) I see a difference. With Brent's method I get results for M0.35 (1.4 deg), and for M0.30 (3.1 deg) but not for M0.25. With the new search method I get the same results for M0.35 and M0.30, but also get results for M0.25 (6.0 deg) and M0.20 (11.5 deg).

Since this is an overload, the behaviour should be the same, instead move the pressure > 0 check to the implementation.

I would remove this check and let the caller handle such a case instead.

I can move both checks to the StabilityDerivGUI.cs file. Is that better?

I would keep the default values, easier to call if any future methods use it

Can simply use FARUtils.FARLogger.Info and remove [FAR] tag

Sure, I can do that. I will just add another commit on top of the current merge candidate.

dkavolis commented 5 years ago

I can move both checks to the StabilityDerivGUI.cs file. Is that better?

Yes, remove the checks from StabilityDerivCalculator.cs and only handle the case when pressure <= 0, remove the null case altogether or maybe spawn a different popup. Having the same one for pressure and solver failure is a bit misleading.

If possible, I would like to merge this before KSP 1.7 ships. I'll do a release then if nothing breaks.

Rodhern commented 5 years ago

... Having the same one for pressure and solver failure is a bit misleading.

I have split the check into two. Feel free to provide better separate error messages.

dkavolis commented 5 years ago

I'll check this later and if everything works will merge it. Thanks

dkavolis commented 5 years ago

For some reason git won't let me push anything onto the branch

Also, the popup when solver fails is quite annoying, I'd restore the old functionality for now.