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

macOS: fix opacity handling #3722

Merged
merged 1 commit into from
Jun 15, 2024
Merged

macOS: fix opacity handling #3722

merged 1 commit into from
Jun 15, 2024

Conversation

kchibisov
Copy link
Member

Not using NSColor::clearColor() results in Quartz thinking that the window is not transparent at all, which results in artifacts.

However, not setting the windowBackgroundColor in Window::set_transparent results in border not properly rendered.

Fixes: 94664ff (Don't set the background color)

--

This is kind of a revert, given that alacritty/alacritty#7965 not really doing anything and the window is not really tranparent as it should (it kind of is, but the value is different to what it should be and Quartz glitches behind the window with damage tracking).

Though, Quartz seems to generally glitch with e.g. shadows if you reload opacity with these changes as well. Maybe we should do something completely different, but I only care to fix the regression.

@kchibisov kchibisov requested a review from madsmtm as a code owner June 6, 2024 16:40
@kchibisov kchibisov mentioned this pull request Jun 6, 2024
@kchibisov kchibisov force-pushed the kchibisov/macos-fix-opacity branch from fbaad91 to cc59407 Compare June 6, 2024 17:04
Copy link
Member

@madsmtm madsmtm left a comment

Choose a reason for hiding this comment

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

I'm not against this per-say, but I think we should experiment and finish this in Alacritty first, and only once we know what the desired behaviour is, then we can consider doing the change in Winit.

@kchibisov
Copy link
Member Author

I mean, that breaks not only alacritty, but basically everything that wants to put something transparent on screen, like glutin example, etc, etc.

@kchibisov
Copy link
Member Author

I'd also note, that it won't align with how all platforms work in winit, so not setting proper transparency sounds like we're not trying to do reliable cross platform behavior.

Copy link
Member

@madsmtm madsmtm left a comment

Choose a reason for hiding this comment

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

Sorry it took me a while, I went and tested this with the following rendering methods:

  • Raw CALayer (softbuffer with a fix to make alpha channels work there).
  • OpenGL (glutin).
  • Metal (metal with layer.set_opaque(false)).
  • CGContext (cacao).
  • NSView's positioned with different background colors (cacao).

And indeed for all of them, the correct behaviour is to set the background color to clearColor, setOpaque didn't seem to even do anything (!).

Also noting for the future: -[NSWindow setAlphaValue:] is similar to what we're doing here, but distinct in that it sets the opacity of the entire window, including decorations / title bar, and this cannot be "overridden" by rendering something with full opacity.

Approved after comments are resolved.

src/platform_impl/macos/window_delegate.rs Outdated Show resolved Hide resolved
src/platform_impl/macos/window_delegate.rs Show resolved Hide resolved
unsafe { NSColor::windowBackgroundColor() }
};

self.window().setBackgroundColor(Some(color).as_deref());
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Slightly clearer.

Suggested change
self.window().setBackgroundColor(Some(color).as_deref());
self.window().setBackgroundColor(Some(&color));

@kchibisov
Copy link
Member Author

And indeed for all of them, the correct behaviour is to set the background color to clearColor, setOpaque didn't seem to even do anything (!).

setOpaque is a hint for compositor and nothing more, IIRC. Which means that handling it reduces the load on the compositor, but that's about it. It doesn't actually check whether something is opaque/not.

Also noting for the future: -[NSWindow setAlphaValue:] is similar to what we're doing here, but distinct in that it sets the opacity of the entire window, including decorations / title bar, and this cannot be "overridden" by rendering something with full opacity.

This is not desired in most cases, because it makes the text transparent as well. So should just leave it.

@kchibisov kchibisov force-pushed the kchibisov/macos-fix-opacity branch from cc59407 to 55815d7 Compare June 12, 2024 12:42
Comment on lines 824 to 829
self.window().setOpaque(!transparent)
self.window().setOpaque(!transparent);
Copy link
Member

Choose a reason for hiding this comment

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

setOpaque is a hint for compositor and nothing more, IIRC. Which means that handling it reduces the load on the compositor, but that's about it. It doesn't actually check whether something is opaque/not.

Maybe document that here?

@kchibisov kchibisov force-pushed the kchibisov/macos-fix-opacity branch from 55815d7 to 899e745 Compare June 14, 2024 16:40
Not using `NSColor::clearColor()` results in Quartz thinking that the
window is not transparent at all, which results in artifacts.

However, not setting the `windowBackgroundColor` in
`Window::set_transparent` results in border not properly rendered.

Fixes: 94664ff (Don't set the background color)
@kchibisov kchibisov force-pushed the kchibisov/macos-fix-opacity branch from 899e745 to aa0f4be Compare June 15, 2024 12:35
@kchibisov kchibisov merged commit eef6977 into master Jun 15, 2024
52 checks passed
@kchibisov kchibisov deleted the kchibisov/macos-fix-opacity branch June 15, 2024 12:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants