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

Acrylic brushes change the values I provide #1639

Closed
SpencerHurd opened this issue Nov 19, 2019 · 18 comments · Fixed by #2017
Closed

Acrylic brushes change the values I provide #1639

SpencerHurd opened this issue Nov 19, 2019 · 18 comments · Fixed by #2017
Assignees
Labels
area-Materials Reveal, Acrylic, lighting, etc. bug Something isn't working help wanted Issue ideal for external contributors team-Controls Issue for the Controls team

Comments

@SpencerHurd
Copy link

SpencerHurd commented Nov 19, 2019

When using an acrylic brush, there is math applied to the color value, tint value, and luminosity value that change these values to make the acrylic work more like rs4 acrylic - this conversion changes our desired results, and means designers literally cannot achieve their designs - the values they hand off to code do not get reproduced by the brush. not only that, because there are value caps in place, certain values are not achievable at all.

This conversion logic is not desired by designers, and is almost entirely unknown so it cannot be worked around by many. This needs to be an option to opt in or opt out option, or a new version produced that does not have this conversion built in so expected results can be achieved.

The conversion is not documented, and is currently ignored by MS! Lets fix this to make design achievable and the brush simpler!

Bug Report

Describe the bug

There is "auto-magic" logic built into the current Acrylic brush (that was put in place to replicate the RS4 acrylic visual appearance at the time), that now conflicts with the contrast ratios needed in order to pass WCAG 2.1 accessibility requirements.

This logic affects the color of the luminosity blend mode and the tint color properties and sets them automatically based on the tint color of the brush.

Steps to reproduce the bug

Steps to reproduce the behavior:

  1. Attempt to set the TintLuminosityOpacity to below #202020 and observe that it will lock at that value instead, regardless of what you put it's hex color value to.

Expected behavior

If TintLuminosityOpacity property is set, then all automatic RS4 logic will be be ignored, and the values specified by the user will be set in the brush 1:1.

Screenshots

Version Info

NuGet package version:

Windows 10 version Saw the problem?
Insider Build (xxxxx) 19493
May 2019 Update (18362)
October 2018 Update (17763)
April 2018 Update (17134)
Fall Creators Update (16299)
Creators Update (15063)
Device form factor Saw the problem?
Desktop yes
Mobile yes
Xbox yes
Surface Hub yes
IoT yes
@msft-github-bot msft-github-bot added the needs-triage Issue needs to be triaged by the area owners label Nov 19, 2019
@kikisaints kikisaints added the feature proposal New feature proposal label Nov 19, 2019
@msft-github-bot msft-github-bot added this to Freezer in Feature tracking Nov 19, 2019
@jevansaks jevansaks added area-Materials Reveal, Acrylic, lighting, etc. team-Controls Issue for the Controls team team-Rendering Issue for the Rendering team and removed needs-triage Issue needs to be triaged by the area owners team-Controls Issue for the Controls team labels Nov 20, 2019
@msft-github-bot msft-github-bot added the needs-triage Issue needs to be triaged by the area owners label Nov 20, 2019
@kikisaints kikisaints added bug Something isn't working and removed feature proposal New feature proposal labels Dec 4, 2019
@jevansaks jevansaks added help wanted Issue ideal for external contributors team-Controls Issue for the Controls team and removed needs-triage Issue needs to be triaged by the area owners team-Rendering Issue for the Rendering team labels Dec 5, 2019
@msft-github-bot msft-github-bot added the needs-triage Issue needs to be triaged by the area owners label Dec 5, 2019
@jevansaks jevansaks removed the needs-triage Issue needs to be triaged by the area owners label Dec 6, 2019
@jevansaks
Copy link
Member

Per email discussion it sounds like the request is to do the color adjustment only when TintLuminosityOpacity = 1.0. If TintLuminosityOpacity < 1 then don't adjust the color. Seems simple enough? Adding help wanted. :)

@SpencerHurd
Copy link
Author

SpencerHurd commented Dec 6, 2019 via email

@kikisaints
Copy link

@jevansaks, that's correct. Please see the Expected Behavior section of this bug - tried to supply the behavior desired.

@SpencerHurd
Copy link
Author

SpencerHurd commented Jan 8, 2020 via email

@marcelwgn
Copy link
Contributor

Hi @SpencerHurd , I would like to look into this issue, however I am not quite sure how I would be able to test/verify a suitable fix. Do you have a demo or a code snippet that I could try or use?

Also I assume with LuminosityTintColor you mean TintColor ?

@kikisaints
Copy link

Thanks @SpencerHurd - in the future please refrain from posting internal email on issues - just for other people's privacy. :)

I have not been able to loop back on this since you first requested it. So I have no updates.

@chingucoding - he's actually referring to the property on Acrylic called TintLuminosityOpacity - which I now realize is a bug in the initial report above, I will fix.

@marcelwgn
Copy link
Contributor

@kikisaints Thanks for the clarification!

@SpencerHurd
Copy link
Author

SpencerHurd commented Feb 19, 2020 via email

@SpencerHurd
Copy link
Author

Hi @chingucoding! Fantastic you are interested in helping, thank you so much! Sorry for the delayed response - I was figuring out how to use Github.

The issue: its kind of hard to describe whats happening. I am a designer and cannot refer to the code of it. But Ill describe it as best I can. The acrylic brush has is comprised of a few layers that stack together to create the effect, from top to bottom:

  1. Noise layer - this is an image asset repeated over the area of the acrylic
  2. Tint Layer - this is a color value, an opacity, and is set to a 'color' blend mode
  3. Luminosity layer - this is a color value, an opacity, and is set to a 'luminosity' blend mode
  4. Blurred background image

When using the brush, developers may supply a tint color, a tint opacity, and a luminosity opacity.

  1. The tint color is supplied by the develop and fed to the color value in both the NavigationView TogglePaneTopPadding issue #2 Tint layer, and the A few problems with the new NavigationView control on winui library #3 Luminosity layer. However the luminosity layer caps how bright and dark the color can get (it cannot get to black or white).

  2. The tint opacity is supplied by the developer, then the brush adjusts it via some fancy math, and the new value is fed to the NavigationView TogglePaneTopPadding issue #2 tint layer (the value you input is not the value used by the brush).

  3. The luminosity opacity is an optional value, if it is not supplied by the developer then the opacity is mathematically derived from the tint color and opacity. If it is supplied then the value provided by the developer is fed directly to the A few problems with the new NavigationView control on winui library #3 luminosity layer unaltered.

So there is a lot of mathematical stuff happening inside the brush that changes the values supplied to the brush. There is a reason for this - all the conversions and adjustments occur in order to make the acrylic brush look like the original acrylic brush. However the bug is this: when the luminosity opacity value is supplied, all the fancy math logic should not be used. When luminosity opacity is provided by the developer we don't want it to look like the original acrylic, we want the values provided used unaltered by the brush.

So: if no luminosity opacity value is supplied, leave the current system in place.

The following should happen

  1. The tint color is used directly by the NavigationView TogglePaneTopPadding issue #2 tint layer, and the A few problems with the new NavigationView control on winui library #3 luminosity layer (no adjustment, no capping of the color value when given to the luminsity layer)
  2. The tint opacity supplied to the brush should be fed to the NavigationView TogglePaneTopPadding issue #2 tint layer unadjusted.
  3. The luminosity opacity value should be used as is, not auto calculated (currently this is correct).

I hope this helps!

Spencer

@marcelwgn
Copy link
Contributor

@SpencerHurd Thank you for the detailed explanation! This sure helps a lot. So the expected behavior is the following:

No luminosity opacity specified: Color will be taken if V (from HSV) is between 0.125 and 0.96, otherwise take the closest of those values.

Luminosity opacity specified: Take opacity and color as is, no "bounds" checking. (Currently this case also does bounds checking, which is not what we want)

Is this correct?

If that's the case, I would like to fix this, as long as its fine with @jevansaks, @kikisaints and @ranjeshj .

@kikisaints
Copy link

@chingucoding your summary is correct to my understanding of the problem. If you'd like to go ahead and take this bug/issue, that's more than okay with me 😄 design has been looking forward to this change for a little bit now.

@SpencerHurd
Copy link
Author

@chingucoding What you are saying is correct, if Luminosity Opacity is specified then we want to remove the bounds on the luminosity color value so it inherits the tint color value unaltered. We also want to make sure tint opacity uses the supplied tint opacity value unaltered.

Very cool, thank you!

@marcelwgn
Copy link
Contributor

marcelwgn commented Feb 21, 2020

From the source code, it seems like the tint opacity does get in fact get altered, when we use it for the tint layer:

// This method supresses the maximum allowable tint opacity depending on the luminosity and saturation of a color by
// compressing the range of allowable values - for example, a user-defined value of 100% will be mapped to 45% for pure
// white (100% luminosity), 85% for pure black (0% luminosity), and 90% for pure gray (50% luminosity). The intensity of
// the effect increases linearly as luminosity deviates from 50%. After this effect is calculated, we cancel it out
// linearly as saturation increases from zero.

For the luminosity layer, we are using the "raw" value i.e. we take the tint color and set its alpha value to the tint opacity value, and then continue calculating the luminosity color.

// Purposely leaving out tint opacity modifier here because GetLuminosityColor needs the *original* tint opacity set by the user.
tintColor.A = static_cast<uint8_t>(round(tintColor.A * tintOpacity));

Is this what you would expect @SpencerHurd ?

@SpencerHurd
Copy link
Author

Lets see 'm not 100% sure.

Once luminosity opacity is supplied:

  • We do not want to adjust the tint opacity value at all, but use the original provided value.
  • The luminosity color should have nothing to do with tint opacity at all, just use the raw color (not opacity) of the tint color value, with no brightness caps on getting too white or black.
    -The luminosity opacity should use the luminosity opacity value provided by the dev.

So we want to remove all adjustments to values if luminosity opacity is supplied by the dev.
Tint Color = Original Tint Color
Tint opacity = Original Tint Opacity
Luminosity color = Original Tint Color
Luminosity Opacity = Original Luminosity Opacity

Does that answer the question? From how I understand what I'm seeing (I'm sorry I don't know code :/), the tint opacity is being altered, and the luminosity layer is being affected by the tint layer opacity. I suspect the brightness cap is also still in play on luminosity color.

@kikisaints
Copy link

It's important to call out, since we don't want to eliminate any pre-existing behavior that app developers may be relying on, that:

  • If the luminosity opacity/alpha value is not set, it operates the way it does today in the code snippets you've highlighted
  • If the luminosity opacity/alpha is set then it should be set directly and in the ways Spencer has detailed above

@marcelwgn
Copy link
Contributor

Yes the only case where we want to change stuff is when luminosity opacity is actively set.

Yes this whole maths is a bit complicated. So summary:

The Luminosity layer should behave the following:

  • Luminosity not specified: Leave as is
  • Luminosity specified: Use tint color, tint opacity and luminosity opacity all unmodified

The other layers are already correct as they are. Is this correct @SpencerHurd and @kikisaints ?

@kikisaints
Copy link

@chingucoding that looks correct to me. That you for summarizing.

@SpencerHurd
Copy link
Author

That is correct! Love it! thanks for stating it so simply.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-Materials Reveal, Acrylic, lighting, etc. bug Something isn't working help wanted Issue ideal for external contributors team-Controls Issue for the Controls team
Projects
None yet
5 participants