-
Notifications
You must be signed in to change notification settings - Fork 69
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Handle collisions ourselves #2639
Conversation
foreach (Vessel vessel1 in | ||
FlightGlobals.Vessels.Where(v => !v.packed && is_manageable(v))) { | ||
if (plugin_.HasVessel(vessel1.id.ToString())) { | ||
if (vessel1.isEVA && (vessel1.evaController.OnALadder || | ||
is_clambering(vessel1.evaController))) { | ||
var vessel2 = vessel1.evaController.LadderPart?.vessel; | ||
if (vessel2 != null && !vessel2.packed && is_manageable(vessel2)) { | ||
Log.Info("Reporting climbing a ladder"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, the provenance of the collision is useful information. The actual part-to-part collison report also logs on the C++ side (with IDs and part names, but without the added context), so it’s not like this makes logspam much worse.
foreach (var collider in part1.currentCollisions) { | ||
#endif | ||
var collision_reporter = | ||
part1.gameObject.GetComponent<CollisionReporter>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this going to be expensive? I think we had performance problems with this kind of operation in the past.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are getting a component of this specific game object, and it should almost always be there.
We do kind of thing every frame for the navball already:
Principia/ksp_plugin_adapter/ksp_plugin_adapter.cs
Lines 1725 to 1731 in aef945b
private void RenderNavball(Vessel active_vessel) { | |
if (navball_ == null) { | |
navball_ = (KSP.UI.Screens.Flight.NavBall)FindObjectOfType( | |
typeof(KSP.UI.Screens.Flight.NavBall)); | |
} | |
var navball_material = | |
navball_.navBall.GetComponent<UnityEngine.Renderer>().material; |
Principia/ksp_plugin_adapter/ksp_plugin_adapter.cs
Lines 1836 to 1847 in aef945b
private void SetNavballVector(UnityEngine.Transform vector, | |
Vector3d direction) { | |
vector.localPosition = (UnityEngine.QuaternionD)navball_.attitudeGymbal * | |
direction * navball_.VectorUnitScale; | |
vector.gameObject.SetActive(vector.localPosition.z > | |
navball_.VectorUnitCutoff); | |
vector.GetComponent<UnityEngine.MeshRenderer>().materials[0].SetFloat( | |
"_Opacity", | |
UnityEngine.Mathf.Clamp01(UnityEngine.Vector3.Dot( | |
vector.localPosition.normalized, | |
UnityEngine.Vector3.forward))); | |
} |
I think you are thinking of incidents involving FindObjectOfType
, which looks at all objects; that is slow.
collisions.Clear(); | ||
} | ||
|
||
private void OnCollisionEnter(UnityEngine.Collision collision) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would vote for skipping the Add
if collision.collider
happens to be null
. That would simplify the client code somewhat.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not immediately obvious that this would be equivalent: what if the collider dies during the physics step?
Note that Unity objects have custom comparison operators, so != null
checks not just that the object
is not null (which would remain true as long as we keep a reference to it), but also that the UnityEngine.Object
has not been destroyed.
continue; | ||
} | ||
foreach (var collision in collision_reporter.collisions) { | ||
var collider = collision.collider; | ||
if (collider == null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my comment above and remove this statement. The comment about #1447 can move to the CollisionReporter
class.
@@ -1229,6 +1237,8 @@ class PartCentredForceHolder { | |||
// better ignore the collision, the Kerbal will soon become | |||
// ready anyway. | |||
} else if (is_manageable(vessel2)) { | |||
Log.Info($@"Reporting collision with collider {collider.name} ({ | |||
(UnityLayers)collider.gameObject.layer})"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as the ladder, this is useful.
It appears that
currentCollisions
is often wrong (reference counting never works).This improves matters with respect to the ejection part of #2607: the elimination of spurious collisions means that the Kerbal is no longer considered part of its aircraft, and thus we no longer impart erratic velocity and position corrections after ejection.
The issue of the attitude under parachute remains, as we do not understand how the parachute acts upon the Kerbal’s attitude.