Refactor SearchContainer filtering operation and fix potential flaws

[frenzibyte]

• Closes Search container is flawed in multiple ways osu#18003

I've initially intended to look at just adding guards against non-present drawables, but the current logic for filtering out drawables was quite convoluted that I looked at first refactoring it with flexibility in mind, then adding the necessary guards to solve the issues pointed out in ppy/osu#18003.

blocker resolved

[bdach]

So the source looks sane more or less, but what worries me a touch is the fact that - as far as I can tell - the search container's filtering process will now always traverse its full subtree when looking for children to filter. Previously IHasFilterableChildren served as an optimisation of sorts in this respect, because the search process would only descend into composites that were marked as such while culling others that didn't fit, which is no longer the case.

So in general I'd like to see how much slower (if any slower at all?) it makes filtering work in the game-side settings sidebar. Yes the results are cached and invalidated rarely, but I still want to see what I'm dealing with here exactly in that respect, as the settings sidebar has quite a few things in it already.

[frenzibyte]

Not sure how valid this is, but I've measured the time spent in SearchContainer.performLayout (in release configuration) and this is the output:

# master
2022-05-05 04:25:48 [verbose]: Performing layout
2022-05-05 04:25:48 [verbose]: Finished layout: 0.37120799999956944ms, processed 43 containers, and 144 drawables in total.
2022-05-05 04:25:48 [verbose]: Performing layout
2022-05-05 04:25:48 [verbose]: Finished layout: 0.36929200000031415ms, processed 43 containers, and 144 drawables in total.
2022-05-05 04:25:48 [verbose]: Performing layout
2022-05-05 04:25:48 [verbose]: Finished layout: 0.39216600000054314ms, processed 43 containers, and 144 drawables in total.

# PR
2022-05-05 04:20:48 [verbose]: Performing layout
2022-05-05 04:20:48 [verbose]: Finished layout: 0.40475000000014916ms, processed 317 containers, and 792 drawables in total.
2022-05-05 04:20:48 [verbose]: Performing layout
2022-05-05 04:20:48 [verbose]: Finished layout: 0.38362499999993815ms, processed 317 containers, and 792 drawables in total.
2022-05-05 04:20:48 [verbose]: Performing layout
2022-05-05 04:20:48 [verbose]: Finished layout: 0.5557079999998678ms, processed 317 containers, and 792 drawables in total.

master diff

diff --git a/osu.Framework/Graphics/Containers/SearchContainer.cs b/osu.Framework/Graphics/Containers/SearchContainer.cs
index 789d92e88..ba5bbd877 100644
--- a/osu.Framework/Graphics/Containers/SearchContainer.cs
+++ b/osu.Framework/Graphics/Containers/SearchContainer.cs
@@ -7,6 +7,7 @@
 using System.Linq;
 using osu.Framework.Caching;
 using osu.Framework.Extensions.IEnumerableExtensions;
+using osu.Framework.Timing;
 
 namespace osu.Framework.Graphics.Containers
 {
@@ -84,10 +85,22 @@ protected override void Update()
             }
         }
 
+        private static readonly StopwatchClock perf_clock = new StopwatchClock(true);
+        private static int container_count = 0;
+        private static int count = 0;
+
         private void performFilter()
         {
+            Logging.Logger.Log("Performing layout");
+            double timeBefore = perf_clock.CurrentTime;
+
+            container_count = 0;
+            count = 0;
+
             string[] terms = (searchTerm ?? string.Empty).Split(new[] { ' ' }, StringSplitOptions.RemoveEmptyEntries);
             Children.OfType<IFilterable>().ForEach(child => match(child, terms, terms.Length > 0, allowNonContiguousMatching));
+
+            Logging.Logger.Log($"Finished layout: {perf_clock.CurrentTime - timeBefore}ms, processed {container_count} containers, and {count} drawables in total.");
         }
 
         private static bool match(IFilterable filterable, IEnumerable<string> searchTerms, bool searchActive, bool nonContiguousMatching)
@@ -97,11 +110,14 @@ private static bool match(IFilterable filterable, IEnumerable<string> searchTerm
                 !filterable.FilterTerms.Any(filterTerm =>
                     checkTerm(filterTerm, term, nonContiguousMatching))).ToArray();
 
+            count++;
             bool matching = childTerms.Length == 0;
 
             //We need to check the children and should any child match this matches as well
             if (filterable is IHasFilterableChildren hasFilterableChildren)
             {
+                container_count++;
+
                 foreach (IFilterable child in hasFilterableChildren.FilterableChildren)
                     matching |= match(child, childTerms, searchActive, nonContiguousMatching);
             }

PR diff

diff --git a/osu.Framework/Graphics/Containers/SearchContainer.cs b/osu.Framework/Graphics/Containers/SearchContainer.cs
index 74018aca2..8353e61b2 100644
--- a/osu.Framework/Graphics/Containers/SearchContainer.cs
+++ b/osu.Framework/Graphics/Containers/SearchContainer.cs
@@ -6,6 +6,7 @@
 using System.Globalization;
 using System.Linq;
 using osu.Framework.Caching;
+using osu.Framework.Timing;
 
 namespace osu.Framework.Graphics.Containers
 {
@@ -83,10 +84,22 @@ protected override void Update()
             }
         }
 
+        private static readonly StopwatchClock perf_clock = new StopwatchClock(true);
+        private static int container_count = 0;
+        private static int count = 0;
+
         private void performFilter()
         {
+            Logging.Logger.Log("Performing layout");
+            double timeBefore = perf_clock.CurrentTime;
+
+            container_count = 0;
+            count = 0;
+
             string[] terms = (searchTerm ?? string.Empty).Split(new[] { ' ' }, StringSplitOptions.RemoveEmptyEntries);
             matchSubTree(this, terms, terms.Length > 0, allowNonContiguousMatching);
+
+            Logging.Logger.Log($"Finished layout: {perf_clock.CurrentTime - timeBefore}ms, processed {container_count} containers, and {count} drawables in total.");
         }
 
         /// <summary>
