Handle USD PointInstancer - Rendering and Point Promotion #6221

danieldresser-ie · 2025-01-18T04:57:21Z

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 · 2025-01-22T16:09:24Z

startup/GafferUSD/usdInstancers.py

+	result["collectValueQuery"] = Gaffer.ContextQuery()
+	result["collectValueQuery"].addQuery( Gaffer.StringPlug( "value", defaultValue = '', ), 'collect:value' )


Is this necessary? Can we just put ${collect:value} in expandInstancersAttribQuery.location instead?

johnhaddon · 2025-01-22T16:12:11Z

startup/GafferUSD/usdInstancers.py

+	result["attrAttributesQuery"].setup( Gaffer.StringPlug( "value", direction = Gaffer.Plug.Direction.Out, defaultValue = '', ) )
+	result["attrAttributesQuery"]["location"].setValue( '${scene:path}' )
+	result["attrAttributesQuery"]["inherit"].setValue( True )
+	result["attrAttributesQuery"]["attribute"].setValue( 'gaffer:usdInstancerAttributes' )


May I suggest gafferUSD:pointInstancerAdaptor:attributes as the name for this attribute? And perhaps gafferUSD:pointInstancerAdaptor:enabled in place of gaffer:expandUsdInstancers? I'm unsure about the names too, so happy to consider alternatives. I do like using the same prefix for both attributes though.

johnhaddon · 2025-01-22T16:13:14Z

startup/GafferUSD/usdInstancers.py

+# Register a render adaptor that converts PointsPrimitives in the usd:pointInstancers set
+# into actual Instancers. This supports how USD PointInstancers are currently loaded from
+# USD files. In the future, they may be first class objects, and this will be unnecessary.


This file needs to be in startup/GafferScene, otherwise it doesn't get loaded for batch rendering unless there is a GafferUSD node somewhere in the script (which there usually isn't).

Let's also call it usdPointInstancerAdaptor.py. USD has "scenegraph instancing" and "point instancers" and it can get confusing if we're not explicit about which one we're talking about.

johnhaddon · 2025-01-22T16:22:55Z

startup/GafferUSD/usdInstancers.py

+
+	return result
+
+GafferScene.SceneAlgo.registerRenderAdaptor( "USDPointInstancerAdaptor", __usdPointInstancerAdaptor, "*", "*" )


Should we be limiting the client to SceneView *Render here?

I wasn't thinking of any other clients where this could apply - I guess the render pass editor can trigger adaptors? Makes sense that we want to avoid it applying there.

johnhaddon · 2025-01-22T16:25:19Z

python/GafferSceneUI/StandardAttributesUI.py

+		"attributes.expandUsdInstancers" : [
+
+			"description",
+			"""
+			Controls whether USD PointInstancers should be automatically converted to
+			Gaffer instances at render time, so the render will include all final
+			instances. USD PointInstancers are currently represented in Gaffer as
+			PointsPrimitives in the usd:pointInstancers set. Without this attribute,
+			the render will just show the point cloud and the prototype objects.
+			""",
+
+			"layout:section", "USD Instancers",
+
+		],
+
+		"attributes.usdInstancerAttributes" : [
+
+			"description",
+			"""
+			Names of primitive variables that should be converted to attributes with
+			the user: prefix on instances automatically created from USD PointInstancer.
+			These attributes can then be used to drive shaders that differ between instances.
+			""",
+
+			"layout:section", "USD Instancers",
+
+		],


I'm unsure about enshrining this in StandardAttributes - I think this stuff is highly likely to be transitional, and would be pretty happy to leave it for CustomAttributes to deal with. Alternatively, USDAttributes is a little bit more out of the way (although I'm unsure if that is meant to be before usd-specific-features-of-gaffer or attributes-that-get-translated-to-usd).

I spoke about it with Murray, we concluded StandardAttributes was more appropriate than USDAttributes - I guess the USDAttributes description currently makes this pretty clear: Authors attributes which have specific meaning in USD, but which do not influence Gaffer's native behaviour in any way (in which case they would belong on the StandardAttributes node). - though we could change that if you thought it was appropriate. But it sounds like you're fine with just not exposing these attributes anywhere in the UI?

johnhaddon · 2025-01-22T17:15:41Z

python/GafferSceneUI/SceneViewUI.py


 		m = IECore.MenuDefinition()
 		m.append( "/Expand Selection", { "command" : self.getPlug().node().expandSelection, "active" : not expandAll, "shortCut" : "Down" } )
 		m.append( "/Expand Selection Fully", { "command" : functools.partial( self.getPlug().node().expandSelection, depth = 999 ), "active" : not expandAll, "shortCut" : "Shift+Down" } )
 		m.append( "/Collapse Selection", { "command" : self.getPlug().node().collapseSelection, "active" : not expandAll, "shortCut" : "Up" } )
 		m.append( "/Expand All Divider", { "divider" : True } )
 		m.append( "/Expand All", { "checkBox" : expandAll, "command" : Gaffer.WeakMethod( self.__toggleMinimumExpansionDepth ) } )
+		m.append( "/USD Instancers Divider", { "divider" : True } )
+		m.append( "/Expand USD Instancers When Rendering", { "checkBox" : expandUsdInstancers, "command" : Gaffer.WeakMethod( self.__toggleExpandUsdInstancers ) } )


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)?

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.

johnhaddon · 2025-01-22T17:19:44Z

startup/GafferUSD/usdInstancers.py

+# into actual Instancers. This supports how USD PointInstancers are currently loaded from
+# USD files. In the future, they may be first class objects, and this will be unnecessary.
+
+def __usdPointInstancerAdaptor() :


I think we'd benefit from some unit tests for this node. I suggest we move it to GafferUSD._PointInstancerAdaptor (note the underscore for protection), add tests in GafferUSDTest.PointInstancerAdaptorTest and then keep the registration here as a one-liner.

Should it be stored as a .gfr as well, for consistency with the PromoteInstances node?

johnhaddon · 2025-01-22T17:22:58Z

startup/GafferUSD/usdInstancers.py

+	# to get all the paths, and a Collect to remove the ones where the attribute value is
+	# set to 0.
+	result["usdPointInstancerSet"] = GafferScene.SetFilter()
+	result["usdPointInstancerSet"]["setExpression"].setValue( 'usd:pointInstancers' )


I'm getting errors when rendering scenes in Arnold where I've already done my own manual Instancer+SetFilter. This is a concern, because it's likely to be what everyone else has been doing to date. Deleting the usd:pointInstancers set fixes it for me, but I don't know the exact mechanism for the error, which is this :

00:06:00 1814MB WARNING | [ginstance] /MediterraneanHills/Buildings/instances1: trying to clone a NULL object

That's using this very simple node network :

