diff --git a/Changes.md b/Changes.md index cda5467df69..ceb6d67c4fe 100644 --- a/Changes.md +++ b/Changes.md @@ -10,6 +10,9 @@ Fixes ----- - RenderMan : Fixed handling of V3f data with non-geometric interpretation. Common sources include `float3` primvars and attributes loaded from USD files. +- Cycles : + - Fixed potential crashes when using `render:` attributes on objects due to passing invalid data. + - Removed passing of `render:` matrix attribute types to objects as they're not supported currently. Build ----- diff --git a/python/GafferCyclesTest/IECoreCyclesPreviewTest/RendererTest.py b/python/GafferCyclesTest/IECoreCyclesPreviewTest/RendererTest.py index 3783b065a69..7447d2671a8 100644 --- a/python/GafferCyclesTest/IECoreCyclesPreviewTest/RendererTest.py +++ b/python/GafferCyclesTest/IECoreCyclesPreviewTest/RendererTest.py @@ -2938,5 +2938,80 @@ def assertVolumesVisible() : del volume2 del vdb + def testCustomAttributes( self ) : + + ignoreWarningMessage = "Custom attribute \"unsupportedMatrix\" has unsupported type \"M44fData\"." + self.ignoreMessage( IECore.Msg.Level.Warning, "IECoreCycles::Renderer", ignoreWarningMessage ) + + renderer = self.createRenderer() + + renderer.output( + "testOutput", + IECoreScene.Output( + "test", + "ieDisplay", + "rgba", + { + "driverType" : "ImageDisplayDriver", + "handle" : "testCustomAttributes", + } + ) + ) + + # Create many float values as this would make Cycles unstable and "glitch" or crash. + # Also add invalid ones like matrices to make sure these get skipped with a warning. + plane = renderer.object( + "/plane", + IECoreScene.MeshPrimitive.createPlane( + imath.Box2f( imath.V2f( -1 ), imath.V2f( 1 ) ), + ), + renderer.attributes( IECore.CompoundObject ( { + "render:displayColor" : IECore.Color3fData( imath.Color3f( 1, 0.5, 0.25 ) ), + "render:displayOpacity" : IECore.FloatData( 1.0 ), + "render:myFloat" : IECore.FloatData( 0.5 ), + "render:anotherFloat" : IECore.FloatData( 0.25 ), + "render:myBool0" : IECore.BoolData( 0 ), + "render:myBool1" : IECore.BoolData( 1 ), + "render:unsupportedMatrix" : IECore.M44fData( imath.M44f() ), + "cycles:surface" : IECoreScene.ShaderNetwork( + shaders = { + "output" : IECoreScene.Shader( "principled_bsdf", "cycles:surface", { "emission_strength" : 1 } ), + "combineColor" : IECoreScene.Shader( "combine_color", "cycles:shader" ), + "attributeMyFloat" : IECoreScene.Shader( "attribute", "cycles:shader", { "attribute" : IECore.StringData( "myFloat" ) } ), + "attributeAnotherFloat" : IECoreScene.Shader( "attribute", "cycles:shader", { "attribute" : IECore.StringData( "anotherFloat" ) } ), + "attributeMyBool1" : IECoreScene.Shader( "attribute", "cycles:shader", { "attribute" : IECore.StringData( "myBool1" ) } ), + }, + connections = [ + ( ( "attributeMyFloat", "fac" ), ( "combineColor", "r" ) ), + ( ( "attributeAnotherFloat", "fac" ), ( "combineColor", "g" ) ), + ( ( "attributeMyBool1", "fac" ), ( "combineColor", "b" ) ), + ( ( "combineColor", "color" ), ( "output", "emission_color" ) ), + ], + output = "output", + ) + } ) ) + ) + plane.transform( imath.M44f().translate( imath.V3f( 0, 0, -1 ) ) ) + + with IECore.CapturingMessageHandler() as mh : + renderer.render() + for m in mh.messages : + if m.level != IECore.MessageHandler.Level.Warning : + continue + self.assertEqual( m.context, "IECoreCycles::Renderer" ) + self.assertEqual( m.message, ignoreWarningMessage ) + break + + image = IECoreImage.ImageDisplayDriver.storedImage( "testCustomAttributes" ) + self.assertIsInstance( image, IECoreImage.ImagePrimitive ) + + # Slightly off-centre, to avoid triangle edge artifact in centre of image. + testPixel = self.__colorAtUV( image, imath.V2f( 0.55 ) ) + self.assertEqual( testPixel.r, 0.5 ) + self.assertEqual( testPixel.g, 0.25 ) + self.assertEqual( testPixel.b, 1.0 ) + + del plane + if __name__ == "__main__": unittest.main() diff --git a/src/GafferCycles/IECoreCyclesPreview/Renderer.cpp b/src/GafferCycles/IECoreCyclesPreview/Renderer.cpp index 6ee574b2a2d..040bb29a1f9 100644 --- a/src/GafferCycles/IECoreCyclesPreview/Renderer.cpp +++ b/src/GafferCycles/IECoreCyclesPreview/Renderer.cpp @@ -994,7 +994,7 @@ class CyclesAttributes : public IECoreScenePreview::Renderer::AttributesInterfac for( const auto &attr : customMap ) { ccl::ParamValue paramValue = SocketAlgo::setParamValue( attr.first, attr.second.get() ); - if( paramValue.data() ) + if( paramValue.nvalues() == 1 ) { m_custom.push_back( paramValue ); } diff --git a/src/GafferCycles/IECoreCyclesPreview/SocketAlgo.cpp b/src/GafferCycles/IECoreCyclesPreview/SocketAlgo.cpp index c725002b636..d418b00adcd 100644 --- a/src/GafferCycles/IECoreCyclesPreview/SocketAlgo.cpp +++ b/src/GafferCycles/IECoreCyclesPreview/SocketAlgo.cpp @@ -633,23 +633,11 @@ ccl::ParamValue setParamValue( const IECore::InternedString &name, const IECore: return ccl::ParamValue( name.string(), ccl::TypeFloat4, 1, &result ); } break; - case M44fDataTypeId : - { - const M44fData *data = static_cast( value ); - const ccl::Transform result = setTransform( data->readable() ); - return ccl::ParamValue( name.string(), ccl::TypeMatrix, 1, &result ); - } - break; - case M44dDataTypeId : - { - const M44dData *data = static_cast( value ); - const ccl::Transform result = setTransform( data->readable() ); - return ccl::ParamValue( name.string(), ccl::TypeMatrix, 1, &result ); - } - break; default : { - // A ParamValue that we can test with .data() to see if it's a nullptr. + // Don't check with nullptr as OpenImageIO's implementation will store the value + // directly if it is small enough and won't be a pointer of null when using + // .data(), instead check for .nvalues() > 0 on the return ParamValue struct. return ccl::ParamValue(); } }