Shader browser search

[ericmehl]

This adds a search / filter input to the shader browser, used by the ShaderTweaks and ShaderQuery nodes.

Checklist

• I have read the contribution guidelines.
• I have updated the documentation, if applicable.
• I have tested my change(s) in the test suite, and added new test cases where necessary.
• My code follows the Gaffer project's prevailing coding style and conventions.

[ericmehl]

I pushed an update that tweaks some of the early-out checks in _PathMatcherPathFilter, to make them a bit more local to what is being done later in the function. But all the main logic is still the same as the original.

[johnhaddon]

Thanks Eric - comments inline as usual.

This is a pre-existing problem, but I'm finding the generic Filter... label to be a bit nonspecific, with the state about what we're filtering on being hidden inside an unlabelled dropdown menu. Any thoughts on ways of making that more obvious? Perhaps we could change the placeholder text to be Filter by <Name>... so it shows the state from the dropdown menu?

It would also be great if you could include (#5293) in the Changes.md comment, and Fixes #5293 in the commit message so that we're linking and auto-closing the associated ticket.

Cheers...
John

[johnhaddon]

I think it might improve the separation of responsibilities if the filter had a setRootPath() method instead of a setPaths() method, and it did the iteration over paths itself in __updatePathMatcher(). This would also allow the filter to connect to rootPath.changedSignal() to dirty the pathmatcher. Wouldn't make a visible difference in this use case, but gives us a better chance of reusing _PathMatcherPathFilter at some point.

[ericmehl]

That does set us up nicely to factor this out to a reusable _PathMatcherPathFilter some day. That's changed in 02de60f where I think I got the signalling and dirty handling correct...

[ericmehl]

Thanks John! I have the comments addressed, and the change in the placeholder text is in 705fbd0. I believe the only other place where the MatchPatternPathFilterWidget is being used is in the file browser, where that change seems to be working nicely as well.

The last four commits are fixes the changes.md I spotted when rebasing.

I'm not sure why I'm getting the message on Github about the commit not being part of the repository, it's probably related to the rebase I did to insert the Fixed in #5293 that changed the commit hashes from my original comments. But they're synced now, so not sure what the disconnect is.

Read for a new look

[johnhaddon]

Thanks Eric - please could you squash everything down while taking care of the final couple of comments?

[johnhaddon]

This is going to rebuild the paths immediately any time pathChanged() is signalled. Why don't we just set __pathMatcherDirty and iterate the paths lazily in __updatePathMatcher? In other words, why do we even need self.__paths?

[ericmehl]

I think __pathMatcherDirty will be true quite often - with every change to the filter - but the paths will be changed much less often. In the specific case of the shader browser, paths will only be set once and then left alone.

That suggests to me to keep the self.__paths so they can be updated only as needed, and adding a new self.__pathsDirty flag, and call updatePaths() before __updatePathMatcher() in the _filter() method.

How does that sound?

[johnhaddon]

That is a reasonable argument for sure. I guess maybe I'd counter by saying that storing all the Path objects could be quite expensive in the general case, vs just storing the matches in a PathMatcher. And I quite liked the simplicity of removing the additional state. All a bit hypothetical given the current use case, I know. I'll defer to your judgment - certainly if it makes a tangible performance difference to the current use case, then storing __paths is worthwhile.

[ericmehl]

I'll give both a try and see if I can pick up on a performance difference.

[johnhaddon]

Won't a call to setPropertyName( "" ) or setMatchPatterns( [] ) fail to update the path matcher?

[ericmehl]

Right! Fixed in 100525f

[ericmehl]

I squashed down, but still have a WIP commit where I'm trying to update the paths lazily. What I have in a974d53 is what I thought should work, but when I enter something valid in the filter all of the paths are removed.

It seems like __rootPathChanged() may be called at a time when I'm not expecting it and therefore not handling something correctly. If I brute force __updatePathMatcher() at the end of function __rootPathChanged() it works, but then it's not a lazy update.

I'll look into it more on Monday...

[ericmehl]

John and I sorted out the filter problem offline and it's implemented in 9e2bb14 along with some other cleanups.

The issue was giving the same path to the filter and the path widget. In __updatePathMatcher() we call self.__paths() which will call _filter(), which will call __updatePathMatcher()... creating infinite recursion.

The solution is to give the filter a copy of the root path so it's not affected by filtering and always has access to the full tree of shader paths.

[johnhaddon]

Thanks Eric - I've squashed the fixup in and merged.