Encapsulate : Use correct rendering options #5474
Conversation
|
All looks pretty reasonable to me. The theoretical inelegance of setRenderOptions doesn't overly bother me. I guess my main question now is whether you would plan to handle attribute inheritance the same way ... is there a reason it wouldn't work to do the same for attributes, and remove the remaining listed limitations on Capsules? |
Ideally, attribute inheritance is something that should be done by the renderer. In the Arnold case it's super weird, because user attributes are automatically inherited in Arnold, but other ones aren't, and some do the opposite of what you want and clobber the internal attributes. By leaving this to the renderer backend, our Arnold renderer is able to make cheap attribute edits for user attributes, and then request re-expansion for the ones it can't handle. That's the general case though, and you might be talking just about motion blur attributes which affect render translation, but aren't used by the renderer. I think that should be fixable in the same way, yes. But I'm not aware of it having come up, whereas the globals come up all the time. And I'm trying to limit the scope somewhat (I'm really just meant to be implementing |
Yeah, that's what I was thinking of. And I don't see a rush on it either - was just the main question I had about this approach. |
| > attributes within the encapsulated hierarchy are | ||
| > considered. | ||
| > - The `usd:purpose` attribute is not inherited - only | ||
| > attributes withing the encapsulated hierarchy are |
There was a problem hiding this comment.
Thanks for spotting that - squashed a fix into 8f98b8e.
johnhaddon
force-pushed
the
renderPurposes
branch
from
fad41ba to
22df6fd
Compare
|
I've pushed one more commit that does a little dance to preserve ABI compatibility with |
johnhaddon
force-pushed
the
renderPurposes
branch
from
ce56f31 to
cdc1470
Compare
|
I don't see anything wrong with the ABI commit, though it will certainly be nice to get rid of it. |
The dirty flags track things that may no longer be in sync with the Gaffer scene, and therefore require an update. The changed flags track things that actually changed when we performed that update. Use this consistently for camera shutter updates, and improve comment to clarify why we're doing what we're doing. As far as I can tell, this doesn't affect the correctness of any outcome, but it does mean we'll avoid some redundant work when restarting after a cancellation part way through an update. This is because we clear the dirty flags as soon as we've updated the globals, but don't clear the changed flags until we've updated the whole scene.
We know that type isn't LightType or LightFilter type already, because there is a return statement for that case at line 856. I've also replaced the non-const `isNull` variable with a test directly inside the `if`, so there's one less piece of state to have to bear in mind throughout the rest of the function (where the variable isn't actually used again).
Both RenderController and LocationOutput had a little class for dealing with the most important rendering options. Consolidating them into a single shared RenderOptions class lets us remove a bit of duplicate code and simplify a few function signatures. But the main purpose of this rejig is to set us up so that we can pass the RenderOptions to Capsules so that they respect the correct motion blur settings.
Although the members in question are private, we still need to preserve `sizeof( Capsule )` and `sizeof( RenderController )` if we are to include the RenderOptions changes in `1.3.x`. We'll revert this commit as soon as it is merged to main, so `1.4` will get a cleaner version of the code.
johnhaddon
force-pushed
the
renderPurposes
branch
from
cdc1470 to
2daface
Compare
Until now, the Encapsulate node would use the render globals from its input to decide how to motion blur the locations within the capsule, and which
usd:purposesto render. This did not meet user expectations, because the globals can be changed freely downstream, and everything else uses the globals that are input into the terminatingRendernode. This PR smuggles the correct downstream globals into Capsules prior to outputting them, and has the necessary tracking to make edits to this as needed during IPR. It's not particularly pretty, but I don't think its turned out too bad.Note that there are currently two ABI breaks here, as I've added private members to both Capsule and RenderController. I'd be willing to bet they wouldn't affect anyone (I doubt anyone is using those classed from C++) but still we shouldn't release 1.3 like this. Once this is approved I can add a commit that does some hackery to maintain ABI on 1.3, which I will immediately revert once this gets merged through to
main. Or we can just merge this tomain, depending on how important it is that this lands in 1.3.