Remove automatic value change when specifying luminosity opacity on AcrylicBrush

[marcelwgn]

Description

Remove unwanted clamping/rounding of colors when luminosity opacity is set on acrylic brush.

Motivation and Context

Fixes #1639

How Has This Been Tested?

Added new test page for visual verification.

Screenshots (if appropriate):

[kikisaints]

@SpencerHurd - please review the screenshots to ensure the visuals chingucoding is showing is what's desired for this Acrylic fix.

[ranjeshj]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[DmitriyKomin]

double GetTintOpacityModifier(winrt::Color tintColor)

Following Spencer's description of the proposed change in the GitHub discussion for issue#1639, I would expect a change to this function, or GetEffectiveTintColor() above [to avoid applying any tintOpacityModifier to get the effective tint color in case TintLuminosityOpacity is set]

---

Refers to: dev/Materials/Acrylic/AcrylicBrush.cpp:344 in 647dac8. [](commit_id = 647dac8, deletion_comment = True)

[DmitriyKomin]

GetEffectiveTintColor: there should be a change either here or in GetTintOpacityModifier to skip any modifications of TintColor when TintLuminosityOpacity is specified by the app.

[marcelwgn]

For the calculation of the Luminosity color, the values specified for TintColor and TintOpacity are used as provided by user.

From what I understood, only the luminosity color was not calculated correctly, thus this was the only change I made.

[StephenLPeters]

I'd prefer you put the remainder of this function in an else block, I think it makes it easier to read at a glance than having to recognize the early return

[marcelwgn]

I am personally more a fan of the "return early" pattern, but sure, I will change it.

[StephenLPeters]

You are still returning early but this puts the logic that is now in the else block at the correct indentation level. before this change its easier to look at the code that is now in the else block and assume it will be run regardless of whether or not the above if statement was executed or not. Now it is obvious from the indentation level that it will only be run if we do not meet the if statement.

---

In reply to: 386686086 [](ancestors = 386686086)

[marcelwgn]

Oh haha, I see. So your preferred way would be have a variable and we either set it to the "normal" luminosity brush or run the calculations and return our saved variable at the end of the function? If you want I can still change that to only have one return at the end.

[DmitriyKomin]

In GetEffectiveTintColor() we obtain the effective TintColor, and its Alpha is affected by luminosity via the tintOpacityModifier:
336: double tintOpacityModifier = GetTintOpacityModifier(tintColor);
339: tintColor.A = static_cast<uint8_t>(round(tintColor.A * tintOpacity * tintOpacityModifier));

That is the tint color we actually use in the recipe (sometimes directly, sometimes to determine if we're in the special case of 'opaque acrylic'):
295: bool shouldUseOpaqueBrush = GetEffectiveTintColor().A == 255;
308: auto newColor = GetEffectiveTintColor();
930: winrt::Color tintColor = GetEffectiveTintColor();
1010: bool shouldUseOpaqueBrush = GetEffectiveTintColor().A == 255;

To complete the change, we can eg update line 339 to exclude the tintOpacityModifier term in the case that luminosityOpacity is provided.

---

In reply to: 591684517 [](ancestors = 591684517)

[marcelwgn]

@DmitriyKomin I am not sure what's the right change is here, this is something @SpencerHurd (and possibly @chigy) need to decide.

Sorry if I am CC'ing the wrong people here, I am not sure who exactly is the expert for that specifically 😅

[chigy]

@DmitriyKomin I am not sure what's the right change is here, this is something @SpencerHurd (and possibly @chigy) need to decide.

Sorry if I am CC'ing the wrong people here, I am not sure who exactly is the expert for that specifically 😅

@kikisaints , did we not receive actual recipe from design team when this was discussed originally?

[kikisaints]

@DmitriyKomin I am not sure what's the right change is here, this is something @SpencerHurd (and possibly @chigy) need to decide.
Sorry if I am CC'ing the wrong people here, I am not sure who exactly is the expert for that specifically 😅

@kikisaints , did we not receive actual recipe from design team when this was discussed originally?

We received a design detailing the luminosity integration. Internally as the PM team we decided on a "fallback" behavior that was:

1. If the luminosity was not set, we would default and auto-correct to values that would make Acrylic look like the "old" one
2. If the luminosity value was specified then we would apply it

It looks like DI didn't implement this quite the way we had decided, resulting in some additional luminosity adjustments even when the property was set. I think this was to combat Shadows, but since the DI doesn't remember why it was adjusted and design is unhappy with it I'm inclined to remove it so 1 above operates as we originally intended.

[StephenLPeters]

[StephenLPeters]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[StephenLPeters]

@DmitriyKomin could you please take another look and approve if this is ready to go in?

[DmitriyKomin]

@SpencerHurd : can you review the PR overall and also look at my comment above requesting a change? I believe that is needed to achieve the behavior you outlined...

[SpencerHurd]

Hi everyone,

Dimitiry was kind enough to walk me through the code changes. It looks to me like two of the three alterations are being correctly suppressed when Luminosity Opacity is provided. So the desired LuminosityTintColor is being used, and the desired LuminosityTintOpacity is being used.

However it looks like the TintOpacity is still being altered, which is undesirable.

Thank you very much!

[marcelwgn]

Thanks for your feedback @SpencerHurd and @DmitriyKomin !
I have changed the code not interfere in the tint opacity calculation if users have specified the tint luminosity opacity. If there is anything else missing, feel free to tell me!

[StephenLPeters]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[DmitriyKomin]

Thanks

Thanks for your feedback @SpencerHurd and @DmitriyKomin !
I have changed the code not interfere in the tint opacity calculation if users have specified the tint luminosity opacity. If there is anything else missing, feel free to tell me!

Thanks! Looks good, I approved the PR.