Implement background image over acrylic or solid color #1107

Summon528 · 2019-06-02T19:29:37Z

Summary of the Pull Request

Implement acrylic over background image. Also make backgroundImageOpacity works with respect to the profile's background color

References

PR Checklist

Closes Enable a background image on top of an acrylic brush #1075 Closes Allow brightness/darkness adjustments on background images #1762
CLA signed. If not, go over here and sign the CLA
Tests added/passed
Requires documentation to be updated
I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

Detailed Description of the Pull Request / Additional comments

Introduce a new root Element that holds the original container and a background image. The root element's background sits behind _bgImageLayer and is responsible for acrylic and solid background.

zadjii-msft

This might also conflict with some work being done in #1092, as a heads up

src/cascadia/TerminalControl/TermControl.h

zadjii-msft · 2019-06-03T14:16:34Z

src/cascadia/TerminalControl/TermControl.cpp

@@ -312,9 +326,10 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
            }
            else if (!_settings.BackgroundImage().empty())


I'm not sure this will update the background image+acrylic scenario properly - looks like it'll only update the acrylic in that scenario

Yes, it will only update the acrylic in this scenario, however, it is intended to do that. This function is for background color only and whenever acrylic is enabled we don't actually draw the solid background layer, we apply the color as acrylic's tint whether or not background image is set

So the logic is just acrylic or not. It becomes a bit confusing because of this workaround.

// Set the default background as transparent to prevent the // DX layer from overwriting the background image _settings.DefaultBackground(ARGB(0, R, G, B));

Oh I'm more thinking: what happens when you change the background image url for a profile that has a background image+acrylic? Does the image change appropriately?

Yes, it changes appropriately, although the code for it is around L251 not here.

Hrm. Does the alpha for the DX renderer ever need to be non-zero?

Just to clarify my previous response:

In all cases, the background is drawn in Xaml space by the brush.

In every case except a solid-color background (with no other modifications), the DX rendering drawing anything over it is erroneous.

Even in the solid-color background case, the brush drawing the background (as opposed to the DX renderer) is necessary because the DX renderer doesn't draw to the entire window area.

So I'm not sure there's a good reason to have the DX renderer aware of any sort of background information, since it can't "help".

In this specific case (Windows Terminal)¹, I'm entirely inclined to agree. The default background doesn't need an alpha. @miniksa nominally owns the DX renderer, so he likely has a stronger opinion here.

[1] dxrenderer can be used by conhost, too, and conhost has a slightly different set of rendering requirements

src/cascadia/TerminalControl/TermControl.cpp

DHowett-MSFT · 2019-06-03T19:02:05Z

This is interesting, and I never thought about how a user might want a background image with acrylic on top of it. I expected this:

but I never expected this:

I'm wondering if it's more reasonable to support the first one (_root => acrylic brush, _backgroundLayer => image brush) because the second one can easily be done with some image manipulation (and it's a lot more performant because the application doesn't need to constantly blur the same static image)?

Summon528 · 2019-06-04T00:57:03Z

The fact that we can set a gif as our background makes some sense for the second case but I agree that we should have a more robust spec. Also to support the first case you mention we need to have a setting for image alignment too.

Edit: Actually, blurring a gif is also a relatively easy task. So I am probably inclined to agree with the image on top of acrylic. Exposing the whole background stack to setting seem way overkill IMO.

mdtauk · 2019-06-04T01:02:07Z

When the idea of a Background Image was suggested, My first thought went to the Mail App, and how the sidebar will blur the background image you pick.

Summon528 · 2019-06-04T02:40:41Z

That's because they display blurred background and unblurred background in different part simultaneously though. We only display one version of the image at a time, so you can head toward Photoshop and blur the image in 5 seconds if you really want blurry background

DHowett-MSFT · 2019-06-12T01:14:01Z

Edit: Actually, blurring a gif is also a relatively easy task. So I am probably inclined to agree with the image on top of acrylic. Exposing the whole background stack to setting seem way overkill IMO.

Sounds great to me. I'd love to accept a pull request that put the background on top of the acrylic!

Co-Authored-By: Mike Griese <[email protected]>

Summon528 · 2019-06-12T06:10:57Z

Sounds great to me. I'd love to accept a pull request that put the background on top of the acrylic!

Roger that, I've updated this PR to enable background on top of the acrylic 😄

DHowett-MSFT · 2019-07-01T20:51:44Z

Wow, I never realized that this got updated. So sorry. Force pushes don't send e-mails to the team!

DHowett-MSFT · 2019-07-01T20:52:14Z

/azp run

azure-pipelines · 2019-07-01T20:52:22Z

Azure Pipelines successfully started running 1 pipeline(s).

DHowett-MSFT · 2019-07-01T22:17:30Z

This now puts the background on top of the acrylic, yes? Should we update the title?

Summon528 · 2019-07-02T07:37:33Z

This now puts the background on top of the acrylic, yes? Should we update the title?

Yes we should 😄

Summon528 · 2019-07-02T16:05:12Z

This solution, unfortunately, causes the image to shift if padding is set, I propose #1778 to fix that

miniksa

I'm fine with this.

DHowett-MSFT · 2019-07-10T19:53:34Z

This has two approvals and 7 successful checks, so I'm going to merge it. @Summon528 thanks for bearing with us and working on this feature!

ghost · 2019-08-03T01:48:44Z

🎉Windows Terminal Preview v0.3.2142.0 has been released which incorporates this pull request.:tada:

Handy links:

zadjii-msft reviewed Jun 3, 2019

View reviewed changes

CodyTseng and others added 7 commits June 12, 2019 13:05

Add acrylic background image

027c8bc

Background -> bg

ab91a3d

Update comment

5d6860e

Use Controls::Image

eda98df

Update src/cascadia/TerminalControl/TermControl.h

d36d0d0

Co-Authored-By: Mike Griese <[email protected]>

img on top of acrylic

23e6b4c

Simplify _BackgroundColorChanged()

b09b765

Summon528 force-pushed the acrylic-bg-img branch from 27d2c94 to b09b765 Compare June 12, 2019 06:07

Summon528 mentioned this pull request Jul 1, 2019

Allow brightness/darkness adjustments on background images #1762

Closed

DHowett-MSFT added the Needs-Attention The core contributors need to come back around and look at this ASAP. label Jul 1, 2019

DHowett-MSFT requested a review from zadjii-msft July 1, 2019 22:17

Summon528 changed the title ~~Implement acrylic over background image~~ Implement background image over acrylic or solid color Jul 2, 2019

Summon528 mentioned this pull request Jul 2, 2019

Make padding applied as swap chain panel's margin #1778

Merged

5 tasks

zadjii-msft approved these changes Jul 2, 2019

View reviewed changes

zadjii-msft mentioned this pull request Jul 2, 2019

Bug Report - Screen opacity does not change when AcrylicOpacity is changed #1773

Closed

miniksa approved these changes Jul 3, 2019

View reviewed changes

DHowett-MSFT approved these changes Jul 10, 2019

View reviewed changes

DHowett-MSFT merged commit 3ce53ad into microsoft:master Jul 10, 2019

mcpiroman pushed a commit to mcpiroman/terminal that referenced this pull request Jul 23, 2019

Implement background image over acrylic or solid color (microsoft#1107)

42a5259

ghost mentioned this pull request Aug 3, 2019

Enable a background image on top of an acrylic brush #1075

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement background image over acrylic or solid color #1107

Implement background image over acrylic or solid color #1107

Summon528 commented Jun 2, 2019 •

edited

Loading

zadjii-msft left a comment

zadjii-msft Jun 3, 2019

Summon528 Jun 3, 2019

Summon528 Jun 3, 2019

zadjii-msft Jun 3, 2019

Summon528 Jun 3, 2019

d-bingham Jun 3, 2019

d-bingham Jun 3, 2019

DHowett-MSFT Jun 3, 2019 •

edited

Loading

DHowett-MSFT commented Jun 3, 2019

Summon528 commented Jun 4, 2019 •

edited

Loading

mdtauk commented Jun 4, 2019

Summon528 commented Jun 4, 2019

DHowett-MSFT commented Jun 12, 2019

Summon528 commented Jun 12, 2019

DHowett-MSFT commented Jul 1, 2019

DHowett-MSFT commented Jul 1, 2019

azure-pipelines bot commented Jul 1, 2019

DHowett-MSFT commented Jul 1, 2019

Summon528 commented Jul 2, 2019

Summon528 commented Jul 2, 2019

miniksa left a comment

DHowett-MSFT commented Jul 10, 2019

ghost commented Aug 3, 2019

		@@ -312,9 +326,10 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
		}
		else if (!_settings.BackgroundImage().empty())

Implement background image over acrylic or solid color #1107

Implement background image over acrylic or solid color #1107

Conversation

Summon528 commented Jun 2, 2019 • edited Loading

Summary of the Pull Request

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

zadjii-msft left a comment

Choose a reason for hiding this comment

zadjii-msft Jun 3, 2019

Choose a reason for hiding this comment

Summon528 Jun 3, 2019

Choose a reason for hiding this comment

Summon528 Jun 3, 2019

Choose a reason for hiding this comment

zadjii-msft Jun 3, 2019

Choose a reason for hiding this comment

Summon528 Jun 3, 2019

Choose a reason for hiding this comment

d-bingham Jun 3, 2019

Choose a reason for hiding this comment

d-bingham Jun 3, 2019

Choose a reason for hiding this comment

DHowett-MSFT Jun 3, 2019 • edited Loading

Choose a reason for hiding this comment

DHowett-MSFT commented Jun 3, 2019

Summon528 commented Jun 4, 2019 • edited Loading

mdtauk commented Jun 4, 2019

Summon528 commented Jun 4, 2019

DHowett-MSFT commented Jun 12, 2019

Summon528 commented Jun 12, 2019

DHowett-MSFT commented Jul 1, 2019

DHowett-MSFT commented Jul 1, 2019

azure-pipelines bot commented Jul 1, 2019

DHowett-MSFT commented Jul 1, 2019

Summon528 commented Jul 2, 2019

Summon528 commented Jul 2, 2019

miniksa left a comment

Choose a reason for hiding this comment

DHowett-MSFT commented Jul 10, 2019

ghost commented Aug 3, 2019

Summon528 commented Jun 2, 2019 •

edited

Loading

DHowett-MSFT Jun 3, 2019 •

edited

Loading

Summon528 commented Jun 4, 2019 •

edited

Loading