PointInstancer : Add new node for making PointInstancer objects - #7077
PointInstancer : Add new node for making PointInstancer objects#7077johnhaddon wants to merge 16 commits into
Conversation
As with ContactSheet and ContactSheetCore, we'll be combining a small C++ core with a node-network to implement the user-facing node.
This converts any points-based primitive to a PointInstancer, collecting prototypes and parenting them below. It works around the limitations of the USD instancing spec by allowing variations of the prototypes to be generated using time offsets and arbitrary context variables. Fixes GafferHQ#6810
murraystevenson
left a comment
murraystevenson
left a comment
There was a problem hiding this comment.
Thanks John, a few comments inline for what I managed to get through today. I've also pushed two fixups with spelling/typo corrections (seemed simpler to just fix them rather than make a bunch more comments).
Is the shuffling of the transform primitive variables actually useful, or would we be better off making people do that beforehand?
It may be useful, but it does feel secondary/tertiary to setting up your prototypes, their variations and any shading attributes. Maybe the Transform and IDs sections are better demoted to below Prototypes, Prototype Variation, and Attributes?
Should the None prototype modes be removed or renamed? Would the add-variation-to-existing-instancer feature they enable be better off in a separate PrototypeVariations node?
I haven't tried using it too much just yet, but the None modes do seem like they're muddying the waters of the node a bit. I'm also wondering about the current behaviour of the attributes plug when you're using the PointInstancer node to modify the prototypes of an existing point instancer. You need to remember to set attributes to * to ensure any previously included primitive variables aren't deleted, which seems easy to overlook. Maybe this would all be simpler with a dedicated PrototypeVariations node?
| - `{timeOffset}` : The time offset. | ||
| - `{contextVariable}` : The value of a context variable. | ||
| - `{hash}` : A hash value that uniquely idenfies the protoype. | ||
|
|
There was a problem hiding this comment.
Would it be worth mentioning the - -> n . -> _ transformations that occur to sanitise float values?
There was a problem hiding this comment.
Thanks, this now just needs to be propagated through to the plug on the PointInstancer node, as it still has the old description baked in.
| __children["PointInstancer"]["Expression1"]["__expression"].setValue( 'parent["__out"]["p0"] = "prototypeRoots prototypeIndex P scale orientation instanceId invisibleIds " + parent["__in"]["p0"]\n' ) | ||
| __children["Expression"]["__engine"].setValue( 'OSL' ) | ||
| __children["Expression"]["__expression"].setValue( 'parent.__out.p0 = .25 * ( 1 + sin( time * 10 ) );' ) | ||
|
|
There was a problem hiding this comment.
We're missing a description for the PointInstancer node itself.
There was a problem hiding this comment.
Added in 665a7a2.
Maybe the Transform and IDs sections are better demoted to below Prototypes, Prototype Variation, and Attributes?
I did this in the same commit, as well as opening the Prototypes section by default, and adding a divider between the prototypes and prototype index plugs.
I also wonder if it's worth driving visibilityActivator rather than activator with the prototype mode plugs, to reduce clutter further?
There was a problem hiding this comment.
I did this in the same commit, as well as opening the Prototypes section by default, and adding a divider between the prototypes and prototype index plugs.
Thanks, that is a great improvement to the layout.
I also wonder if it's worth driving visibilityActivator rather than activator with the prototype mode plugs, to reduce clutter further?
Yeah, visibilityActivator might be more appropriate in this situation, there is a fair jumble of plugs and I don't think we're too concerned about keeping the other plugs visible for discoverability. Yesterday, I was tempted to ask for the RandomPrimitiveVariables' "seed" plug to be promoted to make the Random instanceMode a little more useful, but didn't want to add to the clutter of plugs. If we switch to visibilityActivator maybe that's a worthwhile addition?
There was a problem hiding this comment.
Fix NameFormatter bugs.
Remove unnecessary code.
Document custom floating point formatting.
Fix message context.
Remove `unittest.main()`.
Remove `unittest.main()`.
- Add description. - Move prototype sections to top, and default first section open. - Add divider between prototype and prototype indices sections.
Check interpolation for `timeOffsets` and `prototypeIndex`. The new `vertexVariable()` function also has the advantage of returning IndexedView directly instead of `optional<IndexedView>`, which simplifies later usage slightly. At the same time, tighten up size checks for context variables. We don't want to accept Constant variables just because they happen to be the right length, because if the point count animates it will be super confusing for the variable to pop in and out of existence. There's a bit of tension here between error reporting and wildcards - we don't report errors so you can use `*` to use all vertex variables, but that means you don't get an error if you name `aConstantVariable` explicitly.
Thanks Murray - I've pushed fixups for everything and noted them inline.
That is seeming pretty reasonable. I started prototyping such a node and the first thing that crops up is the layout of the prototypes on output. For PointInstancer I felt it was pretty reasonable to be opinionated, enforcing that prototypes are always parented below the instancer, always in a flat list, etc. It's still possible to import a less/differently structured instancer from USD, or build one from primitives in Gaffer. But for PrototypeVariations I wonder if a user might reasonably expect us to build the variations in-place, respecting the original layout of prototypes. Might it be better to lay them out as |
|
Testing this a bit today, I'm finding the |
Prefer `layout:visibilityActivator` for plugs enabled by top-level modes.
2 participants
This converts any points-based primitive to a PointInstancer, collecting prototypes and parenting them below. It works around the limitations of the USD instancing spec by allowing variations of the prototypes to be generated using time offsets and arbitrary context variables specified as primitive variables on the source points.
Like ContactSheet, the implementation is split between an internal C++ node and a user-facing node authored as a Box and exported via ExtensionAlgo. I'm pretty open to opinions about the user-facing design - questions on my mind include :
Noneprototype modes be removed or renamed? Would the add-variation-to-existing-instancer feature they enable be better off in a separate PrototypeVariations node?