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

feat: No Physics Shapes On The CharacterComponent Causes Unhandled Exception #2410

Merged

Conversation

dloe
Copy link
Contributor

@dloe dloe commented Aug 13, 2024

PR Details

Having no Physics shapes on character component causes unhandled exceptions when user tries to use physics methods on corresponding character component. (Note: Originally showed as a bug)
Originally threw: Unhandled Exception: System.InvalidOperationException: Attempted to call a Physics function that is available only when the Entity has been already added to the Scene.

Now gives warning from logger when a character component has no shapes and shows error via logger when physics method is ran on character component (Much earlier than original exception). Also gives good logging info to user as to where the physics method was called for tracing purposes.

Related Issue

Issue: #760

Types of changes

  • Docs change / refactoring / dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My change requires a change to the documentation.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I have built and run the editor to try this change out.

…issing 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.
+ fixed typo in comment
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
@Eideren
Copy link
Collaborator

Eideren commented Aug 17, 2024

Thanks a bunch, I think your stacktrace and return is the right idea but you might want to wrap that inside a method instead given the amount of code duplication. And log GetFrame(2) instead once that is done of course.

- Removed check that occurs after DisableSimulation check and now has it ahead of simulation check
Cosmetic change with comments and summary
Maybe it could go ever farther and be put in the Logger class? May update
@dloe
Copy link
Contributor Author

dloe commented Aug 27, 2024

Thanks a bunch, I think your stacktrace and return is the right idea but you might want to wrap that inside a method instead given the amount of code duplication. And log GetFrame(2) instead once that is done of course.

Good idea, I moved it to a dedicated function inside of CharacterComponent, but could it be moved even farther out to maybe the Logger.cs file?

@Eideren
Copy link
Collaborator

Eideren commented Aug 28, 2024

could it be moved even farther out to maybe the Logger.cs file?

I'm not sure we should make it part of the public api, in this exceptional case it does make sense, but we rarely need this log as it is fairly brittle; the second caller up the stack may not be as important to log as the third, or the fourth, and creating a stacktrace is a fairly slow operation.

Thanks a bunch for taking care of this one :)

@Eideren Eideren merged commit c7519cd into stride3d:master Aug 28, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants