DRVeyl / RealAntennas

KSP Mod to add better antenna / link calculations to CommNet.
29 stars 26 forks source link

Add support for Breaking Ground deployable science #41

Closed austinjames314 closed 3 years ago

austinjames314 commented 3 years ago

As initially discussed on the Kerbalism Discord:

So my entire knowledge of deployed science parts is currently limited to: what I have learned in the past 30 minutes by 
looking at the modules via DotPeek.

@Snoman So step 1: Add a ModuleRealAntenna PartModule to some parts [the one with a ModuleGroundExpControl 
PartModule, I believe] and see what happens.
@PART[...]:BEFORE[RealAntennas] { %MODULE[ModuleRealAntenna] { %referenceGain = 2.0 } }

Step 2: Make a save file with the deployed ground science, well, deployed.  Look in the save file for the vessel that corresponds 
to the deployed science.  It'll be one that has that ModuleGroundExpControl part in it.  Does the Vessel have a 
RACommNetVessel on it?  (Not just a CommNetVessel, would need to be the modified form, but it should get one by default)

I followed the directions above, and initial results were promising. This screenshot shows that the module was added to the part: https://i.imgur.com/K1dXNLj.png

The obvious problem was that I had not been able to configure the antenna in the VAB, as the part goes straight into the cargo bin. It defaults to an L band antenna, which I added to a nearby vessel and tried again to test. No connection still.

(As a slightly separate issue, how about the idea of allowing kerbals on EVA to reconfigure antenna bands? Possibly restricted to engineer kerbals or something. Would sidestep the problem above of the default antenna being a type not otherwise in use).

There is an RACommNetVessel on the 'vessel' as well:

            RACommNetVessel
            {
                powered = False
                controlState = ProbeFull
                canComm = True
            }

It seems likely that the lack of power is causing a problem, but the deployed science vessel type doesn't seem to use power.

I hope this gives you some info towards figuring this out.

sfs file: https://drive.google.com/file/d/1j91TSEaL4BoygSem_y782BCDZGCTwyu-/view?usp=sharing

DRVeyl commented 3 years ago

That is promising. You can set more fields in the config to adjust the defaults. %RFBand = S for instance. Another useful one is perhaps %TxPower = 20.

That powered = False would be an issue, yes. Looks like deployed science vessels do something weird/custom for power, based on the screenshot. Ick. https://github.com/DRVeyl/RealAntennas/blob/master/src/RealAntennasProject/RACommNetVessel.cs#L23-L27 is probably where that is going wrong. Can you stuff a RESOURCE[ElectricCharge] onto that same part, and see if that does anything?

austinjames314 commented 3 years ago

OK, yeah adding a bunch of electric charge to the deployed science part gave me a connection. There's no way for it to charge however, as the stock system is unlike a normal vessel, and those solar panels don't add any EC: https://i.imgur.com/JkEiPXd.png

Still no science though, which I suspect come from the Kerbalism side.

austinjames314 commented 3 years ago

OK so after further testing, there is science being transmitted. So far my patch file is:

@PART[DeployedCentralStation]:BEFORE[RealAntennas]
{
    %MODULE[ModuleRealAntenna]
    {
        %referenceGain = 2.0
    }
    %RESOURCE[ElectricCharge]
    {
        %amount += 1000000
        %maxAmount += 1000000
    }
}

How would I add the default RF band to this? Another @PART line with :AFTER[RealAntennas] maybe?

DRVeyl commented 3 years ago

% with += is risky. Those operators don't have an expected behavior if there is no value to change. You might need to separate that part of the patch, if you want to -increase- the electricCharge of what's there, or give it some if there isn't any. Using @PART[DeployedCentralStation]:HAS[!RESOURCE[ElectricCharge]] for instance.

However, since giving the part ec is a complete hack with deployed science, that won't end up going into the main RA release. Instead it needs a fix in code in the RACommNetVessel module to check ec differently. This hack "works" but only for the duration of that ec, and not sure what side effects.

Add %RFBand = <whatever> and %TxPower = 20 right after %referenceGain = 2.0 above.

austinjames314 commented 3 years ago

OK, fair enough. I know very little about writing module manager patches. I was mainly trying to prevent it from breaking anything that might change in the future. I'll change it to just = for now, because you say, it's a temporary hack. I'll add the RFBand via the method you give as well, and wait for an update when you're able to make one :).

