Fairings still bork stowed engines

[Scarecrow88]

I was hoping with this release that the problem that generates the message 'cannot activate engine while stowed' would have been fixed.

Several of my designs use a fairing rather than an engine shroud to cover the engine of the stage above. The idea is that you stage as normal and extract the engine from the fairing, rather than activate the fairing thus jettisoning fairing bits all over the place, which is kind of messy. Unfortunately, if you stage before activating the fairing the game thinks the engine is still inside the fairing and won't let you activate it.

I'm not sure if this is a bug, or a game design, as I can see the logic of not wanting to activate the engine whilst it is still genuinely inside the fairing, but surely this comes down to staging management. It's no different to accidentally leaving the parachutes in the same stage as your engine in a single stage design, so that when you stage to take off, the chutes deploy as well, and everything goes to worms. The game doesn't stop the engines firing in that particular scenario, so why are they prevented from firing if they happen to be ensconced inside a fairing? Proper staging would prevent a disaster, but would also allow you to build designs that don't require the fairing to actually be activated.

[sal_vager]

The fairings and cargobays check occlusion when their state or the state of other cargobays and fairings that make up a connected space changes between "open" and "closed", such as when opening a bay, staging a decoupler covering the end or staging the fairing, they also check on load.

An unstaged fairing counts as closed, sorry.

[Scarecrow88]

I can understand that, to a point, but why would the fairing state affect an engine if the stage that the fairing is attached to has just been jettisoned, and the engine is attached to the stage that is still in flight?

[sal_vager]

Because the state is set ahead of time, and unless the fairing is staged there is no update, so the engine and any other occluded part that was in the unstaged fairing remain occluded until physics reload and a new occlusion check is made.

You're supposed to stage the fairing, it's by design, and I know it's disliked by many players.

But in all this time no one has put this on the Squad bug tracker, they instead complain about it here on the forum or Reddit or wherever, but if you want this changed it needs to be on the tracker, that's how the developers work.

So I took the liberty.

Feel free to vote it up so the devs can see it's something the players want.

[Scarecrow88]

I didn't even know the bug tracker existed, and I've been playing KSP for ages now. How daft do I feel!

[Azimech]

On 15-11-2015 21:05:51, sal_vager said:

So I took the liberty.

Feel free to vote it up so the devs can see it's something the players want.

Hmmm ... just created an account there but not sure how to upvote it. I did click a star though.

[sal_vager]

When in a report you should see a "Votes" entry with a green and red arrow, just above the description.

Currently it's on 4 upvotes.

[Majorjim!]

Hang on.. This is a very well known bug. We know, Squad know we have all known this since the fairings where made.

  What gives? I was also under the impression that Squad fixed this in the last patch.

 

[Claw]

They did fix some occlusion issues, but there's still no fix for interstage occlusion issues. The problem turns out to be annoying complex because of how events are currently triggered. The issue being that when you separate an interstage fairing, there's no Event that gets fired. And even if there was an event that was fired, the engine would still be "in" the fairing for the next couple frames, which would still result in a hit when running the test to see what's inside the fairing.

I've tried to fix this in my BugFixes, but since there's no event trigger, I think this one needs to be fixed inside the CargoBay module.

[sal_vager]

On 15/11/2015, 20:05:51, sal_vager said:

Because the state is set ahead of time, and unless the fairing is staged there is no update, so the engine and any other occluded part that was in the unstaged fairing remain occluded until physics reload and a new occlusion check is made.

Just like Claw and I mentioned, there is no update until the fairing is staged, a lot of work was done to make sure that parts in a cargobay or a fairing do not remain occluded when the bay or fairing is opened, but the bay or fairing has to be opened.

And for fairings that means jettisoning the fairing via staging or the action menu, unfortunately as Claw says that is the only event that fairings are checking for.

If you want this changed help me raise the votes on the tracker issue above.

[xEvilReeperx]

3 hours ago, Claw said:

The issue being that when you separate an interstage fairing, there's no Event that gets fired. And even if there was an event that was fired, the engine would still be "in" the fairing for the next couple frames, which would still result in a hit when running the test to see what's inside the fairing

I haven't come across this issue much myself (playtime :() but if the reference craft posted by sal_vager is representative of the problem, why not use OnVesselCreate? The problem with that craft isn't that the bay isn't updated but rather that at the moment the bay is decoupled from the rest of the ship, it captures (and shields) Parts belonging to the original ship and then doesn't ever check if they've moved out of range. Since its own update is now based on a separate ship (probably some debris), those incorrectly shielded Parts are now "stuck" unless that separate ship receives another event that triggers an update...

One quick bandaid is to prevent bays from shielding parts not belonging to their own vessels. A cleverer solution might be to keep an eye on the distance between a Part and the bay that shields it whenever they belong to different Vessels, and then remove the shield if they exceed a certain distance.

Here's my proof of concept:

[KSPAddon(KSPAddon.Startup.Flight, false)]
public class CargoBayStowageBandaid : MonoBehaviour
{
    private void Start()
    {
        GameEvents.onVesselCreate.Add(OnVesselCreated);
    }


    private void OnDestroy()
    {
        GameEvents.onVesselCreate.Remove(OnVesselCreated);
    }


    private void OnVesselCreated(Vessel data)
    {
        if (!data.loaded) return;

        UpdateFairingShieldsForLoadedVessels();
    }


    private void UpdateFairingShieldsForLoadedVessels()
    {
        foreach (var v in FlightGlobals.Vessels
            .Where(v => v.loaded && !v.packed)
            .Where(v => v.rootPart != null))
                        UpdateFairingShields(v);
    }


    private void UpdateFairingShields(Vessel vessel)
    {
        var fairingModules = vessel.FindPartModulesImplementing<ModuleProceduralFairing>()
            .Where(fm => fm.gameObject.GetComponent<ModuleCargoBay>() != null)
            .ToList();

        if (!fairingModules.Any())
            return;

        fairingModules.ForEach(fm =>
        {
            var bay = fm.gameObject.GetComponent<ModuleCargoBay>();

            var nearby = FindNearbyParts(bay).ToList();

            if (!nearby.Any())
                return;

            var partsWeShouldNotEnclose = FindPartsBelongingToOtherVessel(vessel, nearby);

            foreach (var p in partsWeShouldNotEnclose)
                StartCoroutine(WaitAndRemoveShield(bay, p));
        });
    }


    private static IEnumerable<Part> FindPartsBelongingToOtherVessel(Vessel vessel, IEnumerable<Part> parts)
    {
        return parts.Where(p => !ReferenceEquals(p.vessel, vessel));
    }


    private static IEnumerable<Part> FindNearbyParts(ModuleCargoBay bay)
    {
        if (bay == null) throw new ArgumentNullException("bay");

        var bayColliders = ModuleCargoBay.FindPartColliders(bay.part).Select(pc => pc.collider);

        var allNearbyParts = Physics.OverlapSphere(bay.transform.TransformPoint(bay.lookupCenter), bay.lookupRadius, 1 << 0) // Parts on layer 0
            .Except(bayColliders)
            .Where(c => c.gameObject.GetComponentInParent<Part>() != null)
            .Select(c => c.gameObject.GetComponentInParent<Part>())
            .ToList();

        return allNearbyParts;
    }


    private static IEnumerator WaitAndRemoveShield(IAirstreamShield shield, Part part)
    {
        yield return new WaitForEndOfFrame();

        print("Removing shield from " + part.partInfo.name);

        part.RemoveShield(shield);
    }
}

 

[sal_vager]

That's the idea I put forward, on decoupling do a check, but fairings don't know about the parts capping the ends so they can't know they are open on one end like cargobays do.

So it'd take modifying the module to keep a record of the part the fairing was closed around, and checking for it when onVesselCreate was called.

[Claw]

Also, the cargo bay module was explicitly designed to detect parts that are in the bay and aren't part of the vessel. The idea being that you can go capture kerbals, satellites, or whatever and hold them loose in the bay. (There are other practical problems with this, but that was the idea.)

Not shielding parts that belong to a vessel does fix the problem, but also isn't the intended behavior.

There is a variable that tracks the interstage part, but it's internal to the module, which is why it needs to be fixed internally. But the big sticking point (as mentioned) is that there is no event that fires when the engine leaves the bay.

I agree that there is a problem here. I just think that (unfortunately) there is still a disconnect between desired and actual behavior.

[xEvilReeperx]

