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

ShaderTweaks : Support wildcards in shader name #5940

Merged

Conversation

danieldresser-ie
Copy link
Contributor

Theoretically extremely simple - just do a loop with StringAlgo::match if there are wildcards in the shader name, but ended up feeling fiddlier than I expected.

The tests note some of the possible corner cases ( like trying to use wildcards in parameter names, which currently is not supported ). The main thing I've thought of that maybe I should change is that currently I've hardcoded missingMode to ignore when a wildcard is present ... my reasoning was that if you're trying to tweak something like "*.exposure", it's going to be annoying if it fails because one of the shaders in the network doesn't have exposure ... but I think maybe it's more consistent to just make the user take the extra step of setting ignoreMissing in that case?

@johnhaddon
Copy link
Member

ended up feeling fiddlier than I expected.

I found the diff a bit difficult to read as well, and in the end it was easier just to read the final code. While I was doing that I did a tiny bit of shuffling that seems to have managed to reduce the line count and simplify the parameter passing a little. I've pushed that as individual commits on the end of this PR for your consideration. LGTM.

@johnhaddon
Copy link
Member

my reasoning was that if you're trying to tweak something like "*.exposure", it's going to be annoying if it fails because one of the shaders in the network doesn't have exposure

Originally I read this as being that we didn't complain if * didn't match a shader, which I think is absolutely correct (obviously * would match something but foo* might not). But now I see you are talking about when * does match a shader but the shader doesn't have .exposure. I think this is still OK, but it does seem to imply that conceptually we are matching the whole thing against all the shader.parameter pairs available rather than doing a two phase match. Which maybe implies to folks that they could use wildcards on parameters too. I think we can leave that for now, but wondered if that reasoning makes sense to you...

@danieldresser-ie
Copy link
Contributor Author

3 of those fixes are unambiguous improvements. "Remove shader parameter" is, as you say, a matter of taste. I had considered it, and went for performance over simplicity of an API that has no visibility. But I don't feel strongly.

Should I merge the fixes, or do you want to?

I agree it would be kind of philosophically natural to assume that this interface implies parameter wildcards as well - the reason I haven't done them is because I can't think of much in the way of practical use cases. But if you want to add support for parameter wildcards purely for the sake of consistency, it probably wouldn't be hard to do, and we could just put it in now.

@johnhaddon
Copy link
Member

I've squashed in the 3 fixups you were happy with, and will merge now. We can add parameter wildcards if and when we have a use case...

@johnhaddon johnhaddon merged commit a84fc54 into GafferHQ:1.4_maintenance Jul 11, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants