Proposed refactoring: separate maintenance of vessel state from GUI and other functionality
cake-pie opened this issue ยท 6 comments
CLSAddon currently keeps a CLSVessel representation of the active vessel, listens for vessel modifications, and update the CLSVessel (and fire onCLSVesselChange) as needed. None of this is currently possible for other vessels. The newly implemented (#82) getCLSVessel returns a snapshot of the requested vessel's CLS data, but does not hang on to a copy of it or keep it updated. This means that each time another mod uses getCLSVessel on the same vessel, its CLS state has to be recalculated from scratch, even if there would have been no change from previous invocation.
In more concrete terms, here is one example use case. In my mod AirlockPlus, when a player clicks on an airlock, I call getCLSVessel to fetch a the vessel's CLS data, in order to determine which kerbals in the vessel are able to reach that airlock (obeying CLS rules) and exit through it. Suppose the player realizes this wasn't the airlock they'd wanted to use, so they cancel out of this action (without board/alight any kerbals) and instead clicks on another airlock on the same vessel. AirlockPlus will call getCLSVessel again, but since no "cache" of the data exists, it must be recalculated.
I cannot safely "reuse" the CLS data from it's earlier call, since there is no guarantee that it hasn't changed in the meantime (e.g. the player might indeed have alighted a kerbal from the first airlock, and then wants to alight another kerbal from a different airlock).
To get around this, we'd need to separate the maintenance of vessel state from GUI and other functionality. CLSAddon currently contains a mix of code that does all of this.
- The keeping+updating of a vessel's CLS state (upon loading, modification, etc) can be moved into a VesselModule, that would be active for loaded (off rails) vessels.
- CLSAddon is then left handling only the GUI aspects, and serve as the CLS API "point of contact" for other mods.
This approach would also confers advantages to CLS itself. Currently, when the player switches between vessels, the CLS data is recalculated upon each vessel switch. This also leads to "unnecessary" recalculations, e.g. in situations where a player is operating several vessels in close proximity to one another and is using [ and ] to switch between them frequently. By moving vessel state maintenance out from CLSAddon into VesselModule for each vessel, CLSAddon would only need to modify its "active vessel" reference to point at the corresponding VesselModule on vessel switching. There is no need to trigger a recalculation, since the vessels' CLS states aren't actually changing.
I've got the beginnings of this already started, but will need help making sure I don't break any existing functionality + ensure recoupler support is preserved.
Based on changes made by linuxgurugamer, I think these changes have been made and added to beta version 1.2.5.4. I will wait for response before closing, but I did see discussions of the Forum about these changes being incorporated. Review of the code does indeed show a CLSVesselModule now exists to support this feature.
Wait 1 week before closing issue.
The issue I raised in the forum is that linuxgurugamer merged my WIP code without asking, thus breaking recoupler support. I've since reinstated recoupler (cake-pie/ConnectedLivingSpace@379a29f) and also fixed a couple of other flaws I found (cake-pie/ConnectedLivingSpace@609e887).
I've requested Booots to check that recoupler support is working properly, since he'd know best, but I haven't heard back yet. If you want to go ahead and merge+close, the PR with the necessary fixes is #94
Thanks!
After discussing with Booots, (cake-pie/ConnectedLivingSpace@1caa47c) adds a bit of housekeeping to help ensure that the list of additional connections for Recoupler support stays short.
I'm for this. I've been considering a refactor for awhile. This provides a good basis for this. I will be looking forward to what you produce. This code was created a long time ago, and is in need of some love to take advantage of many changes to KSP. You should have seen the original hatch to docking port sync code.... but codepoet made it do what it needed within the constraints of a KSP .22 release
This is where I'm at right now. Could use a code review and with some testing to make sure the basics are all functioning correctly, before moving on to figuring out the best way to put recoupler support back in.