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

Experimental backends: fade/hide blur-texture for transparent windows #364

Merged
merged 5 commits into from
Apr 8, 2020

Conversation

tryone144
Copy link
Collaborator

The new experimental backends did not honor the --blur-background-fixed option. This implements handling of said option in the new backends.

If set to false, the (premultiplied) opacity of the blur-texture is dependent on the window opacity preventing awkward-looking blurred rectangles behind transparent windows especially with low --inactive-opacity values.

Example: https://imgur.com/a/sCDVqAI
In-depth discussion in tryone144/compton#2

Stuff for further discussion:
The current (longtime) default of false did irritate some users who used a strong blur with active-opacity and opacity-rule: tryone144/compton#25, tryone144/compton#33

@codecov
Copy link

codecov bot commented Apr 1, 2020

Codecov Report

Merging #364 into next will increase coverage by 1.57%.
The diff coverage is 70.27%.

Impacted file tree graph

@@            Coverage Diff             @@
##             next     #364      +/-   ##
==========================================
+ Coverage   32.35%   33.93%   +1.57%     
==========================================
  Files          46       45       -1     
  Lines        8526     8552      +26     
==========================================
+ Hits         2759     2902     +143     
+ Misses       5767     5650     -117     
Impacted Files Coverage Δ
src/picom.h 66.66% <ø> (ø)
src/win.h 0.00% <ø> (ø)
src/x.c 46.07% <0.00%> (+2.84%) ⬆️
src/event.c 48.46% <14.28%> (+6.20%) ⬆️
src/picom.c 67.33% <50.00%> (+1.35%) ⬆️
src/win.c 59.36% <79.01%> (+3.57%) ⬆️
src/backend/backend.c 70.37% <91.66%> (+12.85%) ⬆️
src/compiler.h
src/render.c 5.18% <0.00%> (+0.01%) ⬆️
src/log.c 62.50% <0.00%> (+0.59%) ⬆️
... and 13 more

@yshui
Copy link
Owner

yshui commented Apr 1, 2020

@tryone144 the only concern i have is: does this transitions smoothly when window is fading?

@yshui
Copy link
Owner

yshui commented Apr 1, 2020

In fact, the current experimental backend behavior is not "blur-background-fixed: true", as during fading, the blur opacity would transition smoothly. I think blur should always transition smoothly during fading, regardless what the "blur-background-fixed" setting is. This kind of makes this option awkwardly named.

@yshui
Copy link
Owner

yshui commented Apr 1, 2020

Also I want to explain the rational behind the current behavior a bit:

I consider the opacity as part of the "intrinsic" property of the window. And when a window is displayed in its "natural" state, its background should be fully blurred.

Consider this: a window could itself have a transparent background (e.g. transparent terminals), or it could have an opacity value set. Visually, these two cases are barely distinguishable, however, they will get very different blur if blur-background-fixed is false. Personally I think that's inconsistent.

But I do agree this should be configurable (probably with a better named option).

@tryone144
Copy link
Collaborator Author

@tryone144 the only concern i have is: does this transitions smoothly when window is fading?

Yes, it is as smooth as window opacity itself is.

In fact, the current experimental backend behavior is not "blur-background-fixed: true", as during fading, the blur opacity would transition smoothly. I think blur should always transition smoothly during fading, regardless what the "blur-background-fixed" setting is. This kind of makes this option awkwardly named.

Totally agree on that. Fading should always be smooth. Another solution would be to explicitly check if the window is fading from/to fully transparent which could be always on like the current behavior for mapping/unmapping . Naming was just taken from the legacy backend, but can be changed accordingly.

Consider this: a window could itself have a transparent background (e.g. transparent terminals), or it could have an opacity value set. Visually, these two cases are barely distinguishable, however, they will get very different blur if blur-background-fixed is false. And personally I think that's inconsistent.

The last part has bugged me already while testing. I have tried making the blur fully opaque if the window is in its mapped && focused state. However, I was unable to find a reliable way of tracking state with the current win struct to enable smooth transition between the focused and unfocused state. I am open to any suggestions on how to add additional state variables.

But I do agree this should be configurable.

What about blur-background-(no-)fade = true/false to enable/disable globally, or inactive-dim-blur = true/false to change blur-opacity for inactive windows relative to their active opacity (i.e. similar to the current behavior of blur-background-fixed = false).

@tryone144 tryone144 changed the title Experimental backends: honor --blur-background-fixed Experimental backends: fade/hide blur-texture for transparent windows Apr 2, 2020
@tryone144
Copy link
Collaborator Author

Based on your thoughts I have reverted the initial proposal of honoring blur-background-fixed in the same way like the legacy backend did.

Instead, the blur-texture for fully transparent windows (i.e. w->opacity < 0.004) is now always fully transparent as well. For windows that are fading from/to fully transparent the blur-texture is fading as well like previously only for windows in state MAPPING, UNMAPPING or DESTROYING.
Blur-texture opacity always fades from transparent (window invisible) to fully-opaque (window at w->opacity_target). This is realized by a new field w->opacity_target_old, that is updated on changes of w->opacity_target.

While testing I found the blur-texture to be alpha-clipping for UNMAPPING/DESTROYING windows if inactive-opacity is used. In this case win_calc_opacity_target() returned the value of inactive-opacity which could be as low as 0, resulting in values larger than 1 for blur_opacity. Changing to the value of w->opacity_target_old fixes this issue as well.

On further examination I am not sure if a feature like the legacy blur-background-fixed is even needed as the blur-texture is always fully opaque per default and windows transitioning between fully transparent and visible are fading smoothly now. This should be the expected behavior as far as I am concerned.

@yshui
Copy link
Owner

yshui commented Apr 4, 2020

While testing I found the blur-texture to be alpha-clipping for UNMAPPING/DESTROYING windows if inactive-opacity is used. In this case win_calc_opacity_target() returned the value of inactive-opacity which could be as low as 0, resulting in values larger than 1 for blur_opacity. Changing to the value of w->opacity_target_old fixes this issue as well.

Thanks for that. I believe this should fix #314 / #318

@yshui
Copy link
Owner

yshui commented Apr 4, 2020

Let me make sure I understand your intention correctly:

You want to make ~0% opacity windows to be treated like an unmapped window blur-wise. i.e. blur won't be shown if a window has ~0% opacity, and when transitioning to/from 0% opacity, blur will be faded as well.

I like this idea.

Also, do you think we should add an option for enabling/disabling this? I think we don't need to, unless someone complains.

Copy link
Owner

@yshui yshui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just some small things.

src/backend/backend.c Outdated Show resolved Hide resolved
src/backend/backend.c Show resolved Hide resolved
@yshui
Copy link
Owner

yshui commented Apr 4, 2020

While testing I found the blur-texture to be alpha-clipping

BTW, can you list the steps to reproduce this problem? I intend to write a test case for it.

Edit: never mind, I figured it out

yshui added a commit that referenced this pull request Apr 5, 2020
This should be fixed by merging #364.

Thanks @tryone144 for pointing out the root cause.

Signed-off-by: Yuxuan Shui <[email protected]>
@tryone144
Copy link
Collaborator Author

You want to make ~0% opacity windows to be treated like an unmapped window blur-wise. i.e. blur won't be shown if a window has ~0% opacity, and when transitioning to/from 0% opacity, blur will be faded as well.

Exactly. This should feel more intuitive to users as well.

Also, do you think we should add an option for enabling/disabling this? I think we don't need to, unless someone complains.

I don't think we need an option just for the sake of having one. A quick workaround would be to set the inactive-opacity to 0.004 (i.e. as close to invisible as it gets with 8 bits).

Regarding #314 / #318:
My change only addresses the incorrect explosion of blur_opacity in the unmapping state. Your new testcase correctly checks for that. This however doesn't fix the problem of the dividend (w->opacity) becoming larger than divisor (w->opacity_target) while mapping the window.

@tryone144
Copy link
Collaborator Author

More regarding the new testcase:

You can still trigger the assert() when changing the opacity of a window in the MAPPING state as can happen when changing focus to inactive or if set directly (e.g. via picom-trans) while still fading (given the new target opacity is low enough and fading takes long enough).

I haven't managed to trigger it while in FADING or UNMAPPING/DESTROYING though.

I will run some more tests with

	// Gradually increase the blur intensity during
	// fading in.
	if (w->opacity < w->opacity_target) {
		blur_opacity = w->opacity / w->opacity_target;
	} else if (w->opacity_target < (1.0 / MAX_ALPHA)) {
		// `opacity_target` changed during fade in.
		// Fade out if suitable or leave at 1.
		blur_opacity = w->opacity / w->opacity_target_old;
	}

for the MAPPING state, but so far it looks to handle all my mad window mapping/focusing/unmapping well....

@yshui
Copy link
Owner

yshui commented Apr 6, 2020

You can still trigger the assert() when changing the opacity of a window in the MAPPING state as can happen when changing focus to inactive or if set directly (e.g. via picom-trans) while still fading (given the new target opacity is low enough and fading takes long enough).

I think we could maybe set the window state to FADING when opacity target changed in MAPPING state?

@tryone144
Copy link
Collaborator Author

I think we could maybe set the window state to FADING when opacity target changed in MAPPING state?

Changed that with some additional logic to only change to FADING if new target is already reached. Also reduced the tracking of old opacity_target values to fix another problem with repeatedly setting the opacity (either with picom-trans or stupid VLC on its fullscreen-controls) while still fading around.

I will play around with it a bit and write a second testcase for repeatedly updating the opacity.

@yshui
Copy link
Owner

yshui commented Apr 6, 2020

👍

Let me know when this is ready to be merged.

These two flags are intended for subtly different things. I can probably
justify the distinction, but it's definitely difficult to explain. And
there is no obvious benefits to keep them separate.

Signed-off-by: Yuxuan Shui <[email protected]>
src/win.c Outdated Show resolved Hide resolved
src/win.c Outdated Show resolved Hide resolved
@tryone144
Copy link
Collaborator Author

tryone144 commented Apr 7, 2020

I've expanded the logic around tracking changes of w->opacity_target_old to make sure w->opacity is between w->opacity_target and w->opacity_target_old at all times:

  • In state MAPPED, save old opacity target (=w->opacity) and fade to new target.
  • In state MAPPING, accept opacity_target updates if current opacity is below the new target. No update is needed in this case. If the new target opacity is already reached, the window transitions to FADING, clamping opacity_target_old to be higher than current opacity.
  • In state FADING, clamp opacity_target_old to be higher than current opacity if changing from fading in to fading out. Otherwise no update needed.

The updated testcases should check for correct handling of all three cases and as well as the destruction/unmap of a window that is currently fading out.

src/win.c Outdated Show resolved Hide resolved
* Add new field `opacity_target_old` to `struct managed_win` for
tracking relevant `opacity_target` changes.
* Smoothly fade blur-texture opacity on window opacity changes (based on
window opacity), when the window was or will be fully transparent
(`w->opacity ~< 0.004`).
* Fixed alpha-clipping of the blur-texture when using `inactive-opacity` or
repeatedly setting window opacity with large fade intervals (should fix yshui#314).
remove unrequired parameter `ignore_state` of
`win_calc_opacity_target()` that was always `false` now.
Handle more (hopefully all) edge-cases when updating `opacity_target_old`:
* `MAPPING` transitions to `FADING` if new target is reached. Otherwise
no update.
* `FADING` clamps to current `opacity` if old target is too small.
* `UNMAPPING`/`DESTROYING` clamps to current `opacity` if old target is
too small.
@yshui yshui force-pushed the fix/blur-background-fixed branch from d390631 to c1d985e Compare April 8, 2020 22:22
@yshui
Copy link
Owner

yshui commented Apr 8, 2020

@tryone144 I made a small change to make sure win_skip_fading is called when !ps->redirected, just in case.

Otherwise everything looks good. This is a really well thought out change, good job.

@yshui yshui merged commit 3ad4ce9 into yshui:next Apr 8, 2020
@tryone144 tryone144 deleted the fix/blur-background-fixed branch August 27, 2020 18:13
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.

2 participants