StringPlug substitutions fix #1860
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We perform substitutions automatically in getValue() when called on an input plug within a compute. This provides necessary features to the user while freeing the developer from the burden of having to think about substitutions. When computing and hashing a plug, we first travel of the chain of inputs (if there is one), and do the compute at the source, to avoid the cost of repeating a series of intermediate computations and inserting them in the cache - this is an important optimisation when many nodes contain pass-throughs internally.
We had a bug in the interaction between these two things, whereby we were determining whether substitutions should occur in `getValue()` using the direction of the plug itself, and whether they should be accounted for in `hash()` using the direction of the source plug. When the source was an output, this led to a non-time-varying hash, which meant that results were reused from the cache inappropriately.
There are two fixes here. Firstly, we make sure the `hash()` and `getValue()` methods perform substitutions identically. Secondly, we also take account of substitution flags on all intermediate plugs when a chain of connections leads to an input. This resolves the "which plug flags are relevant?" question, by taking them all into account, rather than only those at the end of the chain. This means the "go straight to the source" optimisation now acts as if the value was passed along by a series of `getValue()` calls on the intermediate plugs - that is, as if the optimisation did not exist.
This will be visible to users as follows :
- When driving a plug with an expression outputting strings containing '#' or '${variable}' substitutions, the downstream node will now compute correctly in all contexts, instead of appearing frozen on the results of the first compute.
- If a plug with substitutions turned off (ImageReader fileName, or FileSequenceParameter for instance) receives its value from an intermediate plug with substitutions turned on, it will not receive substitutions. If the intermediate plug was created by promotion, it will have the appropriate flags anyway, and act is before, but if created by other means, the behaviour may change.
I'm concerned about the latter change. In practice, substitution settings are largely determined based on the needs of the node owning the plug, so having it react different based on where the input comes from seems inappropriate. Stand by...
Whether or not substitutions are performed now depends entirely on the properties of the plug on which `getValue()` is called, and has nothing to do with upstream plugs. See comments for further details.
johnhaddon
force-pushed
the
substitutionsFix
branch
from
a5d06c3
to
06c8915
Compare
| /// argument may be used to disable specific substitutions. | ||
| /// | ||
| /// > Note : This feature does not affect the values passed | ||
| /// > internally between string plugs - substitutions are only |
|
The approach you ended up with certainly seems tidier. But I'll have to think (or more likely test) if/how it affects all the dodgy things we're doing with substitutions on SubtaskedOpHolder plugs for Dailies and whatnot. |
The top level was getting too cluttered, and giving too much emphasis to reference docs over tutorials.
|
I've pushed a couple of commits to add some reference documentation for the string substitutions - the previous commits remain unchanged. |
|
@lucienfostier tested this on the case I was worried about, so hopefully all is well. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes a nasty bug whereby substitutions were performed incorrectly when driving a StringPlug with an expression which itself outputs a string containing substitutions (e.g. a file sequence). There are two things going on in this PR - a fix for that bug, and an attempt to rationalise the behaviour when a chain of StringPlug connections contain conflicting substitutions flags. I flip-flopped on the correct way of dealing with the latter issue, and although I now feel strongly that my final approach is the correct one, I've included an intermediate commit with my first attempt, in case it provokes any thoughts either way.