diff --git a/Changes.md b/Changes.md index be7c3247983..3896ec7e576 100644 --- a/Changes.md +++ b/Changes.md @@ -6,6 +6,14 @@ Improvements - NumericWidget : Added the ability to use Ctrl + scroll wheel to adjust values in the same manner as Up and Down (#6009). +Fixes +----- + +- Viewer, ImageGadget : + - Fixed partial image updates when an unrelated InteractiveRender was running (#6043). + - Fixed "colour tearing", where updates to some image channels became visible before updates to others. + - Fixed unnecessary texture updates when specific image tiles don't change. + 1.4.13.0 (relative to 1.4.12.0) ======== @@ -803,7 +811,13 @@ Build 1.3.16.x (relative to 1.3.16.8) ======== +Fixes +----- +- Viewer, ImageGadget : + - Fixed partial image updates when an unrelated InteractiveRender was running (#6043). + - Fixed "colour tearing", where updates to some image channels became visible before updates to others. + - Fixed unnecessary texture updates when specific image tiles don't change. 1.3.16.8 (relative to 1.3.16.7) ======== diff --git a/include/GafferImageUI/ImageGadget.h b/include/GafferImageUI/ImageGadget.h index 0c8bfe36fa2..0cd2f02dab5 100644 --- a/include/GafferImageUI/ImageGadget.h +++ b/include/GafferImageUI/ImageGadget.h @@ -292,6 +292,7 @@ class GAFFERIMAGEUI_API ImageGadget : public GafferUI::Gadget // Applies previously computed updates for several tiles // such that they become visible to the UI thread together. static void applyUpdates( const std::vector &updates ); + void resetActive(); // Called from the UI thread. const IECoreGL::Texture *texture( bool &active ); diff --git a/python/GafferImageUITest/ImageGadgetTest.py b/python/GafferImageUITest/ImageGadgetTest.py index 8a947a2ed0e..80c1a3927ec 100644 --- a/python/GafferImageUITest/ImageGadgetTest.py +++ b/python/GafferImageUITest/ImageGadgetTest.py @@ -34,6 +34,7 @@ # ########################################################################## +import time import unittest import imath @@ -120,5 +121,29 @@ def testStateChangedSignal( self ) : self.assertEqual( len( cs ), 2 ) self.assertNotEqual( gadget.state(), gadget.State.Paused ) + def testNoUnecessaryUpdates( self ) : + + script = Gaffer.ScriptNode() + script["image"] = GafferImage.Checkerboard() + script["image"]["format"].setValue( GafferImage.Format( GafferImage.ImagePlug.tileSize(), GafferImage.ImagePlug.tileSize() ) ) + + gadget = GafferImageUI.ImageGadget() + gadget.setImage( script["image"]["out"] ) + gadget.setContext( script.context() ) + + with GafferUI.Window() as window : + GafferUI.GadgetWidget( gadget ) + + GafferImageUI.ImageGadget.resetTileUpdateCount() + window.setVisible( True ) + while GafferImageUI.ImageGadget.tileUpdateCount() < 4 : + self.waitForIdle() + + for frame in range( 2, 4 ) : + script.context().setFrame( frame ) + time.sleep( 0.5 ) + self.waitForIdle() + self.assertEqual( GafferImageUI.ImageGadget.tileUpdateCount(), 4 ) + if __name__ == "__main__": unittest.main() diff --git a/src/GafferImageUI/ImageGadget.cpp b/src/GafferImageUI/ImageGadget.cpp index 8b4f981ae21..dbb4c7add1f 100644 --- a/src/GafferImageUI/ImageGadget.cpp +++ b/src/GafferImageUI/ImageGadget.cpp @@ -53,6 +53,7 @@ #include "Gaffer/BackgroundTask.h" #include "Gaffer/Context.h" #include "Gaffer/Node.h" +#include "Gaffer/Process.h" #include "Gaffer/ScriptNode.h" #include "IECore/MessageHandler.h" @@ -717,25 +718,18 @@ ImageGadget::Tile::Tile( const Tile &other ) ImageGadget::Tile::Update ImageGadget::Tile::computeUpdate( const GafferImage::ImagePlug *image ) { - try - { - const IECore::MurmurHash h = image->channelDataPlug()->hash(); - Mutex::scoped_lock lock( m_mutex ); - if( m_channelDataHash != MurmurHash() && m_channelDataHash == h ) - { - return Update{ this, nullptr, MurmurHash() }; - } - - m_active = true; - m_activeStartTime = std::chrono::steady_clock::now(); - lock.release(); // Release while doing expensive calculation so UI thread doesn't wait. - ConstFloatVectorDataPtr channelData = image->channelDataPlug()->getValue( &h ); - return Update{ this, channelData, h }; - } - catch( ... ) + const IECore::MurmurHash h = image->channelDataPlug()->hash(); + Mutex::scoped_lock lock( m_mutex ); + if( m_channelDataHash != MurmurHash() && m_channelDataHash == h ) { return Update{ this, nullptr, MurmurHash() }; } + + m_active = true; + m_activeStartTime = std::chrono::steady_clock::now(); + lock.release(); // Release while doing expensive calculation so UI thread doesn't wait. + ConstFloatVectorDataPtr channelData = image->channelDataPlug()->getValue( &h ); + return Update{ this, channelData, h }; } void ImageGadget::Tile::applyUpdates( const std::vector &updates ) @@ -747,7 +741,7 @@ void ImageGadget::Tile::applyUpdates( const std::vector &updates ) for( const auto &u : updates ) { - if( u.tile ) + if( u.channelData ) { u.tile->m_channelDataToConvert = u.channelData; u.tile->m_channelDataHash = u.channelDataHash; @@ -761,6 +755,12 @@ void ImageGadget::Tile::applyUpdates( const std::vector &updates ) } } +void ImageGadget::Tile::resetActive() +{ + Mutex::scoped_lock lock( m_mutex ); + m_active = false; +} + const IECoreGL::Texture *ImageGadget::Tile::texture( bool &active ) { const auto now = std::chrono::steady_clock::now(); @@ -858,38 +858,45 @@ void ImageGadget::updateTiles() auto tileFunctor = [this, channelsToCompute] ( const ImagePlug *image, const V2i &tileOrigin ) { - vector updates; - ImagePlug::ChannelDataScope channelScope( Context::current() ); - for( auto &channelName : channelsToCompute ) + try { - channelScope.setChannelName( &channelName ); - Tile &tile = m_tiles[TileIndex(tileOrigin, channelName)]; - updates.push_back( tile.computeUpdate( image ) ); - } + vector updates; + ImagePlug::ChannelDataScope channelScope( Context::current() ); + for( auto &channelName : channelsToCompute ) + { + channelScope.setChannelName( &channelName ); + Tile &tile = m_tiles[TileIndex(tileOrigin, channelName)]; + updates.push_back( tile.computeUpdate( image ) ); + } - Tile::applyUpdates( updates ); + Tile::applyUpdates( updates ); - if( refCount() && !m_renderRequestPending.exchange( true ) ) + if( refCount() && !m_renderRequestPending.exchange( true ) ) + { + // Must hold a reference to stop us dying before our UI thread call is scheduled. + ImageGadgetPtr thisRef = this; + ParallelAlgo::callOnUIThread( + [thisRef] { + thisRef->m_renderRequestPending = false; + thisRef->Gadget::dirty( DirtyType::Render ); + } + ); + } + } + catch( ... ) { - // Must hold a reference to stop us dying before our UI thread call is scheduled. - ImageGadgetPtr thisRef = this; - ParallelAlgo::callOnUIThread( - [thisRef] { - thisRef->m_renderRequestPending = false; - thisRef->Gadget::dirty( DirtyType::Render ); - } - ); + // We don't want to call `Tile::applyUpdates()` because we won't have + // a complete set of updates for all channels. But we do need to turn off + // the active flag for each tile. + for( auto &channelName : channelsToCompute ) + { + m_tiles[TileIndex(tileOrigin, channelName)].resetActive(); + } + throw; } - }; + }; - // callOnBackgroundThread requires a "subject" that will trigger task cancellation - // when dirtied. This subject usually needs to be in a script, but there's a special - // case in BackgroundTask::scriptNode for nodes that are in a GafferUI::View. We - // can work with this by passing in m_image, which is passed to us by ImageView. - // This means that any internal nodes of ImageGadget are not part of the automatic - // task cancellation and we must ensure that we never modify internal nodes while - // the background task is running ( this is easier now that there are no internal nodes ). Context::Scope scopedContext( m_context.get() ); m_tilesTask = ParallelAlgo::callOnBackgroundThread( // Subject @@ -897,8 +904,24 @@ void ImageGadget::updateTiles() // OK to capture `this` via raw pointer, because ~ImageGadget waits for // the background process to complete. [ this, channelsToCompute, dataWindow, tileFunctor ] { - ImageAlgo::parallelProcessTiles( m_image.get(), tileFunctor, dataWindow ); - m_dirtyFlags &= ~TilesDirty; + + try + { + ImageAlgo::parallelProcessTiles( m_image.get(), tileFunctor, dataWindow ); + m_dirtyFlags &= ~TilesDirty; + } + catch( const Gaffer::ProcessException & ) + { + // No point starting a new compute if it's just + // going to error again. + m_dirtyFlags &= ~TilesDirty; + } + catch( const IECore::Cancelled & ) + { + // Don't clear dirty flag, so that we restart + // on the next redraw. + } + if( refCount() ) { ImageGadgetPtr thisRef = this; @@ -908,6 +931,7 @@ void ImageGadget::updateTiles() } ); } + } );