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

Make UniformGrid account for layout rounding. #17700

Closed
wants to merge 2 commits into from

Conversation

grokys
Copy link
Member

@grokys grokys commented Dec 5, 2024

What does the pull request do?

As described in #17699, UniformGrid was ported from WPF but Avalonia's layout rounding is slightly different. Update ArrangeOverride to account for this.

Instead of arranging each child without considering whether it was snapped to the pixel grid, layout each each child with it's top-left aligned against the previous column/row, with the bottom-right placed in the ideal position. This ensures that there are no overlapping/spaces between controls, and also ensures that the overall uniform grid is preserved.

Fixed issues

Fixes #17699

When layout rounding is available, children may be laid out with a slightly different position/size than requested. Layout each each child with it's top-left aligned against the previous column/row bounds and request the bottom-right to be placed in the ideal position.
@grokys grokys added bug customer-priority Issue reported by a customer with a support agreement. backport-candidate-11.2.x Consider this PR for backporting to 11.2 branch labels Dec 5, 2024
@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.3.999-cibuild0053723-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

Copy link
Member

@MrJul MrJul left a comment

Choose a reason for hiding this comment

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

I don't think we can use the bounds from the children at all. The UniformGrid should constrain its children, not the reverse.

An alternative fix might be to simply apply the layout rounding to the cell size. Conceptually, it's not really different from having a panel for each cell, which should work "naturally".

// requested. Layout each each child with it's top-left aligned against the
// previous column/row bounds and request the bottom-right to be placed in
// the ideal position.
var topLeft = new Point(x, y);
Copy link
Member

Choose a reason for hiding this comment

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

We can't rely on the previous child's bounds here: it might effectively be completely different (try any child with non-stretch alignment) from the cell's size, breaking the uniformity.

Example:

<UniformGrid Columns="2">
  <Border Width="100" Height="100" Background="Blue" />
  <Border Width="100" Height="100" Background="Gray" />
  <Border Width="50" Height="100" Background="Blue" HorizontalAlignment="Left" />
  <Border Width="50" Height="100" Background="Gray" HorizontalAlignment="Left" />
</UniformGrid>

It might even be out of bounds of the UniformGrid (if the children are larger than their container), causing an invalid rectangle to be passed to Arrange, and an InvalidOperationException.

Example:

<UniformGrid Width="50" Height="50">
  <Border Width="100" Height="100" />
  <Border Width="100" Height="100" />
</UniformGrid>

@grokys
Copy link
Member Author

grokys commented Dec 6, 2024

An alternative fix might be to simply apply the layout rounding to the cell size. Conceptually, it's not really different from having a panel for each cell, which should work "naturally".

My first attempt started out doing this, but I was finding that you wound up in situations where the children didn't fill the panel completely, or went outside the panel bounds. However thinking more about it, it might because i was using LayoutHelper.RoundLayoutSizeUp. It may work better if we add a LayoutHelper.RoundLayoutSize which rounds to the nearest pixel (like WPF).

We can't rely on the previous child's bounds here: it might effectively be completely different (try any child with non-stretch alignment) from the cell's size, breaking the uniformity.

Good catch! Yes I should have thought of this ;) We need more unit tests for this control obviously.

@grokys
Copy link
Member Author

grokys commented Dec 6, 2024

One thing we need to decide: when snapping to pixels, there are situations where we have to choose between having the children not completely fill the panel, or having the child sizes not be completely uniform. Need to confirm what WPF does (from looking at the code, I think it will sacrifice uniformity but need to check this in practise).

@MrJul
Copy link
Member

MrJul commented Dec 6, 2024

It may work better if we add a LayoutHelper.RoundLayoutSize which rounds to the nearest pixel (like WPF).

I think we should keep RoundLayoutSizeUp(), because that's what every control in Avalonia uses, and we need to be consistent.

Imagine that instead of having a UniformGrid responsible for layouting the controls, we're generating the matching cells instead (with each cell being its own container) and stacking them using a standard StackPanel.

In that case, the cells would naturally "compose" without overlap, because they're already using RoundLayoutSizeUp in MeasureCore/ArrangeCore.

With code, I expect the following two panels to have the exact same layout at any DPI size:

<StackPanel Orientation="Horizontal" Width="99" Height="100" Background="Black">
  <Border Width="33" Background="Red" />
  <Border Width="33" Background="Green" />
  <Border Width="33" Background="Blue" />
</StackPanel>

<UniformGrid Width="99" Height="100" Columns="3" Background="Black">
  <Border Background="Red" />
  <Border Background="Green" />
  <Border Background="Blue" />
</UniformGrid>

That won't work if we apply a different algorithm just for UniformGrid. Note that in this case, the combined width of the 3 borders (100.8px) gets larger than the width of the StackPanel (99.2px) at 125% DPI.

@grokys
Copy link
Member Author

grokys commented Dec 9, 2024

I tested your two examples on WPF with 125% scaling, modifying the code to make the panel width 100 (i.e. not divisible by 3) and to also make clear the child bounds in relation to the parent:

<Window x:Class="WpfApp1.MainWindow"
        xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
        xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
        Title="MainWindow" Height="450" Width="800"
        UseLayoutRounding="True">
  <Grid>
    <Grid.ColumnDefinitions>
      <ColumnDefinition Width="*"/>
      <ColumnDefinition Width="*"/>
    </Grid.ColumnDefinitions>

    <Border Background="Beige" Padding="0 4" HorizontalAlignment="Center" VerticalAlignment="Center">
      <StackPanel x:Name="stack" Orientation="Horizontal" Width="100" Height="100">
        <Border Width="33" BorderBrush="Red" BorderThickness="1"/>
        <Border Width="33" BorderBrush="Green" BorderThickness="1"/>
        <Border Width="33" BorderBrush="Blue" BorderThickness="1"/>
      </StackPanel>
    </Border>

    <Border Background="Beige" Padding="0 4" Grid.Column="1" HorizontalAlignment="Center" VerticalAlignment="Center">
      <UniformGrid Name="uniform" Width="100" Height="100" Columns="3">
        <Border BorderBrush="Red" BorderThickness="1"/>
        <Border BorderBrush="Green" BorderThickness="1"/>
        <Border BorderBrush="Blue" BorderThickness="1"/>
      </UniformGrid>
    </Border>
  </Grid>
  
</Window>

We get the following results

StackPanel

0,0,32.8,100
32.8,0,32.8,100
65.6,0,32.8,100

image

UniformGrid

0,0,33.6,100
33.6,0,33.6,100
66.4,0,33.6,100

image

And look what happens. The UniformGrid has exactly the same overlapping problem as Avalonia! (the right-most green border is obsured).

@grokys
Copy link
Member Author

grokys commented Dec 9, 2024

The docs for the WinUI UniformGrid say (highlighting added):

The UniformGrid control is a responsive layout control which arranges items in a evenly-spaced set of rows or columns to fill the total available display space. Each cell in the grid, by default, will be the same size.

So the two constraints for its behavior is that

  1. The children fill the panel
  2. The children are the same size

Obviously, these two requirements are in conflict when the pixel size of the panel is not divisible by the number of children. In this case there are two possible ways out of the conundrum:

  1. Don't snap to pixels
  2. Overlap the children

When UseLayoutRounding == true then 1) is not an option, which leaves us with 2), which is our (and WPF's, and I assume WinUI's) current behavior.

So I'm tempted to say that the control is working as intended and this PR should be closed.

@grokys
Copy link
Member Author

grokys commented Dec 17, 2024

Closing as the current behavior matches WPF.

@grokys grokys closed this Dec 17, 2024
@maxkatz6 maxkatz6 removed customer-priority Issue reported by a customer with a support agreement. backport-candidate-11.2.x Consider this PR for backporting to 11.2 branch labels Dec 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UniformGrid layout rounding is incorrect
4 participants