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 fixes for Margin, Padding and Thickness properties with UseLayo… #7867

Merged
merged 8 commits into from
Apr 21, 2022

Conversation

AndrejBunjac
Copy link
Contributor

@AndrejBunjac AndrejBunjac commented Mar 24, 2022

Added fixes to resolve issues I raised in the issue #7798.

Even though Avalonia supports UseLayoutRounding - the pixel perfect positions were incorrectly calculated when scaling was present with objects that use Margins, Padding and Borders.

Margin was previously included in the calculation but the rounding was done after doing "availableSize = size - margin" and since rounding function is not linear the relation F(a) + F(b) == F(a + b) does not hold. That means that margin rounding to pixel perfect positions sometimes resulted in one pixel offset (visible in sample images).

The same treatment was never given to Padding and BorderThickness so I included those.

The goal of this fix is to support this layout rounding for all current and future users and I'm willing to make further fixes if needed.

List of Changes:
Layoutable.ArrangeCore - added separate rounding for Margin before any calculation.

LayoutHelper - added RoundLayoutThickness method
LayoutHelper.ArrangeChild - included a calculation of the rounding for padding and thickness if the parent uses LayoutRounding.
Border and ContentPresenter - added LayoutThickness private property that's passed to the renderer instead of BorderThickness. The LayoutThickness is calculated once and then cached. It makes sure that thickness will always be pixel perfect with UseLayoutRounding rather than having blurry edges in higher scales (to avoid borders looking blurry in 4k for example).

@dnfadmin
Copy link

dnfadmin commented Mar 24, 2022

CLA assistant check
All CLA requirements met.

@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 0.10.999-cibuild0019487-beta. (feed url: https://nuget.avaloniaui.net/repository/avalonia-all/index.json) [PRBUILDID]

@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 0.10.999-cibuild0019488-beta. (feed url: https://nuget.avaloniaui.net/repository/avalonia-all/index.json) [PRBUILDID]

@jmacato
Copy link
Member

jmacato commented Apr 4, 2022

@AndrejBunjac this looks great! I wonder if we can have any tests to check this on CI?

@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 0.10.999-cibuild0019609-beta. (feed url: https://nuget.avaloniaui.net/repository/avalonia-all/index.json) [PRBUILDID]

@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 0.10.999-cibuild0019659-beta. (feed url: https://nuget.avaloniaui.net/repository/avalonia-all/index.json) [PRBUILDID]

@AndrejBunjac
Copy link
Contributor Author

Also to reply to a comment from @jmacato. I would most likely want to add a ScreenShot test but I would need a way to be able to force the scaling to certain values for the test to make sense. I don't think that other type of test would make sense as this is a very visual issue visible only on a scaling different from 1.

Maybe I can do something with the TestRoot but I have to think about it.

@AndrejBunjac AndrejBunjac requested a review from maxkatz6 April 19, 2022 14:24
@grokys
Copy link
Member

grokys commented Apr 21, 2022

I would most likely want to add a ScreenShot test but I would need a way to be able to force the scaling to certain values for the test to make sense. I don't think that other type of test would make sense as this is a very visual issue visible only on a scaling different from 1.

Agreed, and the problem with render tests is that they either have to be pixel perfect, or have to do a fuzzy match. The pixel perfect checks can fail on different machines, and the fuzzy tests aren't going to pick up such issues, so I'm fine with leaving this without render tests.

@grokys
Copy link
Member

grokys commented Apr 21, 2022

@AndrejBunjac hope you don't mind - I updated and fixed a merge error and made a small change too (moved some fields to the top of the class in Border).

Copy link
Member

@grokys grokys left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! I've confirmed the problem on master and this seems to fix it well, and all the code looks good to me.

@grokys grokys enabled auto-merge April 21, 2022 14:32
@grokys grokys merged commit f3324dc into AvaloniaUI:master Apr 21, 2022
@AndrejBunjac
Copy link
Contributor Author

Great! Looking forward to seeing the changes in action!
@grokys Of course I don't mind, you are the maintainer of Avalonia after all. :)
When can we expect these changes in a stable release?
(maybe it's a dumb question but I haven't picked up how you release new versions)

@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 0.10.999-cibuild0020087-beta. (feed url: https://nuget.avaloniaui.net/repository/avalonia-all/index.json) [PRBUILDID]

@grokys
Copy link
Member

grokys commented May 6, 2022

Hmm this seems to have caused a problem with context menus and 150% DPI scaling:

image

Investigating.

@grokys
Copy link
Member

grokys commented May 6, 2022

When can we expect these changes in a stable release?

Probably for the release after next, 0.10.15 - I wanted to leave some time to get some testing, and indeed we've found an issue as reported above ;)

@AndrejBunjac
Copy link
Contributor Author

Hmmmm, I will also try to figure it out in parallel with you guys as soon as I catch a breather from my regular work.

@AndrejBunjac
Copy link
Contributor Author

If I had to bet on something - I think the size of the popup might not be updated correctly with these changes. I'll look into it.

@grokys
Copy link
Member

grokys commented May 6, 2022

Thanks @AndrejBunjac - I've opened an issue with my initial findings here #8092

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants