Animation PR #1032

fdev31 · 2023-03-10T21:04:12Z

Borrowed most of the code from FTLabs which was the only one I found without graphical glitches.

Fixed some obvious merge problems in the HEAD of this fork
Adapted the code to support animations in a more generic way (desktop change is still a bit heuristic)

Some of the code couldn't be made clean, reusing the current APIs, with some hints I can fix it.

It supports:

open/close (by window type)
special transient hint support for popup windows
slide windows on desktop change
resize/move window
support "special" desktops to be ignored (for instance: Qtile's scratchpads)

This could eventually cover #635 and #217

Most of the code is from FTLabs' fork, I mostly changed some structures & the win.c file.

absolutelynothelix · 2023-03-10T21:40:42Z

is @FT-Labs ok with «borrowing» his work? i thought he’ll eventually consider his fork stable and make a pull request himself.

fdev31 · 2023-03-10T21:50:35Z

Hum,
I didn't consider that given what I saw in the code... with references to dwm and comments such as "animations will not work out of dwm". For instance for me this fork doesn't display transient windows...
Also I didn't consider it being "borrowing", since I don't claim or hide anything and it's opensource... but I made this PR maybe to revive a discussion on the best options.
I'm a bit shy with communication, but you are right, we should involve them at least for awareness.
I didn't see any contact on the github page, is there a way to "poke" someone in a comment ? Else maybe I can open a ticket to open this topic on the FTlabs fork side ?

absolutelynothelix · 2023-03-10T21:54:16Z

i’ve already poked him with a mention. let’s see what he’ll say and if he’s ok with this, i’ll start reviewing. for now you can (actually must, if you want this to be merged) fix all ci’s failures (until ci’s failing for unknown reasons).

fdev31 · 2023-03-10T21:57:45Z

Thank you!
I'll wait for some feedback eventually, if it's a dead end I'll not fix the CI... and as you'll see there is a little more to fix in my code before it can really be a candidate for merging ;) For instance I struggled defining the atoms in picom style and had to do direct xcb calls...

absolutelynothelix · 2023-03-10T21:59:24Z

you may want to draft this pull request if there’s still work to be done.

fdev31 · 2023-03-10T22:05:00Z

So it depends, technically it's ready for review... but I know the review will not pass at least for items I wrote, but for those remaining I need some assistance to make it clean, it works, but not everything with the general style.

I already cleaned my code, the remaining bits are mistakes or lack of competence/knowledge in this programming type.

I didn't even know draft pull requests were added actually, what is the advantage ? (I can update my commits as well in a normal PR isn't it ?)

absolutelynothelix · 2023-03-10T22:10:51Z

ok, let’s postpone any fixes while waiting for @FT-Labs reply to prevent wasting of at least your time.

draft pull requests basically indicate that there’s still work to be done and they can’t be merged. i usually make my pull requests draft when they’re technically ready for review or merging, but i still want/need to fix something. it’s optional, though.

FT-Labs · 2023-03-11T06:55:18Z

Hey guys just saw this.

I am open for anyone wants to help, but there is some work to do.

i) Shadows needs to be fixed
ii) You seem to be using old code, please update your fork. It uses xrandr now.
iii) dwm related places needs to be changed

I'm working on it too, just too busy these days. Once these problems are solved, it will be good to go.

fdev31 · 2023-03-11T08:35:54Z

Thank you for the quick feedback & amazing work!

I didn't notice any issue with shadows and probably don't have the skills to fix anything there unless I spend a lot of time
I used FT-Labs@1c7466c as a base which seems to be 3 weeks old (next branch), the "nextt" branch looked older. Did I miss something ?
I changed dwm related places - but had to drop the "minimize" animation since I couldn't find a generic way to do it

I had time & motivation for a week, I can't commit for the future but I'ld like to see better animations on picom and willing to help.

fdev31 · 2023-03-12T11:45:59Z

It also fails on the same test (299) on my computer, with or without my PR, on similar check but even earlier... (line 77).

codecov · 2023-03-12T11:52:14Z

Codecov Report

Merging #1032 (715eba7) into next (cee1287) will decrease coverage by 1.46%.
The diff coverage is 10.94%.

❗ Current head 715eba7 differs from pull request most recent head cee36cd. Consider uploading reports for the commit cee36cd to get more accurate results

@@            Coverage Diff             @@
##             next    #1032      +/-   ##
==========================================
- Coverage   37.82%   36.37%   -1.46%     
==========================================
  Files          48       48              
  Lines       10844    11368     +524     
==========================================
+ Hits         4102     4135      +33     
- Misses       6742     7233     +491

Impacted Files	Coverage Δ
src/atom.h	`100.00% <ø> (ø)`
src/common.h	`76.00% <ø> (ø)`
src/config.h	`23.52% <ø> (ø)`
src/options.c	`17.55% <0.00%> (-1.83%)`	⬇️
src/utils.h	`48.38% <ø> (ø)`
src/win.h	`78.12% <ø> (ø)`
src/x.c	`44.71% <ø> (ø)`
src/win.c	`55.74% <8.02%> (-12.51%)`	⬇️
src/config.c	`45.33% <9.09%> (-3.39%)`	⬇️
src/picom.c	`60.07% <13.10%> (-6.68%)`	⬇️
... and 1 more

fdev31 · 2023-03-12T11:53:35Z

I pushed a style fix and the test 299 is passing now (but still not locally).

absolutelynothelix · 2023-03-12T11:59:40Z

fyi, to fix the style quickly you’re supposed to use clang-format -i /path/to/file. i usually run it on all files that i change before comitting.

fdev31 · 2023-03-12T13:29:01Z

I had the same idea but unfortunately I have to undo many chunks which are not supposed to be in the PR... so in the end it turned to be more time consuming. Adding such rules late makes things a bit more tricky...

shasherazi · 2023-04-19T09:02:31Z

bump?

FT-Labs · 2023-04-22T14:56:45Z

It would be great if someone can check this with different window managers! Thanks

#1052

yshui · 2023-06-13T23:29:19Z

also cc @dccsillag and @Arian8j2

Arian8j2

really good animation system, good job, things i think is missing:

add commands and descriptions to man docs
add examples to picom.sample.conf
animation rule (like opacity rule), eg: can declare specific open animation for apps that match a rule

also it has some bugs with my setup with dwm and multiple monitors, windows in second monitor will come all the way from first monitor to second, tried animation-extra-desktops but it didn't help it

Arian8j2 · 2023-06-14T08:54:19Z

src/config.h

@@ -40,6 +40,24 @@ enum backend {
 	NUM_BKEND,
 };

