Skip to content
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

Merged
merged 3 commits into from
Jul 12, 2020
Merged

Conversation

eggrobin
Copy link
Member

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.

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");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove?

Copy link
Member Author

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>();
Copy link
Member

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.

Copy link
Member Author

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:

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;
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.

Sorry, something went wrong.

collisions.Clear();
}

private void OnCollisionEnter(UnityEngine.Collision collision) {
Copy link
Member

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.

Copy link
Member Author

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) {
Copy link
Member

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})");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove?

Copy link
Member Author

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.

@pleroy pleroy added the LGTM label Jul 12, 2020
@eggrobin eggrobin merged commit 8248da0 into mockingbirdnest:master Jul 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants