From a6e6cbfd415643e829e42303b7e34b29a372cf70 Mon Sep 17 00:00:00 2001 From: Eric Mehl Date: Fri, 13 Sep 2024 11:42:38 -0400 Subject: [PATCH 1/5] NumericWidget : Mouse wheel adjust value Fixes #6009. --- Changes.md | 3 +++ python/GafferUI/NumericPlugValueWidget.py | 2 +- python/GafferUI/NumericWidget.py | 19 +++++++++++++++++++ python/GafferUI/Widget.py | 6 +++++- 4 files changed, 28 insertions(+), 2 deletions(-) diff --git a/Changes.md b/Changes.md index 42412e23423..be7c3247983 100644 --- a/Changes.md +++ b/Changes.md @@ -1,7 +1,10 @@ 1.4.x.x (relative to 1.4.13.0) ======= +Improvements +------------ +- NumericWidget : Added the ability to use Ctrl + scroll wheel to adjust values in the same manner as Up and Down (#6009). 1.4.13.0 (relative to 1.4.12.0) ======== diff --git a/python/GafferUI/NumericPlugValueWidget.py b/python/GafferUI/NumericPlugValueWidget.py index 6226437b8b8..7c6a4be4c71 100644 --- a/python/GafferUI/NumericPlugValueWidget.py +++ b/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 Ctrl + scroll wheel to increment/decrement\n" result += " - Use `+`, `-`, `*`, `/` and `%` to perform simple maths\n" return result diff --git a/python/GafferUI/NumericWidget.py b/python/GafferUI/NumericWidget.py index 2ed1f2811f9..5581f2681db 100644 --- a/python/GafferUI/NumericWidget.py +++ b/python/GafferUI/NumericWidget.py @@ -60,6 +60,7 @@ def __init__( self, value, **kw ) : self.__dragStart = None self.keyPressSignal().connect( Gaffer.WeakMethod( self.__keyPress ), scoped = False ) + self.wheelSignal().connect( Gaffer.WeakMethod( self.__wheel ), scoped = False ) self.buttonPressSignal().connect( Gaffer.WeakMethod( self.__buttonPress ), scoped = False ) self.dragBeginSignal().connect( Gaffer.WeakMethod( self.__dragBegin ), scoped = False ) self.dragEnterSignal().connect( Gaffer.WeakMethod( self.__dragEnter ), scoped = False ) @@ -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 diff --git a/python/GafferUI/Widget.py b/python/GafferUI/Widget.py index 15b3b77e7f4..25cf66e2bae 100644 --- a/python/GafferUI/Widget.py +++ b/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 From a510842a6ed793ab8420120a2c89e5942b7946d8 Mon Sep 17 00:00:00 2001 From: John Haddon Date: Tue, 17 Sep 2024 15:05:39 +0100 Subject: [PATCH 2/5] ImageGadget : Remove outdated comment The ImageGadget no longer has any internal nodes. --- src/GafferImageUI/ImageGadget.cpp | 8 -------- 1 file changed, 8 deletions(-) diff --git a/src/GafferImageUI/ImageGadget.cpp b/src/GafferImageUI/ImageGadget.cpp index 8b4f981ae21..378ee47b1f1 100644 --- a/src/GafferImageUI/ImageGadget.cpp +++ b/src/GafferImageUI/ImageGadget.cpp @@ -882,14 +882,6 @@ void ImageGadget::updateTiles() } }; - - // 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 From 2e4a415c132d9d90b5105850af58e67ef7e66310 Mon Sep 17 00:00:00 2001 From: John Haddon Date: Tue, 17 Sep 2024 14:22:37 +0100 Subject: [PATCH 3/5] ImageGadget : Fix unnecessary texture updates When an image tile doesn't need updating, `Tile::computeUpdate()` still returns an `Update` struct, but with `channelData == null`. When processing that in `Tile::applyUpdates()`, we were zeroing out `Tile::m_channelDataHash`, which would cause the next update to do completely unnecessary work. Now we don't do that. --- Changes.md | 3 +++ python/GafferImageUITest/ImageGadgetTest.py | 25 +++++++++++++++++++++ src/GafferImageUI/ImageGadget.cpp | 2 +- 3 files changed, 29 insertions(+), 1 deletion(-) diff --git a/Changes.md b/Changes.md index 0fb7d99fc82..e53c52a0ac3 100644 --- a/Changes.md +++ b/Changes.md @@ -1,7 +1,10 @@ 1.3.16.x (relative to 1.3.16.8) ======== +Fixes +----- +- Viewer, ImageGadget : Fixed unnecessary texture updates when specific image tiles don't change. 1.3.16.8 (relative to 1.3.16.7) ======== 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 378ee47b1f1..00423dec669 100644 --- a/src/GafferImageUI/ImageGadget.cpp +++ b/src/GafferImageUI/ImageGadget.cpp @@ -747,7 +747,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; From c8ba392ea6a27c02888a9cb11a8cfaadf743c635 Mon Sep 17 00:00:00 2001 From: John Haddon Date: Wed, 18 Sep 2024 11:55:27 +0100 Subject: [PATCH 4/5] ImageGadget : Fix "colour tearing" under cancellation Because of the way we were suppressing exceptions in `Tile::computeUpdate()`, we could end up calling `Tile::applyUpdates()` with an incomplete set of updates (where some have null `channelData`). This could cause us to display a tile with a mix of updated and stale channels, causing the very "colour tearing" that `applyUpdates()` was intended to avoid. We now handle exceptions one level higher up in the `tileFunctor`, allowing us to clear the active flags for all channels, but _not_ apply any channel data updates. This approach is much closer to the one originally proposed in #4011, which I replaced with a botched version in #5348. --- Changes.md | 5 +- include/GafferImageUI/ImageGadget.h | 1 + src/GafferImageUI/ImageGadget.cpp | 79 +++++++++++++++++------------ 3 files changed, 51 insertions(+), 34 deletions(-) diff --git a/Changes.md b/Changes.md index e53c52a0ac3..a22c8f457a1 100644 --- a/Changes.md +++ b/Changes.md @@ -1,10 +1,13 @@ + 1.3.16.x (relative to 1.3.16.8) ======== Fixes ----- -- Viewer, ImageGadget : Fixed unnecessary texture updates when specific image tiles don't change. +- Viewer, ImageGadget : + - 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/src/GafferImageUI/ImageGadget.cpp b/src/GafferImageUI/ImageGadget.cpp index 00423dec669..d7ee10afdd2 100644 --- a/src/GafferImageUI/ImageGadget.cpp +++ b/src/GafferImageUI/ImageGadget.cpp @@ -717,25 +717,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 ) @@ -761,6 +754,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,28 +857,42 @@ 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(); + } } + }; Context::Scope scopedContext( m_context.get() ); From 2683c5d48760dfda135ccd1e3fba6f6814c2b622 Mon Sep 17 00:00:00 2001 From: John Haddon Date: Wed, 18 Sep 2024 15:20:24 +0100 Subject: [PATCH 5/5] ImageGadget : Don't clear dirty flag after cancellation In most cases this didn't matter, because we were only getting cancelled by a graph edit which would eventually dirty the input plug anyway. But cancellation could be due to an edit in an unrelated part of the graph, in which case our input plug won't be dirtied again, but we do want to restart the update. Fixes #6043 --- Changes.md | 2 +- src/GafferImageUI/ImageGadget.cpp | 23 +++++++++++++++++++++-- 2 files changed, 22 insertions(+), 3 deletions(-) diff --git a/Changes.md b/Changes.md index a22c8f457a1..4108f9ce326 100644 --- a/Changes.md +++ b/Changes.md @@ -1,4 +1,3 @@ - 1.3.16.x (relative to 1.3.16.8) ======== @@ -6,6 +5,7 @@ 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. diff --git a/src/GafferImageUI/ImageGadget.cpp b/src/GafferImageUI/ImageGadget.cpp index d7ee10afdd2..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" @@ -891,6 +892,7 @@ void ImageGadget::updateTiles() { m_tiles[TileIndex(tileOrigin, channelName)].resetActive(); } + throw; } }; @@ -902,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; @@ -913,6 +931,7 @@ void ImageGadget::updateTiles() } ); } + } );