@@ -102,6 +115,8 @@ private static bool matchSubTree(Drawable drawable, IReadOnlyList<string> search
 
             if (drawable.IsPresent && drawable is IContainerEnumerable<Drawable> container)
             {
+                container_count++;
+
                 foreach (var child in container.Children)
                     matching |= matchSubTree(child, nonMatchingTerms, searchActive, nonContiguousMatching);
             }
@@ -124,6 +139,7 @@ private static bool matchSubTree(Drawable drawable, IReadOnlyList<string> search
         /// <param name="nonMatchingTerms">The search terms which do not match the specified drawable.</param>
         private static bool match(Drawable drawable, IReadOnlyList<string> searchTerms, bool nonContiguousMatching, out IReadOnlyList<string> nonMatchingTerms)
         {
+            count++;
             nonMatchingTerms = searchTerms;
 
             if (drawable is IFilterable filterable)

Unsure how to write a proper benchmark that shows the speed of this new implementation though, since the prior relies on IHasFilterableChildren, and that's deprecated now in this PR.

[peppy]

Just a thought: Would making MatchingFilter a get;set; property and relying on that instead of IsPresent not clean things up? I'm still not fully on board with this PR due to the IsPresent dependency..

ie. for any containers osu-side which are hiding groups of settings, they would need to implement IFilterable and return false to MatchingFilter if they were hidden, or something like that..

[peppy]

Another alternative would be to require the use of VisibilityContainer for any "hiding" containers, and explicitly mention that in the SearchContainer usage instructions. Then we could rely on VisibilityContainer.State instead of IsPresent for the traversal checks.

[frenzibyte]

ie. for any containers osu-side which are hiding groups of settings, they would need to implement IFilterable and return false to MatchingFilter if they were hidden, or something like that..

Not sure how to feel about implementing IFilterable for a component that isn't even filterable. I personally think the IsPresent method makes sense since if a container is not present, we should automatically ignore it and filter it out. I don't know---

Another alternative would be to require the use of VisibilityContainer for any "hiding" containers, and explicitly mention that in the SearchContainer usage instructions. Then we could rely on VisibilityContainer.State instead of IsPresent for the traversal checks.

Hmm... maybe that could work, but I'm not entirely sure how valid that is, since it means we'll have to enforce using VisibilityContainer anywhere we want to hide something for one reason or another.

Note that the IsPresent check is also done for settings which hide themselves (i.e. resolution dropdown). Would you be fine with wrapping those cases in a visibility container? VisibilityContainer is abstract, so we'll have to locally implement that as well. Maybe make SettingsItem<T> inherit from VisibilityContainer instead?

[peppy]

I'd be okay with wrapping resolutionDropdown, yeah.

[frenzibyte]

Looks to be feasible after trying that. I've gone and refactored the "hide-able" components in game settings to use VisibilityContainer (branch for visibility), and so far I don't see any issues with the new pattern.

I did notice one issue regarding the layout invalidation change I pushed, which is that the search container is actually be invalidated per-frame, destroying the point of a searchCache.

I’m not entirely sure what’s going on, but unless someone beats me to it, I’ll try investigating tomorrow.

[peppy]

I think this documentation should be on the SearchContainer class, mentioning something like:

Summary:
A container which handles filtering of the child hierarchy.

Remarks:
- Children which are searchable should be marked with the `IFilterable` interface. They do not need to be direct children.
- Containers above `IFilterable`s which should be hidden only when all children are filtered away should be marked with the `IHasFilterableChildren` interface.
- Containers which have the potential to hide children via external means should be `VisibilityContainer`s, in order to correctly exclude hidden child `IFilterable`s from search.

[frenzibyte]

As mentioned, containers above IFilterables which should hide based on their children IFilterables will still have to be marked with an IFilterable interface, to allow for cases where some child IFilterables may be nested below others:

 - Root
 -   IFilterable Header
 -     IFilterable 1
 -     IFilterable 2
 -     IFilterable 3
 -     (VisibilityContainer)
 -         IFilterable 4

Which, with an IHasFilterableChildren interface, would require explicitly specifying that in the property (FilterableChildren => filterables.Append(filterableInVisibility)), and that can get worst if the FilterableChildren impleemntation is inherited by a base class (i.e. how we have the settings overlay currently structured).

In addition, with an IHasFilterableChildren interface, it won't be simple to tell whether a filterable is hidden by a parent VisibilityContainer, unless we mark said container with an IFilterable interface and append it to the children list.

[peppy]

Do you not find it weird that you may have containers that define as IFilterable and specify zero filter terms? It feels very counter intuitive.

[frenzibyte]

Is there an existing case of that? I’m going by the concept that no container hide on child filterables without having its own search terms that group them (i.e. a header container).

[peppy]

I guess it's maybe quite uncommon?

[frenzibyte]

I did notice one issue regarding the layout invalidation change I pushed, which is that the search container is actually be invalidated per-frame, destroying the point of a searchCache.

I’m not entirely sure what’s going on, but unless someone beats me to it, I’ll try investigating tomorrow.

Turns out this is happening due to the SearchContainer in test scene having a margin applied, which causes this logic to invalidate the layout per-frame:

osu-framework/osu.Framework/Graphics/Containers/CompositeDrawable.cs

Lines 1844 to 1852 in a379f77

	private Vector2 computeAutoSize()
	{
	MarginPadding originalPadding = Padding;
	MarginPadding originalMargin = Margin;
	try
	{
	Padding = new MarginPadding();
	Margin = new MarginPadding();

The SearchContainer used for game settings also has a margin applied by the surrounding SectionsContainer, and I'm not entirely sure if this is considered a bug. But since it traverses the full container subtree, it may be better to work around it at least.

Have pushed one that ignores the MiscGeometry invalidation, as it should also be redundant in the case of filtering (only DrawSize matters to catch addition of children in the hierarchy).

[frenzibyte]

Actually I'm just going to revert that because it only ended up breaking tests, and I'm not entirely sure what can be done here... going to block for now at least.

[bdach]

What needs to happen for this to be unblocked again (besides resolving the merge conflicts)? I am interested in getting this in as I'm trying and failing to work around multiple SearchContainer idiosyncrasies in order to add a search box to the mod select overlay, and there's a good chance that this diff would help with the issues I'm running into.

[frenzibyte]

If I recall correctly, the filterValid cache gets invalidated per-frame whenever autosize is enabled along with padding/margin as mentioned above.

I was quite lost in the invalidation flow and kept this blocked for now since it makes the cache pointless and potentially result in performance regression (since the settings overlay seems to have autosize + padding/margin).

[bdach]

OK, but are you planning to get back to this at some point or would I need to go try and fix myself? That's what I'm most interested in.

[frenzibyte]

You might know the invalidation flow better than me, but I plan to give this a second attempt this week. So feel free if you want to beat me to it.

[bdach]

After looking at this pull again after a while I think it has a fatal internal contradiction in the special handling of VisibilityContainers. On one side, the most common implementation of MatchingFilter (the one in the visual tests included) will use Show() and Hide() for achieving the visual effects of the filtering. On the other, if you have a Thing : VisibilityContainer, IFilterable, then if you write MatchingFilter using Show() and Hide(), you will end up inadvertently changing Thing.State to Visibility.Hidden and therefore exclude the entire Thing from further search, which this PR is hacking around with this:

osu-framework/osu.Framework/Graphics/Containers/SearchContainer.cs

Lines 139 to 140 in 26c722e

	// reset filter to ensure presence is not influenced by previous filter state when checking.
	filterable.MatchingFilter = true;

which I'm pretty sure is the ultimate cause of the issue described above wherein the search container will invalidate every frame if no item is matching (because it's flapping between the "reset everything to unfiltered" state and the "everything is filtered" state).

I don't know what the fix is yet, but I'd say there is too much in-band data passing to my liking here. Likely we need a separate API to exclude IFilterables from search that does not rely on visual properties like alpha or VisibilityContainer.State. Something like that has already been suggested above.

[peppy]

Superseded by #5530.