Simplistic Node Graph Error Visualization

[andrewkaufman]

Add the error signal to the Node class, and then display that as an alert on the StandardNodeGadget.

Breaking this nugget off of #883

[johnhaddon]

I'm still working out some of the details of the threading on this one, but I wanted to get some feedback on the UI side of it while that's ongoing.

What I have so far is this :

In this example, the RenderManOptions is selected and being viewed, but the filename in the SceneReader is incorrect. So the Viewer has received an error while computing from RenderManOptions, but the new Node::errorSignal() has been emitted on the node which is the original source of the error.

The StandardNodeGadget displays the error as a triangle overlaid on the problematic node. The tooltip for the triangle shows the error itself. The little cross dismisses the error.

One thing that may slightly counterintuitive is that the error status will stay there until dismissed - even if you correct the filename and the viewer starts cheerfully showing the right result, the error indicator will show until you press the cross. The reason for that is that there's no error on/off state for a node, just like there's no dirty on/off state. This is a product of the multithreaded/multicontext nature of Gaffer - there's simply no static state on a node anywhere. So a node can error during one context, but it might succeed in another, and we still need to display the error for the first one even if the latter one was OK.

How does that strike you? Annoying? Or kindof makes sense? Would you like errors to disappear magically or do you think having people read them and dismiss them might be a good thing?

While I was writing this it occurred to me that we could clear the error indicator when a plug gets dirtied, on the assumption that it'll get triggered again if the computation is reperformed due to the dirtying. Might be something to explore?

[andrewkaufman]

Yeah, I think I'd rather have it clear on its own when I correct the mistake... otherwise fixing an issue is a two step process, and it seems likely that I type the right value the second time around, I still see the error, so I assume I was wrong and try a different value.

I see your point about an error in one context not being an error in another though... do you think the plug dirtying clearing the errors could lead to missed error reporting? Or missed error fixing?

[johnhaddon]

I think there are cases where the dirtying approach would clear things a bit prematurely, because the Node Graph is only displaying errors which occurred when other bits of UI were computing (it doesn't do it's own). Say you put the wrong filename in, get an error in the viewer and you see it in the graph. Then you put in a second wrong filename, but this time the viewer is paused so there's no compute. The node graph error would be cleared, so things would look OK for now. The error indicator would only come back when something tried a compute next.

I agree though, I think this approach is better. I'll stick a plug in error signal (so we know to clear the error only when the specific problem plug is dirtied) and see how it feels in practice.

[johnhaddon]

OK, think I've got the threading sorted, but now have more questions about the clearing mechanism.

My initial implementation emitted Node::errorSignal() for the node at which the error originated (SceneReader in the example above), but not for all the intermediate errors as the exception propagated down the call chain to the node on which Plug::getValue() was being called (RenderManOptions above). The idea being that all those intermediate locations aren't responsible for the problem, so it's not useful to flag errors on them for the UI. But it also emitted the signal on all ancestors of the problem node, so that Boxes could display errors that occurred within them, and so we'd still have error signalling for Reference nodes and GiftBoxes/doohickeys in the future.

Moving to our idea of clearing the error status on plugDirtiedSignal() isn't totally straightforward at that point. It means adding a plug argument to Node::errorSignal() so we know which plug was being computed when the problem occurred. We would then only clear the error indicator when that specific plug was dirtied. But I'm not sure it makes sense to pass up that plug while propagating the error to ancestors, because Node signals to date have always been passed a plug directly held on the node, not a distant descendant. More importantly though, plugDirtiedSignal() is only emitted for direct plugs and not distant descendants, so either way on Boxes and the like we'd have no way of clearing the error indicator.

One possibility is to change the rules by which computes actually emit the error signal. Previously we did it for only the first error, and ignored the propagation of the error back through the node chain to the caller. The new rules would emit the error signal for that first error, and then also emit it at points where the error propagated from one subgraph (Box/Reference etc) to another level. So there'd be no explicit propagation of the initial error to all ancestors, just a signalling if the error emerged from within a subgraph via a promoted plug. The errors on Boxes etc would then be associated with a plug on the Box itself, and could be cleared by the dirtying of that specific plug.

With this new scheme, a Box wouldn't display an error at all if an error occurred within it but was not propagated out via a promoted plug. Does that sound problematic? Or is there any concern that these rules seem quite tailored to the specific needs of the NodeGraph? I think it's probably the best option currently, and I actually can't think of a client for errorSignal() other than the node graph - most other UIs are actually doing computes and will deal directly with the exceptions that they throw anyway.

Hmm, one more problem. Because errors are things that occur, not a state that exists (due to threading/contexts), if a node gadget doesn't exist at the time the error occurs, the error won't be displayed by subsequently created ones. So for instance, an error occurs on a SceneReader within a Box, is displayed on the Box, so you dive in, but the error will not be visible on the newly created gadget for the SceneReader until the error occurs again. I don't know what to do about that, other than have some nasty global error tracking thing outside of the node graph. Is that a big issue?

[andrewkaufman]

a Box wouldn't display an error at all if an error occurred within it but was not propagated out via a promoted plug. Does that sound problematic?

So lets say we have a Box with nothing promoted except the out ScenePlug of a Group. That node works fine, but some node upstream of it has an error (bad fileName or whatever). Do we see an error on the Box because the "out" plug can't give us a scene? Or do we see nothing because the actual problematic plug isn't exposed to us? If it's the later, then that sounds problematic to me.

an error occurs on a SceneReader within a Box, is displayed on the Box, so you dive in, but the error will not be visible on the newly created gadget for the SceneReader until the error occurs again.

Can the error on the box indicate which node inside the relevant plug was promoted from? And will selecting the SceneReader once we dive in end up requesting it's scene be computed (assuming an unpinned pane is present) and trigger the error on that gadget?

[andrewkaufman]

I'm trying to think how other DCCs deal with internal errors (and warnings). One such I can think of is a bit flaky with them really, but it's pretty easy to force a re-compute, so if the errors get dismissed prematurely, they come back quite readily. I think it propagates errors up to Boxes via explicit metadata on the Box saying which nodes inside are allowed to make noise. I'm not a huge fan of that approach really, as it leaves the Box creator to explicitly mark nodes they think would be problematic...

[johnhaddon]

In your Box example, you would see an error on the Box if the error occurred while viewing the Box or something downstream of it, and the bad internal node feeds into the Box's output, even if it feeds in indirectly through a big chain. So I think that case is fine.

The error on the Box could easily indicate which plug was promoted, but not which internal upstream plug caused the error. Diving in to the Box and selecting an internal node would trigger a recompute (because a different node would be selected in the viewer or wherever), and then that would display the error inside the box. It's just that when you first dived in all would look OK, because you haven't changed the selection yet. I guess the intuitive next step is to start exploring by clicking on nodes though, so maybe thats not too bad?

I'm not a fan of explicitly saying what can be an error or not either. In practice with the setup I've described above, internal nodes would trigger errors on the Box if they broke the computation of one of its outputs, but not if they didn't affect the output in any way. That seems pretty reasonable to me...

[andrewkaufman]

Yeah, that sounds good to me too.