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

STAGE:LIQUIDFUEL counts inaccessible fuel. #513

Closed
geoffromer opened this issue Jan 18, 2015 · 33 comments
Closed

STAGE:LIQUIDFUEL counts inaccessible fuel. #513

geoffromer opened this issue Jan 18, 2015 · 33 comments
Assignees
Labels
bug Weird outcome is probably not what the mod programmer expected.

Comments

@geoffromer
Copy link

I'm experiencing an issue where, when I run this script on this .craft file, STAGE:LIQUIDFUEL seems to include fuel from the next stage up, even though the current engine can't use it. Specifically, when stage 2 activates, the script reports that it has 90 liquidfuel, when in fact it only has 45. Consequently, when the stage 2 engine flames out, kOS still thinks it has 45 liquidfuel, so it doesn't activate the next stage.

@surge9000
Copy link

Confirmed here too. Only occurs during/after staging, and only sometimes.
Incidentally it would be fantastic if there was permanent object/variable (perhaps something like STAGE:ACTIVE) telling you what stage is currently active. Autostaging is a nightmare at the moment.

kOS version 0.15.5, KSP 0.90

@erendrake
Copy link
Member

I haven't made the time to test this report yet but I wanted to be clarify.
There is a lot of new code in STAGE: that relies on some parts of the KSP
api.

Is this a problem only for the few updates while you are staging? Or once
you hit a certain stage is it wrong until the next stage starts?

On Wed, Jan 21, 2015, 01:54 surge9000 notifications@github.com wrote:

Confirmed here too. Only occurs during/after staging, and only sometimes.
Incidentally it would be fantastic if there was permanent object/variable
(perhaps something like STAGE:ACTIVE) telling you what stage is currently
active. Autostaging is a nightmare at the moment.

kOS version 0.15.5, KSP 0.90


Reply to this email directly or view it on GitHub
#513 (comment).

@surge9000
Copy link

