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

fix: Found and fixed bug pertaining to ReAttach method for PR #2422 #2439

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dloe
Copy link
Contributor

@dloe dloe commented Sep 9, 2024

PR Details

Test cases inside Stride.Physics.Tests.SendCollisionEndedWhenEntityIsRemovedTest suite started failing after introduction of PR #2422. Further investigation showed that while entities are being setup inside Attach, the ReAttach method could potentially run and cause duplicate entities to be added to simulation, resulting in possible null ref exceptions and in this case, test failures.

Added flag attachInProgress that only allows the ReAttach method to run when Entity has fully competed all Attach behaviors. Still maintains same behavior that was tested and verified in Stride.Physics.Tests.ColliderShapesTest.VerifyColliderShapeSetup.

Related Issue

Original Issue: #1504
Merged PR: #2422

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.

@Eideren
Copy link
Collaborator

Eideren commented Sep 12, 2024

Thanks, although this one kind of looks like a band-aid solution, would there not be a way to prevent re-entry in the first place - something like setting up whatever triggers the re-attach at the very last step of attach itself. If you need to make major changes to re-organize how the logic for those functions is structured for that, do go ahead, although I understand if you want to cut this one short

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants