Handle USD PointInstancer - Rendering and Point Promotion

[danieldresser-ie]

I think this in a fairly good place for review. This should let you load a USD file containing a PointInstancer, immediately see it rendering in Arnold, and select points to promote to live geo.

There are lots of names here that I'm not very confident in - happy to switch to whatever wording feels best to people.

The PromoteInstances tool itself is currently a naive adaptation of an ExtensionAlgo.exportExtension - I think for a tool like this, I would probably prefer to reformat it to be a bit more maintainable, and probably blow away the backdrops and uiPositions - but it might be easier for you to inspect interactively in it's current form. There are a couple of places where we could simplify it if we added simple features to the C++ nodes, but I thought it makes sense to look at the current version first.

One thing I'll mention in terms of user experience: you clarified that you don't consider this a bug, but it still feels weird in Cycles when no matter how much I shift click in the Hierarchy View to expand everything, I still just see a bounding box for my instances, and the only way to get them to actually show up is to turn on "Expand All" in the expansion menu. ( It works exactly how you'd expect in Arnold, the instances appearing when you expand the location of the instancer - that's because we can use encapsulation in Arnold ).

[johnhaddon]

Thanks Daniel! It's nice to have the expansion working "out of the box", and the new PromoteInstances node seems like it'll be very useful in production.

I'd like to think I've done a reasonably thorough review of most of this, but I did run out of steam a bit on PromoteInstances. I'm hoping a quick layout/naming pass on your side might help me get in a bit deeper there when I come back to it.

There are lots of names here that I'm not very confident in - happy to switch to whatever wording feels best to people.

I've made a few suggestions in that regard, but I'm not super confident in them. Happy to consider alternatives too.

Cheers...
John

[johnhaddon]

This feels kindof wordy. I guess you're trying to get across the difference between using OpenGL and a renderer like Arnold? Is it worth having a separate option for each - seems like it might be useful to expand in OpenGL sometimes (but it would default off)?

[danieldresser-ie]

Happy to do whatever you think is best. Something like a top level menu "Expand USD Instancers" with child checkboxes menu options "OpenGL" and "CPU Renderers" or something? Not sure what to call the latter option - "Not OpenGL" is ugly, but it's probably unnecessary to have separate checkboxes for every renderer we support?

Note that adding an OpenGL option here makes the interaction I mentioned before between Hierarchy Expansion and locations that don't exist in the original scene even more of a problem - OpenGL doesn't support capsules, and in a production scene, you're usually not going to want to have "Expand All" on in OpenGL. I suspect users will be confused when they turn on "Expand USD Instancers > OpenGL", and don't see any instances, because "Expand All" isn't on.

[danieldresser-ie]

As per conversation, this now shows in the UI as just "Expand USD Instancers", but there is a different value for each renderer. This is implemented as an AtomicCompoundData plug on the adaptor with a BoolData per renderer ... feels like a bit of a weird interface, but it's fully internal.

[johnhaddon]

Trying to think how else we might do this, the best I can come up with is an enabledRenderers StringPlug that takes a space-separated string used that is passed to a PatternMatch node. That fits patterns elsewhere in Gaffer a bit better I think, but might be a bit more annoying to setup - startup/GafferScene/usdPointInstancerAdaptor.py would need to get a list of all renderers except OpenGL.

[johnhaddon]

One other thing in case I forget - we'll need to update Changes.md before we can merge.

[danieldresser-ie]

OK, I think all feedback has now been addressed or replied to.

I think everything should be in a pretty good place at this point.

There were the two prerequisite changes that helped simplify the network for PromotePointInstances - I ended up doing both. invertSelection in PrimitiveVariableTweaks wasn't really necessary, but it was extremely easy to add. Support for casting data in TweakPlug is perhaps a bit trickier, but I realized it would otherwise be extremely cumbersome to support all possible types of input inactiveIds masks in the network.

You had a concern about inconsistency of types being output by PrimitiveVariableTweaks when using converted types ... I don't recall it exactly? I don't think it's that bad? Most modes never change the input type, and simply fail if the input isn't found. Create always outputs the tweak type. The only mode that can output either a source type or the tweak type is CreateIfMissing mode ... but I haven't changed that at all here.

The other annoying thing about the new TweakPlug code is that my initial version failed to compile on MSVC. I haven't quite figured out whether it was out of spec ... the spec says "Outside a template, a discarded statement is fully checked." ... and it's not clear to me whether a lambda using auto counts as a template ... I guess perhaps not. But if that's the case, and MSVC was going by the spec, it should have failed trying to compile the beginning of the lambda, as soon as I reference TweakDataType::ValueType, since it gets compiled with a data type of Data* ( if it is actually not discarding constexpr if, then it shouldn't discard on the IsTypedData check ). Instead, that seems to succeed, but then it fails when I do tweakConverted.reserve, compiling it for non-vector types despite the IsVectorTypedData check. The fact it succeeds in a simple case but fails in a complex one has me feeling like it's more likely to be a compiler bug, but we still need to work around it somehow. I guess that last commit isn't too horrid.

[johnhaddon]

Thanks for the updates Daniel. I have a few notes based on testing PromotePointInstances, but otherwise LGTM :

• I don't think the destination is working as advertised. Should be ${scene:path}/.. rather than .. I think?
• I think promotedInstances might be a better default value for name, and I think users are going to want control over it - can we promote it? I wonder if we should even promote destination too?
• We don't seem to be using copySourceAttributes, so the promoted instances lose any attributes from the point instancer itself.

There's also some additional stuff that should go in Changes.md, but I'll just update that myself after we merge.

[danieldresser-ie]

OK, I've cleaned up that fix to TweaksPlug that got things working on MSVC a bit, and addressed those last few comments about PromotePointInstances.

Question that might be worth asking before we merge this: if we're planning on switching to loading PointInstancers as having relative prototype paths ( controlled with an env var for backwards compatibility ), will we need to make PromotePointInstances and _PointInstancesAdaptor check that env var for whether to set the relativePrototypeRoots flag on the Instancer node? Or do we release these new nodes as something that only works if you have the env var set to relative mode?

[danieldresser-ie]

Forgot to mention - I've got no idea what's going on with the Windows test failures now. Previously, I was dealing with some template compilation failure that we're clearly caused by me. But these new failures really don't seem related at all to what I'm doing, and didn't happen before I rebased ontop of other recent changes. Anyone else got any idea what's happening here?

[danieldresser-ie]

will we need to make PromotePointInstances and _PointInstancesAdaptor check that env var for whether to set the relativePrototypeRoots flag on the Instancer node?

Replying to myself here: there's probably no reason not to just make these nodes check the env var. I wasn't sure how we would implement checking the env var so it doesn't happen repeatedly ( in C++ we usually use global statics ) ... but if we just have an expression with no inputs that reads the env var, that should be cached like anything else ... that's probably fine.

[johnhaddon]

will we need to make PromotePointInstances and _PointInstancesAdaptor check that env var for whether to set the relativePrototypeRoots flag on the Instancer node?

I hadn't anticipated there being a relativePrototypeRoots flag on the instancer node - that's a level of faff I don't want to expose users to. I was expecting paths starting with "/" to be treated as absolute and others to be treated as relative. I suppose there is some concern about a change of behaviour there for old paths without a leading "/", but I still hope we can do this without another plug.

I've got no idea what's going on with the Windows test failures now

I think that is unrelated to this PR. We'll find out soon, because I've merged!