Merge branch '1.4_maintenance'

Changes.md

@@ -1,7 +1,15 @@
 1.x.x.x (relative to 1.5.0.0a1)
 =======
 
+Fixes
+-----
+
+- Viewer, ImageGadget : [^1]
+  - 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]: To be omitted from 1.5.0.0 release notes.
 
 1.5.0.0a1 (relative to 1.4.13.0)
 =========
@@ -152,7 +160,18 @@ Build
 1.4.x.x (relative to 1.4.13.0)
 =======
 
+Improvements
+------------
 
+- NumericWidget : Added the ability to use <kbd>Ctrl</kbd> + scroll wheel to adjust values in the same manner as <kbd>Up</kbd> and <kbd>Down</kbd> (#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)
 ========
@@ -948,7 +967,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)
 ========

include/GafferImageUI/ImageGadget.h

@@ -291,6 +291,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<Update> &updates );
+			void resetActive();
 
 			// Called from the UI thread.
 			const IECoreGL::Texture *texture( bool &active );

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

python/GafferUI/NumericPlugValueWidget.py

@@ -77,7 +77,7 @@ def getToolTip( self ) :
 			if result :
 				result += "\n"
 			result += "## Actions\n"
-			result += " - Cursor up/down to increment/decrement\n"
+			result += " - Cursor up/down or <kbd>Ctrl</kbd> + scroll wheel to increment/decrement\n"
 			result += " - Use `+`, `-`, `*`, `/` and `%` to perform simple maths\n"
 
 		return result

python/GafferUI/NumericWidget.py

@@ -60,6 +60,7 @@ def __init__( self, value, **kw ) :
 		self.__dragStart = None
 
 		self.keyPressSignal().connect( Gaffer.WeakMethod( self.__keyPress ) )
+		self.wheelSignal().connect( Gaffer.WeakMethod( self.__wheel ) )
 		self.buttonPressSignal().connect( Gaffer.WeakMethod( self.__buttonPress ) )
 		self.dragBeginSignal().connect( Gaffer.WeakMethod( self.__dragBegin ) )
 		self.dragEnterSignal().connect( Gaffer.WeakMethod( self.__dragEnter ) )
@@ -72,6 +73,8 @@ def __init__( self, value, **kw ) :
 		self.__numericType = None
 		self.setValue( value )
 
+		self.__scrollWheelRotation = 0
+
 	def setValue( self, value ) :
 
 		self.__setValueInternal( value, self.ValueChangedReason.SetValue )
@@ -141,6 +144,20 @@ def __keyPress( self, widget, event ) :
 
 		return False
 
+	def __wheel( self, widget, event ) :
+
+		assert( widget is self )
+
+		if not self.getEditable() or not self._qtWidget().hasFocus() or event.modifiers != GafferUI.ModifiableEvent.Modifiers.Control :
+			return False
+
+		self.__scrollWheelRotation += event.wheelRotation
+		if abs( self.__scrollWheelRotation ) >= 15.0 :
+			self.__incrementIndex( self.getCursorPosition(), int( math.copysign( 1, self.__scrollWheelRotation ) ) )
+			self.__scrollWheelRotation %= 15.0
+
+		return True
+
 	def __incrementIndex( self, index, increment ) :
 
 		text = self.getText()
@@ -276,6 +293,8 @@ def __editingFinished( self, widget ) :
 
 		self.__emitValueChanged( reason )
 
+		self.__scrollWheelRotation = 0.0
+
 	def __setValueInternal( self, value, reason ) :
 
 		# update our validator based on the type of the value

python/GafferUI/Widget.py

@@ -1278,7 +1278,11 @@ def __wheel( self, qObject, qEvent ) :
 				Widget._modifiers( qEvent.modifiers() ),
 			)
 
-			return widget._wheelSignal( widget, event )
+			result = widget._wheelSignal( widget, event )
+			if result :
+				qEvent.accept()
+
+			return result
 
 		return False
 

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"
@@ -714,25 +715,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<Update> &updates )
@@ -744,7 +738,7 @@ void ImageGadget::Tile::applyUpdates( const std::vector<Update> &updates )
 
 	for( const auto &u : updates )
 	{
-		if( u.tile )
+		if( u.channelData )
 		{
 			u.tile->m_channelDataToConvert = u.channelData;
 			u.tile->m_channelDataHash = u.channelDataHash;
@@ -758,6 +752,12 @@ void ImageGadget::Tile::applyUpdates( const std::vector<Update> &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();
@@ -855,47 +855,70 @@ void ImageGadget::updateTiles()
 
 	auto tileFunctor = [this, channelsToCompute] ( const ImagePlug *image, const V2i &tileOrigin ) {
 
-		vector<Tile::Update> 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<Tile::Update> 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
 		m_image.get(),
 		// 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;
@@ -905,6 +928,7 @@ void ImageGadget::updateTiles()
 					}
 				);
 			}
+
 		}
 	);