Add initial ConstrainedBox control for AspectRatio and Scaling #4104
Conversation
|
Thanks michael-hawker 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 🙌 |
ghost
requested review from
ghost
added
feature request 📬
Microsoft.Toolkit.Uwp.UI.Controls.Primitives/ConstrainedPresenter/ConstrainedPresenter.cs
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| /// <summary> | ||
| /// Gets or sets the scale for the width of the panel. Should be a value between 0-1.0. Default is 1.0. |
There was a problem hiding this comment.
And if it is not between 0 and 1? What if it is negative? I think both cases would cause issues with the layout-system, especially if the scale value is negative.
There was a problem hiding this comment.
Yeah, could probably coerce the value, would be nice if UWP XAML had a better system for that. Could at least clip to the bounds in the setter though.
There was a problem hiding this comment.
Yes, I think that would be a good idea. That's also what NumberBox does.
|
|
||
| private Size CalculateConstrainedSize(Size initialSize) | ||
| { | ||
| var availableSize = new Size(initialSize.Width * ScaleX, initialSize.Height * ScaleY); |
There was a problem hiding this comment.
Question: What if the content inside says "I want to be 100px wide at most"? According to docs, since we are more or less acting like a panel, we should respect that. However we are not unless base.MeasureOverride/base.ArrangeOverride does that. Thoughts?
There was a problem hiding this comment.
Pretty sure the base calls should follow those constraints, the sample I had kind of tests that scenario, but we could test some others to be sure.
There was a problem hiding this comment.
Maybe as part of some automated UI tests but should be fine then. Just a bit unsettling not knowing what exactly happens on the base calls.
Microsoft.Toolkit.Uwp.UI.Controls.Primitives/ConstrainedPresenter/ConstrainedPresenter.cs
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| /// <summary> | ||
| /// Gets or sets the scale for the width of the panel. Should be a value between 0-1.0. Default is 1.0. |
There was a problem hiding this comment.
Yes, I think that would be a good idea. That's also what NumberBox does.
|
|
||
| private Size CalculateConstrainedSize(Size initialSize) | ||
| { | ||
| var availableSize = new Size(initialSize.Width * ScaleX, initialSize.Height * ScaleY); |
There was a problem hiding this comment.
Maybe as part of some automated UI tests but should be fine then. Just a bit unsettling not knowing what exactly happens on the base calls.
| @@ -28,7 +28,7 @@ public Case CurrentCase | |||
| } | |||
|
|
|||
| /// <summary> | |||
| /// Indicates the <see cref="CurrentCase"/> property. | |||
| /// Identifies the <see cref="CurrentCase"/> property. | |||
There was a problem hiding this comment.
Could argue this is not part of the ContrainedPresenter :P
| <controls:ConstrainedPresenter AspectRatio="16:3" VerticalAlignment="Top"> | ||
| <Image Source="/Assets/Photos/WestSeattleView.jpg" Stretch="UniformToFill"/> | ||
| </controls:ConstrainedPresenter> |
There was a problem hiding this comment.
If this is a WIP ignore the comment below.
I think if we truly want to showcase the controls potential, we have a "root" panel that you can change in size and inside the ContrainedPresenter have a control that does not preserve aspect ratio on it's own. Maybe an ellipse with alignments set to stretch so that you can showcase how it maintains the aspect ratio.
|
Can't use x:Bind on a double field to the |
With what error is it failing? Could the property not being a DP be an issue there? |
michael-hawker
force-pushed
the
constrained-presenter
branch
from
5dfb5c8 to
4fe6c67
Compare
|
@chingucoding had to add implicit operator conversions: see commit 4fe6c67. Hit some issues trying to inherit directly from Did find that ParallaxView does handle the alignment of the child, so that'll be something extra we need to do being lower-level: https://github.com/microsoft/microsoft-ui-xaml/blob/main/dev/ParallaxView/ParallaxView.cpp#L117 - Beyond that though seems hopefully straight-forward once we know the "magic"? |
Hmm yes, my fear is that it might not be as easy as we think and there might be complexity hidden underneath that. But I would say it's worth a shot. If that fails, we could put a ContentPresenter inside the control and let that control handle the alignments in case we have no idea other idea. |
michael-hawker
changed the title
|
|
…ample. TODO: Tests and trying out more scenarios and better sample.
… mathmatical expressions and double can be bound from x:Bind directly!
1px Rounding was caused by multiple unnecessary passes of Arrange, so only re-calculated if needed (when parent Panel doesn't respect Measure request i.e. Grid Stretch) Also adds in handling for Infinity scenarios like StackPanel/ScrollViewer and can try and respect child measurements Adds initial basic tests as a starting point for more tests
michael-hawker
force-pushed
the
constrained-presenter
branch
from
1a68b20 to
0fd2a37
Compare
|
Alright, fixed a lot of issues with the implementation to handle the 1px rounding issue as well as panels that provide Started adding some unit tests, will add more next to check other scenarios. @chingucoding adding extra constraints should be handled, but I'll write some tests to be sure for Min/Width/Max etc... |
|
I have a use-case where I'd like to constrain a control's width/height to multiples of 4, irrespective of the control's actual aspect ratio. Is it possible with this control? |
Not currently, but I could see us adding a I think Scale always occurs first. Then the question is if you apply the Aspect Ratio next and snap to the closest values (which means your Aspect Ratio would be slightly off) or if you'd snap the remaining area next and then apply the AspectRatio (which could mean your snap is off)... We could theoretically have another setting that defines the order of operations, but then that may be a bit much... I think having these types of properties on one panel is useful, some of them are useful combined, but then I don't know if using them all together or in certain combinations is always useful... Thoughts? |
|
Had thought of something like this: <controls:ContrainedBox VerticalAlignment="Top">
<controls:ConstrainedBox.Constraints>
<controls:ScaleConstraint ScaleX="0.9" ScaleY="0.9"/>
<controls:AspectRatioConstraint AspectRatio="16:9"/>
<controls:MultipleConstraint OfX="4" OfY="4"/>
</controls:ConstrainedBox.Constraints>
<Image Source="/Assets/MyImage.png" Stretch="UniformToFill"/>
</controls:ConstrainedBox>With ability maybe to pull them out as a resource: That adds a lot of complexity though and verbosity. So think just going to add the Multiple properties; order will be Scale, Multiples, then AspectRatio. We'll see if we need configuration or more generalization in the future. |
michael-hawker
added
the
next preview ✈️
Image isn't DPI aware, so it's a bit weird at different DPI monitor.
|
I added a checkerboard pattern to the sample to test the I'd like to use this scenario to test the layout, but at the same time, it's extra code in other areas of the project. I may split that out as a separate PR for the future and just go back to some tests to check the layout numbers instead of making it look pretty first... I'll open a new issue for tracking: #4150 |
UnitTests/UnitTests.UWP/UI/Controls/Test_ConstrainedBox.AspectRatio.cs
Outdated
Show resolved
Hide resolved
UnitTests/UnitTests.UWP/UI/Controls/Test_ConstrainedBox.AspectRatio.cs
Outdated
Show resolved
Hide resolved
UnitTests/UnitTests.UWP/UI/Controls/Test_ConstrainedBox.Multiple.cs
Outdated
Show resolved
Hide resolved
UnitTests/UnitTests.UWP/UI/Controls/Test_ConstrainedBox.Scale.cs
Outdated
Show resolved
Hide resolved
UnitTests/UnitTests.UWP/UI/Controls/Test_ConstrainedBox.Scale.cs
Outdated
Show resolved
Hide resolved
| /// <summary> | ||
| /// Identifies the <see cref="MultipleX"/> property. | ||
| /// </summary> | ||
| public static readonly DependencyProperty MultipleXProperty = | ||
| DependencyProperty.Register(nameof(MultipleX), typeof(int), typeof(ConstrainedBox), new PropertyMetadata(null)); | ||
|
|
||
| /// <summary> | ||
| /// Gets or sets the integer multiple that the height of the panel should be floored to. Default is null (no snap). | ||
| /// </summary> | ||
| public int MultipleY | ||
| { | ||
| get { return (int)GetValue(MultipleYProperty); } | ||
| set { SetValue(MultipleYProperty, value); } | ||
| } |
There was a problem hiding this comment.
Does this need to be an integer? What if I want the value to be a multiple of 3.5.
There was a problem hiding this comment.
I suppose, but why would you want to snap to an un-even number of pixels?
| /// The <see cref="ConstrainedBox"/> is a <see cref="FrameworkElement"/> control akin to <see cref="Viewbox"/> | ||
| /// which can modify the behavior of it's child element's layout. <see cref="ConstrainedBox"/> restricts the | ||
| /// available size for its content based on a scale factor, multiple factor, and/or a specific <see cref="AspectRatio"/>, in that order. | ||
| /// This is performed as a layout calculation modification. |
There was a problem hiding this comment.
Could we rephrase this to make it more clear that this is the key difference compared to ViewBox?
There was a problem hiding this comment.
That's what the rest of the paragraph goes to describe? What would you clarify?
UnitTests/UnitTests.UWP/UI/Controls/Test_ConstrainedBox.Multiple.cs
Outdated
Show resolved
Hide resolved
UnitTests/UnitTests.UWP/UI/Controls/Test_ConstrainedBox.Multiple.cs
Outdated
Show resolved
Hide resolved
UnitTests/UnitTests.UWP/UI/Controls/Test_ConstrainedBox.Multiple.cs
Outdated
Show resolved
Hide resolved
UnitTests/UnitTests.UWP/UI/Controls/Test_ConstrainedBox.Scale.cs
Outdated
Show resolved
Hide resolved
UnitTests/UnitTests.UWP/UI/Controls/Test_ConstrainedBox.Scale.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Marcel Wagner <[email protected]>
|
This PR has been marked as "needs attention 👋" and awaiting a response from the team. |
|
Alright think we're in a good enough state for an initial preview. Can add some more tests and samples later (want to fix #4150 as well after preview). @RosarioPulella @XAML-Knight thoughts on |
While a |
I agree with @XAML-Knight, double feels like premature optimization. As a side note, I actually think the aspect ratio should only ever be represented as a pair of |
|
Thanks for the note @RosarioPulella, as discussed. While I think it makes sense, it does seem there are a couple of more common Aspect Ratios that are irrational and can't be represented as easily with just two |
There was a problem hiding this comment.
Yeah it seems I made an assumption based on how I have seen aspect ratios in the wild. Doubles for the aspect ratio's width and the height could be necessary, not just a good idea.
As for the Value property and the implicit conversion so a single double, I still think its better to simply do that math explicitly, but I don't think its that bad.
Fixes #4103
Adds the new
ConstrainedPresentercontrol which can be used around an element to constrain it's layout based on a scale and/or aspect ratio. Adds anAspectRatiohelper as well so you can just type in16:9as an aspect ratio as well.PR Type
What kind of change does this PR introduce?
What is the new behavior?
The control will first scale the available size by the
ScaleXandScaleYproperties (default to 1.0) and then constrain that space to the desiredAspectRatiosize.TODO:
Can't use x:Bind to bind to Double (can we use an implicit type conversion on AspectRatio?):
Investigate 1px rounding error on edges (click image to zoom and see the light-gray strip on left side):
Unit Tests
Add bounds check to Scale properties, probably want to ensure AspectRatio is non-zero and positive as well.
Try out more scenarios
Better sample
PR Checklist
Please check if your PR fulfills the following requirements:
Other information
FYI @chingucoding @JustinXinLiu if you want to give it a whirl.