Skip to content

Conversation

LukeUnityDev
Copy link

Description

Fix memory leak - make sure to unsubscribe started, and canceled actions for m_PointerDownAction on destroy when m_UseIsolatedInputActions is set to true.

Changes made

Added private method OnDestroy.

Testing

Fore testing is required to add OnScreenStick component and check UseIsolatedInputActions, and setup for-example Hold interaction there.

Risk

There is no risk.

@unity-cla-assistant
Copy link

unity-cla-assistant commented Jul 4, 2024

CLA assistant check
All committers have signed the CLA.

Fix memory leak - make sure to unsubscribe started, canceled actions for m_PointerDownAction on destroy when m_UseIsolatedInputActions is set to true.
@ekcoh ekcoh self-requested a review August 7, 2024 10:22
Copy link
Collaborator

@ekcoh ekcoh left a comment

Choose a reason for hiding this comment

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

Thanks for providing this fix @LukeUnityDev. It would be great if you could add a line to the CHANGELOG.md file describing under title fixed for unreleased version that a memory leak has been adressed in OnScreenStick which would leak memory when component was destroyed. When this have been updated we can land the contribution.

Added under Unreleased tag comment for memory leak OnScreenStick
@LukeUnityDev
Copy link
Author

@ekcoh thank you for review, I have added

@LukeUnityDev LukeUnityDev requested a review from ekcoh August 21, 2024 11:55
@ekcoh ekcoh changed the title Update OnScreenStick.cs - fix memory leak FIX: Update OnScreenStick.cs Sep 11, 2024
@ekcoh
Copy link
Collaborator

ekcoh commented Sep 13, 2024

Seems problematic to land from fork. I rerouted your fix via #2006 and marked your contrib in the CHANGELOG so will close this PR and let the other land when reviewed and approved by the team. Thanks @LukeUnityDev

@ekcoh ekcoh closed this Sep 13, 2024
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.

3 participants