6 hours ago, sal_vager said:

So it'd take modifying the module to keep a record of the part the fairing was closed around, and checking for it when onVesselCreate was called.

? The cargo bay already keeps track of parts it's shielded. It's not that it isn't triggering properly (see the delay in my proof of concept where I wait to make sure it's already run) but that it assumes everything it's shielding will stay shielded unless a significant event happens to its Vessel like being modified in some way (OnVesselWasModified, OnVesselPack/Unpack). There's no consideration for what might happen if the Parts it captured aren't part of the same Vessel... and since it ignores those events for every other Vessel you end up in a weird place where the only thing that can change whether your active ship engine is shielded is an event happening to the debris (containing the cargo bay) you just created.

Take your reference craft into orbit, stage until the two small engines run. Give it a bit of thrust. Watch the log until the debris is packed (triggering new occlusion update). Poof, stowage issue gone. That's the problem

[Claw]

Yes. Again, you are very correct. But removing the part from the fairing does not trigger an event. There's no natural thing that happens when removing the engine (or other part) that triggers the event.

What probably needs to happen is that the cargo bay module needs to know when it's an interstage, but the interstage piece is actually controlled by the fairing module. So the fairing module needs to track interstage status, and (probably the best bet) is to detect when a OnVesselWasModified happens. Then check if the interstage part has become part of a separate vessel (which is different from checking if all parts inside the faring are part of another vessel). In the case where the fairing is an interstage fairing, and the interstage receiver is now part of another vessel, the whole list should be unshielded and the fairing shielding rendered inoperable.

That's different than just constantly checking all parts in the bay/fairing, and seeing if they are part of another vessel. That will create unnecessary slowdown and undesirable behavior.

[xEvilReeperx]

36 minutes ago, Claw said:

That's different than just constantly checking all parts in the bay/fairing, and seeing if they are part of another vessel. That will create unnecessary slowdown and undesirable behavior.

I don't think I suggested such a thing (although creating a convex trigger collider would be my preferred solution which does kind of fall under that umbrella)... As for your solution, it won't catch every case. The reference craft provided by sal looks like an interstage fairing but isn't for example

 

Edited December 5, 2015 by xEvilReeperx

[Claw]

And now we see why this is such a complex problem.

[xEvilReeperx]

I honestly don't. You don't seem to agree with my simple solution because of the capture issue but I have two problems with that:

1. How often is an intact fairing made such that you can capture something inside it and it is reasonable that it be shielded? I keep thinking of a "basket" type fairing to catch stuff and having the items in the open end completely shielded from the airstream seems unrealistic
2. The capture mechanic is already broken in that should you end up in such a case as 1) and the fairing occlusion gets triggered, everything caught in the fairing will be shielded from the airstream and dragless/generate no lift even were it to fall out of the basket, until the catching Vessel triggers a new update

I'm not a rocket wizard so there might be some unconventional way to use a fairing as a catching mitt that I haven't thought of, but I think trading the ability to catch stuff [edit:in an aerodynamic-efficient manner] with a fairing for the ability to use interstage fairings properly is worthwhile

Edited December 5, 2015 by xEvilReeperx

[Claw]

You are right, you wouldn't use a fairing as some sort of catchers mitt. And even if you did, an open ended fairing isn't really useful for blocking airflow anyway.

I will go back and re-read your postings, as well as download sal's craft. Perhaps I'm misunderstanding what you're getting at. A lot of times, people are worried about not being able to fire an engine while inside an intact interstage. The other thing that some people don't realize is that the module that does the occlusion is the same module for fairings and cargo bays. So if you shield parts inside a fairing that aren't part of the same vessel, you've forced the same thing with cargo bays. The module was originally written for cargo bays, which is probably part of the reason it's suffering from some of these issues. The fairing module does some interfacing to the cargo bay module, but that's the piece that's lacking.

And yes, it's still possible to have loose parts inside a closed fairing. I don't know what need there is for this, but it's possible. And I'm certain that if it happened, someone, somewhere would be equally as upset that this thing they built to be loose inside a fairing isn't being shielded anymore.

Also, I'm not personally advocating for it to be one way or the other. Just trying to point out how things currently work. So I will take it as a personal fault, that I maybe assumed too much about your meaning when I was first replying.

[Claw]

On 11/15/2015, 12:46:14, Scarecrow88 said:

The game doesn't stop the engines firing in that particular scenario, so why are they prevented from firing if they happen to be ensconced inside a fairing?

It was an intentional game design decision.

 

On 11/15/2015, 9:38:00, Scarecrow88 said:

I can understand that, to a point, but why would the fairing state affect an engine if the stage that the fairing is attached to has just been jettisoned, and the engine is attached to the stage that is still in flight?

Because the problem is that the fairing only runs the occlusion checks (which reside inside the cargo bay module) on certain events. The cargo bay module runs the check only once for when called upon by certain Event triggers. So it actually is checking when you stage the decoupler, but the engine is technically still inside the ModuleCargoBay's search space. So it stays occluded. After the engine (or other part) has been physically removed from the fairing (which is really removal from the ModuleCargoBay's search space), there is no Event trigger that pokes ModuleCargoBay into running another update. That's why. It's currently designed that way, which is the crux of the problem.

 

11 hours ago, xEvilReeperx said:

I haven't come across this issue much myself (playtime :() but if the reference craft posted by sal_vager is representative of the problem, why not use OnVesselCreate? The problem with that craft isn't that the bay isn't updated but rather that at the moment the bay is decoupled from the rest of the ship, it captures (and shields) Parts belonging to the original ship and then doesn't ever check if they've moved out of range. Since its own update is now based on a separate ship (probably some debris), those incorrectly shielded Parts are now "stuck" unless that separate ship receives another event that triggers an update...

Yes, exactly. Your proposed "check to see if it's out of range" is another potentially viable solution. I will say that on the developer side, they are always keen on avoiding "constant checks" because it can cause a hit in performance to have too many of those going on. That's actually the reason why the list exists, and triggers a check only under certain Events. Rather than "am I far enough away? How about now? Now? Now? Am I?..."

 

4 hours ago, xEvilReeperx said:

There's no consideration for what might happen if the Parts it captured aren't part of the same Vessel... and since it ignores those events for every other Vessel you end up in a weird place where the only thing that can change whether your active ship engine is shielded is an event happening to the debris (containing the cargo bay) you just created.

Your assessment that I trimmed out is again spot on. This quote above was another specific design decision. That stuff inside the CargoBayModule's search area is supposed to get occluded, even if it's not part of the same vessel. That was an intentional design decision, but was made before fairings existed. The end result is a case where we end up with the issue being discussed in this thread.

 

3 hours ago, xEvilReeperx said:

The reference craft provided by sal looks like an interstage fairing but isn't for example

This statement made me think that sal had a different craft posted. I went back to download and I actually already have that one which I have used to confirm the interstage part tracking failures. It is, in fact, an interstage fairing. It's quite impossible to build an open ended fairing like that without making an interstage. Though I'm pretty sure the game fails in properly keeping track of the "interstage part" in-flight. (If you can build an open ended fairing in the editor, please let me know.) This part tracking failure also causes other issues, such as interstage fairings that wobble as if the end isn't connected (because technically it's not).

 

