blowfishpro / B9PartSwitch

A Kerbal Space Program plugin designed to implement switching of part meshes, resources, and nodes
GNU Lesser General Public License v3.0
50 stars 33 forks source link

Little tested module switching feature + QOL improvements #100

Closed gotmachine closed 4 years ago

gotmachine commented 5 years ago

Edit :

Reworked PartModule switcher :

Todo :

Subtype config syntax : A MODULESWITCH allow to change the state and fields one or several PartModule(s) of a single type We can have several MODULESWITCH on a subtype, but they must target different PartModules When a subtype is switched off, the module state and fields are resetted to their default values Note that having two different ModuleB9PartSwitch managing the same PartModule won't work as expected. A MODULESWITCH can have the following fields and sub-nodes :

Type of the PartModule name = ModuleEngines

Default behaviour is to selected the first found PartModule The following options can be use to select a specific module, or several modules : Option 1 : select all modules findAll = true/false Option 2 : find by zero-based index moduleIndex = 1 // Will select the second ModuleEngine on the PartModule Option 3 : find by field and value pair Can be used with bool, string, floats and enums

// Will select an engine that have the field EngineType = LiquidFuel
fieldIdentifier = EngineType
valueIdentifier = LiquidFuel    

Disable the PartModule disableModule = true

Alter the PartModule stats trough a MODIFIERS node A MODIFIERS node can be used together with "disableModule = true" to force the state of the module before being disabled (can be useful to reset animations for example). A MODIFIERS node is able to delete, edit or create all values and sub-nodes that are accepted by the PartModule. To edit or create a value, just write it as you would have in the PartModule :

MODIFIERS
{
    maxThrust = 35
}

Same with sub-nodes :

MODIFIERS
{
     PROPELLANT
     {
        name = MonoPropellant
        ratio = 0.9
        DrawGauge = True
     }
}

Basic ModuleManager example thats adds a Monopropellant mode to the LV-909 LFO engine and alter thrust and ISP :

@PART[liquidEngine3]
{
    MODULE
    {
        name = ModuleB9PartSwitch
        moduleID = EngineFuelSwitch
        switcherDescription = Fuel mode
        switcherDescriptionPlural = Fuel modes
        SUBTYPE
        {
            name = LFO
        }
        SUBTYPE
        {
            name = MonoProp
            MODULESWITCH
            {
                name = ModuleEngines
                MODIFIERS
                {
                    maxThrust = 40
                    atmosphereCurve
                    {
                        key = 0 280
                        key = 1 85
                        key = 3 0.001
                    }
                    PROPELLANT
                    {
                        name = MonoPropellant
                        ratio = 0.9
                        DrawGauge = True
                    }
                }
            }
        }
    }
}
gotmachine commented 5 years ago

Thanks for reviewing this. Indeed my coding skills are pretty basic so this may need a full review.

As proven by Shadowmage for SSTU, adding and removing the modules is a dead end unless the modules are carefully designed to handle that. On the other hand, the enable/disable approach has been proven to be relatively doable by ShotgunNinja's Kerbalism. The PAW is correctly updated, and I can confirm that engine switching is correctly handled by KER, I assume that MechJeb should handle it too since this is the method used by the stock MultiModeEngine module.

Some handling is needed for jettison (shrouds), fairings, AnimateGeneric, AnimationGroup to revert them to their initial state on disabling, but nothing game-breaking.

I didn't commit my .csproj since I edited it to fit my post-build events. I will edit it manually.

I will try to refine all this in the following days.

blowfishpro commented 5 years ago

Hmm. I have wanted to implement module data switching for a while. However, I have some concerns about this approach. And I feel bad for voicing these now since you're written all this code. Nonetheless, maybe some of them can be addressed:

Some further thoughts:

gotmachine commented 5 years ago

Don't feel bad, I've already rewritten this thing twice, and I'm doing it as a happy hobby. It would indeed be simpler to require the full module config to be present in the switcher config but it would be a bit heavy from the config file writer point of view, it means a lot of duplicated values. Honestly the MODIFIERS code is quite simple and should be able to handle every possible scenario (edit, create or delete), and is perfectly compatible with ModuleManager. In fact I did this because in my mind it would allow easier patching with MM.

