Fixes artifacts by correctly adding old window area to damage #22
Conversation
|
Whoa, can't believe I let that error pass! Thanks for your contribution! I'll test it a bit before merging, but I bet that solves it all. |
Seems like sometimes windows aren't animated in case like when you open them in the same spot. You can test this doing the following: 1. Right click in Firefox (anywhere on a website) to open the context menu (will show with animation) 2. Right click again but without moving the mouse (will hide the context menu) 3. Right click again but without moving the mouse (will show the context menu without animation) With adding those flags when the windows gets unmaped (but I imagine not destroyed) it will animate them correctly even if you open them in the same place.
|
Yep, can confirm that unfortunately only fixes it in GLX with experimental-backend. Not sure why with xrender it doesn't work but once I get some more free time I will check that out too. |
…ons for menus and transient windows. This should add slide animations and also possibility to define different animations depending if the window is a menu or a window that is transient of another one (usually dialog windows that you cannot focus out of like "are you sure to delete xxx file" or about widows and so on). New animations are: slide-down slide-up slide-left slide-right and the option to define different animations for menus and transient windows: animation-for-menu-window = "slide-down"; animation-for-transient-window = "slide-up";
|
Hi Daniel, Added some more changes but github automatically adds these to this pull request so please ignore them if you don't find them of any use but if you do it would be my pleasure to have them in your fork :) |
|
Wow, these are really cool changes. Will be merging them for sure. Thanks! |
…eted for a window. The new animations kept on running even if there wasn't anything to do and so processor usage went up badly when animations turned on. This change checks if animation process completed for a window and in that case it simply jumps to the next window without doing all the calculations and other things. For me this changed picom usage to the same as without animation so 50%+ cpu went down to ~9% just like without animations turned on from config. Also because of this change animation_running=false is being used correctly as before this it was always true because in the for cycle it always set it to true even if it wasn't anything to do.
|
Thanks Daniel, just added another commit that I think is a pretty big change (in case of daily usage). For now everything works perfectly here but if that change is affecting something that I didn't realized please let me know. |
|
Ok, will test later. I think that the If this observation does indeed hold for cpus other than mine, maybe we should look into updating animation_running without needing conditionals? |
|
I don't think there has to be anything done on the animation_running part as of now as it's exactly as it's with fading and that works just fine too, it's just that without the above modification animation_running was never false as the loop kept setting it to true all the time even if you just idled on your pc :) |
This should fix animations from starting at the wrong location as w->animation_velocity_... was never reset and because of the spring animation when a window was zoomed (for e.g. maximized and then restored) the next animation like shading became misplaced meaning it started at a wrong location because of the old velocity value. Also we reset animation_time to 0 in case there was no animation running at all (just like it's for fading) and so we prevent for e.g. in case clamping turned off to start zooming in windows from thousands of times of your screen and freezing/braking everything.
|
Ah, something worth noting -- |
|
Well, LGTM. The only thing I should mention is that, for me, there is a notable regression in performance from the current code to your code. But it's not clear to me what causes this. I'll play around with the code a bit, see if I can optimize it. |
|
Hi Daniel, sorry to hear, can you please let me know the exact config you are using so I can check too? also what does it mean performance? meaning it's slower when animates? |
This seems to fix the artifacts in xrender too now, we simply add w->reg_ignore_valid = false; to all changes when doing animation and that fixes it. Not sure how this does affect performance but so far works just fine for me.
|
Can you please check the latest patch as it seems to fix the artifacts now for both GLX and Xrender too. |
Yeah, the animation is a bit jerky (i.e. skips a bit). My config is available at https://raw.githubusercontent.com/dccsillag/dotfiles/6dd0e74e0005b5a7d7a69cb7d0ae36b706045c47/.config/picom.conf. I tried to run a profiler yesterday, and in your branch it pointed that paint_preprocess occupied only about 0.20% of the runtime; in my branch, it doesn't even appear, but I think that's because it somehow got inlined. I'll try to pass you some proper profiling information to confirm with numbers that performance got worse. On the other hand, from yesterday to today I rebooted my computer, which emptied my RAM quite a bit -- and now I don't really see the performance regression anymore (on the contrary). So maybe it's just some exceptional situation we should ignore.
Whoa! I can confirm that solved it. And it makes sense too (and also ticks off one of the only remaining doubts on the code -- whether we should update reg_ignore). Great going! |
|
FYI: I'm now starting a period of testing for your code -- that is, I'm going to be using it for a while, and trying to cause issues over time. Hopefully everything will be fine, and then this should be good to merge :) |
|
Very happy to hear that the last change fixes the remaining bug. the only thing remaining for me is the #13 case as it happens all the time with Firefox, really hope can find that bug too and then it's ready on my end :) Also FYI: I already combined on my end the animation's win_stack_foreach_managed_safe loop and the fading's win_stack_foreach_managed_safe loop (as really don't see any reason to loop over the same thing twice) and the processor usage goes down from 4% to only 1% or even lower when idling and have one more fix that should finally fix the bug without clapping enabled, so will merge those shortly and then please go ahead and stress test it yourself too as all these things are pretty new to me and still don't get everything in the code but very satisfied with what you did here! :) Really appreciate your work on this and will definitely show my gratitude soon too :) |
Great! While I can't really reproduce that, feel free to ping me if you want help understanding something, or even to code something (though I might not be able to test it). Hopefully it all goes [relatively] smoothly!
Ooooh, great! Also, since the two loops are now together, it may be worth it to join the animation timer and the fading timer into a single one. If you want, you can let me do that (unless you want to go hunting for all the places the timers are interacted with :P).
I'm the one who should thank you -- you did quite a bit of amazing work here, which [I think, to be confirmed once this blacking-out thing gets solved] brings us quite close to be ready to PR. |
Added fading to newly created widows so animation doesn't start with a small rectangle fully opaque. Fixed moving transparent windows by not adding old_image overlay as that way it will start flickering and if you move it just so slightly it will keep flickering. Fixed opacity value for new_image as it has to be always 1 because w->opacity is already applied later on.
Merged animation loop into the already existing loop used by fading. Fixed animation Segment fault in case clamping wasn't enabled and the window size got very small (for e.g. in case of shading a window) as width and height became negative number. Fixed animation_progress to not go back down in case of clamping disabled so that we can rely on it being 1 and this fixes old_image opacity to be hidden and then reappear when maximizing windows and animation goes above real window size. Fixed animation completed check as 1 = 1L isn't always the same, so we instead check now >= 0.999999999 Wrapped fading code around w->opacity != w->opacity_target for some more optimization. Overall these changes for me bring down picom from 3-4% cpu to 0.7-1% when idling and hopefully don't brake anything.
… and left there by mistake.
|
Ok, except the black windows in case of Firefox I think I'm done now :) I was thinking about merging the animation_time with fade_time as you suggested but for now I'm happy with the results and anyway have one thing that would like to ask you about. In the commit you can see I was playing around with: I realized that (double)(now - ps->animation_time) is timeout dependent, meaning if x time passed after the first animation (for e.g. some other process took place and picom was hold up) the animation jumped to the end instead of continuing where it left of. If I set this to 0.01 the animations run all the time even if they get hold up by some other processes but they do continue where they left of instead of jumping to the end of animation. Not sure if this was done on purpose nor if you did like it better this way but my vote would go for freezing the animation and continue where it left of instead of jumping to the end, what do you think? |
Nice!
It was done on purpose. Suppose you some frames take more time to render than others (which does happen) -- if you fix the delta_time then animations will appear uneven, which can be very unpleasant. If you really want it, how about having some config option (e.g. EDIT: or maybe something like |
This will add animations for windows when closing (destroying) just like it works for unmapping (e.g. minimizing).
Changed window is_transient to animation_transient because we can't rely on transient status of the window as sometimes it happens that the flag is removed before unmap animation starts and so window ends up with the wrong animation on close. So now we simply use animation_transient and this is only set when window opens and close animation will check this to be sure it uses the same animation for closing.
This was missed as in some cases WSTATE_DESTROYING was quicker set then WSTATE_UNMAPPING ended and so window ended up being animated but not faded out. This should now take care of it as both statuses being handled correctly now.
This adds animation-delta = 10; configuration option in milliseconds just like for fading which you can use to define the time between animation steps. By default it's set to 10 being the same it was originally set by Daniel. Also added a new animation-force-steps = true; option which you can use to force the animation steps to not be skipped even if on higher cpu usage so instead of skipping an animation step and going right to the end this will force the script to go trough each and every step even if it's a bit slower instead of just jumping to the end. By default this is disabled so animations behave just as Daniel implemented them and the same way as fading but I prefer it turned on.
|
I just tried the workspace animation, but there are some issues to specify workspace switching animation (I can't find the correct option or something is wrong) I started ./build/src/picom --config ./picom.conf --animations --animation-for-workspace-switch-out nonebut result in an error: If I remove the last cli argument and add to still couldn't see the animation changing to zoom. When I run Edit: I am on 982bb43 and built with |
|
Hey @pijulius, I've been doing some thinking and decided that it'd probably be a good idea to merge this, despite that issue with the size transition opacity -- it's become quite apparent that it's going to be hard to solve, and it's relatively well-contained in the code. I've noticed that you've been doing some more work. Could you ping me once/if you think this is ready for a merge? Thanks |
|
BTW, here are my two cents about workspace animation stuff:
I know I've dumped these here, but I'm more than willing to fix these myself after a merge (or maybe via a PR into your fork? I feel like that'd start to get a bit messy...). |
|
Hi Daniel, Unfortunately lately had a lot of work so wasn't able to work on this but will try trough the weekend and fix that damn fatal on inavlid workspace-switch. The rest should be pretty clear I think, you can now use wintypes to specify animations too, also there is one for open and one for close too (the close it's called animation-for-unmap-window) it's called unmap because it only worked when minimizing/hiding windows but not when closing but fortunately was able to fix that bug too and so now it works for both hiding but closing windows too. Re this animation-for-workspace-switch-{in,out} = "none", it will never work unless you have a window manager that is changing the _NET_CURRENT_DESKTOP on the root window before doing the windows hiding/showing logic and if you do not have that it will simply use unmap_animation as that's what picom is doing when hiding windows. This is noted here: unfortunately I wasn't able to find a better solution to detect workspace switch than this. As I'm a fluxbox user I have patched fluxbox too that does this and with that it works perfectly with except a small bug when window is larger than the screen height but will fix that too. Also workspace switch will only work for up and down workspaces but not left and right. Overall I'm done with picom changes so far so will try to fix those small bugs but as I have not everything I was dreaming of I'm more for optimizing now, for some reasons after a day picom starts to become sluggish, so will try to see what is that I can do there but no plans from here on as far as I can think of. Will definitely ping you once I have fixed these fatal errors. |
|
I don't know if this is the exact right place to put this, but here goes: I'm using @pijulius' fork, and the animations are awesome. I just have a feature request: For dropdowns (or popups, I don't remember, whichever covers right click contextual menus) I use the Could there be something like I guess maybe this would need to have a vertical and a horizontal version, since Thanks! 2021-11-11.12-30-32.mp4 |
Ok; take your time, no rush. If you want, I could take a look at this "after a day, picom starts to become sluggish" thing (I've been through there 😄).
I use XMonad, and I believe I'd already confiigured it to do that. I'll try to figure out what's happening with this a bit later.
I think that's alright. I'd just like to make sure that we can disable this workspace detection, exactly for cases where it doesn't work. (I can do that myself after the merge, so don't worry about it.)
That seems pretty sensible. I'm just not quite sure we, as the compositor, have access to the mouse position. But, assuming we have it, then we should name these options in a more descriptive manner, such as @pijulius, if you want, I can do this after the merge. |
|
Hi :) Animations work great with this PR, but for some reason some windows are (randomly) redrawn:
Now for some reason the terminal is redrawn. I expected a blank screen. With yshui/picom this does not happen. (I am using KDE with bspwm as WM) |
UPDATE: I managed to get most of the animations I want with the code: You can see all the options with
|
|
Hi, I am experiencing problems with damage tracking on this fork/PR. It seems to be related to what @LDAP reported. It tracks open windows quite fine, but it doesn't seem to clear the root window from previously open windows that are now closed. So if I open Spotify then switch to another workspace Spotify stays drawn in the background. This happens both with Thanks for creating the animations they are awesome. I hope my feedback is helpful. EDIT: I should add that I use the experimental glx backend |
|
Hey @pijulius, I'd like to merge this so I can start to wrap things up for a PR into upstream picom. Looking at the commit log, it seems that merging up to 58c9c11 should be fine. Am I right, or is there some fix introduced later that I should merge as well? Or is there some issue introduced in the later commits that I should look out for / that you have already fixed later? |
|
I've just merged up to 58c9c11. I'm still testing a bit (especially the logic in Since the more relevant bits are now merged, I'll be closing this. The workspace switch animations and window close animations will be implemented at a later phase. |
This should fix cases #21 #18 as the artifacts happened because the damage for old window was added after w->g already updated above and so in reality old area was never added to damage.
Have rewritten so that it uses already existing variables and this way the first add_damage_from_win does uses the original window area and so it won't leave any artifacts behind.
Have tested it on glx with --experimental-backend and all artifacts are gone.
ps: thanks for this animation fork and the work that you guys have been doing on this!