Visualize selection rotation properly #1724
Conversation
- Rotation is now properly visualized - Transformed object after rotating keeps correct transform - Cleaned up selection painting logic - Removed last selection visual - Moved setModified to Editor instead of scribblearea - Fixed selection info should diff from center of rect
Given that a lot of our rendering and transform code is based around the current layer index and not the modified frame and layer, and the fact that there's shared responsibility across the codebase... we apply the changes when switching layer, otherwise the selection transform might affect the wrong layer.
This happens because our logic assumes the current layer changes, meaning you can't apply vector transformations while you're on bitmap layer and vise versa. This will fix that, by making sure you are on the correct layer. In the future we should figure out how we want this to behave...
J5lx
left a comment
There was a problem hiding this comment.
Sorry for the wait, but I wanted to finish reviewing David’s PR first. I looked over your changes and they look good to me for the most part. I did some cleanup and smaller fixes which I’ll push in a moment. Everything seems to work as intended as well, with the exception of selection flipping.
First of all, it seems to suffer from that cache issue which you took care of a while ago. That is, the change doesn’t become visible until you change the zoom level or something like that. Secondly, the selection is no longer flipped in place like it was before but it also changes position. Additionally, flipping the selection vertically now also flips it horizontally in addition to vertically. And last but not least, the action is no longer undoable.
That aside, I noticed that at least in debug builds the move tool feels quite a bit more sluggish than before. It’s not that noticeable in release builds, and perhaps it’s exacerbated by the fact that I’m currently running on a ten year old low-end notebook, but maybe it’d be a good idea to whip out a profiler and make sure there are no performance bottlenecks. I might take a look myself tomorrow, but for now I need to get some sleep.
Those issues aside, I’m very happy about these changes. Great job!
|
Thanks for a thorough review Jacob! I've pushed a bunch of commits, fixing the issues you mentioned and re-adding undo/redo. It's funny how when I reviewed the selection flipping last time, it worked fine.. I must have accidentally moved the selection first before testing, which is why it worked for me. I agree that it feels more sluggish now, though I haven't been able to figure out the root cause yet. Doing a quick profile does not seem to point to anything particular except painting but what I've done shouldn't really affect that much. Your changes looks good too, thanks for applying them 👍 |
|
Thanks for addressing those issues, those changes all look good. I did some profiling myself and that so much time is spent painting is indeed kind of the issue. Editor::frameModified is now emitted in both MoveTool::pointerMoveEvent and MoveTool::transformSelection. Because MoveTool::pointerMoveEvent also calls MoveTool::transformSelection, that effectively means that, when transforming a selection, Editor::frameModified is emitted twice for every move event, which therefore causes the entire timeline (!) to be redrawn twice. (It also results in ScribbleArea caches being invalidated, but that doesn’t make nearly as much of a difference than the timeline redraw.) On master, transforming the selection does not cause the timeline to be repainted at all. With that in mind, please update this PR so that transforming the selection no longer causes timeline repaints, like before. The double frameModified signal should be fixed as well, I reckon. Technically the timeline should probably also defer the actual painting until the next paintEvent, but that is clearly out-of-scope for this PR, so no need to worry about that. Once this performance issue is dealt with, the PR should be ready for merge. Thank you for all of your work! |
By not updating bounds excessively
|
Ah yes good point, I did notice the timeline calls but as I said I didn't spend too much time on the profiling but that was indeed the main culprit. The performance issue should have been addressed now. Given the way frameModified used now for triggering cache invalidation's we'll still call timeline and therefore still cause a repaint, but it will only happen on release events. |
J5lx
left a comment
There was a problem hiding this comment.
Alright, that sounds good to me. Thanks for taking care of it! Time to merge this :)
This PR sought to fix that a selection would be drawn correct, however to make this viable a lot of assumptions had to be changed about the current implementation.
Demonstration:
Main points:
Known issues:
Mitigations:
Reason:
A lot of our core logic assumes the current layer changes when painting and applying changes, meaning you can't apply vector transformations while you're on bitmap layer and vise versa. This will fix that, by making sure you are on the correct layer.
In the future we should figure out how we want this to behave...
There's still room for improvement, especially when it comes to QRect vs QRectF... this is a mess tbh... but it's not something i'm going to fix with this PR. I believe that the selectionManager should be purely for bitmap and vector should either have its own or be kept to the VectorImage object.