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

A new grid layout algorithm to improve performance and fix some bugs #1517

Merged
merged 34 commits into from May 14, 2018

Conversation

Projects
None yet
5 participants
@walterlv
Contributor

walterlv commented Apr 22, 2018

New algorithm

  • Fix the bugs, see issue #1516
  • Easier to read (Most methods are Pure, which means it doesn't modify states.)
  • Better performance (See the performace test below.)
  • The whole layout algorithm is totally different from the existed ones (such as the WPF one).

Performance test

My algorithmic time complexity is O(n), which means I enumerate the columns and rows separately only several times. Notice that the children count is different for each test case, so the elapsed difference between these test cases does not directly reflect the elapsed difference required by the algorithm.

Create 1 grid then measure and arrange children. Each cell has a TextBlock.

Test case Original Grid(ms) SimpleGrid(ms) WPF Grid(ms) New Grid(ms)
300 × 3 199040 55 140 102
300 × 300 200384 3489 11724 6370
3000 × 3 Too Long (> 180 min) 336 1141 1602

Create 10,000 grid then measure and arrange children.

Test case Original elapsed(s) New elapsed(s)
0 rows and 0 columns 0.5049 0.3067
3 rows and 4 columns 1.4546 1.4930

Remarks

Unit tests coverage

image

image


Attached code

The performance test code:

var grid = new Grid();
for (var i = 0; i < 3000; i++) grid.RowDefinitions.Add(new RowDefinition(0.0, GridUnitType.Auto));
for (var j = 0; j < 3; j++) grid.ColumnDefinitions.Add(new ColumnDefinition(0.0, GridUnitType.Auto));
for (var i = 0; i < 3000; i++) for (var j = 0; j < 3; j++) grid.Children.Add(new TextBlock { Text = $"{i},{i}", [Grid.RowProperty] = i, [Grid.ColumnProperty] = j });
grid.Measure(Size.Infinity);
grid.Arrange(new Rect(grid.DesiredSize));
@jkoritzinsky

This looks great! Any chance you can add a unit test for the fix? Other than that, it LGTM!

@walterlv

This comment has been minimized.

Contributor

walterlv commented Apr 26, 2018

@jkoritzinsky I can't build the solution successfully, could you please give me a suggestion? (The way I fix this issue is to copy Grid code into my own project.)

The first error is in Avalonia.Markup.Xaml and prints Converters\SetterValueTypeConverter.cs(8,21,8,27): error CS0234: The type or namespace name 'Markup' does not exist in the namespace 'Portable.Xaml' (are you missing an assembly reference?).

image

@jkoritzinsky

This comment has been minimized.

Member

jkoritzinsky commented Apr 26, 2018

Did you restore submodules when you cloned?

@walterlv

This comment has been minimized.

Contributor

walterlv commented Apr 26, 2018

@jkoritzinsky Sorry I didn't notice that there were submodules here.

And now I succeed. Thanks! I'll write unit test code.

@walterlv

This comment has been minimized.

Contributor

walterlv commented Apr 28, 2018

When I finished writing some unit tests for Grid layout, I found that the MaxWidth/MaxHeight didn't work correctly.

@jkoritzinsky I have to fix that...and this is my test code.


Edit: When I try to debug the reason why the unit test fails, I find that the Grid layout calculates inefficiently. I feel that the MaxValue working correctly is just a coincidence.


// Arrange & Action
var rowGrid = GridMock.New(new RowDefinitions
{
    new RowDefinition(1, GridUnitType.Star) { MaxHeight = 200 },
    new RowDefinition(1, GridUnitType.Star),
    new RowDefinition(1, GridUnitType.Star),
}, arrange: 800);
var columnGrid = GridMock.New(new ColumnDefinitions
{
    new ColumnDefinition(1, GridUnitType.Star) { MaxWidth = 200 },
    new ColumnDefinition(1, GridUnitType.Star),
    new ColumnDefinition(1, GridUnitType.Star),
}, arrange: 800);

// Assert
GridAssert.ChildrenHeight(rowGrid, 200, 300, 300);
GridAssert.ChildrenWidth(columnGrid, 200, 300, 300);
@lindexi

This comment has been minimized.

Contributor

lindexi commented Apr 28, 2018

Thx

@walterlv

This comment has been minimized.

Contributor

walterlv commented Apr 28, 2018

I'm writing a new version of Grid layout and it's performance may be better than the original one.

image

@walterlv walterlv changed the title from Grid.RowDefinitions now supports RowDefinition.MinHeight, so do ColumnDefinitions. to WIP: Grid.RowDefinitions now supports RowDefinition.MinHeight, so do ColumnDefinitions. Apr 28, 2018

@walterlv walterlv changed the title from WIP: Grid.RowDefinitions now supports RowDefinition.MinHeight, so do ColumnDefinitions. to WIP: Write a new grid layout algorithm to fix some layout bugs Apr 30, 2018

@walterlv walterlv changed the title from WIP: Write a new grid layout algorithm to fix some layout bugs to Write a new grid layout algorithm to fix some layout bugs May 1, 2018

@walterlv

This comment has been minimized.

Contributor

walterlv commented May 1, 2018

@jkoritzinsky @danwalmsley It seems that Avalonia's layout system is different from the WPF one.

I arrange a child in Rect(0,0,100,100) but the child bounds is (-25,0,150,100).

This is my UI test code:

<Grid Grid.Row="1" ColumnDefinitions="100,*,2*">
  <Border Grid.Column="0" Background="CornflowerBlue" Width="150" />
  <Border Grid.Column="1" Background="Tomato" Width="150" />
  <Border Grid.Column="2" Background="Teal" Width="150" />
</Grid>

This my unit test code:

[Fact]
public void Test()
{
    // Arrange
    var grid = new Grid();
    grid.ColumnDefinitions = new ColumnDefinitions("100,*,2*");
    grid.Children.Add(new Border { Width = 150, [Grid.ColumnProperty] = 0 });
    grid.Children.Add(new Border { Width = 150, [Grid.ColumnProperty] = 1 });
    grid.Children.Add(new Border { Width = 150, [Grid.ColumnProperty] = 2 });

    // Action
    grid.Measure(new Size(205, 100));
    grid.Arrange(new Rect(0, 0, 205, 100));

    // Assert
    GridAssert.ChildrenWidth(grid, 100, 35, 70);
}

image

And this is the reason (See Avalonia/Layoutable.cs#586):

image

image
▲ It seems that when the child is larger than the grid column, it will render out of the column limitation.

@grokys

This comment has been minimized.

Member

grokys commented May 3, 2018

Just taken a first look at this, but the Calendar layout in the Control Catalog seems to be broken:

image

Is this a known issue?

@grokys

This comment has been minimized.

Member

grokys commented May 3, 2018

Also tree view items look wrong:

image

Their vertical layout doesn't seem to be quite right.

@grokys

This comment has been minimized.

Member

grokys commented May 3, 2018

However, enough of the negativity! Previously the DevTools had to use its own custom SimpleGrid because the Grid control was too slow. I just tried replacing the SimpleGrid with this new Grid and it's fast! :)

@walterlv walterlv changed the title from Write a new grid layout algorithm to fix some layout bugs to A new grid layout algorithm to improve performance and fix some bugs May 4, 2018

@walterlv

This comment has been minimized.

Contributor

walterlv commented May 4, 2018

HELP HELP HELP HELP HELP

I find that this situation is very hard to understand. How does the WPF Grid calculate this result?

See my post for more detials: The undefined behaviors of WPF Grid (the so-called bugs) - walterlv.

image

By solving this situation, my Grid will make the Calendar laying out well. Otherwise it will make the Calendar like this:

image


POST After talking with my friend @lindexi , we agree that all Auto columns with multi-span children behaviors strange. VERY STRANGE! and CAN'T UNDERSTAND!

@grokys

This comment has been minimized.

Member

grokys commented May 5, 2018

@walterlv very interesting! Unfortunately i don't know the answer - I suspect you know more about grid layout than almost anyone at this point!

I say: make our grid work in the way that most makes sense. If we have to change the layout of the grid in the Calendar control to make it work with the new grid, i'm fine with that.

I notice that the problem with the TreeView is still there. Is this caused by the same problem?

@walterlv walterlv changed the title from A new grid layout algorithm to improve performance and fix some bugs to WIP: A new grid layout algorithm to improve performance and fix some bugs May 6, 2018

@walterlv walterlv changed the title from WIP: A new grid layout algorithm to improve performance and fix some bugs to A new grid layout algorithm to improve performance and fix some bugs May 6, 2018

@grokys

This is amazing stuff @walterlv! First of all I've gone through and tried to fix some bits of grammar in the comments. The comments really help make this code understandable (you've done a great job there) so the more readable the better! I Hope you don't take this as criticism, I know English isn't your first language.

I'm amazed at how much faster this Grid is than our current Grid! I can't wait to get this in. I notice there are quite a few uses of LINQ in the code, removing these could also help speed things up even more, but lets leave that for later as a separate optimization.

@@ -183,300 +182,94 @@ public static void SetRowSpan(AvaloniaObject element, int value)
element.SetValue(RowSpanProperty, value);
}
/// <summary>
/// Gets the result of last column measuring procedure.

This comment has been minimized.

@grokys

grokys May 6, 2018

Member

"Gets the result of the last column measurement."

private GridLayout.MeasureResult _columnMeasureCache;
/// <summary>
/// Gets the result of last row measuring procedure.

This comment has been minimized.

@grokys

grokys May 6, 2018

Member

"Gets the result of the last row measurement."

CreateMatrices(rowCount, colCount);
// Situation 1/2:
// If the grid doesn't have any column/row definitions,
// it behaviors like a nomal panel.

This comment has been minimized.

@grokys

grokys May 6, 2018

Member

"If the grid doesn't have any column/row definitions, it behaves like a normal panel."

// Situation 2/2:
// If the grid defines some columns or rows.
// Debug Tip:
// - GridLayout doesn't hold any states, so you can drag the debugger execution

This comment has been minimized.

@grokys

grokys May 6, 2018

Member

"GridLayout doesn't hold any state"

// Pre-process the grid children so that we know what types of elements we have so
// we can apply our special measuring rules.
GridWalker gridWalker = new GridWalker(this, _rowMatrix, _colMatrix);
// Calculate for measuring result.

This comment has been minimized.

@grokys

grokys May 6, 2018

Member

"Calculate measurement"? I think this is what you mean?

This comment has been minimized.

@walterlv

