Deep Pixel Inspector #5658
Deep Pixel Inspector #5658
Conversation
There was a problem hiding this comment.
Thanks Daniel, this is super handy!
As requested, I haven't reviewed DeepSampleGadget yet, but I've reviewed the rest of the code, and tried to provide some direction with regard to the "plugs in the UI that are probably naughty".
I also have some notes from a usability perspective, some of which I'm sure you're already aware of :
- We need a different icon for the "show deep info" button. Maybe a stair-steppy graph?
- The buttons should be disabled when the image is flat, or the inspector is area-based.
- It feels like the windows need to do something when the image becomes flat while they are open - probably just close themselves?
- The auto-framing seems to include an unnecessary bunch of the negative Y axis at times. I think it probably makes sense to always have
Y = 0in the same place at the bottom of the viewport at all times (unless there are actually negative values) so there's at least one consistent visual anchor. I find the auto-framing useful, but also slightly disorienting, and I think that might help. - The auto-framing doesn't seem to work in logarithmic mode, when there are colour values greater than 1.
- The label for
Y==0sometimes says-0.000rather than0.000. - Maybe "Deep Inspector" or "Deep Samples Inspector" would be a better window title?
- Perhaps the window title could include the numeric index of the colour inspector to which it belongs?
Cheers...
John
| if self.__deepPixelInfo: | ||
| self.__updateDeepInBackground( sources ) |
There was a problem hiding this comment.
I wonder if it's worth doing this in the main __updateInBackground call, rather than spawning two background tasks? They're both going to want the same data, so there's a decent chance that they'll be computing something redundantly if they're in separate tasks.
| @@ -1269,8 +1270,10 @@ class ImageView::ColorInspector : public Signals::Trackable | |||
| : m_view( view ), | |||
| m_contextQuery( new ContextQuery ), | |||
| m_deleteContextVariables( new DeleteContextVariables ), | |||
| m_deepDeleteContextVariables( new DeleteContextVariables ), | |||
There was a problem hiding this comment.
I seem to be missing the reason why we need a separate DeleteContextVariables - as far as I can see, it is set up identically to the original one.
| V2iPlugPtr v2iTemplate = new Gaffer::V2iPlug( "v2iTemplate" ); | ||
| m_contextQuery->addQuery( v2iTemplate.get(), "colorInspector:source" ); |
There was a problem hiding this comment.
Do we definitely need another query? What happens if we just connect the V2f output into our V2i plug on the DeepSampler?
| self.__p1 = GafferUI.PlugWidget( self.__pixel ) | ||
| self.__p2 = GafferUI.PlugWidget( GafferImageUI.RGBAChannelsPlugValueWidget( self.__channels ) ) | ||
| self.__p3 = GafferUI.PlugWidget( self.__autoFrame ) | ||
| self.__p4 = GafferUI.PlugWidget( self.__logarithmic ) |
There was a problem hiding this comment.
No need to store these as member variables if we don't access them later.
| self.__pixel = Gaffer.V2iPlug( "pixelCoordinates" ) | ||
| Gaffer.Metadata.registerValue( self.__pixel, "readOnly", True ) | ||
| self.__channels = Gaffer.StringVectorDataPlug( "channels", Gaffer.Plug.Direction.In, IECore.StringVectorData() ) | ||
| Gaffer.Metadata.registerValue( self.__channels, "readOnly", True ) |
There was a problem hiding this comment.
It feels awkward to be using plugs for these if they aren't intended to be modified by the user. My first guess for channels was that it was intended to be editable, but defaulted to the viewer's settings. So I unlocked it, and...that didn't work. I think it would make more sense to just have these as a Label, like the main color inspector's stats in the Viewer itself.
| self.__logarithmic = Gaffer.BoolPlug( "logarithmic" ) | ||
|
|
||
| # HACK for ChannelMaskPlug | ||
| self.__inputPlugs = Gaffer.ArrayPlug( "in", element = GafferImage.ImagePlug() ) |
There was a problem hiding this comment.
I guess this is the reason why I didn't get a list of layers to choose from when I unlocked channels. I was very recently considering updating RGBAChannelsPlugValueWidget to update the label to indicate when the selected channels don't currently exist, and that would fall foul of this too.
| self.__autoFrame = Gaffer.BoolPlug( "autoFrame", defaultValue = True ) | ||
| self.__logarithmic = Gaffer.BoolPlug( "logarithmic" ) | ||
|
|
There was a problem hiding this comment.
One of the long-term intentions behind using plugs to store UI settings is that we'll be able to serialise the whole Editor hierarchy to save and restore layouts. With that in mind, I think it makes more sense for this plug to be parented to ColorInspectorPlug, where it is naturally a part of the viewer's settings.
| import GafferUI | ||
| import GafferImageUI | ||
|
|
||
| class DeepPixelInfo( GafferUI.Widget ) : |
There was a problem hiding this comment.
If feels like this component doesn't really sit logically in our existing API layers. It doesn't do simple source-agnostic value display (like say ColorSwatch), because it has plugs to define settings (like Editors). Yet it's not really usable as a higher level component (like PlugValueWidget or View) because it relies on the client for orchestrating all the background compute.
I think the simplest thing to do is just to have it as a private component of ImageViewUI, but even then I think it makes sense to take out the plugs (see below).
There was a problem hiding this comment.
I think what I'd actually do is refactor this to be a Window subclass in ImageViewUI, so that it can take responsibility for everything, including the layout and window title. In terms of publicly-available, reusable functionality, I think DeepSampleGadget alone is probably sufficient for now?
danieldresser-ie
force-pushed
the
deepInspect
branch
from
2fab7aa
to
8c9c08f
Compare
I might as well upload where I got with this on Friday, for folks to at least have a play with.
There probably isn't too much point in reviewing DeepSampleGadget, which I haven't really cleaned up since it got copied from the anim keys editor - I know what I need to clean up there.
But the rest of it is probably worth reviewing - not so much because I'm confident it's done, but because I don't know how we want to structure it. I'm doing some things with plugs in the UI that are probably naughty, but I don't know what I'm supposed to be doing.