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: bug pertaining to ReAttach method for PR #2422 #2439

Merged
merged 1 commit into from
Sep 22, 2024

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

@Eideren Eideren merged commit 763a529 into stride3d:master Sep 22, 2024
2 checks passed
@Eideren Eideren changed the title fix: Found and fixed bug pertaining to ReAttach method for PR #2422 fix: bug pertaining to ReAttach method for PR #2422 Sep 22, 2024
@Eideren
Copy link
Collaborator

Eideren commented Sep 22, 2024

I'll merge this one in to make sure release doesn't throw in the context mentioned in the PR, and opened a new issue for what I mentioned above.

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