RadialGradientBrush initial commit #1831
Conversation
Initial commit of basic RadialGradientBrush implementation
| case winrt::BrushMappingMode::RelativeToBoundingBox: | ||
| compositionGradientBrush.MappingMode(winrt::Windows::UI::Composition::CompositionMappingMode::Relative); | ||
| break; | ||
| } |
There was a problem hiding this comment.
Are these the only two modes? should we throw if a different value is provided ? #Closed
There was a problem hiding this comment.
If we get a different value and don't explicitly set any mapping mode then the brush will just use the composition layer's default (Relative) which seems like a reasonable behavior?
I used the existing BrushMappingMode enum which has only ever had two values for ~20 years and I don't expect it will be expanded.
Is it convention to guard against future potential additions to an enum and throw?
We could also make RelativeToBoundingBox the default in the switch statement in addition to the explicit case, but that'd yield the same behavior we get as-is with the composition default. #Closed
There was a problem hiding this comment.
I'm ok with falling back to a default behavior instead of throwing, but vote for making it explicit in the code with a default tag for the switch statement. #Closed
There was a problem hiding this comment.
+1
While it is not explicitly called out as a rule in the C++ guidelines, es79(https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#es79-use-default-to-handle-common-cases-only) and Enum2's (https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#enum2-use-enumerations-to-represent-sets-of-related-named-constants) enforcement rules imply that call switch statement should have a default case, even if it is a do nothing, or a throw. #Closed
There was a problem hiding this comment.
Added default case and made the RelativeToBoundingBox case [[fallthrough]] - let me know if you'd prefer explicit duplication instead
- Disable feature by default in MUXControlsInnerLoop - move compositionbrush cast checks into if statements - use local variable to access x and y values of points to avoid extra property system access - switch gradientstops iteration from index-based to range-based
| @@ -198,6 +198,12 @@ bool SharedHelpers::IsIsLoadedAvailable() | |||
| return s_isAvailable; | |||
| } | |||
|
|
|||
| bool SharedHelpers::IsCompositionRadialGradientBrushAvailable() | |||
There was a problem hiding this comment.
IsCompositionRadialGradientBrushAvailable [](start = 20, length = 41)
Is there an interface we can QI for instead? platform look ups are expensive and should be avoided if possible.
There was a problem hiding this comment.
Do we do that anywhere else?
I was trying to follow the other SharedHelpers checks which also use ApiInformation, and I think this lookup in windows.foundation.metadata should only do the lookup once on first use of the brush per app launch.
| { | ||
| ColorMatchTestResult.Text = "Failed"; | ||
| } | ||
| } |
There was a problem hiding this comment.
Interesting, why is this code here rather than in the test? I think generally we would output the center and outer color value to something the test can read rather than outputing a passed or failed state. As written we would not get very much info from a log of a test failure.
There was a problem hiding this comment.
I was following the precedent of the acrylic tests which do something similar:
- add default case to mappingmode switch statement - switch from event tokens to event_revoker - add per-property change callback methods - reorder .targets imports to ensure commenting is accurate
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@jesbis having different property names will be a big migration pain. There are ton of resources available (and tools that export) that would be very hard to convert needing to do extra math to the XAML property values. Compat with previous versions was a MUST in the spec. Since this is a similar base approach as we had in the Toolkit, you can see how we mapped the properties as defined in WPF to the brush interop for Win2D. I validated this provided a 99% match in output to WPF for relative mode absolute. The relative mapping was the tricky bit as we have no info about the hosting control's coordinates. I didn't see how that was addressed in this PR or is that handled underneath in the composition layer now with the new RadialGradientBrush creation method? |
This is a basic implementation of a RadialGradientBrush for #266.
Description
RadialGradientBrush uses XamlCompositionBrushBase to render a CompositionRadialGradientBrush.
The API is similar to CompositionRadialGradientBrush and RadialGradientBrush in WPF.
For spec details see:
microsoft/microsoft-ui-xaml-specs/pull/27
Notes:
FallbackColor) on pre-1909 OS versions - the spec link above has more infoIAnimationObject, but facade animations aren't currently available downlevel below 1809 (RS5) - we could revisit this after WinUI 3 rather than attempting to fork the implementation now since it would rely on the brush implementing an interface that wouldn't exist on all supported OS versions. The spec link above has more infoMotivation and Context
I was trying out the public developer guidance to see where it could use more details, and it could definitely use a few more details 🙂 So, I made some assumptions for this feature - hopefully nothing majorly wrong? I'm hoping to update the developer guidance at some point based on how this goes.
How Has This Been Tested?
The test page
RadialGradientBrushPageexercises introduced APIs via markup, code-behind and x:Bind. It includes different usage models such as locally set brushes, shared resources and dynamically created brushes.This change also includes basic automated tests for:
which pass locally on desktop 1909 and 1903. I did not manually test other versions or devices.
Screenshots (if appropriate):