Duplicate layer function added to layer buttons.

[davidlamhauge]

I made a small PR that can duplicate Bitmap and Vector layers. I've only tested it on bitmap layers, but the code should work with both.
I can't find any reason to duplicate camera or sound layers, so until anyone can see a use-case for that, it is only artwork layers.

Solves #498

[MrStevns]

If we don't want the duplicate button to do anything for camera, then it should not be there when on the camera layer. Simply writing "only for artwork" is not good enough imo. We need to hide or disable the button based on the layer type.

[scribblemaniac]

I can't find any reason to duplicate camera or sound layers, so until anyone can see a use-case for that, it is only artwork layers.

IMO I can't think of any good reason why it shouldn't work for all layers. Most of the copy code is redundant and can be done with generic Layer and Keyframe types rather than how it is implemented currently in this PR. I'm not even convinced that we need a conditional statement for the create layer calls. It would probably be better done as a generic create layer function in LayerManger that takes a LayerType as argument, and then handles calling creating the correct layer type internally.

If you still need reasons here are a couple off the top of my head. I think it would be useful to duplicate the camera layer if you want to experiment with different variations of the same camera movement; create a camera movement, duplicate the layer, modify it, and then you can compare the two easily. Sound layer duplication might be useful for creating an echo; duplicate a sound layer and offset all the frames in the new layer by on and tada. Finally, and this is the most important to me personally, having it work for all layers avoids having to answer the inevitable support questions that would arise about how to duplicate the camera/sound layer or why it doesn't work.

[davidlamhauge]

All four layer types can be duplicated. I changed the tooltip, and removed a Debug include in ActionCommands.cpp , that I had forgot.
One problem: I couldn't move the sound clip. Neither on the original layer or on the duplicated layer. Is that a known bug? I used Win 10.

[Jose-Moreno]

@davidlamhauge Well there is a new bug that got into Pencil2d after the timeline actions were merged, where the sound clip visualization is offset when selected, however the "real" position is the same.

So If you import a sound on frame 15, and the sound lasts another 15 frames, when you select that it will show as if it starts on frame 1 (or probably at a theoretical 0th frame) but when you play it back it will start from frame 15 nonetheless.

Edit: I reported that here #1686

[MrStevns]

PR updated

@davidlamhauge Well there is a new bug that got into Pencil2d after the timeline actions were merged, where the sound clip visualization is offset when selected, however the "real" position is the same.

So If you import a sound on frame 15, and the sound lasts another 15 frames, when you select that it will show as if it starts on frame 1 (or probably at a theoretical 0th frame) but when you play it back it will start from frame 15 nonetheless.

Edit: I reported that here #1686

Thanks for reporting that, I didn't know! it must have broken at some point during a refactor because i know it used to work for sound frames too.
This has now been fixed in master.

[Jose-Moreno]

@MrStevns Awesome thanks a lot!

[davidlamhauge]

Now the sound clip is loaded when you duplicate a sound layer. I copied some code from importSound, so it could probably be made more elegant, but at least it works.
edited: Arghhh!!?! - I forgot the QDebug include...

[davidlamhauge]

I've cleaned up the code and removed the unused include. It's as good as I can make it. I also added some 'emit ', since it gave warnings in my compiler. If you have improvements, please come forward.
Today I tested duplicating a sound layer with two sound clips on same laye, one *.wav and one *.mp3, and it worked like a charm.

[scribblemaniac]

I've added some changes to more closely follow the simpler keyframe duplicate process used by ActionCommands. Technically setting the file name isn't necessary as it's overwritten when saving the layers.

Unfortunately, this feature currently crashes for me under specific circumstances. If I load a file with a bitmap frame at position 45, but do not load the frame (ie. don't scrub near it), then attempting to duplicate that layer will cause the application to crash. It seems to be caused by the BitmapImage constructor used by BitmapImage::clone. Loading the images into memory will probably fix the issue, but loading an entire layer into memory to duplicate it is not ideal.

[davidlamhauge]

I've looked at the requested changes, and it looks good to me. How can the be merged into the PR?

[davidlamhauge]

Loading the images into memory will probably fix the issue, but loading an entire layer into memory to duplicate it is not ideal.

I think you're right @scribblemaniac . It's not ideal.
But then again, if you want to duplicate a bitmap layer, it must be because you want to test something, a new timing or whatever, and you can't do that without seeing your new timing being played, and that must mean that the images must be in memory.

I admit that there are many things in the inner workings that I don't fully understand, but what is it I don't get here?

[scribblemaniac]

When you're playing a project, frames are loaded into memory yes. This is done about 5 frames in advance of the play head. However these frames are loaded into the memory in the active frame pool, which manages it's memory to stay within the user-defined limit by removing non-modified frames from memory if necessary. If you load all the frames in memory when duplicating a layer and mark them as modified, the will take up that much memory at minimum until the project is saved. With layers contain lots of frames, this could be a very significant amount of memory, and is not something users would necessarily expect by doing a single operation. A user may only be retiming a small segment of the layer for example, or maybe they just want a non-visible "backup" before doing some modifications to the original layer.

I don't necessarily expect this issue to be fixed in this PR as it's part of a broader problem with how we manage dirty frames. I mostly made note of it in hopes some had a better solution for this particular case.

I've looked at the requested changes, and it looks good to me. How can the be merged into the PR?

I pushed the changes I've made (mostly refactoring) to directly to your branch already so it's already in the PR. I have not coded any fix for the aforementioned crashing issue.

[chchwy]

I've added some changes to more closely follow the simpler keyframe duplicate process used by ActionCommands. Technically setting the file name isn't necessary as it's overwritten when saving the layers.

Unfortunately, this feature currently crashes for me under specific circumstances. If I load a file with a bitmap frame at position 45, but do not load the frame (ie. don't scrub near it), then attempting to duplicate that layer will cause the application to crash. It seems to be caused by the BitmapImage constructor used by BitmapImage::clone. Loading the images into memory will probably fix the issue, but loading an entire layer into memory to duplicate it is not ideal.

I think I can make a fix for this crash issue later on.
My solution is copying the image file with a temporary file name in BitmapImage::clone if the image is not loaded.

[chchwy]

Unfortunately, this feature currently crashes for me under specific circumstances. If I load a file with a bitmap frame at position 45, but do not load the frame (ie. don't scrub near it), then attempting to duplicate that layer will cause the application to crash. It seems to be caused by the BitmapImage constructor used by BitmapImage::clone. Loading the images into memory will probably fix the issue, but loading an entire layer into memory to duplicate it is not ideal.

I am going to merge this PR and open another PR to address the issue @scribblemaniac said above.

[scribblemaniac]

I don't really like the idea of merging in code we know crashes. As it stands nobody has even finished reviewing this.

[chchwy]

Sorry @scribblemaniac, I don't know you're not done reviewing yet. I thought it should be ok if the only concern is the crashing issue and I can make a fix PR right after.

[Jose-Moreno]

I know @J5lx merged this after some reviewing, but was the PR @chchwy mentioned already merged? Was the crash mentioned by @scribblemaniac already addressed?

[J5lx]

Sorry Jose, your comment kinda flew under my radar. The one who merged this is @chchwy, not me, and that was already about half a year ago. I just happened to notice that the associated issue was still open despite the merge, so I closed it. Tbh I wasn’t even aware of the problems with this PR since the discussion happened at a time when I wasn’t able to follow development very actively. I simply assumed that if there were any issues, it wouldn’t have been merged in the first place. At the very least, Matt’s PR that was meant to fix the crash mentioned by scribble is marked as merged.