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

Added Thickness support for BorderThickness and CornerRadius #1462

Merged
merged 9 commits into from Apr 10, 2018

Conversation

Projects
None yet
4 participants
@Gillibald
Copy link
Contributor

Gillibald commented Mar 22, 2018

BorderThickness and CornerRadius are now defined by a Thickness. To get a better performance for uniform borders a fallback to old fast rendering is in place.

{
backgroundGeometry = new StreamGeometry();

if (cornerRadius.IsEmpty)

This comment has been minimized.

Copy link
@DmitryZhelnin

DmitryZhelnin Mar 23, 2018

Contributor

Why there are two identical if-else branches?

This comment has been minimized.

Copy link
@Gillibald

Gillibald Mar 23, 2018

Author Contributor

Good catch. Those checks are not needed anymore.

{
var borderGeometry = new StreamGeometry();

if (cornerRadius.IsEmpty)

This comment has been minimized.

Copy link
@DmitryZhelnin

DmitryZhelnin Mar 23, 2018

Contributor

And two more identical if-else braches


StreamGeometry backgroundGeometry = null;

if (!boundRect.Width.Equals(0) && !boundRect.Height.Equals(0))

This comment has been minimized.

Copy link
@DmitryZhelnin

DmitryZhelnin Mar 23, 2018

Contributor

Shouldn't it supposed to be innerRect inside if?

@Gillibald

This comment has been minimized.

Copy link
Contributor Author

Gillibald commented Mar 23, 2018

Fast rendering needs some improvment dont know how yet. The inner corners dont match if you have a CornerRadius > 0. In my faster rendering path i just take the same radius and it looks smooth.

if (background != null)
{
     context.FillRectangle(background, rect.Deflate(borders), cornerRadius);
}

if (borderBrush != null && borderThickness > 0)
{
     context.DrawRectangle(new Pen(borderBrush, borderThickness), rect.Deflate(borderThickness), cornerRadius);
}

FillRectangle and DrawRectangle seem to produce diffirent rounded corners.

@Gillibald

This comment has been minimized.

Copy link
Contributor Author

Gillibald commented Mar 23, 2018

Microsoft checks for UseLayoutRounding and calculates everything in terms of ui scaling (DPI). Should i do the same and how do i do that? Everything about scaling is private.

@grokys

This comment has been minimized.

Copy link
Member

grokys commented Mar 23, 2018

@Gillibald the layout rounding should have already been applied by the layout pass, so the control should be snapped to pixels. This is done here.

Is it that you want to also snap the border thickness to pixels? To get the layout scaling for a control you can copy this method: https://github.com/AvaloniaUI/Avalonia/blob/master/src/Avalonia.Layout/Layoutable.cs#L698 - it may be something we need to expose publicly, but for the moment just copy it into your codebase and see if it solves your problem, we can look into exposing it once we know it's needed.

FillRectangle and DrawRectangle seem to produce diffirent rounded corners.

I'll look into this.

@Gillibald

This comment has been minimized.

Copy link
Contributor Author

Gillibald commented Mar 23, 2018

Looks like with the current complex render path all pixels are already snapped to boundaries. I no longer use a Pen in my DrawGeometry call. Don't know why a brush is different but for now it is enough. Only have tested Direct2D maybe other Frameworks behave differently.

@grokys
Copy link
Member

grokys left a comment

Added a few small comments (mainly nits) in the code.

However I think there's something wrong with the complex border drawing:

image

The first border above has a CornerRadius="16 0 0 0" and the second CornerRadius="16", however the corner on the top left of the first border doesn't look the same the corners on the second border - it looks "fatter". Any idea why this is?

Also we should probably have some more render tests to test this new functionality.

return finalSize;
}

private double GetLayoutScale()
{
var result = (VisualRoot as ILayoutRoot)?.LayoutScaling ?? 1.0;

if (result == 0 || double.IsNaN(result) || double.IsInfinity(result))
if (result.Equals(0) || double.IsNaN(result) || double.IsInfinity(result))

This comment has been minimized.

Copy link
@grokys

grokys Mar 23, 2018

Member

Why the change here? result is a double, so == is what you'd usually use to compare.

This comment has been minimized.

Copy link
@Gillibald

Gillibald Mar 23, 2018

Author Contributor

Wasn't sure about rounding errors but I will revert it. Usually it should do the same.

This comment has been minimized.

Copy link
@grokys

grokys Mar 23, 2018

Member

I wasn't aware of any differences between double.Equals and ==, are there differences?!?

This comment has been minimized.

Copy link
@grokys
return new Size(padding.Left + padding.Right, padding.Bottom + padding.Top);
}

internal class BorderRenderer

This comment has been minimized.

Copy link
@grokys

grokys Mar 23, 2018

Member

I think I'd rather have this class moved out of Border. Maybe put it as an internal class under Utils?

This comment has been minimized.

Copy link
@Gillibald

Gillibald Mar 23, 2018

Author Contributor

Will have a look at present render tests and create new ones to represent my additions. That way we can make sure everything looks as it should be.

@@ -20,7 +21,7 @@
<SolidColorBrush x:Key="ErrorBrush">Red</SolidColorBrush>
<SolidColorBrush x:Key="ErrorBrushLight">#10ff0000</SolidColorBrush>

<sys:Double x:Key="ThemeBorderThickness">2</sys:Double>
<avalonia:Thickness x:Key="ThemeBorderThickness">2</avalonia:Thickness>

This comment has been minimized.

Copy link
@grokys

grokys Mar 23, 2018

Member

Thickness should be picked up without a namespace. To do this you need to add an XmlnsDefinition attribute to the Avalonia.Visuals assembly with the plain Avalonia namespace. There are already a couple of these definitions: https://github.com/AvaloniaUI/Avalonia/blob/master/src/Avalonia.Visuals/Properties/AssemblyInfo.cs#L11


public static bool operator ==(CornerRadius cr1, CornerRadius cr2)
{
return ((cr1.TopLeft.Equals(cr2.TopLeft) || double.IsNaN(cr1.TopLeft) && double.IsNaN(cr2.TopLeft)))

This comment has been minimized.

Copy link
@grokys

grokys Mar 23, 2018

Member

What does a NaN as a portion of a CornerRadius mean exactly?

This comment has been minimized.

Copy link
@Gillibald

Gillibald Mar 23, 2018

Author Contributor

Good catch. Can't think of a case where we need NaN. I should probably check for NaN and throw an exception. Is NaN allowed for Thickness? Have to check that first.

@@ -26,8 +26,15 @@ public GeometryImpl(Geometry geometry)
/// <inheritdoc/>
public Rect GetRenderBounds(Avalonia.Media.Pen pen)
{
var factory = AvaloniaLocator.Current.GetService<Factory>();
return Geometry.GetWidenedBounds((float)pen.Thickness).ToAvalonia();
//var factory = AvaloniaLocator.Current.GetService<Factory>();

This comment has been minimized.

Copy link
@grokys

grokys Mar 23, 2018

Member

Should probably delete this line rather than commenting it out.

}
else
{
return Geometry.GetWidenedBounds((float)pen.Thickness).ToAvalonia();

This comment has been minimized.

Copy link
@grokys

grokys Mar 23, 2018

Member

Couldn't this block be written more succinctly as: return Geometry.GetWidenedBounds((float)(pen?.Thickness ?? 0)).ToAvalonia();?

This comment has been minimized.

Copy link
@Gillibald

Gillibald Mar 23, 2018

Author Contributor

I usually do that wasn't sure about readability and code style. Will have that in mind for the future.

@Gillibald

This comment has been minimized.

Copy link
Contributor Author

Gillibald commented Mar 23, 2018

Thanks for your feedback will have a look at it soon. Looks like my background geometry needs some tweeks. I probably need to use a percentage of the outer corner radius for the inner. Should be manageable.

@Gillibald

This comment has been minimized.

Copy link
Contributor Author

Gillibald commented Mar 26, 2018

Render tests are still missing. Probably need some help there.

grokys added some commits Apr 3, 2018

@grokys

grokys approved these changes Apr 3, 2018

@grokys

This comment has been minimized.

Copy link
Member

grokys commented Apr 3, 2018

Apologies for the delay reviewing this @Gillibald - looks good to me now! I added a couple of render tests. Once CI has passed I'll get this merged.

@Gillibald

This comment has been minimized.

Copy link
Contributor Author

Gillibald commented Apr 3, 2018

Thanks for your help. Can we check ContentPresenter before merging. Wasn't sure how to test measurements. ArrangeOverrideImpl has some changes I wasn't sure about.

@grokys

This comment has been minimized.

Copy link
Member

grokys commented Apr 3, 2018

Sure! What in particular were you unsure of?

@Gillibald

This comment has been minimized.

Copy link
Contributor Author

Gillibald commented Apr 3, 2018

In ContentPresenter.cs line 375 - 381

Before my changes just the uniform BordeeThickness took part of all calculations. Now I'm am using all 4 sides.

@grokys

This comment has been minimized.

Copy link
Member

grokys commented Apr 3, 2018

Ok, looks like the best thing there would be to compare with UWP's behavior then. I won't get chance to do that this evening, but I will try to take a look tomorrow!

@Gillibald

This comment has been minimized.

Copy link
Contributor Author

Gillibald commented Apr 3, 2018

Maybe I have to spend some time with that part myself. Padding is handled only with Left and Top part. Have to check some ContentPresenter examples and make sure all measurements are fine. Thanks for your help in advance.

Gillibald added some commits Apr 6, 2018

@Gillibald

This comment has been minimized.

Copy link
Contributor Author

Gillibald commented Apr 6, 2018

Looks like everything is fine with ContentPresenter. Couldn't find anything unusual. So in my opinion we are ready to merge.

@danwalmsley

This comment has been minimized.

Copy link
Member

danwalmsley commented Apr 6, 2018

I will give this another quick test on AvalonStudio this evening.

@grokys

This comment has been minimized.

Copy link
Member

grokys commented Apr 8, 2018

@danwalmsley did you give it a test? Are we ok to merge?

@grokys grokys merged commit 3b757cf into AvaloniaUI:master Apr 10, 2018

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@grokys

This comment has been minimized.

Copy link
Member

grokys commented Apr 10, 2018

Thanks @Gillibald ! @danwalmsley has given the 👍 on this in gitter so merged!

@grokys grokys referenced this pull request Apr 14, 2018

Closed

Implement CornerRadius #1385

@Gillibald Gillibald deleted the Gillibald:feature/BorderThicknessCornerRadius branch Apr 28, 2018

@grokys grokys added this to the 0.7.0 milestone Apr 3, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.