Directly update tank type and volume for ModuleFuelTanks

[bfrobin446]

If ModuleDataHandlerBasic was allowed to call Load() on a ModuleFuelTanks, it would clobber any resources that ModuleFuelTanks was managing, making it impossible to launch a rocket with fuel in the affected tanks. This code works to set the volume of a tank without resetting its contents, which should be enough to close issue #19 .

[blowfishpro]

I noticed that Procedural Parts does this with an event rather than reflection, maybe we could leverage that here too?

https://github.com/KSP-RO/ProceduralParts/blob/91286598583ba086a1801ed001346581b252a586/Source/ProceduralAbstractShape.cs#L260

https://github.com/NathanKell/ModularFuelSystem/blob/25caf921c9711d10f0d8b0d30532df445b253b41/Source/Tanks/ModuleFuelTanks.cs#L709

[bfrobin446]

Setting tank volume with the event turned out to involve a little bit of extra math, but everything still works the same.

[Bellabong]

Hi there, is there an estimation as to when this pull request is implemented/works? I've been doing RF configs for BDB and a lot of its newer parts are reliant on tank volume switching.

[blowfishpro]

@bfrobin446 can you commit to fixing issues if they come up?

[blowfishpro]

This should be in its own file

[blowfishpro]

Also can we get rid of the inheritance? It doesn't really use any of the base behavior other than the list of events it responds to which is better just copied.

[blowfishpro]

Err to clarify, it should inherit from PartModifierBase rather than ModuleDataHandlerBasic

[blowfishpro]

Can we make setsVolume and setsType (and probably the values too) in the constructor rather than computing them here? The original node will always have these fields so they'll end up getting hit even if the intention is not to modify one of them.

[blowfishpro]

Also I see we're throwing away everything on the node other than volume and type, do we really want that?

[bfrobin446]

@bfrobin446 can you commit to fixing issues if they come up?

I haven't been playing as much KSP in the last couple of months as I was in March and April, so my response time will probably be three to four weeks for the foreseeable future. But if that's a time frame you can live with, I can commit that I won't let anything I'm tagged in go longer than four weeks.

[DRVeyl]

This looks like it might be effectively dead (no new comments since Aug 2020). I think the RO team is interested in this feature, however. Should we fork the source of this PR, make some of the changes as described, and re-PR? I'm not familiar with the B9PS side of things... is this technique still reasonable?

Speaking as a current ProcParts maintainer and also doing refactoring of RF, I'll confirm that sending an event like so is a good way to tell MFT to update the volume. We probably need a better way to change the type dynamically [if you want something responsive while in the VAB] via a similar event. And maybe make the event handler support volume or totalVolume so you don't need to go through these contortions of gathering the utilization setting.

        private void InitVolume(ConfigNode node)
        {
            // If totalVolume is specified, use that, otherwise scale up the provided volume.
            if (node.TryGetValue("totalVolume", ref totalVolume))
                ChangeTotalVolume(totalVolume);
            else if (node.TryGetValue("volume", ref volume))
                totalVolume = volume * 100d / utilization;
        }

is some current code in MFT. So the idea exists, it's just not easily exposed. (And I'm not sure this code "works," it won't go through the handler that updates resource quantities... tho that would be a 1-line fix.)