Don't merge layers that produce renderer outputs. #1296
Conversation
| @@ -589,6 +589,11 @@ ShaderInstance::mergeable (const ShaderInstance &b, const ShaderGroup& /*g*/) co | |||
| if (master() != b.master()) | |||
| return false; | |||
|
|
|||
| // If one or both instances are directly hooked up to renderer outputs, | |||
| // don't merge them. | |||
| if (renderer_outputs() || b.renderer_outputs()) | |||
There was a problem hiding this comment.
I still can't quite decide if this is in any way responsible for the current production issues I'm having, and this code looks very different now to the OSL version we are using, but merging the renderer outputs in the version I am using causes an assertion. That doesn't seem to be the case in this code.
I don't really know enough about the internals to understand the consequences of this, but happy to thumbs up this if you are sure this is necessary @lgritz thanks!
There was a problem hiding this comment.
I'm not sure it's necessary, but I strongly suspect it is. Here's my thinking.
Let's suppose that we have two nodes A and B in the shader group, which are identical except for whether they are directly connected to any shader outputs.
Case 1: which outputs they are connected to are different. Then it seems to me that merging is a bad idea, because only one of them can win, somebody's outputs will never get set. Suppose the app has asked for A.x to go to one output and B.x to go to a different output. The post-merged shader can't be both A and B. It has to be one, with the assumption that the other is identical. So one output will be lost. That has to be wrong, so we shouldn't merge.
Case 2: the outputs are the same. MAYBE it's safe to merge, if it's a pure output and both shader are in all other ways identical so they will get the same value. Then it doesn't matter if they are collapsed, the same thing happens if A runs, then B, or if B runs, then A, or if only one of them runs. But what if the shader is one of these "pass-through with modification" (myoutput *= 2), then merging them and running it once is clearly a different result than running it twice. So again, we should not merge.
Now, there is the small danger that in some cases it was perfectly safe to merge (the safe pure output sub-case of case 2), and now we'll end up running the node twice because we are no longer merging a case that we used to merge. I'm going to argue that that this doesn't happen often enough to worry about. Most shader nodes are not hooked up to renderer outputs at all. Usually the shader graph computes all sorts of things and feeds into just one node at the "root" (or back end) that directly hooks up to the outputs, so there will tend not be two of them. If there are multiple nodes connected to renderer outputs, they will tend NOT to be exactly duplicates. And if they are, it's so ill-advised and maybe ill-defined (for the reasons I outlined above), I'm not going to concern myself with any slight loss of performance by no longer merging them.
If you think this is sound reasoning, feel free to go ahead and approve the PR. It may not be responsible for your current issue, but I do think we were right to question the validity of merging output-generating nodes.
|
Is everybody satisfied with this? |
|
Thank @lgritz after reading your response in detail, and thinking about it some more, I agree. I think this is the right thing to do. |
After talking this over with Luke Emrose it seems unwise to allow merging of layers that produce renderer outputs. Signed-off-by: Larry Gritz <[email protected]>
After talking this over with Luke Emrose it seems unwise to allow
merging of layers that produce renderer outputs.
Signed-off-by: Larry Gritz [email protected]