That's actually the best solution in my opinion, which I've already stated. Which is that the fairing module (not the cargo bay module) properly tracks when the fairing is an interstage (which it should already be doing, but isn't). Then, when an Event occurs, the Fairing module is the responsible party for checking: "Oh hey, I'm an interstage fairing. Some event has been triggered, is my interstage part still attached to this vessel? No...so tell the CargoBay module to unshield everything."

That would fix it, without constantly checking ranges or fiddling with actual Cargo Bay mechanics.

[Azimech]

21 hours ago, sal_vager said:

When in a report you should see a "Votes" entry with a green and red arrow, just above the description.

Currently it's on 4 upvotes.

Probably because I'm a new user and maybe don't have the rights yet, I can't find it anywhere. Oh well...

[cfds]

On 5.12.2015, 08:13:10, Claw said:

That's actually the best solution in my opinion, which I've already stated. Which is that the fairing module (not the cargo bay module) properly tracks when the fairing is an interstage (which it should already be doing, but isn't). Then, when an Event occurs, the Fairing module is the responsible party for checking: "Oh hey, I'm an interstage fairing. Some event has been triggered, is my interstage part still attached to this vessel? No...so tell the CargoBay module to unshield everything."

That would fix it, without constantly checking ranges or fiddling with actual Cargo Bay mechanics.

The best solution would just be to doing away with this pointless "cannot activate while being stowed" check. The game does not prevent activating an engine while it is clipped inside a crew capsule or opening a solar panel that would intersect with a fuel tank. Why should fairings and cargo bays behave in another way.

I know that it is an "intentional game design decision" but so was the souposphere and the infinite thrust RCS thruster.

[sal_vager]

cfds, this is support, if you want this addressed by the devs please see my tracker issue above or request that parts are not disabled while stowed.

Also, I'm glad parts are disabled, that way I don't have to deal with constant reports that peoples craft have exploded due to accidentally staging their stowed engines.

[xEvilReeperx]

Quote

I went back to download and I actually already have that one which I have used to confirm the interstage part tracking failures. It is, in fact, an interstage fairing.

Sorry for being unclear. Even if the game did track interstage fairing status, sal's reference craft still would not count. Somehow the id of the interstage part has been lost inside the .craft. KSP will just blindly create the fairing mesh based on the saved cross sections and because the last one just happens to have exactly the right radius, it's interpreted as a hole.

Here's a tweak of my first solution that will unshield non-vessel parts if the fairing has that hole:

Spoiler

public class FairingStowageFix : PartModule
{
    private ModuleProceduralFairing _host;

    public override void OnStart(StartState state)
    {
        base.OnStart(state);

        if (state == StartState.Editor)
        {
            enabled = false;
            return;
        }

        var fairing = GetComponent<ModuleProceduralFairing>();

        if (fairing == null)
        {
            enabled = false;
            Debug.LogWarning("FairingStowageFix: no fairing module found on " + part.partInfo.name);
            return;
        }

        _host = fairing;
            
        GameEvents.onVesselWasModified.Add(OnVesselModified);
    }


    private void OnDestroy()
    {
        if (!enabled) return;

        GameEvents.onVesselWasModified.Remove(OnVesselModified);
    }


    private void OnVesselModified(Vessel data)
    {
        if (!ReferenceEquals(vessel, data)) return;

        if (IsInterstage()) // build in a delay to be sure the cargo bay has responded to this event
            StartCoroutine(IgnorePartsThatThisVesselDoesNotOwn());
    }


    private IEnumerator IgnorePartsThatThisVesselDoesNotOwn()
    {
        var bay = _host.GetComponent<ModuleCargoBay>();

        if (bay == null)
        {
            Debug.LogError("FairingStowageFix: No CargoBay attached to " + part.partInfo.name);
            yield break;
        }

        yield return new WaitForEndOfFrame();

        foreach (var partWeShouldNotShield in FindNearbyParts(bay).Where(p => p.vessel != vessel))
        {
#if DEBUG
            print("Unshielding " + partWeShouldNotShield.partInfo.name + " because it belongs to " +
                    partWeShouldNotShield.vessel.id + " and not our " + vessel.id);
#endif
            partWeShouldNotShield.RemoveShield(bay);
        }

    }


    private bool IsInterstage()
    {
        if (_host == null) return false;

        var last = _host.xSections.SingleOrDefault(xs => xs.isLast);

        // ReSharper disable once CompareOfFloatsByEqualityOperator
        // note: direct comparison of floats intended (this is how the game does it!)
        return last != null && (!last.isCap || last.r == _host.capRadius);
    }


    private static IEnumerable<Part> FindNearbyParts(ModuleCargoBay bay)
    {
        if (bay == null) throw new ArgumentNullException("bay");

        var bayColliders = ModuleCargoBay.FindPartColliders(bay.part).Select(pc => pc.collider);

        return Physics.OverlapSphere(bay.transform.TransformPoint(bay.lookupCenter), bay.lookupRadius, 1 << 0) // Parts on layer 0
            .Except(bayColliders)
            .Where(c => c.gameObject.GetComponentInParent<Part>() != null)
            .Select(c => c.gameObject.GetComponentInParent<Part>());
    }
}

 

If you're still unsatisfied, the last thing that comes to mind would be to store the interstageID ourselves. The main disadvantage there is that it probably wouldn't retroactively apply to fairings already in flight if they too are missing that data