Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feature] Options when drawing on an empty key-frame #915

Merged
merged 2 commits into from Apr 18, 2018

Conversation

martinvanzijl
Copy link
Contributor

Fixes a "vanishing line" bug in Bitmap layers. To reproduce the issue:

  1. Create a new animation.
  2. Select the Bitmap layer.
  3. Go to frame 2.
  4. Draw a line with the pencil, pen or brush tool.
  5. The line will "disappear".

Video demos below before and after the fix.

Before

vanishing-line-bug-before

After

vanishing-line-bug-after

@MrStevns
Copy link
Member

MrStevns commented Mar 6, 2018

You're actually not supposed to be able to draw on the canvas if you don't have a frame created. The proper solution would be to disable drawing completely when there is no keyframe.

Alternatively if you want to return to the previous valid keyframe, the scrubber should be moved back to that position.

@Jose-Moreno
Copy link
Member

@candyface @martinvanzijl How about we take this pr and go the extra mile so we can finally have a way to "automatically" add a new keyframe when you draw on a frame that is empty? what do you guys think?

@martinvanzijl
Copy link
Contributor Author

@Jose-Moreno Thanks for the suggestion. In the demo below I've added a preference to either:

  1. Create a new key-frame, or
  2. Add to the previous key-frame.

What do you guys think?

demo-of-both-options

Note: This is just a proof-of-concept, so it still needs work. E.g.

  • It adds the new key after the stroke is finished; it should probably be when you start the stroke instead.
  • It only works on bitmap layers. If we decide to go with it, we should probably make it apply to vector layers too.

@MrStevns
Copy link
Member

MrStevns commented Mar 7, 2018

I can't say I understand what the intention is to make it draw on the previous frame, when that's said: It looks like a decent solution.

If we go with this design:
The default setting should be that it creates a new keyframe because nothing currently makes it obvious that it would add to the previous key, so if the user decides to press create keyframe after having drawn something, then it'll disappear.

Do you see there being an actual use case for the "draw on previous frame" design?

@Jose-Moreno
Copy link
Member

@candyface Think of it like this. The current keyframe is being exposed automatically so until you have a new drawing you are in fact handling the previous keyframe for most functions. This is actually how it works in TVPaint using concept of instances. So in fact if you draw on any frame that belongs to that instance there are two possible behaviours. You either create a new drawing by "breaking" the exposure of the first drawing, or continue to draw as part of the current instance in any other frame.

The first case is "normal" when you want to create new drawings. The second case can be helpful when creating guides or modifying a layout or even a background to accommodate for the whole animation.

In toonboom it also works like this, unless you specify you are creating a blank drawing or you duplicate the previous drawing, you will continue to draw on the previous drawing if you are drawing on any of it's instanced / exposed frame.

In flash it worked the same, you created a keyframe, drew something, pressed f5. If you drew on the last frame part of the track you would continue to modify the keyframe, so you had to create a new blank keyframe to be able to make a new drawing.

Most of the time it's obnoxious that you can't actually control this behavior, so it is practical that you can choose between creating new frame vs continue drawing on the current drawing.

@Jose-Moreno
Copy link
Member

@martinvanzijl I like it! I agree the keyframe should be added as soon as the stroke signal is sent but it's a great step forward. I really like that you left both behaviors and as I've outlined on my previous response this is normal behavior in commercial software, where you keep drawing on a previous keyframe while drawing over it's "exposure instances" (this is when a keyframe is held and we see the same drawing repeated over many frames until a new drawing / keyframe is created), and the good thing about this solution is that it allows us to choose between that old behavior and creating a new keyframe automatically so I think we're right on track! If anything we could bring this over to the timeline like TVPaint does:

https://www.tvpaint.com/doc/tvp11/index.php?id=lesson-animate-first-steps-deletion-addition-add-instances (notice the auto-beak instance & auto create instance toggles)

@MrStevns
Copy link
Member

MrStevns commented Mar 7, 2018

@Jose-Moreno

What do you mean that it works the same in flash? you can only move the scrubber/playhead as far as you have either frames or keyframes. it's true that if you add a frame then it'll extend the exposure to that frame but you still can't draw before you've created a frame.

As far as I can tell from the videos i've looked at regarding TVP, it only creates a new instance and if you want an empty frame, you create an empty instance. Same goes with ToonBoom

Conclusion:
From what I've tried to replicate in adobe animate and watched on youtube, In all three cases a new frame or instance has to be created before you can draw.

The problem as I see it is that we don't have any indication of a frame being exposed or not which makes the feature hidden. I'd rather like that we waited with such a feature and instead implemented it properly with the new timeline but I understand that, that might be a bit far fetched...


I also agree that the keyframe should be added before you start drawing.

@Jose-Moreno
Copy link
Member

@candyface Yes that's basically it. In all those programs you need to create a new keyframe to draw a new drawing, as obvious as it might be, sometimes they do have automatic creation commands (TVP), and drawing on the frames that are considered "exposed" or "held" doesn't prompt you to add a new keyframe / instance to continue modifying the previously created drawing.

