From a94fcd5cf605fdc3bbd824e1e70c02e2d6799b87 Mon Sep 17 00:00:00 2001 From: Daniel Dresser Date: Mon, 15 Dec 2025 16:07:56 -0800 Subject: [PATCH 1/2] IECoreRenderMan : Support overscan --- Changes.md | 1 + python/IECoreRenderManTest/RendererTest.py | 99 ++++++++++++++++++++++ src/IECoreRenderMan/Camera.cpp | 39 +++++++-- 3 files changed, 130 insertions(+), 9 deletions(-) diff --git a/Changes.md b/Changes.md index a426a55880..21dfffe9f6 100644 --- a/Changes.md +++ b/Changes.md @@ -8,6 +8,7 @@ Improvements - LightEditor : Added column for Arnold 7.4.4.0's new `sampling_mode` parameter. - ArnoldShader : Moved Arnold 7.4.4.0's new `standard_hair.scattering_mode` parameter to the "Specular" section of the UI. - ArnoldImager : Added activators for Arnold 7.4.4.0's new `lens_effects` imager parameters. +- RenderMan : Added overscan support. - ShaderTweaks : - Improved SceneInspector integration : - Dragging a parameter name or value from the SceneInspector now creates a tweak containing both the parameter name and value. diff --git a/python/IECoreRenderManTest/RendererTest.py b/python/IECoreRenderManTest/RendererTest.py index 57e7379f34..a52423cd4d 100644 --- a/python/IECoreRenderManTest/RendererTest.py +++ b/python/IECoreRenderManTest/RendererTest.py @@ -38,6 +38,7 @@ import os import time import unittest +import random import imath @@ -2323,6 +2324,104 @@ def testCamera( self ) : tolerance = 1e-7 ) + def runOverscanTest( self, res, pixelsTop, pixelsBottom, pixelsLeft, pixelsRight ) : + + with self.subTest( + res = res, + pixelsTop = pixelsTop, pixelsBottom = pixelsBottom, + pixelsLeft = pixelsLeft, pixelsRight = pixelsRight + ) : + + renderer = GafferScene.Private.IECoreScenePreview.Renderer.create( + self.renderer, + GafferScene.Private.IECoreScenePreview.Renderer.RenderType.Batch + ) + + camera = IECoreScene.Camera() + camera.setResolution( res ) + camera.setOverscan( True ) + + # We've got our own rounding in how renderRegion() is computed - to ensure we + # get the exact pixel amounts of overscan we're requesting, we offset by + # a quarter pixel. + camera.setOverscanTop( ( pixelsTop + 0.25 ) / res[1] ) + camera.setOverscanBottom( ( pixelsBottom + 0.25 ) / res[1] ) + camera.setOverscanLeft( ( pixelsLeft + 0.25 ) / res[0] ) + camera.setOverscanRight( ( pixelsRight + 0.25 ) / res[0] ) + + self.assertEqual( + camera.renderRegion(), + imath.Box2i( + imath.V2i( -pixelsLeft, -pixelsBottom ), + res + imath.V2i( pixelsRight, pixelsTop ) + ) + ) + + renderCam = renderer.camera( + "camera", camera, renderer.attributes( IECore.CompoundObject() ) + ) + + renderer.option( "camera", IECore.StringData( "camera" ) ) + + renderer.output( + f"test", + IECoreScene.Output( + str( self.temporaryDirectory() / f"test.exr" ), + "exr", + "rgb", + {} + ) + ) + + renderer.render() + del renderer + + image = OpenImageIO.ImageBuf( str( self.temporaryDirectory() / f"test.exr" ) ) + + self.assertEqual( image.spec().width, res[0] + pixelsLeft + pixelsRight ) + self.assertEqual( image.spec().height, res[1] + pixelsTop + pixelsBottom ) + self.assertEqual( image.spec().x, -pixelsLeft ) + self.assertEqual( image.spec().y, -pixelsTop ) + + self.assertEqual( image.spec().full_width, res[0] ) + self.assertEqual( image.spec().full_height, res[1] ) + self.assertEqual( image.spec().full_x, 0 ) + self.assertEqual( image.spec().full_y, 0 ) + + @unittest.skipIf( True, "Fuzzing the overscan values is too expensive to run regularly" ) + def testFuzzOverscan( self ): + random.seed( 42 ) + for i in range( 40 ): + self.runOverscanTest( + imath.V2i( random.randint( 100, 1000 ), random.randint( 100, 1000 ) ), + random.randint( 0, 500 ), random.randint( 0, 500 ), + random.randint( 0, 500 ), random.randint( 0, 500 ) + ) + + # We can more efficiently check the rounding behaviours for much larger images by + # using test images that are one pixel tall or one pixel wide + for i in range( 1000 ): + self.runOverscanTest( + imath.V2i( random.randint( 100, 16000 ), 1 ), + 0, 0, + random.randint( 0, 1000 ), random.randint( 0, 1000 ) + ) + + for i in range( 1000 ): + self.runOverscanTest( + imath.V2i( 1, random.randint( 100, 16000 ) ), + random.randint( 0, 1000 ), random.randint( 0, 1000 ), + 0, 0 + ) + + def testOverscan( self ): + # Test some simple overscan values, and some that were specifically chosen to + # fail if a naive division without rounding compensation is used. + self.runOverscanTest( imath.V2i( 100, 100 ), 1, 2, 3, 4 ) + self.runOverscanTest( imath.V2i( 200, 200 ), 14, 13, 12, 11 ) + self.runOverscanTest( imath.V2i( 750, 750 ), 249, 249, 249, 249 ) + self.runOverscanTest( imath.V2i( 162, 512 ), 745, 347, 819, 882 ) + def __assertParameterEqual( self, paramList, name, data, tolerance = None ) : p = next( x for x in paramList if x["info"]["name"] == name ) diff --git a/src/IECoreRenderMan/Camera.cpp b/src/IECoreRenderMan/Camera.cpp index 52c33e2478..2764c454dd 100644 --- a/src/IECoreRenderMan/Camera.cpp +++ b/src/IECoreRenderMan/Camera.cpp @@ -59,9 +59,31 @@ const RtUString g_projectionHandle( "projection" ); const RtUString g_pxrCamera( "PxrCamera" ); const RtUString g_pxrOrthographic( "PxrOrthographic" ); +float divRoundDown( int a, int b ) +{ + // PRMan is going to perform a ceil rather than a round on these values when it converts to + // integers, so we need to make sure we return a value less than the precise value. There + // are two sources of imprecision here: + // * converting integers to floats + // * performing the division + // If the arguments are less than 16 million, the first is not an issue, since those integers + // are exactly representable. We don't expect images to be that big, so we ignore the first issue. + // + // This just leaves any imprecision created by the division itself. The floating point result + // should be the closest representable float, so a single call to nextafterf to pick the next + // lowest float should ensure that we are always less than the true value, and PRMan's ceil + // should pick the correct integer. ( This is probably also reliant on the fact that when + // PRMan performs the multiplication by resolution, the resolution is also an integer less + // than 16 million. ) + + return nextafterf( + float( a ) / float( b ), + -std::numeric_limits::infinity() + ); +} + } // namespace -/// \todo Overscan, depth of field Camera::Camera( const std::string &name, const IECoreScene::Camera *camera, Session *session ) : m_session( session ) { @@ -130,14 +152,13 @@ Camera::Camera( const std::string &name, const IECoreScene::Camera *camera, Sess options.SetIntegerArray( Loader::strings().k_Ri_FormatResolution, resolution.getValue(), 2 ); options.SetFloat( Loader::strings().k_Ri_FormatPixelAspectRatio, camera->getPixelAspectRatio() ); - Box2f cropWindow = camera->getCropWindow(); - if( cropWindow.isEmpty() ) - { - /// \todo Would be better if IECoreScene::Camera defaulted to this rather - /// than empty box. - cropWindow = Box2f( V2f( 0 ), V2f( 1 ) ); - } - float renderManCropWindow[4] = { cropWindow.min.x, cropWindow.max.x, cropWindow.min.y, cropWindow.max.y }; + Imath::Box2i renderRegion = camera->renderRegion(); + float renderManCropWindow[4] = { + divRoundDown( renderRegion.min.x, resolution.x ), + divRoundDown( renderRegion.max.x, resolution.x ), + divRoundDown( resolution.y - renderRegion.min.y, resolution.y ), + divRoundDown( resolution.y - renderRegion.max.y, resolution.y ) + }; options.SetFloatArray( Loader::strings().k_Ri_CropWindow, renderManCropWindow, 4 ); // Camera From fa8e230baec290d8f1e07f9327979660f372da60 Mon Sep 17 00:00:00 2001 From: Daniel Dresser Date: Mon, 15 Dec 2025 18:19:49 -0800 Subject: [PATCH 2/2] IECoreRenderManDisplay : Fix XPU support for crop and overscan. --- Changes.md | 1 + .../InteractiveRenderManRenderTest.py | 17 +- src/IECoreRenderManDisplay/Display.cpp | 160 ++++++++++-------- 3 files changed, 106 insertions(+), 72 deletions(-) diff --git a/Changes.md b/Changes.md index 21dfffe9f6..0551a7b97e 100644 --- a/Changes.md +++ b/Changes.md @@ -28,6 +28,7 @@ Fixes - SceneInspector : Fixed cell background colour updates when changing EditScope. - AttributeEditor, SceneInspector : Fixed bug preventing edits from being created in an EditScope for attributes with `.` characters in their name. +- RenderMan : Fixed crop window bugs when rendering with XPU (#6727). 1.6.7.0 (relative to 1.6.6.1) ======= diff --git a/python/GafferRenderManTest/InteractiveRenderManRenderTest.py b/python/GafferRenderManTest/InteractiveRenderManRenderTest.py index 4f0e9971c0..975e13088b 100644 --- a/python/GafferRenderManTest/InteractiveRenderManRenderTest.py +++ b/python/GafferRenderManTest/InteractiveRenderManRenderTest.py @@ -50,14 +50,19 @@ class InteractiveRenderManRenderTest( GafferSceneTest.InteractiveRenderTest ) : renderer = "RenderMan" - @unittest.skip( "Crop window doesn't change data window" ) def testEditCropWindow( self ) : - # RenderMan doesn't reopen the display drivers when the crop - # window decreases in size, only when it increases. This will - # cause the base class test to fail, even though we are passing - # edits and RenderMan is only re-rendering the requested area. - pass + if self.renderer == "RenderMan" : + # RIS doesn't reopen the display drivers when the crop + # window decreases in size, only when it increases. This will + # cause the base class test to fail, even though we are passing + # edits and RenderMan is only re-rendering the requested area. + ## \todo Perhaps we can deal with this ourself by recreating + # the RenderView to force reopening? Or modifying a single camera + # instead of making a new one when the crop changes? + self.skipTest( "Crop window doesn't change data window" ) + else : + GafferSceneTest.InteractiveRenderTest.testEditCropWindow( self ) def _createConstantShader( self ) : diff --git a/src/IECoreRenderManDisplay/Display.cpp b/src/IECoreRenderManDisplay/Display.cpp index 73d58f51a6..f9e56bde79 100644 --- a/src/IECoreRenderManDisplay/Display.cpp +++ b/src/IECoreRenderManDisplay/Display.cpp @@ -449,7 +449,7 @@ struct IEDisplay : public display::Display { IEDisplay( const pxrcore::ParamList ¶mList, const pxrcore::ParamList &metadata ) - : m_parameters( new CompoundData() ) + : m_parameters( new CompoundData() ), m_driverOutOfDate( true ) { // Convert parameter list into the form needed by `IECoreImage::DisplayDriver::create()`. pxrcore::ParamList::ParamInfo paramInfo; @@ -496,78 +496,58 @@ struct IEDisplay : public display::Display const pxrcore::ParamList ¶ms ) override { - // Create an `IECoreImage::DisplayDriver` with the appropriate number of channels, - // and fill `m_channelPointers` with the source data to copy each channel from in - // `Notify()`. - try - { - m_channelPointers.clear(); + // Store the channel names, channel pointers, and buffer width, so that we will know how intepret + // the data when we receive a buffer update. + m_bufferWidth = width; - if( m_driver ) + m_channelNames.clear(); + m_channelPointers.clear(); + + m_driverOutOfDate = true; + + for( size_t outputIndex = 0; outputIndex < noutputs; ++outputIndex ) + { + const display::RenderOutput &output = outputs[outputIndex]; + if( output.nelems == 1 ) { - m_driver->imageClose(); + std::string baseName = output.displayName.CStr(); + if( baseName == "a" ) baseName = "A"; + else if( baseName == "z" ) baseName = "Z"; + m_channelNames.push_back( baseName ); } - - std::vector channelNames; - for( size_t outputIndex = 0; outputIndex < noutputs; ++outputIndex ) + else { - const display::RenderOutput &output = outputs[outputIndex]; - if( output.nelems == 1 ) + string layerName = output.displayName.CStr(); + if( layerName == "Ci" ) { - std::string baseName = output.displayName.CStr(); - if( baseName == "a" ) baseName = "A"; - else if( baseName == "z" ) baseName = "Z"; - channelNames.push_back( baseName ); + layerName = ""; } - else + for( uint8_t elementIndex = 0; elementIndex < output.nelems; elementIndex++ ) { - string layerName = output.displayName.CStr(); - if( layerName == "Ci" ) + std::string baseName = output.displaySuffix[elementIndex].CStr(); + if( baseName == "r" ) baseName = "R"; + else if( baseName == "g" ) baseName = "G"; + else if( baseName == "b" ) baseName = "B"; + + if( layerName.empty() ) { - layerName = ""; + m_channelNames.push_back( baseName ); } - for( uint8_t elementIndex = 0; elementIndex < output.nelems; elementIndex++ ) + else { - std::string baseName = output.displaySuffix[elementIndex].CStr(); - if( baseName == "r" ) baseName = "R"; - else if( baseName == "g" ) baseName = "G"; - else if( baseName == "b" ) baseName = "B"; - - if( layerName.empty() ) - { - channelNames.push_back( baseName ); - } - else - { - channelNames.push_back( layerName + "." + baseName ); - } + m_channelNames.push_back( layerName + "." + baseName ); } } - - const float *channelPointer = reinterpret_cast( static_cast( srfaddr ) + offsets[outputIndex] ); - for( size_t element = 0; element < output.nelems; ++element ) - { - m_channelPointers.push_back( channelPointer ); - channelPointer += width * height; - } } - m_displayWindow = Box2i( V2i( 0 ), V2i( width - 1, height - 1 ) ); - m_dataWindow = m_displayWindow; - if( const auto cropWindow = m_parameters->member( "CropWindow" ) ) + const float *channelPointer = reinterpret_cast( static_cast( srfaddr ) + offsets[outputIndex] ); + for( size_t element = 0; element < output.nelems; ++element ) { - m_dataWindow.min = V2i( cropWindow->readable()[0], cropWindow->readable()[1] ); - m_dataWindow.max = V2i( cropWindow->readable()[2], cropWindow->readable()[3] ); + m_channelPointers.push_back( channelPointer ); + channelPointer += width * height; } - - const StringData *driverType = m_parameters->member( "driverType", /* throwIfMissing = */ true ); - m_driver = IECoreImage::DisplayDriver::create( driverType->readable(), m_displayWindow, m_dataWindow, channelNames, m_parameters ); - } - catch( const std::exception &e ) - { - IECore::msg( IECore::Msg::Error, "IEDisplay", e.what() ); - return false; } + return true; } @@ -583,12 +563,54 @@ struct IEDisplay : public display::Display return; } - const size_t width = m_dataWindow.size().x + 1; - const size_t height = m_dataWindow.size().y + 1 ; + static const pxrcore::UString originalSizeName( "OriginalSize" ); + static const pxrcore::UString originName( "origin" ); + static const pxrcore::UString cropWindowName( "CropWindow" ); + + const int *origSize = metadata.GetIntegerArray( originalSizeName, 2 ); + const int *origin = metadata.GetIntegerArray( originName, 2 ); + const int *cropWindow = metadata.GetIntegerArray( cropWindowName, 4 ); + + if( !( origSize && origin && cropWindow ) ) + { + IECore::msg( IECore::Msg::Error, "IEDisplay", "A built-in RenderMan param was not provided to IEDisplay - this suggests the RenderMan API has changed, and IEDisplay needs updating" ); + return; + } + + Box2i displayWindow = Box2i( V2i( 0 ), V2i( origSize[0] - 1, origSize[1] - 1 ) ); + Box2i newDataWindow( V2i( cropWindow[0], cropWindow[1] ), V2i( cropWindow[2], cropWindow[3] ) ); + + if( newDataWindow != m_dataWindow ) + { + m_driverOutOfDate = true; + m_dataWindow = newDataWindow; + } + + if( m_driverOutOfDate || !m_driver ) + { + // We hold the old driver until after creating the new driver, which allows + // the catalogue to recognize that the driver matches, and should still be writing to the same + // catalogue image. + IECoreImage::DisplayDriverPtr oldDriver = m_driver; + + const StringData *driverType = m_parameters->member( "driverType", /* throwIfMissing = */ true ); + + m_driver = IECoreImage::DisplayDriver::create( driverType->readable(), displayWindow, m_dataWindow, m_channelNames, m_parameters ); + + if( oldDriver ) + { + oldDriver->imageClose(); + } + + m_driverOutOfDate = false; + } + + const size_t dataWidth = m_dataWindow.size().x + 1; + const size_t dataHeight = m_dataWindow.size().y + 1 ; const size_t numChannels = m_channelPointers.size(); - const size_t bufferSize = width * height *numChannels; - const size_t offset = m_dataWindow.min.y * ( m_displayWindow.size().x + 1 ) + m_dataWindow.min.x; - const size_t stride = m_displayWindow.size().x - m_dataWindow.size().x; + const size_t bufferSize = dataWidth * dataHeight * numChannels; + const size_t offset = ( m_dataWindow.min.y - origin[1] ) * m_bufferWidth + m_dataWindow.min.x - origin[0]; + const size_t skippedElementsPerScanline = m_bufferWidth - dataWidth; std::unique_ptr buffer( new float[bufferSize] ); @@ -596,14 +618,14 @@ struct IEDisplay : public display::Display { float *out = buffer.get() + channelIndex; const float *in = m_channelPointers[channelIndex] + offset; - for( size_t y = 0; y < height; ++y ) + for( size_t y = 0; y < dataHeight; ++y ) { - for( size_t x = 0; x < width; ++x ) + for( size_t x = 0; x < dataWidth; ++x ) { *out = *in++; out += numChannels; } - in += stride; + in += skippedElementsPerScanline; } } @@ -619,7 +641,11 @@ struct IEDisplay : public display::Display { try { - m_driver->imageClose(); + // \todo - seems odd that the destructor of DisplayDriver doesn't take care of calling imageClose()? + if( m_driver ) + { + m_driver->imageClose(); + } m_driver = nullptr; } catch( const std::exception &e ) @@ -631,9 +657,11 @@ struct IEDisplay : public display::Display private : CompoundDataPtr m_parameters; - Box2i m_displayWindow; Box2i m_dataWindow; + size_t m_bufferWidth; + bool m_driverOutOfDate; IECoreImage::DisplayDriverPtr m_driver; + vector m_channelNames; vector m_channelPointers; };