Crash with principia when dropping item

[warmist]

I've reported this to principia and it seem you are doing the wrong thing (not them) (tm)
mockingbirdnest/Principia#1549

[pleroy]

Principia co-author here: well "doing the wrong thing" is maybe an overstatement 😉. We believe that you create a vessel with no parts, and then add the part later (in another frame? after it is in the right state?). Is that possible? Do we have a way to tell that such a vessel was created by you? We need the parts to do our physics computations and we fail on a vessel-with-no-parts because we assume that our code broke some invariant.

[ihsoft]

@pleroy Indeed, KIS do weird things in the game since there is no a good way to spawn an arbitrary part (no public interface is offered by the game core). So I had to re-invent the process, and there were some corners cut. You've mentioned the vessel without parts is the problem, are you referring the Vessel.parts collection? I think I can add the newly created part into this collection. The game was just fine without it, but if it makes your mod happy, I'll add it.

[eggrobin]

@ihsoft Other Principia co-author here, since @pleroy is away today.
It is understandable to do weird things when modding KSP in elaborate ways, Principia itself does a lot of that. @pleroy is indeed referring to the Vessel.parts collection.

Specifically, we require the following invariant that seems not to hold in @warmist's save:
After yield return new UnityEngine.WaitForFixedUpdate(), for every Vessel v in FlightGlobals.Vessels which is "manageable by Principia"^[1], if !v.packed, then v.parts contains at least one Part p such that p.rb != null^[2].

@warmist's logs show that a vessel called SP-L 1x6 Photovoltaic Panels was added just before the crash, and that no parts were added to that vessel; since no vessel had parts removed during that frame^[3], the offending vessel must be SP-L 1x6 Photovoltaic Panels.

Further, @warmist mentions dropping an item, and from the logs, seems to have successfully attached solar panels already, so the issue seems to be specifically with dropping. From digging through the KIS codebase I am guessing dropping an item ends up calling CreatePart with null coupleToPart, which in turn calls WaitAndMakeLonePart; this does not seem to populate newVessel.parts, so unless the game does that itself (which is always hard to tell) that could explain the issue.

---

^[1] This essentially means flying sufficiently high, and excluding weird edge cases; we intend to change the requirement of "sufficiently high" to "not touching the ground, including via other vessels" soon anyway).

^[2] Ideally we would like the parts to have been around since the BetterLateThanNever callback, but if that was not the case we work around it.

^[3] Removal and destruction of parts are logged on our side; previous logs are from a solar panel being added to the station three and a half seconds earlier and are thus unrelated.

[warmist]

I just wanted to say that now reading it again it looks like i sound little bit like a jerk. Sorry, i'm a big fan of both mods and they are both great :)

[ihsoft]

@pleroy @eggrobin I did some research on this matter and found out that the problem is not the Vessel.parts collection. It's get properly initialized from the Vessel.Initialize method, which is called when making the part. The real problem is the absence of the rigidbody on the part of the new vessel: the part is created as "packed". It takes couple of fixed frame updates for the part to get unpacked and become truly physical. I've recalled that fight I had when spawning a part (specifically, the issues with the FAR mod). You cannot just create a new part in the scene since the game doesn't assume this is possible. What you can do, is spawning a new vessel. And this vessel is initially spawned packed (i.e. no physics), then it goes thru the physics enabling sequence.

I'm going to break the contract, and promote the part to physical before it's actually expected to become physical. I.e. you'll get it unpacked and fully physical in the onVesselPartCountChanged event. However, it will be just another edge case worked around. In case of more mods (or game situations) get broken, I'll have to roll it back.

[ihsoft]

@warmist No worries. You've reported the problem, and this is what was important.

[eggrobin]

@ihsoft Interesting; for what it's worth, when an EVA Kerbal is spawned (this is one case where the game spawns a vessel and a part while flying), it appears to have a rigid body immediately, since we did not encounter this particular issue* with EVAs.
Looking at the existing code (before your fix), it seems that you are already calling newPart.PromoteToPhysicalPart(), and that should set the rigidbody (even without calling Unpack), so I'm confused as to where the problem actually comes from...

Would it be enough to simply set newPart.physicalSignificance = Part.PhysicalSignificance.NONE before calling newPart.PromoteToPhysicalPart(), as you did in 7d537d5, without moving the Unpack?

---

* We encountered a different issue with unready EVA Kerbals, namely mockingbirdnest/Principia#1452, but that was because the game nudges them for a while, which as far as I can tell is not the case for KIS's single-part vessels.

[ihsoft]

Would it be enough to simply set newPart.physicalSignificance = Part.PhysicalSignificance.NONE before calling newPart.PromoteToPhysicalPart()

It could solve this particular issue, but it would also introduce an inconsistency: a packed part with a physical behavior. One of the differences between the packed and unpacked part is existence of the rigidbody. Since now I'm forcing the RB creation (by resetting the flag), I have to deal with the state as well. I'm still violating the contract since now the init module methods are called on a physical part, but at least there is no state inconsistency.

As for the kerbal vessel as a reference, it's very different from a regular vessel. IIRC, the kerbals don't exist in the packed state.

[eggrobin]

It could solve this particular issue, but it would also introduce an inconsistency: a packed part with a physical behavior.

Is that really an inconsistency? Parts retain their rigid body (and their physical significance) while packed (for instance when timewarping), I think it only changes when a physically insignificant part gets decoupled (in which case that part is made significant).

[ihsoft]

Parts retain their rigid body (and their physical significance) while packed (for instance when timewarping)

Time warping is different. When it starts the parts get packed, when time is back to x1, the parts get unpacked. Of course, the rigidbodies are not dropped since it would destroy all the joints as well. However, when the vessel is loaded (e.g. when it enters into the physical range), all the parts are packed and there are no rigidbodies. Some parts create RB in the PartUnpack method, and some do it in the OnInitialize method. So it's hard to strictly define "the consistency", but to me it looks reasonable to not have RB on a packed vessel that has just spawned. It's kind of loading a vessel, except the vessel is created not from a config.