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

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

Merged
merged 22 commits into from
Apr 10, 2019

Conversation

Rodhern
Copy link

@Rodhern Rodhern commented Feb 4, 2019

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 Rodhern changed the title Dkavolis Rodhern's update 3 (without the export functionality). Feb 4, 2019
Copy link
Owner

@dkavolis dkavolis left a comment

Choose a reason for hiding this comment

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

Looking mostly good to merge, I've left a few comments. 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?

@Rodhern
Copy link
Author

Rodhern commented Apr 5, 2019

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
Copy link
Owner

dkavolis commented Apr 5, 2019

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.

Cosmetic modifications to solvers.
Move FAREditorStabDerivErrorExp check to StabilityDerivGUI.cs and split the check into two.
@Rodhern
Copy link
Author

Rodhern commented Apr 6, 2019

... 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
Copy link
Owner

dkavolis commented Apr 6, 2019

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

@dkavolis
Copy link
Owner

dkavolis commented Apr 6, 2019

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.

@dkavolis dkavolis merged commit 6b1a58f into dkavolis:master Apr 10, 2019
@dkavolis dkavolis mentioned this pull request Apr 10, 2019
@Rodhern Rodhern deleted the dkavolis branch May 20, 2019 18:27
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.

2 participants