I dont know what you mean by 'the few updates while you are staging'. Once stage:*fuel (I've seen it for solidfuel too) becomes wrong, it stays wrong, even if you stage manually, if that's what you mean.
I've tried ctl-c-ing a similar kOS script, and restarting it after a manual stage. It stays locked to the value it had when it failed.

I thought it might be a problem related to identical fuel tanks in different stages, but I checked a few hours ago with several tens of explosive and hilarious launches, but nope, i cant figure out a pattern that's helpful to you. Sorry.

@geoffromer
Copy link
Author

As surge9000 said, it's an ongoing problem- in my repro, the fuel reported by the script starts at 90 at the beginning of stage 2, and drops gradually to 45 at flameout, when it should be starting from 45 and dropping to 0. FWIW, this problem seems to correlate with a problem where KSP itself gets confused about the structure of the craft: when I edit the craft in the VAB, it keeps wanting to put engines and separators in the wrong stages. I manually fix this, of course, but it's possible that KOS is somehow reading whatever the VAB thinks the staging is "supposed to be", instead of what it actually is.

@surge9000
Copy link

I dont want to presume, but searching around for (unrelated) kOS bugs in vecdraw and geoposition happened to present this:
#174
It's marked as solved, but perhaps something like it has resurfaced because it wasnt fixed or understood properly?

A few of my tests showed that ALL parts have stages marked.e.g. try this on a sufficiently complicated rocket:

list parts in fubar.
for part in fubar {
print "Name: " +part:name +", stage: " +part:stage.
}

It's just that some of them like struts and 'useless' bits are numbered higher than or equal to the launch stage. If that fix was implemented the way it seems to have been, it's wrong.
I guess it's a leftover from old versions, where the 'useless' parts (like SAS and fuel tanks) showed up in the GUI? In this case the fuel tanks aren't so useless, so good for Squad's foresight in keeping that data. Now it's up to you to use it correctly!

@Dunbaratu
Copy link
Member

Issue #174's fix is entirely moot now and has been entirely overridden with new code. KSP itself provided an API for how to get stage quantities, and @erendrake replaced all the work I did in #174 with calls to KSP's own API. We figured it makes more sense to use SQUAD's own API for this if they provide one, as that would be more forward-compatible with future versions.

It may be that part of getting it changed to the new system introduced a new bug, but all the comments in issue #174 should be thought of as irrelevant now.

@geoffromer
Copy link
Author

@surge9000 the reason the stage numbers seem bizarre is that the STAGE suffix seems to be reporting the "istg" value of the part. According to this thread, that number is meaningful only for parts that appear in the staging list. For all other parts, istg is set equal to dstg, and dstg is numbered a different way (something like twice the number of decouplers between the part and the root). So the stage numbers can seem out of whack if you expect them to make sense relative to each other, but they make sense so long as you only compare parts in the staging list to other parts in the staging list, and parts not in the staging list to other parts not in the staging list.

@Dunbaratu Is it possible that some sort of istg/dstg mix-up is responsible for this issue?

@erendrake erendrake added the bug Weird outcome is probably not what the mod programmer expected. label Feb 19, 2015
@erendrake erendrake modified the milestone: 0.16 punchlist Feb 19, 2015
@erendrake erendrake self-assigned this Feb 20, 2015
@erendrake
Copy link
Member

@Dunbaratu do you still have the link to the thread where Harv replied to you about what method he advised we use?

@erendrake erendrake modified the milestone: 0.16 punchlist Feb 20, 2015
@cjbush
Copy link

cjbush commented Mar 6, 2015

A workaround for autostaging could be something like this:

until altitude > 100000 or ship:apoapsis > 200000 {
list engines in englist.
for eng in englist {
if eng:stage = stage:number {
if eng:fuelflow < 0.001 {
stage.
}
}
}
wait 0.001.
}

@erendrake erendrake modified the milestone: v0.17.0 Punchlist Apr 2, 2015
@erendrake erendrake removed their assignment Apr 3, 2015
@Dunbaratu
Copy link
Member

This may be related? Do you remember if you were using the procedural fairings interstage fairing?

http://forum.kerbalspaceprogram.com/threads/39512-0-90-Procedural-Fairings-3-11-manual-shape-controls-%28December-17%29/page277?highlight=procedural+fairings

@geoffromer
Copy link
Author

I probably had procedural fairings installed, but I didn't have any fairings on the ship in question - see the .craft file.

@erendrake erendrake self-assigned this Apr 14, 2015
@erendrake
Copy link
Member

so since we have to ditch the KSP API and roll our own logic we have some decisions to make about different types of resources

  • STACK_PRIORITY_SEARCH
    • Liquidfuel and Oxidizer
  • STAGE_PRIORITY_FLOW
    • Monoprop and Xeon
  • ALL_VESSEL
    • ElectricCharge and IntakeAir

each of these types behaves differently and should be included in stage only when appropriate.

@erendrake
Copy link
Member

This is my first stab at some rules

  • STACK_PRIORITY_SEARCH
    1. Find all of the active engines
    2. Walk all of their propellants, aggregating the unique PartResources we find.
    • Potential issue: inactive engines that will be removed in the next stage?
    • Potential issue: tanks that will be removed in the next stage that are not connected to an engine?
  • STAGE_PRIORITY_FLOW
    1. Walk whole craft and get unique resources of that type
    • Potential Issue: this rule would always be the same between SHIP:RESOURCES and STAGE:RESOURCES, we could instead just show this resource for parts that will be staged next?
  • ALL_VESSEL
    • i believe this should always show all of the resource, which again would make the SHIP:RESOURCES and STAGE:RESOURCES the same for ElectricCharge and IntakeAir

@Dunbaratu
Copy link
Member

Potential issue: inactive engines that will be removed in the next stage?
Potential issue: tanks that will be removed in the next stage that are not connected to an engine?

I think that what most people are probably expecting when they ask for stage:liquidfuel is the number corresponding to the little green bars showing up in the side staging list, and NOT the values in the upper-right window when you click "stage values". When the green bars all go to zero, stage:liquidfuel should be zero. So I'd say do not count any fuel that isn't attached to any active engine (they don't appear as green bars in the list).

@erendrake
Copy link
Member

@Dunbaratu im sure you are right for the STACK_PRIORITY_SEARCH resources. how about for monopropellant? should STAGE:MONOPROPELLANT always be the same as SHIP:MONOPROPELLANT ?

@Dunbaratu
Copy link
Member

Does monopropellant always "travel" globally or can it be blocked by a non-crossfeed part?
If it can be blocked, then it needs to be done just like liquidfuel. otherwise it doesn't matter because there's no such thing as "unreachable" mono prop so it's all "in" the current stage.

@space-is-hard
Copy link
Member

I believe that all RCS thrusters draw from the entire ship, but from lower stages first; however the monopropellant engine draws from the stage to which it's attached. This complicates the issue because what the user wants would depend on what type of monopropellant thruster they're trying to pull the fuel levels for. Probably best to keep the two separate.

@erendrake
Copy link
Member

@space-is-hard seriously? i honestly havent used the monoprop engines yet but if they use monoprop like other engines use fuel thats pretty messed up :P

@erendrake
Copy link
Member

well, i guess not that messed up, just not what i expected.

@space-is-hard
Copy link
Member

95% sure that's how it is. The KSP wiki doesn't specifically say, so you'll have to confirm in-game

@space-is-hard
Copy link
Member

Even if it isn't, sometimes I'll throw extra monopropellant on a lower stage despite that stage not having any RCS thrusters. It'll draw monopropellant from that lower stage first, and leave the upper stage full upon separation.

For example, my CSM has thrusters on both command and service modules, and both modules hold monopropellant, but the service module provides all monopropellant while it's attached. Once I drop the service module, I'll still have full monopropellant in the command module so I can orient properly during re-entry (I config reaction wheels down to realistic levels, hence the need for RCS on the capsule).

@Dunbaratu
Copy link
Member

To clarify, it doesn't draw from the LOWEST tank, but from the FURTHEST tank. It's only coincidental that the two are the same in your design.
(whichever reachable tank is more "hops" away from the engine consuming the fuel on the parts tree).

@dvenum
Copy link

dvenum commented Apr 16, 2015

I have the same with tanks and engines from AES. Ship can be reconfigured with correct liquidfuel count, that's strange. Also, more complex control with engine:fuelflow does'nt work because fuelflow always zero.
Is there some strange crutch, that I can use before 0.17 released?

@erendrake
Copy link
Member

@dvenum are you trying to determine if you should stage because an engine ran our of fuel? if so

LIST ENGINES IN elist.

FOR e IN elist {
    IF e:FLAMEOUT and STAGE:READY {
        STAGE.
    }
}

is what i use.

edit: i dont actually know if stage:ready is in release?

@dvenum
Copy link

dvenum commented Apr 16, 2015

@erendrake, You are right. Thank you, It's work now. It was my first rocket with kos.

edit: stage:ready there is in 0.16.2.

@baloan
Copy link

baloan commented May 14, 2015

For me the issue appears only when stage 0 does not include a liquid fuel tank. When it does not (because I added a heatshield) the fuel tank in stage 1 is added to "stage only" fuel in KSP. The fuel tank in stage 2 is ignored. Really annoying. Something really helpful would be stage[4]:liquidfuel...
ksp resource broken

@baloan
Copy link

baloan commented May 14, 2015

I'd be happy with

print resources[9]:stage.
8
print resources[9]:name.
liquidfuel
print resources[9]:amount.
1440

where the index # is provided in brackets. The # should be added as 1st columns in the resource list.
resources
Then I can do

WHEN resource[9]:amount < 0.1 {
    stage.
}

When I try

list resources to rs.

only AggregateResources are available. Kind of inconsistent and unexpected.
Erendrake proposed to traverse the engine list to find out whether to stage. Unfortunately that does not work for me. I'd like to configure staging using WHEN clauses. That way I can separate staging and navigation code.

@baloan
Copy link

baloan commented May 14, 2015

This would work:

// traverse parts for resources
list parts in ps.
for p in ps {
    if p:resources:length > 0 {
        print p:stage + " - " + p:name.
        for r in p:resources {
            if r:enabled {
                set s to "enabled".
            } else {
                set s to "disabled".
            }
            print "    " + r:name + ", (" + round(r:amount) + "/" + round(r:capacity) + "), " + s.
            if p:stage = 8 and r:name = "LiquidFuel" {
                set s0fuel to r:amount.
                // lock s0fuel to r:amount.
            }
        }
    }
}
print s0fuel.

but I can't use lock with s0fuel. Breaks with the error in the bottom.
lock fails
I think I can make it work the ugly way with WHEN THEN PRESERVE.
NB: The traversal loop consumes more than 150 ICUs and cannot be executed in a WHEN clause.

@baloan
Copy link

baloan commented May 14, 2015

Use this to access a parts resource directly.

// traverse parts for resources
list parts in ps.
// for p in ps {
set i to ps:iterator.
until not i:next {
    set p to i:value.
    if p:resources:length > 0 {
        // print p:stage + " - " + p:name.
        // for r in p:resources {
        set j to p:resources:iterator.
        until not j:next {
            set r to j:value.
            if r:enabled {
                set s to "ok".
            } else {
                set s to "disabled".
            }
            print "[" + i:index + "][" + j:index + "] " + p:stage + " " + r:name + ", (" + round(r:amount) + "/" + round(r:capacity) + "), " + s.
        }
    }
}
print ps[17]:resources[0]:amount.

@sshilovsky
Copy link

Still exists in 1.0.4.

Want to note here that quick start tutorial currently relies on correct STAGE:LIQUIDFUEL behaviour, that is very confusing for a beginner. The condition there might probably be changed to whether the thrust is positive.

@hvacengi
Copy link
Member

My quick work around answer is that you should use ship:maxthrustat(0) to control staging. Checking for 0 will work on any engine, and comparing to a previously stored value will work for any engine that isn't a jet (jet's vary thrust with velocity, and we don't currently expose a velocity dependent version). There is a plan to revise that tutorial anyways now that 0.17.3 has been released.

I'm having a hard time replicating this issue other than when the root part is in a stage with no liquid fuel. For my test, I used the vessel linked at the top of this issue, but inserted a stack decoupler between the probe core and the batteries. Launch the vessel, cut the throttle, and then stage. Under those conditions, the value returned for stage:liquidfuel is 90, rather than 45. If you look at the resources panel with "Stage Only" selected, you will see that this appears to be a stock bug, as the panel will also show 90.

Through a little trial and error, I have determined that the stock function GetActiveResources() is also returning the resource for the 2nd tank from the top (3rd from the bottom, stage 2 with the additional decoupler). Changes to which exact parts are used (0.625m decoupler vs 1.25, engines, etc.) does not seem to have an effect.

When I combine stages 2 and 3, the total capacity of the new stage 3 (previously stage 4) correctly shows 45. If this change is made in the VAB prior to launching the vessel or prior to the first "stage" event, then the correct available quantity is shown. But if this is done after the first staging event, only the value from the previous stage 2 is returned and not the value for the previous stage 4. Moving engines and decouplers into different stages only appears to move the problem around.

I've spent a little time walking through the underlying code in KSP trying to see how they calculate this value, and it's complicated to say the least. There is a chance that tonight or over the next couple of days I can dig in to it a little more, but short of grabbing code from alternate resource panel or KER I don't think there is a quick answer.

Last thought that came to me just before clicking comment: We can easily get a list of active engines, could we then use the propellants list to record the value for propellant.totalResourceAvailable and some how use that? That's what is used by ModuleEngines to determine if the engine is deprived or not and to display the green bars for each engine.

@erendrake
Copy link
Member

I have taken a few stabs at this and the issue that keeps coming up is the definition of stage. When users are asking for STAGE:LIQUIDFUEL they are not actually expecting "the fuel in tanks that will be shed on the next stage event" they are asking "the fuel available to the current engines". Those two numbers are often the same with traditional rockets but with the crazy shenanigans we get into with KSP they are often very different.

what is interesting is in the stock resource panel if you hit the Stage Only button it exhibits the same bug we are tracking here. Or at least it did a few patches ago.

My few attempts at squaring this circle ran into me making a bunch of breaking changes to our API. which is a bummer.

@hvacengi
Copy link
Member

I made a change to the code on my local copy today that looks like it has it working. At the very least, it's working the same way it already does. That is to say, it will return the total amount of the resource available for all engines. This won't work for asparagus staging, since there is still fuel available for an active engine at that point.

I recommend using this new implementation anyways due to the following:

  • This is the current behavior already, so we don't need to break anything.
  • I don't think we want to write KER and MechJeb levels of fuel simulation to determine all of the fringe cases.
  • maxthrustat(0) will work for staging on asparagus stages.
  • we could add a suffix to the vessel structure hasflameout which would eliminate the need to loop the engines to find out if there is a flame out.

I think the goal should not be to be perfect for all situations, but rather predictable and reliable for as many situations as possible. If we can eliminate the obvious stage mis-match, that should work well.

Any one else have any thoughts?

hvacengi added a commit to hvacengi/KOS that referenced this issue Jul 1, 2015
Fixes KSP-KOS#513
StageValue.cs
* Re-write the resource getter code to return liquid fuel and oxidizer
(and any other STACK_PRIORITY_SEARCH resource) based on the active
engines.
* This will return the total amount available to all currently active
engines.  It will not return zero when an asparagus stage is depleted.
It will also count any fuel available to an engine that was manually
activated rather than staged.
* All other resources are returned according to what the root part is
able to access (in my testing this aligns with the value shown in the
resource panel).

Fix stage:liquidfuel

Fixes KSP-KOS#513
StageValue.cs
* Re-write the resource
getter code to return liquid fuel and oxidizer
(and any other
STACK_PRIORITY_SEARCH resource) based on the active
engines.
* This will
return the total amount available to all currently active
engines.  It
will not return zero when an asparagus stage is depleted.
It will also
count any fuel available to an engine that was manually
activated rather
than staged.
* All other resources are returned according to what the
root part is
able to access (in my testing this aligns with the value
shown in the
resource panel).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Weird outcome is probably not what the mod programmer expected.
Projects
None yet
Development

No branches or pull requests

10 participants