walterlv May 6, 2018

Contributor

So why the arrange grammar is different to this one?

/// This method implement the last procedure (M7/7) of measure.
/// It expand all the * length to the fixed length according to the <paramref name="constraint"/>.
/// </summary>
/// <param name="conventions">All the conventions that have almost been fixed except the rest *.</param>

This comment has been minimized.

@grokys

grokys May 6, 2018

Member

"rest *" -> "remaining *"?

/// We should clip the columns/rows that have been out of the container bounds.
/// Note: This method may change the items value of <paramref name="lengthList"/>.
/// </summary>
/// <param name="lengthList">All the column width list or the row height list with fixed pixel length.</param>

This comment has been minimized.

@grokys

grokys May 6, 2018

Member

Does this mean "A list of all the column widths and row heights with a fixed pixel length"?

/// <summary>
/// Fix the <see cref="LengthConvention"/>. If all columns/rows are fixed,
/// we can get the double pixel list of all columns/row.

This comment has been minimized.

@grokys

grokys May 6, 2018

Member

What is a "double pixel list"?

This comment has been minimized.

@walterlv

walterlv May 6, 2018

Contributor

System.Double

This comment has been minimized.

@grokys

grokys May 6, 2018

Member

Ahhh. I took it to mean "two pixel lists" ;) Maybe it would be clearer as:

"If all columns/rows are fixed, we can get the size of all columns/rows in pixels"

/// <summary>
/// Contains the convention that comes from the grid children.
/// Some child span multiple columns or rows, so even a simple column/row can have multiple conventions.

This comment has been minimized.

@grokys

grokys May 6, 2018

Member

"Some children"

/// <summary>
/// Gets the container length for this result.
/// This property exists because a measure result is related to it.

This comment has been minimized.

@grokys

grokys May 6, 2018

Member

"This property exists because a measure result is related to it." -- what does this mean?

This comment has been minimized.

@walterlv

walterlv May 6, 2018

Contributor

This property indicates the return value of a Grid.MeasureOverride method.

@danwalmsley

This comment has been minimized.

Member

danwalmsley commented May 6, 2018

Just tested with AvalonStudio, this is the result:
image

  • Some strange spacing at the top of the window

  • Spacing in the treeview

  • Also the caret in my editor control isn't the same size now,

I will break down each of these into a simple repro where possible to help you identify the issues, but I'm also happy if you say I just need to change my markup a little.

@danwalmsley

This comment has been minimized.

Member

danwalmsley commented May 6, 2018

Its also stopped my selected Tab being highlighted blue see:
image

@danwalmsley

This comment has been minimized.

Member

danwalmsley commented May 6, 2018

ignore my last 2 comments,seems to be related to some other change on master (apart from the treeview one)

walterlv added some commits Apr 22, 2018

If the MinHeight/MinWidth is set and it affects the Grid layout, a ne…
…w calculation will be triggered. (Not only the MaxHeight/MaxWidth)
Add unit test for grid layout:
1. Pixel row column
1. Start row column
1. Mix pixel star row column
Add unit test for grid layout:
- start row column with min length.
@danwalmsley

This comment has been minimized.

Member

danwalmsley commented May 10, 2018

@walterlv ok so iv managed to isolate the issues in master that made it difficult to see what this PR had done or not done.

image

This is the only remaining layout issue iv found. If we can investigate what the cause is, and perhaps decide if it needs fixing or not, after that I will be happy to approve the PR so it can be merged. Contact me on gitter to discuss this further :)

@danwalmsley

This comment has been minimized.

Member

danwalmsley commented May 14, 2018

@walterlv ok I'm happy with this PR now, I found that the current Grid implementation cant render this correctly:

<Grid Height="16">
        <Grid.RowDefinitions>
            <RowDefinition Height="*" />
            <RowDefinition Height="*" />
        </Grid.RowDefinitions>
        <StackPanel Orientation="Horizontal">
            <Grid Background="Red" Height="16" Width="16" />
            <TextBlock Text="Test" />
        </StackPanel>
    </Grid>

so I'm now happy this grid layout is ready to merge.

@danwalmsley

LGTM, tested on AvalonStudio seems that this fixes a few bugs the old layout had :)

@grokys

grokys approved these changes May 14, 2018

@danwalmsley danwalmsley merged commit 0e1d494 into AvaloniaUI:master May 14, 2018

2 checks passed

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

This comment has been minimized.

Member

grokys commented May 14, 2018

Thanks so much @walterlv ! This will improve things greatly.

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