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

Improve performance of editor tables #28613

Merged
merged 7 commits into from
Jun 27, 2024
Merged

Conversation

bdach
Copy link
Collaborator

@bdach bdach commented Jun 26, 2024

master:

2024-06-26.10-49-33.mp4

this PR:

2024-06-26.10-56-24.mp4

Map used for testing: https://osu.ppy.sh/beatmapsets/1878528#mania/3895383

Aside from improving performance, this also simultaneously fixes utterly stupid issues like this that can happen on master:

osu_2024-06-26_11-03-35

which are a consequence of the utterly stupid decision to decouple rows from their backgrounds.

The fun part is all of this is a prelude to doing something about #23147. I just couldn't stand looking at the travesty that is EditorTable (begone) while attempting to figure out how to fix that one.

One nice thing about this diff is that it makes the table headers fixed on screen (i.e. they don't scroll away when scrolling the content). Which I think makes infinitely more sense.

@peppy
Copy link
Sponsor Member

peppy commented Jun 26, 2024

Hmm, this isn't looking great:


[runtime] 2024-06-26 14:11:54 [error]: An unobserved error has occurred.
[runtime] 2024-06-26 14:11:54 [error]: osu.Framework.Graphics.Drawable+InvalidThreadForMutationException: Cannot mutate the InternalChildren on a Loaded Drawable while not on the update thread. Consider using Schedule to schedule the mutation operation.
[runtime] 2024-06-26 14:11:54 [error]: at osu.Framework.Graphics.Drawable.EnsureMutationAllowed(String action)
[runtime] 2024-06-26 14:11:54 [error]: at osu.Framework.Graphics.Containers.CompositeDrawable.ClearInternal(Boolean disposeChildren)
[runtime] 2024-06-26 14:11:54 [error]: at osu.Framework.Graphics.Containers.VirtualisedListContainer`2.ItemRow.Unload()
[runtime] 2024-06-26 14:11:54 [error]: at osu.Framework.Graphics.Containers.VirtualisedListContainer`2.ItemRow.Dispose(Boolean isDisposing)
[runtime] 2024-06-26 14:11:54 [error]: at osu.Framework.Graphics.Drawable.Dispose()
[runtime] 2024-06-26 14:11:54 [error]: at osu.Framework.Extensions.IEnumerableExtensions.EnumerableExtensions.ForEach[T](IEnumerable`1 collection, Action`1 action)
[runtime] 2024-06-26 14:11:54 [error]: at osu.Framework.Graphics.Containers.CompositeDrawable.Dispose(Boolean isDisposing)
[runtime] 2024-06-26 14:11:54 [error]: at osu.Framework.Graphics.Drawable.Dispose()
[runtime] 2024-06-26 14:11:54 [error]: at osu.Framework.Extensions.IEnumerableExtensions.EnumerableExtensions.ForEach[T](IEnumerable`1 collection, Action`1 action)

}, true);

table.OnRowSelected += drawable => scroll.ScrollIntoView(drawable);
//table.OnRowSelected += drawable => scroll.ScrollIntoView(drawable);
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

I don't think this is required anymore.

But in testing that this is correctly handled, I did encounter some weirdness with scrolling into view (which prooobably is not a regression of this PR and can be ignored if so):

2024-06-26.23.14.36.mp4

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Turns out hoping that .ScrollIntoView() was going to just-work for this virtualised list thing was too much. From the behaviour I was seeing here it's pretty clear that it just plain doesn't work on rows that haven't been loaded yet, so I replaced it with a local workaround (9384cbc). I'm not super happy about it but still probably worth it for the performance gain?

@bdach
Copy link
Collaborator Author

bdach commented Jun 26, 2024

Hmm, this isn't looking great:

Interesting that this didn't show up framework-side. Probably because I didn't test deparenting thirty levels up like this case seems to be.

I'm not exactly sure what to do with this right now if I'm not allowed to clear a child from this context. I could return it to pool sure but as long as it is in InternalChildren the row wrapper will still try to dispose it.

Will have a think tomorrow.

I did encounter some weirdness with scrolling into view

I thought I saw some at an earlier point of working on this but then couldn't see it anymore. Will check again tomorrow.

@peppy peppy self-requested a review June 27, 2024 08:32
@peppy peppy merged commit 76a1f19 into ppy:master Jun 27, 2024
15 of 17 checks passed
@bdach bdach deleted the control-point-table-is-bad branch June 27, 2024 10:28
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.

2 participants