search

ppy / osu

rhythm is just a *click* away!
https://osu.ppy.sh
MIT License
14.64k stars 2.16k forks source link

Improve performance of editor tables #28613

Closed bdach closed 2 days ago

bdach commented 3 days ago

master:

https://github.com/ppy/osu/assets/20418176/f61b7927-426a-49a1-993f-1955eeafff22

this PR:

https://github.com/ppy/osu/assets/20418176/c86a4298-1a49-491f-9e4c-9be79e374076

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 https://github.com/ppy/osu/issues/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 commented 2 days ago

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)
bdach commented 2 days ago

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.