-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Win2D pipeline brushes #3112
Win2D pipeline brushes #3112
Conversation
Thanks for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wanted to provide some initial feedback on namespaces and integration with the Toolkit. Looks like a good start.
I'll have to pull it down and play with it a bit first-hand. Don't forget about adding a sample page to the sample app as well. 😊
using Windows.UI; | ||
using Windows.UI.Xaml.Media; | ||
|
||
namespace Microsoft.Toolkit.Uwp.UI.Media.Brushes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
namespace Microsoft.Toolkit.Uwp.UI.Media.Brushes | |
namespace Microsoft.Toolkit.Uwp.UI.Media |
We didn't add Brushes on top of the namespace as we were keeping consistency with the older System.Windows.Media
namespace from the RadialGradientBrush. I mean it's the only thing in the package currently, though I see us moving non-brush things in the future. I still think it's fine to have it in the main space.
Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally don't mind that, and I could make a commit to move all of those into just the .Media
namespace then. The only note though is that we also have other public namespaces though, such as .Pipelines
now. We can do either of the following:
- Leave all the brushes in just
.Media
for simplicity and ignore the inconsistency - Move all the brushes in
.Media
to get consistency with the other namespaces (and possibly with new ones being added in the future).
Personally I don't mind having the explicit .Brushes
namespace, but I can see how having just .Media
there would be maybe more intuitive for users. Maybe we should get some more feedbacks from others too? I'd be leaning towards a uniform .Brushes
for now I think 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, let's keep all the brushes in the .Media
namespace for consistency with what we have and the OS brushes from WPF and WinUI (which are all in a .Media
namespace, e.g. RadialGradientBrush and AcrylicBrush).
Microsoft.Toolkit.Uwp.UI.Media/Extensions/System.Threading.Tasks/AsyncMutex.cs
Show resolved
Hide resolved
Microsoft.Toolkit.Uwp.UI.Media/Extensions/System.Threading.Tasks/AsyncMutex.cs
Outdated
Show resolved
Hide resolved
{ | ||
if (uri.Scheme.Equals("ms-resource")) | ||
{ | ||
string path = uri.AbsolutePath.StartsWith("/Files") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will Files
always start with a Capital letter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, from what I've seen that's always the case (I've been using this method for a few years now and never had any issues with it), I assume that's just some constant in the XAML engine somewhere. We can either leave it like that or make the test case insensitive if we want to be extra sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make it case insensitive to be sure
Co-authored-by: Michael Hawker MSFT (XAML Llama) <24302614+michael-hawker@users.noreply.github.com>
…ergio0694/WindowsCommunityToolkit into feature/win2d-pipeline-brushes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are amazing! Thanks @Sergio0694 for all the hard work on these! 🎉🎉🎉
Moved to Windows Community Toolkit: CommunityToolkit/WindowsCommunityToolkit#3112
This PR is part of the group being tracked in #3108.
PR Type
What kind of change does this PR introduce?
What is the new behavior?
This PR includes a number of new types:
PipelineBuilder
, a C# pipeline builder class that lets developers create complex Win2D/Composition pipelines of effects, with the ability to then convert them into brushes that can be applied to visual elementsEffects.*
, a collection of wrappers for Win2D/Composition effects that can be instantiated from XAML, to build Win2D/Composition brushes directly from XAMLPipelineBrush
, a XAML brush to use in conjunction with the effects mentioned aboveAcrylicBrush
, a custom acrylic effect usable from XAML, that lets users customize every aspect of the acrylic pipelineTilesBrush
, a XAML brush that displayes a tiled textures. Useful to quickly create a noise texture, or other visual effects where a repeated image is needed.There are other APIs being added, but they're all marked as internal, and are just needed by the new public APIs being included in this PR.
Open questions
I need some input from @michael-hawker and anyone else interested in this PR.
In particular, some aspects to discuss:
AcrylicBrush
very useful, but the same exact effect can be achieved by just rebuilding the pipeline from XAML, using thePipelineBrush
. Plus, the name might be confusing for users maybe? I wonder whether we should either rename it, or discard that entirely.PipelineBuilder
that could probably use some reorganization in order to make them more flexible. Feel free to glance at the code and provide feedbacks here. In particular, I'm referring to thedelegate
types used to animate effects along a pipeline.ThreadSafeCompositionCache<T>
andThreadSafeCompositionMapCache<TKey, TValue>
) could probably be modified to only use aConditionalWeakTable<TKey, TValue>
instead, targeting a givenCoreDispatcher
instance instead, which should also make the whole thing more efficient. I'll give that a try and see how it goes.PR Checklist
Please check if your PR fulfills the following requirements:
Other information
Marking this PR as draft since I'll expect we'll need to have a conversation on how to better adjust the exposed APIs, etc. Also I'll definitely need some guidance on how to properly structure tests and/or the sample pages for the sample app.