tofof commented 3 years ago

I would suggest that, until this is implemented, the readme declare that this mod completely breaks Breaking Ground deployable science. Anything else about stock KSP that it renders nonfunctional should get a similar callout, e.g. #35.

Breaking things is bad behavior. At best it causes users to lose time troubleshooting, as I did today.

Nistenf commented 3 years ago

I wanted to do a mission using the Deployable Science stuff, so I decided to manualy merge 91e6a10 into the 2.0 codebase for my personal use. The change works fine with the stock science system, but currently conflicts with Kerbalism. The Kerbalism science file is generated correctly, and the vessel has both a connection and a positive bit rate, but the file never starts transmitting. Specifically, this check always fails, which makes sense since the vessel never has any EC.

Just to make sure, I added some EC to the part

@PART[DeployedCentralStation]:HAS[!RESOURCE[ElectricCharge]]:FOR[RealAntennas]
{
    %RESOURCE[ElectricCharge]
    {
        %amount = 100
        %maxAmount = 100
    }
}

and made sure it would not run out of it with a dirty ResourceUpdate in ModuleRealAntennas

public virtual string ResourceUpdate(Dictionary<string, double> availableResources, List<KeyValuePair<string, double>> resourceChangeRequest)
        {
            if ("DeployedCentralStation" == part.partInfo.name)
            {
                resourceChangeRequest.Add(new KeyValuePair<string, double>("ElectricCharge", 10));
            }

            return "thisIsAHack";
        }

With these changes applied, the file correctly gets transferred and the science value credited, both while the vessel is loaded and while it's not (I did add a similarily dirty BackgroundUpdate to make sure I wouldn't run out of EC while testing).

This incompatibility should probably get mentioned in next release's notes.

Let me know if you want me to test anything else!

DRVeyl commented 3 years ago

Specifically, this check always fails, which makes sense since the vessel never has any EC.

Can you raise this as an issue in the Kerbalism GitHub? It is intended for RA to be compatible with Kerbalism. But I cannot control if -they- are requiring an unexpected/non-existent thing in Breaking Ground. The code you referenced in Kerbalism does not look like it would work with BG even without RA.

In the commit you referenced, I had added the code:

if (Vessel.vesselType == VesselType.DeployedScienceController)
{
    var cluster = GetDeployedScienceCluster(Vessel);
    powered = cluster?.IsPowered ?? false;
}

It's purpose is to check the stock deployed science system and simply trust its answer for if the device is powered.

Nistenf commented 3 years ago

The code you referenced in Kerbalism does not look like it would work with BG even without RA

This comment made me think, because you're right, it looks like that but earlier I first tried both mods on their own and Kerbalism alone worked fine. So I looked into it a bit.

Kerbalism without RA passes that check because vd.Connection.ec_idle is 0, so it doesn't need to actually hold any ec. This ec_idle comes from the AntennaInfo object, which in 'stock' BG gets initialized to 0 somewhere and everything works. In RA the behaviour changes because RA creates its own AntennaInfo by injecting the RAKerbalismLinkHandler.

Ultimately, the ec_idle for RA comes from double ecIdle = powered ? raCNV.IdlePowerDraw() : 0;

So to test my theory, I tried to get that to be 0 in case of BG (I just added the if)

public double IdlePowerDraw()
{
            double ec = 0;
            if (Vessel.vesselType != VesselType.DeployedScienceController)
            {
                foreach (RealAntenna ra in antennaList)
                {
                    ec += ra.IdlePowerDraw;
                }
                foreach (RealAntenna ra in inactiveAntennas)
                {
                    ec += ra.IdlePowerDraw;
                }
            }
            return ec;
}

This does indeed work, without the need for the previous EC related hacks. The Kerbalism science file is transmitted and the science score updated accordingly, both in flight and in the space center.

I'm not familiar enough with any of these codebases to be certain this is the ideal fix, but at least it looks like it can be fixed from RA's side by simply controlling the AntennaInfo.ec_idle value.

If you still think this should be fixed by them I can of course raise the issue there, just let me know.

DRVeyl commented 3 years ago

Thanks for tracking that down. RACommNetVessel.IdlePowerDraw does seem a reasonable place to control that. Will add there.