USDMeshLight#6970
Conversation
|
There was a test failure from I didn't see a particularly good fallback, but USD has updated their attribute descriptions in the USDLux schema, so I think it might be ready to use again. That along with a placeholder description for the Arnold parameters satisfies the documentation requirement for all plugs. |
johnhaddon
left a comment
There was a problem hiding this comment.
Thanks Eric! Comments inline as usual. In addition to those, I think it's also worth adding a conversion to 3Delight, since folks have been asking for better support on Discord recently. Similarly I think there's a decent case for adding for Cycles too, since the existing CyclesMeshLight is a bit of a weird anomaly (it doesn't actually make a light).
| def testVisibilityAttributes( self ) : | ||
|
|
||
| # Visibility attributes are renderer-specific. They are handled and tested | ||
| # by the renderer `Attributes` methods. |
There was a problem hiding this comment.
Does this refer to Renderer::attributes(), or something else?
There was a problem hiding this comment.
Yes, that's what I was getting at. I clarified it in bff4c97
There was a problem hiding this comment.
It seems a bit of an odd test then given the method name - all it's really asserting is that there's just one attribute, and it's called light. Can we just put that assertion in one of the other tests?
| @@ -1345,11 +1337,32 @@ class ArnoldAttributes : public IECoreScenePreview::Renderer::AttributesInterfac | |||
|
|
|||
| m_surfaceShader = newLightSurface ? shaderCache->get( newLightSurface.get(), surfaceName, attributes ) : nullptr; | |||
| m_lightShader = newLightShader; | |||
|
|
|||
| modifiedAttributes = attributes->copy(); | |||
| modifiedAttributes->members().try_emplace( g_cameraVisibilityAttributeName, new IECore::BoolData( true ) ); | |||
| modifiedAttributes->members().try_emplace( g_shadowVisibilityAttributeName, new IECore::BoolData( false ) ); | |||
| modifiedAttributes->members().try_emplace( g_diffuseReflectVisibilityAttributeName, new IECore::BoolData( false ) ); | |||
| modifiedAttributes->members().try_emplace( g_specularReflectVisibilityAttributeName, new IECore::BoolData( false ) ); | |||
| modifiedAttributes->members().try_emplace( g_diffuseTransmitVisibilityAttributeName, new IECore::BoolData( false ) ); | |||
| modifiedAttributes->members().try_emplace( g_specularTransmitVisibilityAttributeName, new IECore::BoolData( false ) ); | |||
| modifiedAttributes->members().try_emplace( g_volumeVisibilityAttributeName, new IECore::BoolData( false ) ); | |||
| modifiedAttributes->members().try_emplace( g_subsurfaceVisibilityAttributeName, new IECore::BoolData( false ) ); | |||
| attributes = modifiedAttributes.get(); | |||
There was a problem hiding this comment.
I think this is a bit weird from a separation-of-concerns point of view. We have logic here to determine that it's a mesh light and will need some processing, and to find the surface shader for processing. Then we defer to a function to deal with thos, and then add on the processing of the attributes ourselves. Can we instead have the entire thing behind a single function?
ConstCompoundObjectPtr convertUSDMeshLights( const CompoundObject *attributes )
{
if( notAMeshLight )
{
return inputAttributes;
}
CompoundObjectPtr result = attributes->copy();
// Modify light shader.
// Assign visibility attributes.
return result;
}
This structure would also extend easily to dealing with materialSyncMode, allowing us to hide all the details of that from ArnoldAttributes.
There was a problem hiding this comment.
I've moved the conversion code and all of the attribute handling to a separate function in ec5787c
There was a problem hiding this comment.
I was kindof thinking the single function would be the one exposed by ShaderNetworkAlgo. If we want to extend to respect materialSyncMode then currently things are awkward because we don't have all the information in one place. ShaderNetworkAlgo is technically public API, so it's worth getting the signature right now. Having all the functionality in there may also make testing easier, since we can test the output data instead of querying an Arnold universe.
| ) | ||
| { | ||
| glowColorParameter = { handle, g_glowColorParameter }; | ||
| glowIntensityParameter = { handle, g_glowGainParameter }; |
There was a problem hiding this comment.
I'm not seeing any effect from changing PxrSurface.glowGain.
There was a problem hiding this comment.
That was a victim of my move-glow-to-light approach. It's fixed now with 97f4044
There was a problem hiding this comment.
I'm still seeing behaviour I don't expect :
- The light colour is affecting the colour I see in primary rays.
- The PxrSurface
glowGainis having no effect in primary rays.
There was a problem hiding this comment.
Yeah glowGain is having no effect on the directly visible (to the camera) intensity for me. I can set it to zero and still see the light, and set it to lots and get the same render. Maybe I'm doing something wrong? Happy to share a screen if it would help...
|
|
||
| if( glowColorParameter ) | ||
| { | ||
| newSurfaceShader->parameters()[glowColorParameter.name] = new Color3fData( Color3f( 0.f ) ); |
There was a problem hiding this comment.
As for the Arnold case, I'm not sure why we need to modify the surface shader. But on the other hand, I am seeing the glow color in the camera-visible light, so I assume something is different in RenderMan. Can you explain exactly what we're doing?
There was a problem hiding this comment.
In theory, 97f4044 should fix this by leaving the surface shader alone.
I'm not sure why the glow color in the camera-visible light is still showing up. If I recreate the post-conversion USD light setup using RenderMan shaders (PxrSurface and RenderManMeshLight) I can't see where we could make the emission part not visible to the camera. The surface and light colors seem to be additive for camera rays.
There was a problem hiding this comment.
The surface and light colors seem to be additive for camera rays.
Can we check what happens with a MeshLight/PxrSurface combo in usdview? I'm wondering if there's something wrong with the way I was handling PxrMeshLight in the first place.
| modifiedAttributes = attributes->copy(); | ||
| modifiedAttributes->members().try_emplace( g_visibilityIndirectAttributeName, new BoolData( false ) ); | ||
| modifiedAttributes->members().try_emplace( g_visibilityTransmissionAttributeName, new BoolData( false ) ); | ||
| modifiedAttributes->members().try_emplace( g_visibilityCameraAttributeName, new BoolData( true ) ); | ||
| attributes = modifiedAttributes.get(); |
There was a problem hiding this comment.
Came comment as for Arnold - I think we can encapsulate all this logic in one ShaderNetworkAlgo function.
There was a problem hiding this comment.
Same comment as for Arnold again.
| # various parameters are actually intended to do. | ||
| description = description.replace( "TODO: clarify semantics", "" ) | ||
| return description | ||
| return property.GetDocumentation() |
There was a problem hiding this comment.
The "clarify semantics" stuff has gone from USD, and GetDocumentation() calls GetMetadata( "documentation" ), so this all makes sense. But it doesn't seem to match the commit message.
There was a problem hiding this comment.
I can update the commit message after I squash the other commits down. I think something like USDShaderUI : Add description for MeshLight parameters. GetDocumentation() checks for userDocBrief before falling back to documentation, so it picks up some additional metadata that GetMetadata( "documentation" ) misses out on.
There was a problem hiding this comment.
Thanks for the explanation - I had mistakenly been looking at UsdObject::GetDocumentation() (which doesn't use userDocBrief rather than UsdPrimDefinition::Property::GetDocumentation(), which does.
These match our current descriptions for Arnold light plugs.
|
This is in good shape now for a look at the existing fixups. I added one more commit for Arnold : github.com//pull/6970/commits/6afc7e75cfc387638757eeab12165ed7635d0885. That corrects the behavior to multiply the light color with the surface glow response, creating a new We decided that if a mesh light has an input to I'll take care of the Cycles and 3Delight conversions next. |
Sorry, I should have read the UsdLux docs when we were having this discussion. They seem to be fairly clear that we should multiply the two together in this case :
Might be worth checking via |

This adds USDMeshLight support for Arnold and RenderMan. The USD spec, in the default mode, calls for taking the mesh light color texture from the surface shader, if one exists. There are a couple of other simpler modes I haven't implemented yet, but I can add those in a follow-up PR or add them on here.
There's also new support in this PR for visualising mesh light textures using the existing renderer-specific texture registration system. I didn't implement the surface texture swapping for those visulisations, but it can also be added once we're happy with the overall direction here.
Checklist