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

update RadialGradientBrush property values to be ratios instead of percentages #2193

Closed
ranjeshj opened this issue Mar 30, 2020 · 8 comments
Closed
Assignees
Labels
area-RadialGradientBrush help wanted Issue ideal for external contributors team-Controls Issue for the Controls team

Comments

@ranjeshj
Copy link
Contributor

Update property values to match the spec. See comment here..

@ranjeshj ranjeshj added help wanted Issue ideal for external contributors team-Controls Issue for the Controls team area-RadialGradientBrush labels Mar 30, 2020
@msft-github-bot msft-github-bot added the needs-triage Issue needs to be triaged by the area owners label Mar 30, 2020
@michael-hawker
Copy link
Collaborator

michael-hawker commented Mar 30, 2020

What does it mean for Radius to be 0 to 1 ?

@ranjeshj it's all about proportions of the original size of the parent surface. We spent a lot of time in the Toolkit making sure we interoped 99% with the WPF brush in terms of the main properties (we just couldn't support absolute mapping mode), so we tried to make our documentation robust; you can see how we actually use the values to do the mapping in the brush itself here:

https://github.com/windows-toolkit/WindowsCommunityToolkit/blob/adf771eadba828495a051adcac820a1c56385335/Microsoft.Toolkit.Uwp.UI.Media/Brushes/RadialGradientBrush.cs#L77-L93

I imagine since you're still using the same underlying composition technology that these should map pretty directly in terms of concepts, just that they need to be C++ instead of C#. I believe Jesse wasn't aware of this work during his initial implementation as a reference. Please let us know if you need any more info about the Toolkit implementation.

@marcelwgn
Copy link
Collaborator

@ranjeshj I would like to take this issue, if thats fine.

@michael-hawker Unfortunately, the RadialGradientBrush uses the CompositionRadialGradientBrush which behaves differently than the WPF RadialGradientBrush does, e.g. to get the GradientOrigin to be starting in the top left, we had to transform the input to get that to work. But it should be fixable :)

@michael-hawker
Copy link
Collaborator

@chingucoding we used the CanvasRadialGradientBrush from Win2D in the toolkit and had to do all the translation as well, so I imagine it's pretty similar types of mappings still?

Anyway, feel free to ping me on the discord server if you want to chat about anything for this topic. I implemented the brush in the Toolkit and am passionate about WPF interop for the community. 😊

@marcelwgn
Copy link
Collaborator

marcelwgn commented Mar 31, 2020

@michael-hawker Thank you for that offer! 🦙

However I am not sure if there is actually anything do right now. It seems that the spec is just not correctly aligned with the source code right now. Setting EllipseRadius to (0.5,1.0) stretches the RadialGradientBrush as expected (like in WPF with RadiusX and RadiusY). All points need to be specified with values in [0,1] for the Mappingmode "RelativeToBoundingBox" and in that mode they behave the same like the WPF properties do.

Or am I missing something here?

Edit: To clarify, it seems that this issue is already resolved with the current behavior.

@marcelwgn
Copy link
Collaborator

@MikeHillberg , @ranjeshj , @jesbis FYI

@StephenLPeters StephenLPeters removed the needs-triage Issue needs to be triaged by the area owners label Mar 31, 2020
@MikeHillberg
Copy link
Contributor

I updated the spec:

  • Renamed GradientOriginOffset to GradientOrigin
  • Updated EllipseCenter and EllipseRadius to be [0,1] rather than [1,100].

Does that match?

@marcelwgn
Copy link
Collaborator

Yes that should be matching the current implementation.

@ranjeshj
Copy link
Contributor Author

ranjeshj commented Apr 1, 2020

I updated the spec:

  • Renamed GradientOriginOffset to GradientOrigin
  • Updated EllipseCenter and EllipseRadius to be [0,1] rather than [1,100].

Does that match?

Yes. That is what i see as well. With the default mapping mode, this is accurate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-RadialGradientBrush help wanted Issue ideal for external contributors team-Controls Issue for the Controls team
Projects
None yet
Development

No branches or pull requests

6 participants