The problem as you mention is that since we don't have a way to clearly present that the frames between drawings are indeed "instances" of the previous drawing(s), they appear as if they were empty frames when in fact they are not, or else they wouldn't be shown on the canvas during playback or during export.

This is why I've been saying that Pencil2D has an auto-exposure system. It is indeed hidden but not entirely invisible to the user. It does feel like a pervasive feature, and only until one creates a new keyframe where you draw something else, or duplicate the previous keyframe with a change, is when you really notice that the exposure is truncated by the new keyframe.

Nevertheless, I completely understand your comment as I see Pencil2d has the same restrictions as Flash, where playback will only go up to the last created drawing / keyframe / instance, unlike TB and TVP which have a way to allow playback beyond the current keyframes / drawings. We also don't have enough visual aid to indicate the auto-exposure thing which complicates the user experience. I'm also agreeing that re-writing the timeline would probably change how we approach this, so this might become a disposable PR that might have to be refactored or rewritten once that is implemented.

With that said I'm trying my best to see this PR for what it is as well, since the timeline re-write isn't going to happen anywhere soon, but the PR would allow us to have more immersion:

  1. The enhanced behavior of drawing on any frame would make it more noticeable for the user that these apparently empty frames are actually connected to the previous keyframe.
  2. And additionally allow the user to change to the new behavior which is to add a new, separate, Keyframe when you draw on any of the "instanced exposure frames" that make part of the previously drawn keyframe.

As a user honestly I'd say let's go for it, but perhaps we should ask the other devs to weigh into this as I acknowledge that the problem isn't the PR itself but that Pencil2D is still lacking fundamental paradigms to enable receiving certain features without losing the hard work of all our contributors.

@martinvanzijl
Copy link
Contributor Author

Hi guys, thanks for your comments. Actually, I think there are 3 options:

  1. Create a new (blank) key-frame and start drawing on it.
  2. Duplicate the previous key-frame and start drawing on the duplicate.
  3. Keep drawing on the previous key-frame (i.e. the old behaviour).

I'll close this PR, create a new Issue "[Feature] Options when drawing on an empty key-frame", and link to the discussion here. Then I can submit a new PR when I have the code ready.

How does that sound?

@MrStevns
Copy link
Member

MrStevns commented Mar 8, 2018

Sure go for all three, the second new addition sounds nice to have too, especially if you want to continue from a previous stroke.

There's no reason to make a new PR though, just edit the title and continue with your commits.

@martinvanzijl martinvanzijl changed the title Fix "vanishing line" bug in bitmap layers [Feature] Options when drawing on an empty key-frame Mar 11, 2018
@martinvanzijl
Copy link
Contributor Author

Hi guys, I added the 3 options discussed above. Demo below:

empty-frame-options-demo-v2

These only apply for the Pencil, Pen and Brush tools. They don't apply for Bucket, Smudge, or Poly-line tools - those default to the "old" behaviour of modifying the previous key-frame. Does that sound sensible?

Note: this also works for vector layers, but I've only demonstrated bitmap layers in the video.

@Jose-Moreno
Copy link
Member

@martinvanzijl Awesome work! Thank you for taking the time to do this! From a broad perspective all tools that modify the content of an image either bitmap or vector, should trigger these behaviors (even the move tool when transforming a selection), however I think for now this could work as the reference foundation to properly implement the whole thing once the timeline is re-written, otherwise the bucket, polyline and smudge tool on both bitmap and vector engines would need to be fixed before implementing this with the current paradigm.

What do you think @candyface @chchwy ?

@MrStevns
Copy link
Member

Ideally it should work the same for all tools that modifies the canvas.
The two tools I don't see having any benefits from using this is color picker and clear canvas but everything else should work the same imo.

Smudge and move tool are still WIP; preference does not apply for
them yet.
@martinvanzijl
Copy link
Contributor Author

Thanks guys, I've made it apply to the bucket and poly-line tools too. It doesn't work for the smudge or move tool yet - those may take a while.

I checked in the latest code - please feel free to test it and see what you think.

@chchwy
Copy link
Member

chchwy commented Mar 13, 2018

Thanks for making this. @martinvanzijl Very cool and useful feature! Unfortunately it's about to release 0.6.1, so we will focus on bug fixes and stop accepting new features for a while. It won't take too long, and this PR will definitely be in 0.6.2.

I'm thinking, Could we add an auto key button on Timeline, so users can switch it without opening the preference dialog. What do you guys think?

@MrStevns MrStevns added this to the 6.2 milestone Mar 13, 2018
@MrStevns
Copy link
Member

MrStevns commented Mar 13, 2018

@chchwy I don't think we should add preferences to the timeline unless we know it's something we we're going to change often. We could however either make it a shortcut or make a new menu item called Timeline and add the actions there or do both.

@MrStevns
Copy link
Member

@chchwy when you going to merge this? I'm asking because it might break a part of my backup system so i'd like to test it to be sure that it won't before I make a PR

@chchwy
Copy link
Member

chchwy commented Apr 18, 2018

@candyface I'm doing it :)

@chchwy chchwy merged commit 84c591c into pencil2d:master Apr 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants