Paste from previous frame #1682
Conversation
|
That's interesting. Thanks David! |
|
Sorry for barging in here, I could not help myself :) Nice that you ,David are implementing this :) https://discuss.pencil2d.org/t/rub-through/941 I'm sure your solution will be elegant. 👌 |
There was a problem hiding this comment.
Works overall and the code looks clean, but there are a few behaviours that, even though they are not technically bugs, are be a bit surprising/unintuitive (the list might seem a tad long, but a lot of it should be quite easy to address):
- The feature copies both to the clipboard and to the current frame, which I find unexpected
- The feature does nothing if there is no selection. I would expect it to simply copy the entire frame in that case, like the “copy to clipboard” feature does
- When you use the function on the very first frame, the content of the current frame will be copied to the next frame and the scrub will be moved to the next frame
- Sub-issue: When you use the function on the very first frame when there is no next frame and the “when drawing on an empty frame” setting is set to “duplicate the previous key frame” or “keep drawing on the previous key frame”, the copied selection will be pasted on (potentially a duplicated copy of) itself, which messes with transparency/feathering in the selection and is probably unintended
- When you use the function on the very first key frame but that frame is not at position 1 (that is, there are previous frames but no previous key frames), the feature will paste from the clipboard instead
- When the previous keyframe is empty, the feature will also paste from the clipboard instead
- This might be a bit nitpicky, but I’d say it should be called “Paste from previous frame” or “Duplicate from previous frame” rather than “Copy from previous”
As I said, most of this should be relatively easy to address. The clipboard related items can be sorted out by simply duplicating the contents directly instead of going through the clipboard code. You could use {Bitmap,Vector}Image::copy(…) followed by Editor::pasteToCanvas(…) for that.
I'm not sure what you mean. What I think I do is:
To me that is the logical way to do it.
I can implement that, but I don't find it intuitive. Most of the rest is based on my misunderstanding the getPreviousKeyFramePosition(frame) function. I understand now that it returns 'frame' if there is no previous keyFrame. It will be taken care of. |
Hmm, I can certainly see why that would be logical when you’re thinking of the feature as a “macro” of sorts, used merely as an alias for those existing features chained together. But it’s when considering it as its own feature that I would expect it to achieve a certain task rather than merely executing certain pre-existing steps. It’s that mindset which has me thinking of the altered clipboard as an unexpected outcome – because I’m thinking of this feature not as a combination of those other actions, but rather as its own thing, aimed at facilitating the sort of “rub-through” workflow described by Manu in their thread and also by you in the OP. To put it in a more concrete way, I’m thinking of it as a way to get frame contents from the previous frame onto the current frame, with no consideration of the clipboard at all. Anyway, I hope this allows you to better understand why I found it unexpected. Regarding the behaviour when there is no selection… to be honest I have nothing to say there except the behaviour I described is what I find intuitive… for whatever reason. In the end, I suppose both of this is kind of about personal expectations to a degree. In that regard, it might be good to hear what Manu and perhaps Jose think about it. If they agree with your assessment, I could hardly argue with that. |
|
It would be unwise of me to comment on Davids solution before I tested it and I don't have for the moment any PC that I could build it on myself but if you share a link to a Windows build I would be happy to try it out. I only say this if it is based on selecting something then I hope some of you introduce the lasso select tool soon also :D That said when I thought about this feature a while ago, I just anticipated it as its own tool. This is maybe against P2D philosophy to introduce too many tools and clutter the interface in the long run. Anyways it was just when I was duplicating frames and erasing parts of it to draw it in another position that I realised how nice it would have been to have another tool that works like an eraser but instead of erasing it copys everything I touch with it to my current frame by looking at the onion skin i.e the previous frame (could also be the next frame if such exists) . Now when I think about it today that if it still would be it's own tool you could (in the docked option window) set how far back you want the tool to copy from if you for some reason would not want to copy from the previous and you could also set that it instead copies from the frame after the one you are drawing on. Every time you select the tool it turn off the onion skin used for sketching/drawing (if you have it on), and show the previous (or next if you set so in the options) frame in a different color just to make it more clear that it's two separeate features not to be confused with regular onion skinning. And when you turn the Rub Through tool off onion skin gets turned back on (if you had it on before). I don't know.... this last part is maybe overkill. Finally since I'm no coder and I have no idea how complicated this is comapred to Davids solution, so as I said before I trust David to make it a smooth solution anyways, it doesn't have to behave exactly as it was described in my head at the time. :) There's more than one solution to a problem as they use to say. |
|
I think I've adressed the raised issues.
|
|
After reconsidering, I couldn't find logic answer to why only copying camera rect, if nothing was selected. |
|
@J5lx @davidlamhauge @ManuAros Hey, sorry for the delay in my reply. Practical use discussionWhere do I start. In paper the specific behavior that David's proposing, seems interesting. Being able to copy a previous frame content, be it the entire keyframe (no selection) or part of it (yes selection) in one go. Similar to Jakob, I'm currently thinking of a wider scope that such feature can give way to. For this I always refer back to Adobe Flash's This "mode" allows the animator to modify artwork across multiple via:
At the moment I see David's proposal currently covers a limited iteration of what I understand as clipboard operations via selections, while automating the In consideration of workflow efficiency and time saving labor, I like the current feature in this PR. To be honest though, I don't really know how much time it will save as opposed to using the regular method, and it's precisely because of it, that I'm not entirely picturing myself using it often since I would probably revert back to the monkey-see monkey-do approach in a highly demanding project 🙈 💦 Thinking of possible enhancements, right now I can only think to allow the user to select from which frame they want to "sample" their selection, however to solve that in a productive manner would meant to solve the individual onion skin selection request from issue #393 So for now, without wanting to delay this for too long, at the very least I estimate that we might need to discuss this PR to be merged as part of a larger context so overall it can become a clear foundation for a more robust |
|
My confusion of the feature resonates with what Jacob mentioned previously, I find it odd that it copies to the clipboard. I understand that it does so because you rely on the copy/paste behavior but i'd say that's misuse of the copy/paste implementation. What you're doing right now is both copying and pasting with one command which is not really conventional, which I think we should stick to. If you need to copy a portion of an image then you'd use BitmapImage::copy(..) Additionally I found a few things I'd like addressed:
|
|
@ManuAros Here’s an up-to-date Windows build you can try. @davidlamhauge I’ll wait for your thoughts on the comments that the others have added, as well as the additional issues Oliver found, before I do the next round of review. |
davidlamhauge
changed the title
|
I've added Backup before Paste, as suggested by MrStevns, and it works now. |
|
I saw your corrections to the code @MrStevns , but it still pastes on the existing drawing - at least on my computer? |
That was not what I fixed though, before you simply made a copy but didn't use it so it didn't work at all. Now it copies from the previous key again and pastes to the current frame. I thought we had implemented such that you can paste without applying to the current frame but that's not the case. That will not be part of this PR so that problem can be ignored |
I gave it a shot in the weekend, but I have no idea how to paste a floating selection, not applied to the drawing. |
There was a problem hiding this comment.
Sorry for the delay! I was away for a few days but forgot to communicate that. The PR is looking pretty good now overall. I added one more commit to improve the naming/spelling consistency a bit and invert an if statement, but that’s it from my side basically. What you said about potentially improving upon the functionality later on seems fair to me as well. Unless Manu or Jose still have something to add there I’d say the PR is ready for merge. Good job!
|
Since there haven’t been any additional comments, I’ll go ahead and merge this. |
This is a small feature that I from time to time miss. It simply copies a part of the previous drawing to the present drawing. You select it with help from the onion skin.
Example:
A person is leaning towards a wall. The hand is placed on the wall and is not moving. When he leaves that position he first bends slightly in the elbow, and then pushes away from the wall. Here it would be great if you could copy the hand, and only animate the arm/wrist.
Yes, you can have the hand as a separate layer, but I'd prefer to copy it from the previous drawing, and the change the angle in the wrist, etc.
I've placed it in the 'Edit' menu, with the shortcut: Ctrl+LeftArrow.