import Gaffer import GafferScene import imath Gaffer.Metadata.registerValue( parent, "serialiser:milestoneVersion", 1, persistent=False ) Gaffer.Metadata.registerValue( parent, "serialiser:majorVersion", 5, persistent=False ) Gaffer.Metadata.registerValue( parent, "serialiser:minorVersion", 2, persistent=False ) Gaffer.Metadata.registerValue( parent, "serialiser:patchVersion", 0, persistent=False ) __children = {} __children["SceneReader1"] = GafferScene.SceneReader( "SceneReader1" ) parent.addChild( __children["SceneReader1"] ) __children["SceneReader1"].addChild( Gaffer.V2fPlug( "__uiPosition", defaultValue = imath.V2f( 0, 0 ), flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic, ) ) __children["Instancer"] = GafferScene.Instancer( "Instancer" ) parent.addChild( __children["Instancer"] ) __children["Instancer"].addChild( Gaffer.V2fPlug( "__uiPosition", defaultValue = imath.V2f( 0, 0 ), flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic, ) ) __children["SetFilter"] = GafferScene.SetFilter( "SetFilter" ) parent.addChild( __children["SetFilter"] ) __children["SetFilter"].addChild( Gaffer.V2fPlug( "__uiPosition", defaultValue = imath.V2f( 0, 0 ), flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic, ) ) __children["SceneReader1"]["fileName"].setValue( '/home/john/data/caches/usd/city/PointInstancedMedCity.usd' ) __children["SceneReader1"]["__uiPosition"].setValue( imath.V2f( 9.60000038, 6.0999999 ) ) __children["Instancer"]["in"].setInput( __children["SceneReader1"]["out"] ) __children["Instancer"]["filter"].setInput( __children["SetFilter"]["out"] ) __children["Instancer"]["prototypes"].setInput( __children["SceneReader1"]["out"] ) __children["Instancer"]["prototypeMode"].setValue( 1 ) __children["Instancer"]["prototypeIndex"].setValue( 'prototypeIndex' ) __children["Instancer"]["omitDuplicateIds"].setValue( False ) __children["Instancer"]["orientation"].setValue( 'orientation' ) __children["Instancer"]["scale"].setValue( 'scale' ) __children["Instancer"]["inactiveIds"].setValue( 'inactiveIds invisibleIds' ) __children["Instancer"]["attributePrefix"].setValue( 'user:' ) __children["Instancer"]["__capsuleScene"]["bound"].setInput( __children["Instancer"]["out"]["bound"] ) __children["Instancer"]["__capsuleScene"]["transform"].setInput( __children["Instancer"]["out"]["transform"] ) __children["Instancer"]["__capsuleScene"]["attributes"].setInput( __children["Instancer"]["out"]["attributes"] ) __children["Instancer"]["__capsuleScene"]["globals"].setInput( __children["Instancer"]["out"]["globals"] ) __children["Instancer"]["__capsuleScene"]["setNames"].setInput( __children["Instancer"]["out"]["setNames"] ) __children["Instancer"]["__uiPosition"].setValue( imath.V2f( 8.10000038, -4.76406384 ) ) __children["SetFilter"]["setExpression"].setValue( 'usd:pointInstancers' ) __children["SetFilter"]["__uiPosition"].setValue( imath.V2f( 25.7000008, 4.19999933 ) ) del __children

Looks Arnold outputs this warning when expanding a procedural with no contents, which currently happens whenever you have an encapsulated instancer filtered to locations with no geo. It should be straightforward to put an extra for whether inPlug()->objectPlug()->getValue() is a NullObject in Instancer::computeObject before making a capsule, which should fix the warning. But it won't fix the perhaps more confusing issue in this scenario: nothing rendering, because we filter out any locations that are children of the instancer set, and the user has applied the instancer, but hasn't removed the location from the set.

johnhaddon · 2025-01-22T17:35:14Z

python/GafferUSD/PromoteInstances.gfr

+__children["PromoteInstances"]["Expression4"]["__out"].addChild( Gaffer.StringPlug( "p1", direction = Gaffer.Plug.Direction.Out, defaultValue = '', flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic, ) )
+__children["PromoteInstances"]["Expression4"].addChild( Gaffer.V2fPlug( "__uiPosition", defaultValue = imath.V2f( 0, 0 ), flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic, ) )
+__children["PromoteInstances"].addChild( GafferScene.PrimitiveVariableTweaks( "PrimitiveVariableTweaks4" ) )
+__children["PromoteInstances"]["PrimitiveVariableTweaks4"]["tweaks"].addChild( Gaffer.TweakPlug( Gaffer.IntPlug( "value", defaultValue = 0, ), "tweak0", flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic, ) )


It's early days for our use of ExtensionAlgo, so it's understandable that we haven't settled on some standards for it yet. And admittedly I am also running out of steam at the end of the day, but I do think this could be a bit clearer in terms of the naming of the nodes and their layout. For example, is there a better name than PrimitiveVariableTweaks4, could we use dots so the connections don't obscure the backdrop text, and could we size the backdrops so the text doesn't overflow (or perhaps use annotations)?

I could imagine a "no backdrops, no dots, no annotations" stance on the grounds that they're not needed at runtime. But I went pretty much the opposite way with ContactSheet and tried to make the graph as neat and documented as I could. I think that's a good place to start - later on we can always make ExtensionAlgo strip such things on export as a sort of "compilation" step from "source".

johnhaddon · 2025-01-22T17:38:40Z

python/GafferUSD/PromoteInstances.gfr

+Gaffer.Metadata.registerValue( parent["variables"]["imageCataloguePort"], 'readOnly', True )
+Gaffer.Metadata.registerValue( parent["variables"]["projectName"]["name"], 'readOnly', True )
+Gaffer.Metadata.registerValue( parent["variables"]["projectRootDirectory"]["name"], 'readOnly', True )
+Gaffer.Metadata.registerValue( __children["PromoteInstances"], 'description', 'Convert points in a point cloud into instanced geometry. Assumes the point cloud matches the conventions of a USD PointInstancer.' )


Would be good to go into a bit more detail here, describing how we deactivate points in the instancer etc.

johnhaddon · 2025-01-22T17:49:57Z

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

so we can muck with them later.

danieldresser-ie requested a review from johnhaddon January 18, 2025 04:57

danieldresser-ie force-pushed the usdInstancer branch from debc3fd to be1e08c Compare January 21, 2025 18:24

danieldresser-ie changed the base branch from main to 1.5_maintenance January 21, 2025 18:57

danieldresser-ie force-pushed the usdInstancer branch from be1e08c to 6617448 Compare January 21, 2025 19:05

johnhaddon reviewed Jan 22, 2025

View reviewed changes

danieldresser-ie added 12 commits January 22, 2025 14:21

Instancer : Add test for recursive instancing

db9d7a3

GafferUSD : Add adaptor for rendering USD PointInstancers

562ad90

BranchCreator : Fix typo? in hashTransform with possible perf impact

322a9c3

BranchCreator : Add transferAttributes plug

1da3602

InteractiveRender : Use globalsHash accessor to avoid leaking context

2eabfbf

GafferUSD : Add PromoteInstances node

4d9bf6e

FIX : usdPointInstancerAdaptor.py

8c2d4c2

FIX : usdPointInstancerAdaptor : attrib naming

6b8125b

FIX : PromoteInstances : attrib naming

d2c2cda

SceneViewUI : Add _ExpansionPlugValueWidget.popupExpansionMenuSignal

0191678

SceneAlgo::createRenderAdaptors : Slap a cheeky name on the adaptors

5045245

so we can muck with them later.

usdPointInstancerAdaptor : Add UI for controlling default expansion

8503a6f

danieldresser-ie force-pushed the usdInstancer branch from 6617448 to 8503a6f Compare January 23, 2025 04:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle USD PointInstancer - Rendering and Point Promotion #6221

Handle USD PointInstancer - Rendering and Point Promotion #6221

danieldresser-ie commented Jan 18, 2025

johnhaddon left a comment

johnhaddon Jan 22, 2025

johnhaddon Jan 22, 2025

johnhaddon Jan 22, 2025

johnhaddon Jan 22, 2025

johnhaddon Jan 22, 2025

danieldresser-ie Jan 22, 2025

johnhaddon Jan 22, 2025

danieldresser-ie Jan 22, 2025

johnhaddon Jan 22, 2025

danieldresser-ie Jan 23, 2025

johnhaddon Jan 22, 2025

danieldresser-ie Jan 23, 2025

johnhaddon Jan 22, 2025

danieldresser-ie Jan 23, 2025

johnhaddon Jan 22, 2025

johnhaddon Jan 22, 2025

johnhaddon commented Jan 22, 2025

		result["collectValueQuery"] = Gaffer.ContextQuery()
		result["collectValueQuery"].addQuery( Gaffer.StringPlug( "value", defaultValue = '', ), 'collect:value' )


		return result

		GafferScene.SceneAlgo.registerRenderAdaptor( "USDPointInstancerAdaptor", __usdPointInstancerAdaptor, "", "" )

Handle USD PointInstancer - Rendering and Point Promotion #6221

Are you sure you want to change the base?

Handle USD PointInstancer - Rendering and Point Promotion #6221

Conversation

danieldresser-ie commented Jan 18, 2025

johnhaddon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnhaddon commented Jan 22, 2025