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

animation: fix crashes and cleanup of active vars #42

Merged
merged 7 commits into from
Jan 27, 2025

Conversation

PaideiaDilemma
Copy link
Contributor

@PaideiaDilemma PaideiaDilemma commented Jan 26, 2025

Thanks a lot to @gulafaran and @vaxerski for debugging and finding the crash.
The first commit here adds a test for it.

The rest of the patch is an alternative to resetting the pAnimationManager pointer with a signal, when it gets deleted.

Instead i thought it would be better to just use signals for connect/disconnect in the first place. That removes the need for storing the pointer to pAnimationManager. (We still need it for getCurveValue, but in a next abi breaking change, we could remove it. For now it is guarded by the reference to the events)

And then there seems to be a problem with a growing active vars vector.
I think it happens in Hyprland when we are not ticking and thus not cleaning up active vars. So this adds a lazyDisconnect method that just increments an integer. If we want to add a new var and this integer is > 100, we manually trigger a cleanup.

I also have no problem in reverting the removal of vars to how it worked before moving the anim stuf to hyprutils. (always using forceDisconnect and making sure we are not calling it during tick.) That would be the alternative to the lazyDisconnect thing.

Should not break ABI. Breaks ABI

@vaxerski
Copy link
Member

this does break ABI in like 20 places, but I believe the "> 100" solution kinda sucks. I'd just always schedule a cleanup after a warp

@PaideiaDilemma
Copy link
Contributor Author

Patch for hyprland: patch.txt

@vaxerski
Copy link
Member

can we not break API at least?

@gulafaran
Copy link
Contributor

tested here, seems to work as intended. as a bonus seems to have fixed some micro stuttering ive had on certain animations after a while, probably because the leak has been building up for a while.

@vaxerski vaxerski added the ABI Breaks the ABI label Jan 26, 2025
@vaxerski
Copy link
Member

I'll just fix this up myself, give me a momento

@vaxerski vaxerski force-pushed the fix-animation-crashes branch from 11e5fdc to 21dac77 Compare January 26, 2025 18:34
@vaxerski
Copy link
Member

@gulafaran check now, it doesn't break API (so hyprland doesnt need any patches)

@PaideiaDilemma
Copy link
Contributor Author

Thanks for cleaning it up. Looks good.

Just keep in mind that if we don't set warp(true, false) during tick in hyprland, it will clean up the variable during the tick potentially leading to weirdness (should be ok but I would add it for the release.)

@vaxerski vaxerski merged commit de58286 into hyprwm:main Jan 27, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ABI Breaks the ABI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants