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

Rounded corners and Kawase blur for glx, all in the same fork. #361

Closed
wants to merge 49 commits into from

Conversation

ibhagwan
Copy link

Hi @yshui,

I’d like to present my fork (updated up to your yesterday’s commits):

  • Added the popular kawase blur method for glx for both old and new backend from @tryone144, both his old Compton code and the new code for the new backend in his feature/dual_kawase branch.

  • Added the rounded corners code from @sdhand and also rewrote it to the new XRender backend.

  • Added new code for rounding corners for the glx backend using a GLSL fragment shader.

  • In the next few days I intend to port the rounded corners glx shader to the new experimental backend.

I’d love for this to be included in the main branch.

@sdhand
Copy link
Contributor

sdhand commented Mar 31, 2020

This is fantastic! Thank you so much for picking up the work on this.

A few problems:

  • Currently running with the xrender backend and rounded corners segfaults immediately

  • The shader rounded corners look noticeably different and less smooth than those done with the 9-box method (see the attached images for comparison, 9-box is first).

9-box:
shader:

@yshui
Copy link
Owner

yshui commented Mar 31, 2020

Thanks for the great work!

This PR is pretty big so this could take a while to review. I also want to invite @sdhand and @tryone144 to review this as well, as they have been working on the same thing.

@yshui
Copy link
Owner

yshui commented Mar 31, 2020

It would be great if you can organize your commits into more self-contained units. That would make reviewing these changes much easier for us.

@ibhagwan
Copy link
Author

It would be great if you can organize your commits into more self-contained units. That would make reviewing these changes much easier for us.

How do I go about doing that? I’m somewhat of a github collaboration noob (used to doing things alone), how do I rearrange the commits into units?

Btw the way it is now is somewhat organized, the first commit is porting of @sdhand code followed by a few commits porting @tryone144 code (separate commits for the old and new backend) followed by a commit that ported @sdhand rounded corner code to experimental backends.

All commits after Mar 25th are the new GLSL rounded corners code, it’s a bit of a mess with my trial and error and experimenting but if we can connect somehow I can walk through the final version which ended up being pretty simple and not too invasive to the code (basically a duplication of the blue method code structure).

Let me know and we can take it from there.

I intend to add the rounded corners GLX to experimental backend so if you have any guidelines how to do so in a way that would make things easier let me know.

@tryone144
Copy link
Collaborator

tryone144 commented Mar 31, 2020

How do I go about doing that? I’m somewhat of a github collaboration noob (used to doing things alone), how do I rearrange the commits into units?

Usually you split the commits for each feature into its own branch which can than be merged into master (or next in this case) with a PR.

If I might chime in on the blur part:

I would prefer merging in all above changes with separate PRs to reduce complexity. The feature/dual_kawase branch of my fork should be pretty easy to merge. The issues reported over the last three month seem to be related to the "experimental" part of the new backend, so my changes should not introduce any regressions.

For cleaner merging I would open a new PR of said branch and provide a separate PR for the re-implementation of blur-background-fixed in the new backend which is currently mangled in there.

Regarding the "old" backends: My first implementation was rather hacky and had some bugs. I later reimplemented the algorithm ontop of this fork (improved_fbo) with some optimizations suggested by @alex47, the original author of the Kwin/plasma implementation. I've been running this version for a long time, so these changes should be pretty stable. If requested, I can rebase these changes on feature/dual_kawase in a separate PR.

Other than that, I totally agree we should try to minimize the number of compton forks. I never anticipated and/or planned my fork to become so popular. It was more of a development branch to be eventually be merged back into the "original" compton and not to be used as a replacement. Now that @yshui took over maintenance with picom, I'll be glad to merge the changes back into next.

@yshui
Copy link
Owner

yshui commented Apr 1, 2020

How do I go about doing that? I’m somewhat of a github collaboration noob (used to doing things alone), how do I rearrange the commits into units?

You can use things like git rebase -i, git commit --amend, there are articles online explaining them better than I could.

Preferably each commit should contain a self-contained change that does one thing, and each commit should build and pass tests.

And I agree with @tryone144 that the rounded corners and the blur should be split into separate PRs.

@liquidev
Copy link

I just tried round-borders-rule, and it seems that for some reason my borders are extra thick.
image
without round-borders:
image
Here's my config:

# vim: ft=cfg
experimental-backends = true
backend = "glx"
shadow = true
shadow-offset-x = -12
shadow-offset-y = -6
shadow-radius = 12
shadow-opacity = 0.30
inactive-opacity = 0.95
shadow-exclude = [
  "class_g *?= \"slop\"",
  "class_g *?= \"i3-frame\""
];
corner-radius = 4
focus-exclude = [
  "class_g *?= \"rofi\""
]
round-borders = 1
round-borders-rule = [
  "3:window_type = 'unknown'",
  "3:window_type = 'toolbar'",
  "3:window_type = 'utility'",
  "3:window_type = 'splash'",
  "3:window_type = 'dialog'",
  "3:window_type = 'normal'"
];

@ivanmilov
Copy link

I just tried round-borders-rule, and it seems that for some reason my borders are extra thick.

what border width in your i3 config?

@liquidev
Copy link

@ivanmilov 1

@ibhagwan
Copy link
Author

@liquid600pgm - your borders are thicker because as you replied to @ivanmilov your i3 border thickness is set to 1 but yet specified thickness of 3 in the rules, try changing the rule to "1:window_type = 'unknown'", etc.

@liquidev
Copy link

@ibhagwan oh! I didn't actually know that, just copied my configs over from another person above haha

Thanks for the reply.

@liquidev
Copy link

liquidev commented Sep 23, 2020

Would there be a possibility of making borders pixel-perfect? Right now if you zoom into the pixels, you can see a tiny bit of antialiasing:

border size 1px
image

border size 2px
image

It's much less noticable on larger border sizes, but I can definitely see some blurriness on my thin 1px borders.

@ibhagwan
Copy link
Author

ibhagwan commented Oct 1, 2020

Would there be a possibility of making borders pixel-perfect? Right now if you zoom into the pixels, you can see a tiny bit of antialiasing:

border size 1px
image

border size 2px
image

It's much less noticable on larger border sizes, but I can definitely see some blurriness on my thin 1px borders.

Unfortunately that would require a different shader code.

@AskMeAgain
Copy link

What is the timeline for getting this pr merged?

@ibhagwan
Copy link
Author

ibhagwan commented Oct 3, 2020

What is the timeline for getting this pr merged?

I’m not sure, this PR won’t be merged as is, it is being worked on separately (kawase blur / rounded corners), from what I could tell @tryone144 added support for the dual kawase blur to the experimental backends and @yshui merged the rounded corners separately into this branch.

@nova-nowiz
Copy link

So, if I understood correctly, your work is being redistibuted to other branches and contributors?
In this case, what are the branches to watch for?

@ibhagwan
Copy link
Author

ibhagwan commented Oct 6, 2020

So, if I understood correctly, your work is being redistibuted to other branches and contributors?
In this case, what are the branches to watch for?

I don’t know, I am only the author of this PR, I am not involved in work on other branches.

@nova-nowiz
Copy link

ok! thanks for the quick answer!

evanjs added a commit to evanjs/nixos_cfg that referenced this pull request Oct 13, 2020
Use yshui/picom#361 until
yshui/picom#361 is merged

kitty: use spacecamp-kitty theme
xmonad: enable gaps
evanjs added a commit to evanjs/nixos_cfg that referenced this pull request Oct 14, 2020
Use yshui/picom#361 until
yshui/picom#480 is merged

kitty: use spacecamp-kitty theme
xmonad: enable gaps
@yshui
Copy link
Owner

yshui commented Dec 17, 2020

I finally got to installing i3-gaps and tested the issue, seems like i3 is indeed reporting border_width = 0 in it's xcb_get_geometry_reply.

That's likely because the "border" isn't a window border, it's the i3 frame window, whose size can be retrieved from the _NET_FRAME_EXTENT property.

round-borders-rule doesn't have to exist. and round-borders-exclude probably shouldn't either. Isn't window with borders just going to look wrong if the corners are clipped?

cc @ibhagwan

@ibhagwan
Copy link
Author

I finally got to installing i3-gaps and tested the issue, seems like i3 is indeed reporting border_width = 0 in it's xcb_get_geometry_reply.

That's likely because the "border" isn't a window border, it's the i3 frame window, whose size can be retrieved from the _NET_FRAME_EXTENT property.

round-borders-rule doesn't have to exist. and round-borders-exclude probably shouldn't either. Isn't window with borders just going to look wrong if the corners are clipped?

cc @ibhagwan

round-borders-rule provides a solution for non standard borders (such as the i3 frame), round-borders-exclude can indeed look not so great due to the clipping you mentioned but people have such diverse and creative setups that I personally prefer to provide the flexibility than not, especially with such “cheap” code.

@yshui
Copy link
Owner

yshui commented Dec 18, 2020

@ibhagwan

round-borders-rule provides a solution for non standard borders (such as the i3 frame)

You don't have to have this to support i3 frames properly.

@ibhagwan
Copy link
Author

ibhagwan commented Feb 6, 2021

Hi @yshui, @tryone144,

FYI, I've rebased my fork onto your current work, dropped the code you already implemented and added what wasn't yet implemented (rounded corners+borders on experimental and dual_kawase on legacy).

To not create a total mess with forks I've created another branch for the rebase named next-rebase, needed to solve quite a few conflicts but it seems to be working fine now (thank the lord and tpope for vim-fugitive).

I'm aware it might not end up in the main branch but whatever your plans are regarding the rest of the code (borders, etc) I thought it might be helpful to have something working that resembles more to your current work after the merge of the rounded corners to legacy and the addition of dual kawase to the experimental glx, might also solve some of the issues that were opened on my fork.

@s0nny7
Copy link

s0nny7 commented Feb 16, 2021

The missing shadow of the round corners is really annoying, I tried to modify shadow generation method and shadow paint region, but neither seemed to work under new backend.

I took a poke around the code but did not find out how the shadow cropping stuff is done for new backend, is there anyone who is kindly enough to help pointing me out the right place to start with?

@brongulus
Copy link

Is there any particular reason why this branch hasn't been merged yet? It's almost been an year, I'm just curious :)

@kinglouie
Copy link

Is there any particular reason why this branch hasn't been merged yet? It's almost been an year, I'm just curious :)

this will most likely never get merged because parts of this branch is already merged (the kawase blur) and the rounded corner code is getting cleaned up in a separate pull request.

@anamyk
Copy link

anamyk commented May 16, 2021

Is there any particular reason why this branch hasn't been merged yet? It's almost been an year, I'm just curious :)

this will most likely never get merged because parts of this branch is already merged (the kawase blur) and the rounded corner code is getting cleaned up in a separate pull request.

Can you link to the new pull request, so we can keep track of it?

@BachoSeven
Copy link

Is there any particular reason why this branch hasn't been merged yet? It's almost been an year, I'm just curious :)

this will most likely never get merged because parts of this branch is already merged (the kawase blur) and the rounded corner code is getting cleaned up in a separate pull request.

Can you link to the new pull request, so we can keep track of it?

Here ya go #480

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.