shadowmage45 / SSTULabs

Dev repository for testing/unfinished KSP parts/plugins/etc.
Other
62 stars 41 forks source link

Bug with B9 Part Switch upgrade system #812

Open pixe1reality opened 4 years ago

pixe1reality commented 4 years ago

So I've been trying to patch SSTU's engine system to use B9 Part Switch's upgrade switching feature, as seen in these screenshots with normal parts:

Image of an unupgraded F-1 using B9 Part Switch Image of an upgraded F-1 using B9 Part Switch (screenshots use a BDB part but showcases the idea rather well)

Problem is, when I switch to the upgrade when I cluster the engines, it sets the stats of the clustered engine to that of a single engine with that upgrade.

Any chance of a fix, or some other solution (another mod, standalone solution, etc.) to working around stock's rather wonky upgrade system?

blowfishpro commented 4 years ago

Well I did say to talk to me before trying to use this. I also said that not everything would work right out of the box.

I am not super familiar with the internals of SSTU’s clustering but I suspect it would require some research and then custom handling to get right.

shadowmage45 commented 4 years ago

So I've been trying to patch SSTU's engine system to use B9 Part Switch's upgrade switching feature, as seen in these screenshots with normal parts:

I really can't support any other mods that mess with modules like that. Far too much complexity involved for cross-mod interactions to work properly with SSTUs internal mess of custom code.

Problem is, when I switch to the upgrade when I cluster the engines, it sets the stats of the clustered engine to that of a single engine with that upgrade.

Yep, because the SSTUModularPart manipulates the same stats as the B9 module. Never going to work with them as separate non-communicating modules -- the one needs to know information specific to the other (e.g. either B9 needs to know about the # of engines in the cluster, -OR- SSTU needs to know about whatever thrust/ISP/etc changes have been applied).

I am not super familiar with the internals of SSTU’s clustering but I suspect it would require some research and then custom handling to get right.

I'm sure it would be doable in the end, but certainly not going to work 'out of the box' as was the OPs attempt. Notably, the point above needs to be addressed (both modules manipulating the same stats, ignorant of the other module).

Is there some sort of event that SSTU can call after it has adjusted the engines stats, where the B9 module can come in behind and adjust the ISPs/etc? How might we inform the B9 module about the # of engines in the cluster?

pixe1reality commented 4 years ago

yeah.... I guess I should have expected wonkiness between the two. It was an experiment to see how far I could push it. (hope this bug isn't a stupid question :pensive: )

pixe1reality commented 4 years ago

I remember another mod that manipulated the upgrade system in a similar way.... any idea what that was?

blowfishpro commented 4 years ago

B9PartSwitch doesn’t interact with the upgrade system. It ends up calling the same methods to load data and checks whether upgrades are unlocked but the stuff between the two is completely different.

I could look into adding an event that SSTU could subscribe to. Sounds like that might be the best way to make this work

pixe1reality commented 4 years ago

That sounds like a good idea. Mods other than SSTU could probably benefit from its use which handle things in weird ways :slightly_smiling_face:

blowfishpro commented 4 years ago

@shadowmage45 here's my proposal. On the B9PartSwitch side, there would be code like this that runs every time another module is updated:

BaseEventDetails details = new BaseEventDetails(null);
details.add("module", theModule);
part.SendEvent("ModuleModified", details, 0);

And then on the SSTU side, in a module that may need to refresh some stuff when that module is updated:

[KSPEvent]
private void ModuleModified(BaseEventDetails details)
{
    if (details.Get("module") != engineModule) return;
    // code to deal with refreshing
}

Let me know if this makes sense/if you have concerns/if you want to do it a different way.