Fix canvas showing wrong content after moving frames #1497
Conversation
- Also fixed a cache bug when using actioncommand shortcuts Additionally the swap logic has been reworked. A swap can only occur now when two frames exist, otherwise nothing will happen. So to get the same behavior as before, where the frame would be moved anyway, i've added individual via the new moveKeyFrame and removed moveForward/moveBackward as a result of it.
|
Update: I've also reworked the swapKeyframe logic to make it behave more consistently, so that now a swap can only happen if two keyframes exist where as before one frame could be removed and moved regardless. Of course the result will be the same, the move keyframe logic has just been moved into a new moveKeyFrame function, which replace the moveFrameforward/backwards. This got me reminded of the move keyframes shortcuts which also did not update cache, so that's also fixed now. |
scribblemaniac
left a comment
There was a problem hiding this comment.
I like the move to a more general moveKeyframe function over moveKeyframeForward/Backward, and I'm alright with swapKeyframe failing if both frames don't exit, but this implementation using a temporary selection for single frame moving opens a whole new can of worms. This is going to need either some discussion of how we want the move keyframe actions to work with a selection, or modifications to make it behave like it did before these changes.
536adc1 to
dbbfb39
Compare
- Refactor moveKeyframe method..
|
Ready to be reviewed again. |
| update(); | ||
| } | ||
|
|
||
| void ScribbleArea::updateDirtyFrames() |
There was a problem hiding this comment.
What is the purpose of this function and the dirty frame tracking in general? To me it seems like an unnecessary extra step before removing cached frames.
There was a problem hiding this comment.
You say that but also say that it's important that we update the cache for the frame before and after moving, so I don't think it's an unnecessary step. By all means if you have a better way of doing, do say so, cause I'm running out of ideas.
This way is imo. much better than making it up to the caller to update the frames it's modifying properly.
The name of the function may not be right but the intention is to make it up to scribblearea to clear the cache for the frames that has been modified or can be considered dirty on the currently layer.
There was a problem hiding this comment.
Let me clarify, I think updating the cache for the frame before and after moving is very important and not unnecessary. What I think is unnecessary is having the QList of frames that you are effectively delaying removing them from the cache. Why not just remove them from the cache right away?
There was a problem hiding this comment.
They are being removed at the next frame update, which should happen right after the move, though it could potentially be delayed. it's the least coupled implementation I have come up with and it does the job. I'm not sure how to make it happen right away without involving scribblearea in the process, do you have any ideas? Maybe if we move the list of qpixmapcache into the layer model, then I would be able to clear it right away. What do you think about that?
There was a problem hiding this comment.
I think part of my confusion came from not paying attention to what was in Layer vs ScribbleArea. I see now that you don't have access to ScribbleArea in Layer and it indeed wouldn't be good to couple that.
As for alternative approaches, the qpixmapcache is for caching rendered+composited frames, so it would not be good to have that in each layer. It also wouldn't work because it needs access to the output of the CanvasPainter, which Layer does not have. It's a tricky problem. I thought maybe you could make a signal and connect the layers to Editor when they are created/loaded, but looking at the code I see that it's really object that's managing the layers, not layermanager, so that wouldn't really work. I have my worries that we might make some change or come across some corner-case where removeCacheForDirtyFrames does not get called, but your solution might be the best thing short of doing some major architectural changes.
1ca6d28 to
1af8f94
Compare
scribblemaniac
left a comment
There was a problem hiding this comment.
Thanks for your patience. This is ready to be merged now I think.
closes #1496
The PR includes another bugfix, related to when you removed a frame, the canvas would not be updated either.