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

fix 1kN thruster suicide burn calc #902

Merged
merged 1 commit into from
Jun 15, 2017
Merged

fix 1kN thruster suicide burn calc #902

merged 1 commit into from
Jun 15, 2017

Conversation

lamont-granquist
Copy link
Collaborator

i'm not sure if real fuels changed its API so that this no longer works
correctly or if it was not correct in the first place -- but this
definitely gets minthrust/maxthrust very wrong for the 1kN thruster
in realism overhaul / real fuels.

also applied the thrust limiter w/o the ullage clipping to zero to the
max thrust, which is what i thought this problem was going to be in the
first place -- this still looks like more correct code here, but wasn't
the root of the bug i was seeing in the suicide calcs.

i'm not sure if real fuels changed its API so that this no longer works
correctly or if it was not correct in the first place -- but this
definitely gets minthrust/maxthrust very wrong for the 1kN thruster
in realism overhaul / real fuels.

also applied the thrust limiter w/o the ullage clipping to zero to the
max thrust, which is what i thought this problem was going to be in the
first place -- this still looks like more correct code here, but wasn't
the root of the bug i was seeing in the suicide calcs.

Signed-off-by: Lamont Granquist <lamont@scriptkiddie.org>
@lamont-granquist
Copy link
Collaborator Author

@sarbian do you recall the use case you were trying to fix with the code i backed out here? it definitely causes issues now, but i can't guess at what you were trying to fix...

@sarbian
Copy link
Contributor

sarbian commented Jun 14, 2017

I think the comment is clear "RealFuels engines reports as operational even when they are shutdown"
The engine report(ed) isOperational true even when not ignited.

I guess this is not longer the case ?

@lamont-granquist
Copy link
Collaborator Author

well, shutdown RF engines are not operational, so yes no longer the case?

but there's other conditions where an engine which is activated could be failed: vapor in the fuel lines, out of ignitions, Agathorn strikes again, etc.

but those conditions should be addressed via baking in better integration with RF: https://github.com/MuMech/MechJeb2/blob/master/MechJeb2/VesselState.cs#L1232-L1254

could probably put that into a PartExtensions for isEngineReallyOperational() that checked for vapor, ignitions, and Agathornage and get better code reuse out of that...

@sarbian sarbian merged commit 2962172 into MuMech:dev Jun 15, 2017
@lamont-granquist lamont-granquist deleted the lcg/fix-thruster-suicide-burn branch August 27, 2017 19:50
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.

None yet

2 participants