ShaderNetwork : Allow escaped string substitutions to work correctly #1496
Conversation
There was a problem hiding this comment.
Seems like a pretty reasonable fix?
Definitely. I think this entirely fits the intention of (but not the current documentation for) hashSubstitutions(). I'd suggest the following change to the documentation to make this clear :
Appends all attributes used byapplySubstitutions()into the hash.
Appends to the hash to reflect all changes made byapplySubstitutions( attributes ).
We also need to update the tests to reflect the new behaviour (they're failing) and update the Changes file.
src/IECoreScene/ShaderNetwork.cpp
Outdated
| // If we don't rely on any attributes, but do require substitutions ( for example, | ||
| // because some parameters have escaped substitutions, and need the escape characters | ||
| // removed ), then our hash won't vary with the attributes, but we do still need to | ||
| // do something to the hash, so that Gaffer won't skip running substitutions on this | ||
| // shader. |
There was a problem hiding this comment.
Gaffer's not really relevant here - possible alternative wording.
We don't depend on any attributes, but some parameters have escaped substitutions. Modify hash to reflect the fact that
applySubstitutions()will remove the escaping.
danieldresser-ie
force-pushed
the
shaderEscapeTest
branch
from
e563fb7 to
2bf2b35
Compare
|
Was expecting this to be trivial to update the wording of things, but I did uncover a bit more than I was expecting ... The failing test was actually one I would expect to pass ( we only need to modify the hash if there are actually some parameters using escaped substitutions ). The reason it wasn't passing was because m_parmsNeedingSubstitution didn't work like how I expected ... I was expecting it to be empty if there are no parameters needing substitution, but instead it has an entry for each node containing an empty vector of InternedStrings. The most minimal change would be to modify the new code to check for empty vectors, but it feels worthwhile to avoid putting empty vectors in m_parmsNeedingSubstitution ... it looks like there could be cases where it would be a meaningful performance win. As far as I can tell, previously this line: https://github.com/ImageEngine/cortex/blob/main/src/IECoreScene/ShaderNetwork.cpp#L459 It's end of day so I haven't really thoroughly thought through whether this could affect anything else yet, but it seems like a reasonable change, and all the tests are passing ( including the substitution specific test that triggered the investigation ... I did also add one new test so that there is some testing of the behaviour this PR was supposed to be about ). |
danieldresser-ie
force-pushed
the
shaderEscapeTest
branch
from
2bf2b35 to
1262197
Compare
Definitely - nice catch.
LGTM, but I'm going to give you time to think this through before merging - just lemme know when you're happy. |
danieldresser-ie
force-pushed
the
shaderEscapeTest
branch
from
1262197 to
aead080
Compare
|
OK, this should be good to go now. Only change since yesterday is breaking it into two commits, with separate Changes entries, so there's more visibility on the performance fix just in case it affects anything. |
2 participants
This Gaffer issue notes that string substitutions run by GafferArnold while translating shaders can't account for inherited that differ between multiple instances: GafferHQ/gaffer#6521
A simple workaround would be to escape the string substitution, to force it to occur in Arnold, rather than Gaffer ( setting the texture path to "\attr:user:texture\\" instead of "attr:user:texture". We may want to find a more automatic solution to that issue, but this workaround should work ... currently it doesn't work however, at least not consistently. Currently, escaping substitutions on a shader in Gaffer only works if there is another parameter on a shader in the network using actual substitutions ... if all the substitutions are escaped, then the escaping doesn't function properly.
The issue is this line in Gaffer: https://github.com/GafferHQ/gaffer/blob/3debb8bbdafc5d49b5f0b4e859e4355b53743138/src/IECoreArnold/Renderer.cpp#L228
which assumes there are no substitutions if hashSubstitutions returns the default hash.
I've prototyped a fix here by detecting this scenario, and modifying the hash when there are parameters tagged as needing substitution, but no substitution variables needed ( as occurs when escaping substitutions ). Seems like a pretty reasonable fix? The only real alternative I could think of would be to make the change in Gaffer instead, but we seem to have all the information we need here.
We may want to find a completely different work around to 6521, but this seems like something worth fixing anyway.