From 41b251ca52ccbc507120e174c7befaf8751ea510 Mon Sep 17 00:00:00 2001 From: dloe Date: Mon, 12 Aug 2024 14:12:35 -0700 Subject: [PATCH 1/6] Added early warning and better error logging for CharacterComponent missing physical shapes - Will now incorporate logger with a warning when Attachments of shapes for KinecticCharacter are skipped due to no ColliderShapes being present. - errors given inside CharacterComponent will also include call info for users to trace and find what corresponding code is calling the physic method. --- .../Elements/CharacterComponent.cs | 29 +++++++++++++++---- .../Stride.Physics/Engine/PhysicsComponent.cs | 6 ++++ 2 files changed, 30 insertions(+), 5 deletions(-) diff --git a/sources/engine/Stride.Physics/Elements/CharacterComponent.cs b/sources/engine/Stride.Physics/Elements/CharacterComponent.cs index 11ebcb5105..b0c8a25713 100644 --- a/sources/engine/Stride.Physics/Elements/CharacterComponent.cs +++ b/sources/engine/Stride.Physics/Elements/CharacterComponent.cs @@ -3,6 +3,7 @@ using System; using System.ComponentModel; +using System.Diagnostics; using Stride.Core; using Stride.Core.Mathematics; using Stride.Engine; @@ -26,7 +27,10 @@ public void Jump(Vector3 jumpDirection) { if (KinematicCharacter == null) { - throw new InvalidOperationException("Attempted to call a Physics function that is avaliable only when the Entity has been already added to the Scene."); + StackFrame frame = new StackTrace(true).GetFrame(1); //for capturing tracing info + logger.Error($"Component:[{this}] attempted to call a Physics function that is available only when the Entity has been already added to the Scene. " + + $"This may be due to a {this} without any physical shapes.\nLocation: {frame.GetFileName()} at Line Number: {frame.GetFileLineNumber()} from Method: {frame.GetMethod().Name} "); + return; } BulletSharp.Math.Vector3 bV3 = jumpDirection; KinematicCharacter.Jump(ref bV3); @@ -39,7 +43,10 @@ public void Jump() { if (KinematicCharacter == null) { - throw new InvalidOperationException("Attempted to call a Physics function that is avaliable only when the Entity has been already added to the Scene."); + StackFrame frame = new StackTrace(true).GetFrame(1); //for capturing tracing info + logger.Error($"Component:[{this}] attempted to call a Physics function that is available only when the Entity has been already added to the Scene. " + + $"This may be due to a {this} without any physical shapes.\nLocation: {frame.GetFileName()} at Line Number: {frame.GetFileLineNumber()} from Method: {frame.GetMethod().Name} "); + return; } KinematicCharacter.Jump(); } @@ -209,7 +216,10 @@ public void Teleport(Vector3 targetPosition) { if (KinematicCharacter == null) { - throw new InvalidOperationException("Attempted to call a Physics function that is avaliable only when the Entity has been already added to the Scene."); + StackFrame frame = new StackTrace(true).GetFrame(1); //for capturing tracing info + logger.Error($"Component:[{this}] attempted to call a Physics function that is available only when the Entity has been already added to the Scene. " + + $"This may be due to a {this} without any physical shapes.\nLocation: {frame.GetFileName()} at Line Number: {frame.GetFileLineNumber()} from Method: {frame.GetMethod().Name} "); + return; } //we assume that the user wants to teleport in world/entity space @@ -230,7 +240,10 @@ public void Move(Vector3 movement) { if (KinematicCharacter == null) { - throw new InvalidOperationException("Attempted to call a Physics function that is avaliable only when the Entity has been already added to the Scene."); + StackFrame frame = new StackTrace(true).GetFrame(1); //for capturing tracing info + logger.Error($"Component:[{this}] attempted to call a Physics function that is available only when the Entity has been already added to the Scene. " + + $"This may be due to a {this} without any physical shapes.\nLocation: {frame.GetFileName()} at Line Number: {frame.GetFileLineNumber()} from Method: {frame.GetMethod().Name} "); + return; } KinematicCharacter.SetWalkDirection(movement); @@ -246,7 +259,12 @@ public void SetVelocity(Vector3 velocity) { if (KinematicCharacter == null) { - throw new InvalidOperationException("Attempted to call a Physics function that is available only when the Entity has been already added to the Scene."); + //What was nice about the exception is users can see where this function was called to help trace + StackFrame frame = new StackTrace(true).GetFrame(1); //for capturing tracing info + logger.Error($"Component:[{this}] attempted to call a Physics function that is available only when the Entity has been already added to the Scene. " + + $"This may be due to a {this} without any physical shapes.\nLocation: {frame.GetFileName()} at Line Number: {frame.GetFileLineNumber()} from Method: {frame.GetMethod().Name} "); + return; + //throw new InvalidOperationException("Attempted to call a Physics function that is available only when the Entity has been already added to the Scene."); } KinematicCharacter.SetWalkDirection(velocity * Simulation.FixedTimeStep); @@ -262,6 +280,7 @@ public void SetVelocity(Vector3 velocity) [DataMemberIgnore] internal BulletSharp.KinematicCharacterController KinematicCharacter; + //called after collider shapes are attached to character comp? protected override void OnAttach() { NativeCollisionObject = new BulletSharp.PairCachingGhostObject diff --git a/sources/engine/Stride.Physics/Engine/PhysicsComponent.cs b/sources/engine/Stride.Physics/Engine/PhysicsComponent.cs index a17b513bfb..e90fb5d38b 100644 --- a/sources/engine/Stride.Physics/Engine/PhysicsComponent.cs +++ b/sources/engine/Stride.Physics/Engine/PhysicsComponent.cs @@ -645,6 +645,12 @@ internal void Attach(PhysicsProcessor.AssociatedData data) //this is mostly required for the game studio gizmos if (Simulation.DisableSimulation) { + //Give warning if character doesnt have collider shapes + if(this is CharacterComponent && ColliderShapes.Count == 0) + { + logger.Warning($"Entity {this} has no physical shapes attatched CharacterComponent."); + } + return; } From db6ac0605670effbe3b8b6d0633eb55f799897b4 Mon Sep 17 00:00:00 2001 From: dloe Date: Mon, 12 Aug 2024 14:45:59 -0700 Subject: [PATCH 2/6] Cleaned up comments + fixed typo in comment --- sources/engine/Stride.Physics/Elements/CharacterComponent.cs | 1 - sources/engine/Stride.Physics/Engine/PhysicsComponent.cs | 4 ++-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/sources/engine/Stride.Physics/Elements/CharacterComponent.cs b/sources/engine/Stride.Physics/Elements/CharacterComponent.cs index b0c8a25713..e6988d5dec 100644 --- a/sources/engine/Stride.Physics/Elements/CharacterComponent.cs +++ b/sources/engine/Stride.Physics/Elements/CharacterComponent.cs @@ -264,7 +264,6 @@ public void SetVelocity(Vector3 velocity) logger.Error($"Component:[{this}] attempted to call a Physics function that is available only when the Entity has been already added to the Scene. " + $"This may be due to a {this} without any physical shapes.\nLocation: {frame.GetFileName()} at Line Number: {frame.GetFileLineNumber()} from Method: {frame.GetMethod().Name} "); return; - //throw new InvalidOperationException("Attempted to call a Physics function that is available only when the Entity has been already added to the Scene."); } KinematicCharacter.SetWalkDirection(velocity * Simulation.FixedTimeStep); diff --git a/sources/engine/Stride.Physics/Engine/PhysicsComponent.cs b/sources/engine/Stride.Physics/Engine/PhysicsComponent.cs index e90fb5d38b..f76791fc70 100644 --- a/sources/engine/Stride.Physics/Engine/PhysicsComponent.cs +++ b/sources/engine/Stride.Physics/Engine/PhysicsComponent.cs @@ -645,10 +645,10 @@ internal void Attach(PhysicsProcessor.AssociatedData data) //this is mostly required for the game studio gizmos if (Simulation.DisableSimulation) { - //Give warning if character doesnt have collider shapes + //Give warning if CharacterComponent doesnt have collider shapes if(this is CharacterComponent && ColliderShapes.Count == 0) { - logger.Warning($"Entity {this} has no physical shapes attatched CharacterComponent."); + logger.Warning($"Entity {this} has no physical shapes attatched to CharacterComponent."); } return; From eea0d115195b2b44c2ea196738fcb4f3a4f60ff0 Mon Sep 17 00:00:00 2001 From: dloe Date: Tue, 13 Aug 2024 15:24:49 -0700 Subject: [PATCH 3/6] Added extra check for No Shapes warning Found reference to older code that may have at one point had this check and warning. Added the extra check I had not accounted for --- sources/engine/Stride.Physics/Engine/PhysicsComponent.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sources/engine/Stride.Physics/Engine/PhysicsComponent.cs b/sources/engine/Stride.Physics/Engine/PhysicsComponent.cs index f76791fc70..244dcf4e35 100644 --- a/sources/engine/Stride.Physics/Engine/PhysicsComponent.cs +++ b/sources/engine/Stride.Physics/Engine/PhysicsComponent.cs @@ -646,7 +646,7 @@ internal void Attach(PhysicsProcessor.AssociatedData data) if (Simulation.DisableSimulation) { //Give warning if CharacterComponent doesnt have collider shapes - if(this is CharacterComponent && ColliderShapes.Count == 0) + if(this is CharacterComponent && ColliderShapes.Count == 0 && ColliderShape == null) { logger.Warning($"Entity {this} has no physical shapes attatched to CharacterComponent."); } From 911ae180b3989d90d6505eb925ce9c2c0702648e Mon Sep 17 00:00:00 2001 From: dloe Date: Tue, 27 Aug 2024 13:14:36 -0700 Subject: [PATCH 4/6] Removed repetitive check for lack of collider shap - Removed check that occurs after DisableSimulation check and now has it ahead of simulation check --- .../Stride.Physics/Engine/PhysicsComponent.cs | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/sources/engine/Stride.Physics/Engine/PhysicsComponent.cs b/sources/engine/Stride.Physics/Engine/PhysicsComponent.cs index 244dcf4e35..549933ab60 100644 --- a/sources/engine/Stride.Physics/Engine/PhysicsComponent.cs +++ b/sources/engine/Stride.Physics/Engine/PhysicsComponent.cs @@ -642,27 +642,22 @@ internal void Attach(PhysicsProcessor.AssociatedData data) { Data = data; + if (ColliderShapes.Count == 0 && ColliderShape == null) + { + logger.Error($"Entity {{Entity.Name}} has a PhysicsComponent without any collider shape."); + return; //no shape no purpose + } + //this is mostly required for the game studio gizmos if (Simulation.DisableSimulation) { - //Give warning if CharacterComponent doesnt have collider shapes - if(this is CharacterComponent && ColliderShapes.Count == 0 && ColliderShape == null) - { - logger.Warning($"Entity {this} has no physical shapes attatched to CharacterComponent."); - } - return; } //this is not optimal as UpdateWorldMatrix will end up being called twice this frame.. but we need to ensure that we have valid data. Entity.Transform.UpdateWorldMatrix(); - if (ColliderShapes.Count == 0 && ColliderShape == null) - { - logger.Error($"Entity {Entity.Name} has a PhysicsComponent without any collider shape."); - return; //no shape no purpose - } - else if (ColliderShape == null) + if (ColliderShape == null) { ComposeShape(); if (ColliderShape == null) From 730cde93812b0dcb5d468ec63c1e761106c01e48 Mon Sep 17 00:00:00 2001 From: dloe Date: Tue, 27 Aug 2024 13:20:07 -0700 Subject: [PATCH 5/6] Moved comment to summary of PhysicsComponent OnAttach Cosmetic change with comments and summary --- sources/engine/Stride.Physics/Elements/CharacterComponent.cs | 1 - sources/engine/Stride.Physics/Engine/PhysicsComponent.cs | 3 +++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/sources/engine/Stride.Physics/Elements/CharacterComponent.cs b/sources/engine/Stride.Physics/Elements/CharacterComponent.cs index e6988d5dec..5adf8b4f6d 100644 --- a/sources/engine/Stride.Physics/Elements/CharacterComponent.cs +++ b/sources/engine/Stride.Physics/Elements/CharacterComponent.cs @@ -279,7 +279,6 @@ public void SetVelocity(Vector3 velocity) [DataMemberIgnore] internal BulletSharp.KinematicCharacterController KinematicCharacter; - //called after collider shapes are attached to character comp? protected override void OnAttach() { NativeCollisionObject = new BulletSharp.PairCachingGhostObject diff --git a/sources/engine/Stride.Physics/Engine/PhysicsComponent.cs b/sources/engine/Stride.Physics/Engine/PhysicsComponent.cs index 549933ab60..74b5acbc39 100644 --- a/sources/engine/Stride.Physics/Engine/PhysicsComponent.cs +++ b/sources/engine/Stride.Physics/Engine/PhysicsComponent.cs @@ -701,6 +701,9 @@ internal void Detach() } } + /// + /// Called whenever an entity with this component is added to scene. + /// protected virtual void OnAttach() { //set pre-set post deserialization properties From 2fbd5bdbfea08ac716faa634925c42fb541d28b5 Mon Sep 17 00:00:00 2001 From: dloe Date: Tue, 27 Aug 2024 13:34:28 -0700 Subject: [PATCH 6/6] Reduced code reuse with dedicated function for logging Maybe it could go ever farther and be put in the Logger class? May update --- .../Elements/CharacterComponent.cs | 32 +++++++++---------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/sources/engine/Stride.Physics/Elements/CharacterComponent.cs b/sources/engine/Stride.Physics/Elements/CharacterComponent.cs index 5adf8b4f6d..7e0202cb83 100644 --- a/sources/engine/Stride.Physics/Elements/CharacterComponent.cs +++ b/sources/engine/Stride.Physics/Elements/CharacterComponent.cs @@ -27,9 +27,7 @@ public void Jump(Vector3 jumpDirection) { if (KinematicCharacter == null) { - StackFrame frame = new StackTrace(true).GetFrame(1); //for capturing tracing info - logger.Error($"Component:[{this}] attempted to call a Physics function that is available only when the Entity has been already added to the Scene. " + - $"This may be due to a {this} without any physical shapes.\nLocation: {frame.GetFileName()} at Line Number: {frame.GetFileLineNumber()} from Method: {frame.GetMethod().Name} "); + LogPhysicsFunctionError(); return; } BulletSharp.Math.Vector3 bV3 = jumpDirection; @@ -43,9 +41,7 @@ public void Jump() { if (KinematicCharacter == null) { - StackFrame frame = new StackTrace(true).GetFrame(1); //for capturing tracing info - logger.Error($"Component:[{this}] attempted to call a Physics function that is available only when the Entity has been already added to the Scene. " + - $"This may be due to a {this} without any physical shapes.\nLocation: {frame.GetFileName()} at Line Number: {frame.GetFileLineNumber()} from Method: {frame.GetMethod().Name} "); + LogPhysicsFunctionError(); return; } KinematicCharacter.Jump(); @@ -216,9 +212,7 @@ public void Teleport(Vector3 targetPosition) { if (KinematicCharacter == null) { - StackFrame frame = new StackTrace(true).GetFrame(1); //for capturing tracing info - logger.Error($"Component:[{this}] attempted to call a Physics function that is available only when the Entity has been already added to the Scene. " + - $"This may be due to a {this} without any physical shapes.\nLocation: {frame.GetFileName()} at Line Number: {frame.GetFileLineNumber()} from Method: {frame.GetMethod().Name} "); + LogPhysicsFunctionError(); return; } @@ -240,9 +234,7 @@ public void Move(Vector3 movement) { if (KinematicCharacter == null) { - StackFrame frame = new StackTrace(true).GetFrame(1); //for capturing tracing info - logger.Error($"Component:[{this}] attempted to call a Physics function that is available only when the Entity has been already added to the Scene. " + - $"This may be due to a {this} without any physical shapes.\nLocation: {frame.GetFileName()} at Line Number: {frame.GetFileLineNumber()} from Method: {frame.GetMethod().Name} "); + LogPhysicsFunctionError(); return; } @@ -259,10 +251,7 @@ public void SetVelocity(Vector3 velocity) { if (KinematicCharacter == null) { - //What was nice about the exception is users can see where this function was called to help trace - StackFrame frame = new StackTrace(true).GetFrame(1); //for capturing tracing info - logger.Error($"Component:[{this}] attempted to call a Physics function that is available only when the Entity has been already added to the Scene. " + - $"This may be due to a {this} without any physical shapes.\nLocation: {frame.GetFileName()} at Line Number: {frame.GetFileLineNumber()} from Method: {frame.GetMethod().Name} "); + LogPhysicsFunctionError(); return; } @@ -322,5 +311,16 @@ protected override void OnDetach() base.OnDetach(); } + + /// + /// Run specific error when physics functions are called on components that do not have proper setup. + /// Captures good tracing info for debugging purposes. + /// + private void LogPhysicsFunctionError() + { + StackFrame frame = new StackTrace(true).GetFrame(2); + logger.Error($"Component:[{this}] attempted to call a Physics function that is available only when the Entity has been already added to the Scene. " + + $"This may be due to a {this} without any physical shapes.\nLocation: {frame.GetFileName()} at Line Number: {frame.GetFileLineNumber()} from Method: {frame.GetMethod().Name} "); + } } }