Save all types of contract kerbals to the server and also remove them

[81ninja]

Save all types of contract kerbals to the server (not only tourists, but also the unowned rescue kerbals) and also remove them from the server when they are removed from the game.

Note: in case advancing the protocol version is undesirable, a quick hack is to add the KERBAL_REMOVE items at the end of the *MessageType enums; this makes the client and server both compatible with protocol 44 but of course only newer clients will correctly remove kerbals from the server.

[Xinayder]

Thanks for submitting the PR! However, I'll adapt it if you don't mind, but still keeping you as the original author.

EDIT: I've committed part of your work, and now what's left is figuring out why the generated kerbals are being removed from the Astronaut Complex (just accept a contract to rescue a stranded kerbal and check the Astronaut Complex, the kerbal won't be listed as assigned).

[81ninja]

No problem! I'm rather new to KSP and DMP, and still getting the hang of it.

It seems the generated kerbals aren't assigned to the rescue vessels because the vessels are being spawned without the cockpits/crewable parts in them, that the kerbals would be assigned to. Look at the vessel .txt and client save files - there is no PART {}.

I don't know why, but that was working fine on my branch (rescue-contracts), until I merged commit #8aa1321. Reverting it makes the vessels be correctly spawned again, on my branch and yours, except the vessel then isn't sent to the server (on this).

The difference being that instead of just sending all vessels, my code finds the specific vessel that is spawned for the contract when it's accepted, and sends it. As far as I can tell, the new method for OnVesselCreated (which calls SendVesselUpdateIfNeeded) doesn't touch the in-game assets, but it is somehow conflicting with the game.

[Xinayder]

Well, I noticed it yesterday but thought it was nothing. Maybe when the contract vessel is created, the OnNewVesselCreated event is fired, but later the seats get added to the vessel. Maybe we can fix this issue by handling the contract vessel creation on the OnContractAccepted contract.

[81ninja]

When the contract is accepted, the game already creates a vessel, assigns the kerbal to it, assigns a part ID to the crewable part, and assigns that part ID to the partID value on the Contract. Spawning a vessel, which was what I did before in various ways, creates further conflicts - mismatched vessels between what is seen on the tracking station and the server, duplicate vessels and kerbals, game freezing... It's all more hassle.

I think you're right about OnVesselCreated being fired before the vessel is ready, and also before OnConctractAccepted. Apparently the game doesn't set up the rescue vessel quite synchronously.

But finding the vessel that is created and sending it works, except that the stock API methods for finding the vessel containing the part don't seem to find it - I think for that same reason. I had to "brute force" it by iterating through all vessels and comparing the part IDs.

Probably not a good idea in terms of client performance, but contracts aren't accepted every second anyway. Maybe that can be improved by using OnNewVesselCreated to take note on what vessels are created between it and OnContractAccepted, then looking for the right vessel among those only, rather than all ingame vessels.

Also, as the kerbals have to exist before the contract is accepted, it'd probably be simpler to send them to the server when the contract is first offered, which is when they are actually created. Either by detecting the new contract offering (safer, I think) or the kerbal itself (onKerbalAdded, simpler, but may affect other stuff).

Another TODO is handling what happens when the contracts are cancelled or fail,, either by cancelling them through Mission Control or terminating the rescue vessel from the Tracking Station. Currently the vessels aren't removed from the server, and are loaded again every next time the game is started.

Aaaaand... then there are contracts for rescuing parts and other stuff. But I haven't yet looked at those, partly because until a couple days ago my Career mode game was stuck with 7 out of 7 max contracts at the current facility level, all contracts for rescuing kerbals that didn't exist, from ships that weren't spawned. :stuck_out_tongue_closed_eyes:

[Xinayder]

Okay, I just tested. Moved the vessel send logic to the contract accepted event, but seems like DMP is removing any seats/parts in the vessel, don't know why.

When the vessel is saved on the server, its file differs from the one I print to the log (I'm saving the contract vessel and printing the config node).

[Xinayder]

Alright, progress report! I think I have finally managed to get the vessel to be saved correctly on the server. I have updated the code, and it should work now!

As 1.2.1 is released, this fix should hopefully make it to our next release, along with some other stuff like loading maneuver nodes. Thank you for helping!

EDIT: let me know if everything is still working as it should, before I can merge it to the master branch and prepare for a release.

[81ninja]

Roger that! I'll test it asap.

I've been testing the maneuver nodes, and also citruspress' reputation loss workaround (#361). So far no problems, though I didn't really properly test the reputation fix.

[81ninja]

It's looking good! It only needed a little tweak, which I just added (3647822) - saving the kerbals when the rescue contract is offered.

Also the contractOwner node value has to be added to Unowned kerbals (not just tourists), so that they too aren't loaded by others players.

EDIT: Too soon! I found tourists weren't being removed from the game after their contracts are completed. Or rather, they were being removed twice, but added again. This was caused by VesselWorker sending all kerbals from vessels upon recovery:

1718:            DarkLog.Debug("Removing vessel " + recoveredVesselID + ", name: " + recoveredVessel.vesselName + " from the server: Recovered");
1719:            SendKerbalsInVessel(recoveredVessel);
1720:            serverVessels.Remove(recoveredVesselID);
1721:            NetworkWorker.fetch.SendVesselRemove(recoveredVesselID, false);

So I've added a couple more lines to VesselWorker.cs (a89cbe1), to skip resending the tourists.

[Xinayder]

@81ninja why do we need to generate the kerbals when the contract is offered, and not when it is accepted?

Other than that, I'm sorry for my absence this week but I'll try to get these things done this weekend! Thank you again!

[81ninja]

Short answer is... That's what the game does.

And we have to save the Kerbal, not generate it. (sorry if I caused any confusion about that)

If the kerbal isn't saved, it won't be around the next time the game is loaded, because the game won't generate it again. And if the kerbal doesn't exist / isn't loaded when the contract is accepted, the vessel spawning bugs out, as we've seen.

No problem resting for a bit... I myself haven't been coding for the last couple weeks, got busy around other stuff (and actually playing the game :) ). Don't push yourself!

[Xinayder]

Short answer is... That's what the game does. Well, it was perfectly fine when I just generated the kerbals when the contract was accepted and if they weren't loaded/didn't exist. I think it works that way, so I'll just stick to it (unless Kraken decides it's buggy enough :P)

And yes, the kerbals and vessels are now being saved. I just need to tweak the part where the contract kerbals don't have the node in them.

[81ninja]

Well, it was perfectly fine when I just generated the kerbals when the contract was accepted and if they weren't loaded/didn't exist.

That was my first approach too, until I realized there are events and methods to detect the created kerbals (and vessels). I knew nothing about KSP mod development before this ;) Overall I think sending the stuff the game itself creates is a cleaner solution, less prone to breaking again in the future.

Still, generating the kerbals is needed for contracts that were already there before the fix. That can be dropped, either before the next release or after it.

I just need to tweak the part where the contract kerbals don't have the node in them.

I've added that and also renamed the value to use the same name (contractOwner) for both kerbals and vessels.

[Xinayder]

I don't know why this is open yet (maybe I forgot to add "closes #391" in the commit).

[81ninja]

tourists weren't being removed from the game after their contracts are completed. Or rather, they were being removed twice, but added again.

It'd be nice to merge a89cbe1 to fix the above for all players.

[Xinayder]

The issue described might be result of me not knowing how tourist contract works (I've never done one to be honest). I'm just gonna trust you that this works.