But TBH this is the last of my worries. The hard thing with the whole module switching is correctly loading the data and initializing the modules. The core problem is that we can't just call Load() and expect it to work (and I did enough tests to know that in most cases it doesn't) : since the module has already gone trough it's whole init sequence, plus potentially a bunch of updates and user interaction, it isn't in the expected state (a freshly instantiated prefab). Not to mention that some modules are actually messing with the parts (effects...) or other modules (engine > gimbal...). I did a lot of tests and the current solution is to call every possible initialization method (including the ctor). This works quite well excepted for those *!#!! ModuleEngines and their incredibly annoying effects (note that ModuleEnginesFX works fine).

As for the module identification system, it would indeed be nice to have a global "dictionary" at the B9PartSwitchModule level. As for the identification method, I think the three provided options offer quite some choice and if you think about it it's not very common the have several modules of the same type on a part and they should at least have one common field that has a different value. I also despise the fact that the PartModule class doesn't provide a "ID" field, but as things currently are the index method is robust, since moving modules index at runtime WILL wreak havoc everywhere.

This is the major reason for why I thing going the instantiate/destroy PartModule way is impossible : unity doesn't allow changing components index.

As for the commit thing I'm sorry but I'm not very good at this and I messed up things on my side by making several branches and rebased some on others (I'm also working on an interface thing, customizing the stock part tooltip from the part list with an interactable interface for switching).

blowfishpro commented 5 years ago

Did you see the correction I made to instead suggest calling Load twice (once with original data, 2nd time with partial new data)?

gotmachine commented 5 years ago

Yes, but this has the same issue as using the stock "ConfigNode.CopyTo" method, and the reason why I created my own method with a delete ability : you can only overwrite existing values or add them. For example, you can't switch an engine that has 2 PROPELLANT nodes into one that has only one PROPELLANT.

blowfishpro commented 5 years ago

ModuleEngines and ModuleRCS both clear their propellants every time you load new ones, so simply providing a new PROPELLANT node will accomplish that.

blowfishpro commented 4 years ago

I rewrote the code of how subtypes make modifications to parts, hence the merge conflicts. Overall it's more modular and I'm much happier with it. I also mostly ported this code to the new system. The one thing I'm missing now is most of the custom handling for various modules that need it. Do you have notes on which modules need things done in a certain order, need OnStart or some other method called, etc?

gotmachine commented 4 years ago

I fear and don't have more notes than the comments in the PartModuleSwitchInfo.cs file, and TBH I've forgotten a bit how this thing was working, had to put myself back into it.

From what I remember, the idea was to have "confignode patches" being applied on top of the default module config, the manually reproduce the whole module initialization sequence in an attempt to reset the state of module. Beginning from the ctor and forward to the first fixedupdate, with "confignode-patches" being applied at the right time in between.

This is done in the PartModuleSwitchInfo.Enable method. You can see the where the "config-node patching is loaded here : https://github.com/blowfishpro/B9PartSwitch/blob/e18f249d931fedbc255be36d6cd6a0c3f9838017/B9PartSwitch/PartSwitch/PartModuleSwitchInfo.cs#L81

Now I did a fair bit of testing with the stock partmodules (MM-adding a generic enable/disable switch to all modules on all parts and building the "all parts vessel") to find the "ideal" module initialization sequence, and developed the below hacks when there was still issues.

As for plugins modules, I did literally zero tests and I'm not super optimistic for things like realplume or any partmodule that does animation / effects. This said, I think plugins authors can easily fix the issues, as it's usually just a matter of resetting the state of things in the module awake/start/load sequence.

As for the potential / known issues :

The module specific hacks that I identified and implemented are in the ApplyEarlySpecifics and ApplyLateSpecifics methods.

ApplyEarlySpecifics is called just before Load/OnLoad. The hacks are are mainly to reset the state of the part (animations) after disabling a module. It mainly does so by altering the confignode before it's loaded, in order to not have to deal with thing manually and let the modules set things back to a "disabled state" by themselves.

In order :

ApplyLateSpecifics is called after the whole initialization sequence. Here I'm doing the more complec hacks for things that didn't reset themselves by just setting the right state before onload.

When engines OnStart is called, the module does this :

To fix this we need to manually re-link the engine XXXGroups with the FX that still are on the part, and call the setup method AutoPlaceFXGroup

blowfishpro commented 4 years ago

Closing this as #151 is merged.

154 is left open to handle adding additional module-specific code. Definitely going to reference your work when this is done, but I wanted to see the use cases for each module first before trying to support them.

Even if your implementation didn't get merged, believe me when I say you were hugely instrumental in getting this implemented and released.