ImageGadget fixes #6050
ImageGadget fixes #6050
Conversation
johnhaddon
force-pushed
the
imageGadgetCancellationFix
branch
from
e78ec9b
to
5e04892
Compare
|
Code looks fine in basic inspection. Playing with it interactively, I didn't manage to tune my rendering+imageManip test case to produce really frequent permanently stuck partial updates, but as far as I can tell, it's working. Before:
After
I did still see a little bit of color tearing, but it seems like a completely different problem: it affects single tiles, doesn't depend on a render running to cause cancellations, is cleared by the next screen refresh of any kind ( even if the affected tile hasn't yet been updated again ). So it seems like there is still something that can cause color tearing, but this PR does seem to do it's job. I'm not sure where the remaining tearing is coming from ( I don't think the IE remote desktop software would cause this ) ... the code appears mutexed in a way that color tearing should never happen, but something is still making me see colors. My best repro for this new style of color tearing: create a default Ramp image, connect it to an ImageTransform, shift drag the x of the transform, and then frantically wiggle the mouse as fast as you can to trigger extremely fast updates. The flashes of color I see are all one of 4 colors: blue, cyan, yellow, red. That's consistent with an update sometimes updating just R, or just RG ( to be brighter or darker ). The mutexes in applyUpdate seem correctly designed to prevent this ... maybe there's still a bit of a puzzle here, or maybe you're already aware of this other thing. But as for this PR, looks good. My only concern would be whether we need more testing in a production scene that isn't just me with a render of sphere and manipulating a checkerboard. |
|
I'm able to reliably reproduce #6043 in a local environment and this does appear to fix the issue from my testing. I've also run into the "single tile tearing" that Daniel mentions. For me, it also requires a consistent stream of rapid updates to trigger, and the tearing is quite infrequent and clears quickly (I haven't been able to catch it in a state where the bad tile remains visible, it has always been cleared by a subsequent update). I'd be happy to treat this as a separate issue as this PR does appear to reliably address the issue reported in #6043. |
The ImageGadget no longer has any internal nodes.
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.
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 GafferHQ#4011, which I replaced with a botched version in GafferHQ#5348.
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 GafferHQ#6043
johnhaddon
force-pushed
the
imageGadgetCancellationFix
branch
from
5e04892
to
2683c5d
Compare
|
Rebased onto |
This fixes the bug reported in #6043, and a couple of other little issues I spotted while looking at it. Most of the problems were due to me botching #5348, way back in 1.3. Worth doing plenty of testing on this one I think - I'm not in any rush to get it into the next release.