post-kerbin-mining-corporation / CryoTanks

Adds cryogenic fuel storage options and limited fuel switching to Kerbal Space Program
15 stars 30 forks source link

Patches: Kerbalism support #52

Closed PiezPiedPy closed 6 years ago

PiezPiedPy commented 6 years ago

I'm currently repairing Kerbalism support for CryoTanks as it is currently broken 😞 To help support the changes in Kerbalism I have slightly changed the CryoTanks patches. Changes include the following:

ChrisAdderley commented 6 years ago

Can you explain to me why Kerbalism can't deal with this on its own? eg

AFTER:[CryoTanks]
-> delete stuff
-> do stuff

This is how most mods build compatibility and minimize the integration surface...

PiezPiedPy commented 6 years ago

cuts down on the number of patches MM has to apply and results in faster load times. On my dev build with only Kerbalism, CRP and CryoTanks the number of patches was reduced by approximately 250 by making changes to your patches, include other mods and the number only increases.

Support file for CryoTanks in Kerbalism is here if you would like to take a look https://github.com/steamp0rt/Kerbalism/blob/master/GameData/Kerbalism/Support/CryoTanks.cfg

PiezPiedPy commented 6 years ago

The biggest impact is the dependency on B9PartSwitcher, the changes to the Fuel Cells and ISRU are minimal. If you feel you don't wish to add Kerbalism support to your patches could you at least consider the changes to remove the dependency on the B9PartSwitcher.

ChrisAdderley commented 6 years ago

I will not be removing B9PartSwitch as a dependency - there's a hard requirement due to the model switching in the package. It should never be distributed without it.

PiezPiedPy commented 6 years ago

CryoTanks works perfectly fine without B9PartSwitch, maybe some people don't want to use B9PartSwitch and are happy just to have simple LH2 Tanks without other Fuel Tanks being modified, me being one of them. Personally I think it would be better to let the end users have the option of using B9PartSwitch if they desire to have the extra functionality rather than forcing users to use it.

Gordon-Dry commented 6 years ago

I would say when something works as it is it's not necessary to

  1. change it
  2. change other stuff as well
  3. force it
  4. make it depending of stuff it would not be depending of if 1. haven't happened
ChrisAdderley commented 6 years ago

Well, it doesn't work fine - I'm not sure where you would get the impression. In the case without the plugin, you will have overlapping meshes on all the ZBO tanks that heavily leverage B9Partswitch for displaying part variants.

To make matters worse, the only two mods that ship CryoTanks (CryoEngines and Kerbal Atomics) both have a hard dependency on B9PartSwitch.

PiezPiedPy commented 6 years ago

CryoTanks when shipped with those mods (ZBO, CryoEngines, Kerbal Atomics or any other mod using the B9PartSwitch) will still work fine with the changes I have made to the patches as they have B9PartSwitch installed and thus nothing will change in how CryoTanks is configured by MM. The changes I have made only come into effect when B9PartSwitch is not installed allowing CryoTanks to work standalone but only have Simple LH2 tanks.

I have not removed any of CryoTanks functionality regarding B9PartSwitch, it still operates as it previously did when B9PartSwitch is installed along side CryoTanks.

I have tried and tested as well as other people have the changes I have made and they report CryoTanks as working fine.

B9PartSwitch is only needed if people want the extra functionality, if people don't want to install B9PartSwitch then CryoTanks will not work without the changes I have made unless they patch the files themselves but with the changes I have made CryoTanks will work fine but wont have the extra functionality only simple LH2 Tanks as previously stated.

Why you are resisting letting people install CryoTanks without B9PartSwitch is beyond me, why be so stubborn and force users to use B9PartSwitch when they have no desire to, especially when CryoTanks can work fine without B9PartSwitch when the very minor changes I have made are applied.

Also everything I have said here is regardless to the changes I have made to the FuelCells and the ISRU for Kerbalism support.

PiezPiedPy commented 6 years ago

For reference Issue 45 in Kerbalism https://github.com/steamp0rt/Kerbalism/issues/45

ChrisAdderley commented 6 years ago

I'll be more descriptive then. Here is a nice little snippet which comes from here.

{
        name = ModuleB9PartSwitch
        moduleID = textureSwitch
        switcherDescription = #LOC_CryoTanks_switcher_tankappearance_title

        SUBTYPE
        {
            name = Foil
            transform = COLLIDERA
            transform = 125mStructure
            transform = 125TankExtraStructured
            transform = 125TankFoilStructured
            node = top01
            node = bottom01
            title = #LOC_CryoTanks_switcher_tankappearance_variant1
        }
        SUBTYPE
        {
            name = White
            transform = COLLIDERA
            transform = 125mStructure
            transform = 125TankExtraStructured
            transform = 125TankIsoStructured
            node = top01
            node = bottom01
            title = #LOC_CryoTanks_switcher_tankappearance_variant2
        }
        SUBTYPE
        {
            name = WhiteBare
            transform = COLLIDERB
            transform = COLLIDERC
            transform = 125TankExtra
            transform = 125TankFoil
            node = top02
            node = bottom02
            title = #LOC_CryoTanks_switcher_tankappearance_variant3
        }
        SUBTYPE
        {
            name = FoilBare
            transform = COLLIDERB
            transform = COLLIDERC
            transform = 125TankExtra
            transform = 125TankIso
            node = top02
            node = bottom02
            title = #LOC_CryoTanks_switcher_tankappearance_variant4
        }
    }

You remove B9PartSwitch, you remove this functionality. The result is that this part (and others like it, of which there are 14) will show model clipping and produce unpleasant results. This isn't extra functionality, this is base functionality (as art bugs will directly result without it). I don't see how you could see it differently.

The only thing I can think of is if some user goes and installs CryoTanks, and keeps the Patches and Plugins folders, but deletes the Parts folder. In this case, yes, B9PartSwitch is not required. However, this goes a good distance outside of the supported configuration of the mod, and there's really no reason for me to support it this edge case.

PiezPiedPy commented 6 years ago

Here is a couple of screenshots without B9PartSwitch, are they not displaying correctly ? screenshot0 screenshot1

ChrisAdderley commented 6 years ago

All the clipping and nasty multi-models showing would say that yes, they do not display right.

PiezPiedPy commented 6 years ago

now I understand what you mean. I'll close this PR. Regarding Kerbalism and the FuelCells, ISRU I will remove the ModuleResourceConverter via the Kerbalism support patch in Kerbalism.