search

dccsillag / picom

A lightweight compositor for X11
Other
164 stars 20 forks source link

Fixes artifacts by correctly adding old window area to damage #22

Closed pijulius closed 2 years ago

pijulius commented 2 years ago

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!

dccsillag commented 2 years ago

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.

dccsillag commented 2 years ago

Hm, it probably indeed fixed https://github.com/dccsillag/picom/issues/21 (I can't reproduce it, unfortunately), however, https://github.com/dccsillag/picom/issues/18 remains an issue for me.

pijulius commented 2 years ago

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.

pijulius commented 2 years ago

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 :)

dccsillag commented 2 years ago

Wow, these are really cool changes. Will be merging them for sure.

Thanks!

pijulius commented 2 years ago

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.

dccsillag commented 2 years ago

Ok, will test later.

I think that the animation_running thing is what is actually causing big cpu usage; the reason I wasn't skipping windows in the animation code is that, in my experimentation, I noticed that adding such a condition was actually slower (in runtime) than doing the math itself -- which kind of makes sense, since branching is quite expensive.

If this observation does indeed hold for cpus other than mine, maybe we should look into updating animation_running without needing conditionals?

pijulius commented 2 years ago

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 :)

dccsillag commented 2 years ago

Ah, something worth noting -- w->animation_progress == 1 is only reliable for detecting the end of the animation if clamping is enabled. (I think I commented something about this either in a commit message or in an issue, I'll try to find it.)

dccsillag commented 2 years ago

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.

pijulius commented 2 years ago

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?

pijulius commented 2 years ago

Can you please check the latest patch as it seems to fix the artifacts now for both GLX and Xrender too.

dccsillag commented 2 years ago

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?

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.

Can you please check the latest patch as it seems to fix the artifacts now for both GLX and Xrender too.

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!

dccsillag commented 2 years ago

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 :)

pijulius commented 2 years ago

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 :)

dccsillag commented 2 years ago

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 :)

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!

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! :)

Ooooh, great! The reason they were separate was to be clear that animation code must come before anything that uses the window geometry. But if we get such a performance boost by merging it into the fading loop, I think we should do that instead, as long as we document (via comments) that that piece of code changes the window geometry and as such should be run before anything that uses the window geometry.

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).

Really appreciate your work on this and will definitely show my gratitude soon too :)

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. Thank you very very much!

pijulius commented 2 years ago

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 https://github.com/dccsillag/picom/pull/22/commits/9a32dd8e53afb4353d73f19524c8f9116a2b1614

you can see I was playing around with: double delta_secs = 0.01; //(double)(now - ps->animation_time) / 1000;

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?

dccsillag commented 2 years ago

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 [...]

Nice!

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?

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. animation-delta-time) that, if unspecified evaluates delta_time as is currently done, but if specified evaluates delta_time as its value?

EDIT: or maybe something like delta_time = min(config_delta_time, now - prev_time)

pijulius commented 2 years ago

Last commit hopefully fixes case #13 and so I should be done for now :)

Re the delta_secs I will keep playing around with it but as that may be just a personal taste will leave it as it is for now as I see the reason why you did it the way you did.

pijulius commented 2 years ago

Re last commit on black windows, unfortunately it brakes animations for old_win_image as there is no old_win_image anymore (try maximizing windows).

Will see if I can come up with something tomorrow but open to ideas why this clone_image brakes things as it seems without freeing the clone image the new image becomes garbage for some reason.

dccsillag commented 2 years ago

I might ponder on this later today.

pijulius commented 2 years ago

Daniel, could you please give me a paypal email as I promised something here https://github.com/yshui/picom/issues/688 and would like to keep my promise :) You can find my contacts here http://pijulius.com/about

dccsillag commented 2 years ago

Oh, don't worry about that. Your work here is already much more than enough, and I thank you very much. You can keep your money :)

pijulius commented 2 years ago

Thanks Daniel! appreciate the gesture and won't pressure something that you don't want :)

xlucn commented 2 years ago

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 picom with the following command, with an empty picom.conf

./build/src/picom --config ./picom.conf --animations --animation-for-workspace-switch-out none

but result in an error:

./build/src/picom: unrecognized option '--animation-for-workspace-switch-out'

If I remove the last cli argument and add to picom.conf the next line:

animation-for-workspace-switch-out = "zoom";

still couldn't see the animation changing to zoom.

When I run picom --config /dev/null --animations and switch workspaces, what I see is windows all sliding to and from left. I doubt that's not the feature of this PR?

Edit:

I am on 982bb43e5d4116f1a37a0bde01c9bda0b88705b9 and built with

meson --buildtype=release . build
ninja -C build
dccsillag commented 2 years ago

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

dccsillag commented 2 years ago

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...).

pijulius commented 2 years ago

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: https://github.com/pijulius/picom/commit/b92d4105ea8cc31a22fec64c5241b867b9f92632

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.

Vermoot commented 2 years ago

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 slide-down animation, which looks very nice, but I just realised when my mouse is on the bottom of the screen and the menu appears above the mouse, it doesn't make as much sense (see video below).

Could there be something like slide-out, that would do the same animation but away from the mouse, rather than always the same direction?

I guess maybe this would need to have a vertical and a horizontal version, since slide-out could mean both.

Thanks!

https://user-images.githubusercontent.com/23317434/141291042-caf76b62-df85-4326-85dd-d178a00b64e0.mp4

dccsillag commented 2 years ago

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.

[...]

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.

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 :smile:).

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: pijulius@b92d410

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.

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.

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.)

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 slide-down animation, which looks very nice, but I just realised when my mouse is on the bottom of the screen and the menu appears above the mouse, it doesn't make as much sense (see video below).

Could there be something like slide-out, that would do the same animation but away from the mouse, rather than always the same direction?

I guess maybe this would need to have a vertical and a horizontal version, since slide-out could mean both.

Thanks!

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 slide-away-from-mouse.

@pijulius, if you want, I can do this after the merge.

LDAP commented 2 years ago

Hi :) Animations work great with this PR, but for some reason some windows are (randomly) redrawn:

  1. Open terminal on workspace 1
  2. Open chromium on workspace 2
  3. Switch workspaces 1 -> 2 -> 3

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)

godalming123 commented 2 years ago

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 slide-down animation, which looks very nice, but I just realised when my mouse is on the bottom of the screen and the menu appears above the mouse, it doesn't make as much sense (see video below).

Could there be something like slide-out, that would do the same animation but away from the mouse, rather than always the same direction?

I guess maybe this would need to have a vertical and a horizontal version, since slide-out could mean both.

Thanks! 2021-11-11.12-30-32.mp4

what code do you use to get that really neat effect because this fork broke my fading which would be my one complaint and I am looking to add that back in but if I can get a more complex animation like that it would be even more amazing! @Vermoot

UPDATE: I managed to get most of the animations I want with the code:

#animations
animations: true;

animation-for-open-window = "zoom"; #open window
animation-for-unmap-window = "zoom"; #minimize window
animation-for-workspace-switch-in = "none"; #the windows in the workspace that is coming in
animation-for-workspace-switch-out = "zoom"; #the windows in the workspace that are coming out
animation-for-transient-window = "slide-down"; #popup windows

You can see all the options with picom --help and then filter for animation. This accomplishes:

jzbor commented 2 years ago

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 animation-for-workspace-switch-* set to none and set to slide-up. So I don't think this is about the workspace animations.

Thanks for creating the animations they are awesome. I hope my feedback is helpful. Regards, Julian

EDIT: I should add that I use the experimental glx backend

dccsillag commented 2 years ago

Hey @pijulius,

I'd like to merge this so I can start to wrap things up for a PR into upstream picom. I'm thinking of, for now, not merging workspace animtions -- they aren't reliable enough for me to feel comfortable proposing them for upstream. My hope is that this will allow both of us to get some load off of our shoulders.

Looking at the commit log, it seems that merging up to 58c9c119feb8d757cc2feebaef3db4caaf877dab 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?

dccsillag commented 2 years ago

I've just merged up to 58c9c119feb8d757cc2feebaef3db4caaf877dab. I'm still testing a bit (especially the logic in paint_preprocess in picom.c), but I think everything's fine.

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.