+enum open_window_animation {


imo window_animation is more suited name

Arian8j2 · 2023-06-14T08:55:10Z

src/options.c

+    {"animation-for-transient-window", required_argument, 811, NULL        , "Set animation for transient (child) windows."},
+    // blacklist ??
+    {"animation-for-tag-change", required_argument    , 813, NULL          , "Set animation for switching desktops."},
+    {"animation-extra-desktops", required_argument    , 814, NULL          , "N desktops will not be considered as standard desktops (useful for some window managers)."},


no command for --animation-for-unmap-window?

Indeed, it's always the "mirror" animation of the "open".

Arian8j2 · 2023-06-14T09:02:14Z

src/win.c

+	if (animation == OPEN_WINDOW_ANIMATION_INVALID) {
+		if (transient_window) {
+			int16_t mx, my;
+			animation = OPEN_WINDOW_ANIMATION_SLIDE_DOWN; //ps->o.animation_for_transient_window;


This is on purpose, I couldn't decide, I left what I liked the most as the code but it's quite opinionated so I left the more boring but less debatable way in comments ;)

Arian8j2 · 2023-06-14T09:02:41Z

src/win.c

+			animation = OPEN_WINDOW_ANIMATION_SLIDE_DOWN; //ps->o.animation_for_transient_window;
+			if (get_mouse_position(ps, &mx, &my)) {
+				if (my >= (randr_mon_center_y*2) - w->pending_g.height) {
+					animation = OPEN_WINDOW_ANIMATION_SLIDE_UP;


Yes, same purpose... I really like those animations but maybe it breaks on some toolkits / WM so I left the boring way in comments.

Arian8j2 · 2023-06-14T09:06:43Z

src/config.h

+	/// Whether to clamp animations
+	bool animation_clamping;
+	/// Animation blacklist. A linked list of conditions.
+	c2_lptr_t *animation_blacklist;


animation_exclude is more suitable

Arian8j2 · 2023-06-14T09:09:03Z

src/options.c

+    {"animation-dampening"         , required_argument, 807, NULL          , "Animation dampening ratio (spring dampening as an example)."},
+    {"animation-window-mass"       , required_argument, 808, NULL          , "Animation window mass (lower mass makes animations bumpy)."},
+    {"animation-clamping"          , no_argument      , 809, NULL          , "Enable/disable animation clamping. Disabling increases performance"},
+    {"animation-for-open-window"   , required_argument, 810, NULL          , "Set animation for opening window (Check sample.conf for options)."},


where is sample.conf 😄

Ok, I switched to hyprland in the meantime and quite happy with it so I'm not sure I'll spend a lot of time on this PR, I can do some tiny things but maybe not more... note that most of the code isn't from me, I just simplified some of the code and added a couple of things after fixing it for my own use case.

Looks like FTLabs made another branch which could be worth checking: https://github.com/FT-Labs/picom/tree/generalanimation

FT-Labs · 2023-06-14T18:52:12Z

Hey guys,

I'll try to implement your ideas after all bug's are resolved

colonelpanic8 · 2023-07-29T20:17:33Z

Hey guys,

I'll try to implement your ideas after all bug's are resolved

It seems like the right approach should just be to get SOMETHING merged, and then iterate from there.

colonelpanic8 · 2023-08-06T01:36:43Z

Also, can anyone weigh in on what the status of this PR is vs #772?

Are they just completely unrelated attempts to add animations?

Leaving outstanding PRs like this forever that don't get merged is bad for the community, IMO.

dccsillag · 2023-08-06T09:15:55Z

This PR is further work on top of what was done on #772. I haven't been able to check all of the differences between them, though.

colonelpanic8 · 2023-08-06T16:06:47Z

@dccsillag weird are you sure? I don't see of the original commits from #772 in here.

dccsillag · 2023-08-06T17:01:30Z

I'm fairly sure that @FT-Labs's fork is built on top of mine (in fact, I think they were going to send a PR my way? I may be misremembering that though). It's quite likely that someone did an oopsie with git in the middle. @FT-Labs @fdev31 can you confirm?

fdev31 · 2023-08-06T19:42:48Z

It's quite likely that someone did an oopsie with git in the middle.

I can't confirm for sure, but I struggled to take over FT-lab's code and rebase it on Picom because it was mostly "hard to track" merges, so it's possible. And I probably didn't help as well, with my own "mega patch"... so it's quite likely the history is messed up.

In short, my PR doesn't keep FTlab's history and focused on making code compatible with Picom's upstream, dealing with the history of FTlab wasn't easy (without making a huge fake merge) so I just mentioned it in github to keep track of it. Check FTlab's fork to know more about the history, you can assume my branch was 100% in sync with FTlab's when this PR was made (in case you need to check commit dates).

colonelpanic8 · 2023-08-06T20:04:41Z

I can't confirm for sure, but I struggled to take over FT-lab's code and rebase it on Picom because it was mostly "hard to track" merges, so it's possible. And I probably didn't help as well, with my own "mega patch"... so it's quite likely the history is messed up.

But I had already noted that. I looked at @FT-Labs 's code and I think it also does not include @dccsillag 's original commits either.

git was designed to work in a distributed fashion. Kind of frustrating that people are just taking code without having the original commits/attribution, and it also can land you in legal trouble eventually.

colonelpanic8 · 2023-08-06T20:06:35Z

@FT-Labs is it worth it for me to attempt a merge of your current generalanimation branch?

fdev31 · 2023-08-06T20:13:34Z

Kind of frustrating that people are just taking code without having the original commits/attribution

I fully agree, but once the mess is done it's very hard to clean, especially when you are not the author... if I could have done a better job starting from this state please share some hints. It already took me quite some time with the indentation changes etc which were not kept in history so in the end I decided to trash the fork history.

Animated fork - based of FTLabs

c9b4182

fixing compilation issues

715eba7

fdev31 force-pushed the animation-pr branch from 37ee722 to 715eba7 Compare March 12, 2023 11:49

do not animate desktop windows

cee36cd

fdev31 force-pushed the animation-pr branch from 6db2c5a to cee36cd Compare April 13, 2023 21:54

Arian8j2 reviewed Jun 14, 2023

View reviewed changes

colonelpanic8 mentioned this pull request Aug 6, 2023

@FTLabs generalanimation branch merged with next with original git history and compiling (Animation PR) #1109

Closed

absolutelynothelix mentioned this pull request Sep 2, 2023

animations #1121

Closed

yshui force-pushed the next branch from c8c06cf to 5fa870a Compare February 18, 2024 00:58

yshui closed this Mar 19, 2024

yshui mentioned this pull request Mar 19, 2024

Animation support #1219

Closed

Animation PR #1032

Animation PR #1032

Conversation

fdev31 commented Mar 10, 2023

absolutelynothelix commented Mar 10, 2023

fdev31 commented Mar 10, 2023 • edited Loading

absolutelynothelix commented Mar 10, 2023 • edited Loading

fdev31 commented Mar 10, 2023

absolutelynothelix commented Mar 10, 2023

fdev31 commented Mar 10, 2023

absolutelynothelix commented Mar 10, 2023 • edited Loading

FT-Labs commented Mar 11, 2023

fdev31 commented Mar 11, 2023

fdev31 commented Mar 12, 2023

codecov bot commented Mar 12, 2023 • edited Loading

Codecov Report

fdev31 commented Mar 12, 2023

absolutelynothelix commented Mar 12, 2023 • edited Loading

fdev31 commented Mar 12, 2023

shasherazi commented Apr 19, 2023

FT-Labs commented Apr 22, 2023

yshui commented Jun 13, 2023 • edited Loading

Arian8j2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FT-Labs commented Jun 14, 2023

colonelpanic8 commented Jul 29, 2023

colonelpanic8 commented Aug 6, 2023

dccsillag commented Aug 6, 2023 • edited Loading

colonelpanic8 commented Aug 6, 2023

dccsillag commented Aug 6, 2023 • edited Loading

fdev31 commented Aug 6, 2023

colonelpanic8 commented Aug 6, 2023

colonelpanic8 commented Aug 6, 2023

fdev31 commented Aug 6, 2023

fdev31 commented Mar 10, 2023 •

edited

Loading

absolutelynothelix commented Mar 10, 2023 •

edited

Loading

absolutelynothelix commented Mar 10, 2023 •

edited

Loading

codecov bot commented Mar 12, 2023 •

edited

Loading

absolutelynothelix commented Mar 12, 2023 •

edited

Loading

yshui commented Jun 13, 2023 •

edited

Loading

dccsillag commented Aug 6, 2023 •

edited

Loading

dccsillag commented Aug 6, 2023 •

edited

Loading