Closed andrewkaufman closed 4 years ago
I want clearly defined rules for how collisions/conflicts are handled. I don't much mind what those rules are (eg. main scene wins, layering left to right, etc) so long as they are intuitive and well documented.
Here's my proposal for the merging rules :
GafferImage.Merge
is a left-to-right layering of the input images.outAttr = in[0].attributes(); outAttr.update( in[1].attributes() ); outAttr.update( in[2].attributes )...
in[0]
.These are motivated largely by a desire to keep things simple and explainable, and to keep them compatible with a hypothetical SceneDiff
node I'd like to introduce in the future. This would take two scenes and then output a sparsely authored scene with just the differences. To make that work really well I think we would need the following modifications to our scene representation, but those can come later :
a. We would need to make the transform at a location optional, so a location can have no opinion about the transform. b. We would need to allow attributes to contain null values for particular names, to encode deleted attributes. c. We would need a way of encoding deleted objects too.
In the shorter term, does rule 4) and the lack of a) cause any problems for the use cases you have in mind?
- Globals are passed through from
in[0]
.
...
These are motivated largely by a desire to keep things simple and explainable, and to keep them compatible with a hypothetical
SceneDiff
node I'd like to introduce in the future.
Would there be merit in treating globals and attributes the same in that case, ie globals follow (3), otherwise the diff is limited to non-root changes? There is a lot of benefit in being able to have globals in a diff too.
Would there be merit in treating globals and attributes the same in that case, ie globals follow (3), otherwise the diff is limited to non-root changes? There is a lot of benefit in being able to have globals in a diff too.
Yeah, the globals was the one I was least sure of. I went with first-input-only mainly just to match GafferImage.Merge
, but don't have any particular attachment to it.
Would there be merit in treating globals and attributes the same in that case, ie globals follow (3), otherwise the diff is limited to non-root changes? There is a lot of benefit in being able to have globals in a diff too.
Yeah, the globals was the one I was least sure of. I went with first-input-only mainly just to match
GafferImage.Merge
, but don't have any particular attachment to it.
Feels like it'd be easier to explain to people too?
Feels like it'd be easier to explain to people too?
Definitely, aside from the difference in behaviour to GafferImage.Merge
. I think GafferImage.Merge
should really be called GafferImage.Composite
though. And actually, assuming we go with the proposed rules above, I think we should call this new node MergeScenes
rather than MergeHierarchy
. We were talking about MergeHierarchy
because we originally envisaged something that didn't actually do any merging of properties, but instead just merged in new branches from the rightmost inputs.
How would we treat the situation where we want to have multiple locations in the main input scene be affected by the operation? For example, a use case would be merging a groom hierarchy into multiple identical character hierarchies.
How would we treat the situation where we want to have multiple locations in the main input scene be affected by the operation? For example, a use case would be merging a groom hierarchy into multiple identical character hierarchies.
I think that is out of scope for this node. The ideal approach would be to merge your groom into each character before combining those characters into one large scene. If that's not doable then other possibilities would include using the Copy*
nodes with their sourceLocation
plug, or perhaps using some of the tricks Cinesite have been developing for operations on subsections of a scene.
Could it become part of the scope of the node (or an additional future feature), or is there something about that operation that inherently wouldn't gel with applying it to multiple locations? A nice feature of this node is that it would be easier to generate template node networks without having to put any specific knowledge about the scene structures into the parameters of copy nodes. ie, 'assuming we have some compatible hierarchies whatever they may be, it'll merge' rather than 'I have particular knowledge of the hierarchy and build a custom network, set params to work with that one' - which I assume would be required when using the copy* nodes?
I agree, upfront merging would be nicer - but I was thinking of situations where you might want to upgrade the fidelity of an asset close to camera etc. Say, 20k cows coming from a crowd system, but for the 40 in the front I want to use a full groom for rendering, not only a velvet shader.
I apologize - heave to inform myself about those cinesite subsection operators, might well be they take care of these kind of use cases.
Could it become part of the scope of the node (or an additional future feature), or is there something about that operation that inherently wouldn't gel with applying it to multiple locations?
This would also work well in the SceneDiff scenario, where you want to re-apply the diff to multiple locations - which opens up baked lookdev workflows without us needing to write CopySets
.
Could it become part of the scope of the node (or an additional future feature), or is there something about that operation that inherently wouldn't gel with applying it to multiple locations?
Sorry for the terse response - I was rushing out of the office when I replied and didn't give it enough thought. My knee-jerk reaction comes from a desire to avoid feature creep, because our general design philosophy is to concentrate on small well-defined nodes that interoperate well rather than monolithic nodes that try to do everything and are harder to learn and maintain. I'd only just got my head around how to implement the simple version with decent performance, and didn't see how to work this new request into that. After a bit of sleep I think I maybe see a way, although I'm still not sure this is the right approach to the problem.
That said, I'm now not even sure I actually understood what you were proposing exactly. I assumed you were proposing the addition of a filter to MergeHierarchy, with the incoming hierarchies being merged with the hierarchy below every matching filter position. So the filter would specify "virtual roots" below which we would merge stuff in. Is that right? Off the top of my head, this does seem to have a couple of potential issues :
/a
and /a/b
, meaning overlapping merges. For any given output location we then would have to consider not just N input locations, but up to N*d
input locations, where d
is the depth of the incoming hierarchies. It's unlikely that someone would intentionally do this, which makes me wonder if a parameterisation that allows it is perhaps the wrong one.'assuming we have some compatible hierarchies whatever they may be, it'll merge' rather than 'I have particular knowledge of the hierarchy and build a custom network, set params to work with that one' - which I assume would be required when using the copy* nodes?...Say, 20k cows coming from a crowd system, but for the 40 in the front I want to use a full groom for rendering, not only a velvet shader.
In both scenarios (UberMergeHierarchy and CopyAttributes) you need to know where those 40 cows are (or be able to use a filter to find them). If you can do that, then it is possible to figure out the right parameters for CopyAttributes via expressions, but we would probably want some support nodes to help for performance sake.
Another alternative might be an extension that we've been considering for CollectScenes, whereby it could collect into locations at arbitrary depths in the hierarchy, rather than only into root locations. You could use that to collect your 40 grooms into the appropriate places relative to the global scene, and then pipe the result into a regular MergeScenes node to perform the merge.
I apologize - heave to inform myself about those cinesite subsection operators, might well be they take care of these kind of use cases.
No need to apologise, they're pretty new (one isn't even finished yet). I'll mail you and Alex separately so you can get up to speed.
This would also work well in the SceneDiff scenario, where you want to re-apply the diff to multiple locations - which opens up baked lookdev workflows without us needing to write CopySets.
I'm not trying to invalidate the use cases; they've perfectly reasonable. I'm just trying to make the point that MergeHierarchy may not be the place to solve the problem. If the simple version of MergeHierarchy can be combined easily with another simple node to provide the behaviour of an UberFilteredMergeHierarchy then that would be rather pleasing.
My knee-jerk reaction comes from a desire to avoid feature creep, because our general design philosophy is to concentrate on small well-defined nodes that interoperate well rather than monolithic nodes that try to do everything and are harder to learn and maintain.
...
If the simple version of MergeHierarchy can be combined easily with another simple node to provide the behaviour of an UberFilteredMergeHierarchy then that would be rather pleasing.
Makes total sense. Controversially then, as the original request simply says for "I want to combine scenes, such that locations in the output scene are a union of all locations in all input scenes", is there an even simpler approach:
MergeHierachy
- works like NameSwitch/Spreadsheet
etc.. in that the first input that contains a location wins. There is no merging of anything other than child names and sets. So it really is only merging the hierarchy, not the location data.<ApplyDiff>
- which works more like CopyAttributes
and so and can be used for the 'asset assembly' cases where layering logic is important, and multiple roots may then be more appropriate?EDIT: Just saw though, that though the original request talks about a potential rule such as "main scene wins", the use cases do talk about adding attributes. Not sure if those cases could be covered by the existing CopyAttributes
case vs needing it to be the all-in-one merge node?
MergeHierachy - works like NameSwitch/Spreadsheet etc.. in that the first input that contains a location wins. There is no merging of anything other than child names and sets. So it really is only merging the hierarchy, not the location data.
It's funny you should say that, since when this was an internal ticket, I spent quite a while arguing for something exactly like that. A few things changed my mind :
- which works more like CopyAttributes and so and can be used for the 'asset assembly' cases where layering logic is important, and multiple roots may then be more appropriate?
I think this may fail the "power through simplicity, modularity and composition" test. Without the proposed MergeScenes behaviour to use as a building block, the implementation is likely to be pretty awful (that N*d
stuff I was talking about). Whereas I'm hoping that with the appropriate CollectScenes modification, ApplyDiff could be a SceneProcessor with two internal nodes. I suspect that the simpler MergeHierarchy might fall by the wayside because it was less flexible...
- I think the rules proposed above cover a much wider range of use cases, and more likely to be the intuitive ones now USD is making layering commonplace.
Yeah, I supposed the challenge in my mind for it being a more flexible tool in the medium terms was around:
We do not provide any options for alternative merging operations.
Until we have the ability for a location to 'not express an opinion about a specific topic' as part of the general scene description, rather than just in a diff (why USD works well for layering as it can be entirely sparse by nature). If we do go for this more extensive type of merge, it feels like it might be beneficial to at least have toggle for what might be merged?
Thinking through a simple lookdev case where people often talk about 'applying lookdev to an asset'. To do this with MergeHierarchy
(rather than CopyAttributes
, as you also want to merge sets) you'd need to connect the lookdev scene to the left most input, to avoid picking lookdev transforms, which felt a little counter intuitive to the 'gaffer left input is the passthrough one' principal. Which would ultimately mean you couldn't easily disable the lookdev merge by disabling the node. So it feels like you'd use CopyAttributes
, but then as you still need to merge the sets so you'd need a MergeSets
node.
If you're in a position to re-jig the order application, so you'd be layering on animation afterwards, and actually lookdev is the starting point, not an addition, then the current proposal is totally fine, but then you'd either need meshes in lookdev, or also load some bind pose type model first.
I guess it just seemed like the potential of a wider set of use cases might be constrained by the all encompassing nature of what gets merged?
If we do go for this more extensive type of merge, it feels like it might be beneficial to at least have toggle for what might be merged?
Yes, that sounds very reasonable. When I was trying to rule out "alternative merging operations", it wasn't toggles I was thinking of, it was more things like "take the maximum value of attribute x". I just didn't want the node to explode into a huge parameter space. I'll admit I'm sometimes over-zealous in trying to keep things minimal, but since production is a reliable source of pressure for "just one more parameter" I think there's a place for a grumpy old man in the conversation.
Yes, that sounds very reasonable. When I was trying to rule out "alternative merging operations", it wasn't toggles I was thinking of, it was more things like "take the maximum value of attribute x". I just didn't want the node to explode into a huge parameter space.
Ahh got you. The proposal with globals being like (3), and toggles for globals, attributes, transforms, objects, etc. seems like a winner to me.
I'll admit I'm sometimes over-zealous in trying to keep things minimal, but since production is a reliable source of pressure for "just one more parameter" I think there's a place for a grumpy old man in the conversation.
Always :) got to make features justify themselves...
In the shorter term, does rule 4) and the lack of a) cause any problems for the use cases you have in mind?
I think we could make that work, at least for the immediate use case described in the user story.
the globals was the one I was least sure of. I went with first-input-only mainly just to match GafferImage.Merge, but don't have any particular attachment to it.
I figured that was the reason. I'm not sure which I prefer, but I agree it'd be easier to explain to people if globals have the same update left-to-right behaviour as attributes.
assuming we go with the proposed rules above, I think we should call this new node
MergeScenes
rather thanMergeHierarchy
Sounds great.
How would we treat the situation where we want to have multiple locations in the main input scene be affected by the operation?
Apologies, I hadn't considered the targeting multiple "virtual roots" use case when I wrote up the Issue...
In both scenarios (UberMergeHierarchy and CopyAttributes) you need to know where those 40 cows are (or be able to use a filter to find them). If you can do that, then it is possible to figure out the right parameters for CopyAttributes via expressions...
Finding the cows is trivial with a decent a pipeline; the 40 cows would be in a set called "HeroHerd". Ideally the user of MergeScenes
could make a simple graph that says "apply the cow groom scene to the hero herd", without requiring any knowledge about the hierarchy of an individual cow nor a cow groom.
Another alternative might be an extension that we've been considering for CollectScenes, whereby it could collect into locations at arbitrary depths in the hierarchy, rather than only into root locations. You could use that to collect your 40 grooms into the appropriate places relative to the global scene, and then pipe the result into a regular MergeScenes node to perform the merge.
That sounds useful indeed.
I think the rules proposed above cover a much wider range of use cases, and more likely to be the intuitive ones now USD is making layering commonplace.
Its probably worth mentioning, the (latest) reason to prioritize this Issue came from an example made in USD (ie. "Do we have a way in Gaffer to match this simple USD Sublayer I did in Houdini?").
If you're in a position to re-jig the order application, so you'd be layering on animation afterwards, and actually lookdev is the starting point, not an addition, then the current proposal is totally fine, but then you'd either need meshes in lookdev, or also load some bind pose type model first.
FWIW, that sounds totally normal/canonical to me. Model -> Lookdev -> Animation (-> TechAnim -> CFX)
. If you're able to control the order, I don't see why you'd need it to be Animation -> Lookdev
. Granted we do currently do the later, but we could easily change to the former if we had to, and we'd probably need to do that if we fully embraced layering throughout the pipeline.
The proposal with globals being like (3), and toggles for globals, attributes, transforms, objects, etc. seems like a winner to me.
Me too. Plus the CollectScenes idea (we should spawn a separate Issue for that).
The proposal with globals being like (3), and toggles for globals, attributes, transforms, objects, etc. seems like a winner to me.
Me too. Plus the CollectScenes idea (we should spawn a separate Issue for that).
Great, sounds like we have a plan then. For what it's worth, I now think the scheme I was going to use for efficiently tracking which inputs contribute to which locations could be extended to support the overlapping of lots of "virtual roots" so there may be an option there if the CollectScenes idea doesn't work out. But I think the idea of composing nodes is preferable.
Summary
It can be useful to combine the hierarchy of multiple scenes.
User story
What
I want to combine scenes, such that locations in the output scene are a union of all locations in all input scenes. I want clearly defined rules for how collisions/conflicts are handled. I don't much mind what those rules are (eg. main scene wins, layering left to right, etc) so long as they are intuitive and well documented.
Why
It is common to have multiple artists working on top of the same base hierarchy of a character. Some artists might add new locations or attributes in the course of their work. Downstream artists often need to combine that work together into a single scene representing the whole character.
Example scenario(s)
Lookdev/Surfacing and Grooming might be iterating in parallel off of a base model. Lookdev will be adding attributes to existing locations and grooming will be adding new locations throughout the hierarchy, which often contain objects, attributes, and sets. Lighters need to work with the full character, with Lookdev and Groom already applied to the hierarchy.
Our current workflow relies on a per-character setup of
Parent
nodes andPathFilters
, but this presumes that Lookdev is familiar with the hierarchy generated by Grooming, and it also presumes a level of coordination and manual intervention per-character that doesn't scale well. With a MergeHierarchy node, we could automate the operation and keep it consistent for every character.