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

Implement window shadows #990

Merged
merged 1 commit into from
Jan 17, 2025
Merged

Implement window shadows #990

merged 1 commit into from
Jan 17, 2025

Conversation

YaLTeR
Copy link
Owner

@YaLTeR YaLTeR commented Jan 16, 2025

Closes #927. Shadows are disabled by default and can be enabled on the layout {} or in a window rule.

@timgott want to take a glance just in case? I more or less followed your advice.

Everyone else: please give it a try. I'm looking for feedback:

  • any graphical glitches?
  • any performance issues?
  • do the options and option names make sense? Anything that could be named better?
  • do the defaults make sense?

I checked what options and defaults other implementations have (wayfire-shadows, swayfx, hyprland, fht) and arrived at the system in this PR. width, spread, offset and color mean the same thing as in CSS box-shadow. The default width of 30 is slightly lower than in wayfire-shadows and swayfx (44 and 40 respectively, converted to niri units), but we have a slight default spread of 5.

I set draw-behind-window to true by default because otherwise CSD rounded corners will look broken. However, with draw-behind-window false, niri will fully respect its own rounded corners (geometry-corner-radius) with no artifacts.

There's no "only floating" setting because you can window-rule it.

Contrary to other implementations, I decided to slightly reduce the shadow opacity for inactive windows. This is how CSD shadows usually work and I think it makes sense.

Niri shadows render below the border and focus ring, in contrast to CSD shadows that render above the border and focus ring.

@timgott
Copy link

timgott commented Jan 16, 2025

Based on a quick look over it, it seems good to me.

For different corner radii: You will have to compute the integral bounds for a rectangle with different corner radii and use that instead. It should be in roundedBoxShadowX which computes an integral with the rule $\int_a^b f(x) dx = F(b) - F(a)$, and $a$ and $b$ need to be the x-coordinate of the left and right side of the rounded window (in the right coordinate system).

The options make sense. I have been thinking whether it makes sense to have a color option instead of only opacity for simplicity, as colored shadows usually look bad anyways (I used it for "glow" for a while and eventually made a different shader for that). As for default values, the ones in wayfire-shadows are pretty arbitrary, just my own taste in what looked good and I changed it over time.

@YaLTeR
Copy link
Owner Author

YaLTeR commented Jan 17, 2025

Hm, two questions.

  1. Should draw-behind-window default to false?

This is the default in at least wayfire-shadows and hyprland (and also as far as I can tell in the CSS box-shadow model that our properties are based on). This is also likely what you want as a user when turning on shadows, both in terms of how they look, and in terms of performance (skipping running a heavy shader on a large area inside of the window).

The only reason I set the default to true is that without it, if you just enable shadows on the default config, you'll see broken corners on rounded CSD windows:

image

Compared to enabling shadows with the current default:

image

Admittedly, the default config has focus ring enabled, which hides this problem on the focused window, but still shows it on unfocused windows:

image

Though, I'm thinking if I make draw-behind-window false by default and document it right there in the default config (like how it already is in this PR), then it should be fine.

  1. Should we maybe rename width?

While it does sort of act like a width and is set in logical pixels,

(excerpts from box-shadow MDN and CSS spec)

The larger this value, the bigger the blur

color transition approximately the twice length of the blur radius

, it is not really a width, but a blur radius. Now I don't want to call it radius because it kinda reads like the shadow corner radius, which it isn't. Even "blur-radius" sounds kind of too close?

@calops suggested "softness", maybe it's better than "width"?

@timgott
Copy link

timgott commented Jan 17, 2025

Should draw-behind-window default to false?

wayfire-shadows does this because it draws shadows only on server-side decorated windows by default. Most CSDs already include shadows, and CSD windows can have arbitrary shape so it is hard to make good shadows for them. I am not sure exactly how the focus ring works in niri but the shadows should be drawn in the same way as other decorations.

Another thing to consider is that you will see the shadow through transparent windows (e.g. terminals) if you don't cut it out.

As for performance, I don't even think it is a very heavy shader. It does not perform any texture lookups which is usually the slowest operation. It has some sqrts which takes a few more cycles than other operations and the rounded corner shader does a few extra iterations, but I doubt it is much slower than directly drawing a pre-rendered texture (because of the memory lookup).

Also I agree width or radius are not great names (a gaussian has infinite extent). Something like softness or size is better.

@YaLTeR
Copy link
Owner Author

YaLTeR commented Jan 17, 2025

wayfire-shadows does this because it draws shadows only on server-side decorated windows by default. Most CSDs already include shadows, and CSD windows can have arbitrary shape so it is hard to make good shadows for them. I am not sure exactly how the focus ring works in niri but the shadows should be drawn in the same way as other decorations.

I saw that, yeah. I don't want to do that in niri because by default we use CSD, and it would be even worse if enabling shadows on top of the default config didn't do anything whatsoever. Besides, we have the clip-to-geometry window rule that makes decorations and shadows look as expected even for windows that use CSD.

Another thing to consider is that you will see the shadow through transparent windows (e.g. terminals) if you don't cut it out.

Yes, that's the main reason why I think draw-behind-window false is the expected behavior. Though, you could argue that shadows do in fact show up through semitransparent objects IRL...

As for performance, I don't even think it is a very heavy shader. It does not perform any texture lookups which is usually the slowest operation. It has some sqrts which takes a few more cycles than other operations and the rounded corner shader does a few extra iterations, but I doubt it is much slower than directly drawing a pre-rendered texture (because of the memory lookup).

I see. Unfortunately, we don't have GPU profiling hooked up in Smithay yet to say for sure, and I definitely don't have a wide range of devices at hand to test. I just know there has been a real performance issue in GNOME Shell about it drawing the workspace shadow in the overview behind the (fully opaque, almost fullscreen) workspace, instead of only around it. So I'm trying to lean on the safer side.

Also I agree width or radius are not great names (a gaussian has infinite extent). Something like softness or size is better.

Thanks for your input. I like "softness" the most so far.

@YaLTeR
Copy link
Owner Author

YaLTeR commented Jan 17, 2025

Alright, I changed width to softness, and after much decision paralysis, flipped the default for draw-behind-window to false. I figured that if the Wayland protocol for reporting CSD corner radius happens, many windows should start working properly even with CSD rounded corners.

Also clarified the docs. Let's merge this

@YaLTeR YaLTeR enabled auto-merge (rebase) January 17, 2025 20:04
@YaLTeR YaLTeR merged commit bd559a2 into main Jan 17, 2025
20 checks passed
@YaLTeR YaLTeR deleted the s branch January 17, 2025 20: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.

Server-side window shadows
2 participants