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

Implement captured frames loop region playback #122

Closed
wants to merge 23 commits into from
Closed

Conversation

charlielee
Copy link
Owner

I've had a few attempts at working out how to do this but believe this implementation works quite well.

In this pull request:

  • 2 new buttons are added to the playback controls: "begin selection" and "clear selection" (btn-start-keyframe and btn-clear-keyframe). These are disabled with their opacity reduced when not required.
  • 2 new functions added: addKeyframe() and removeKeyframe(). These have been implemented in such a way to allow to allow additional functionality to be added to the keyframe system in the future. These functions essentially set/clear a new variable curStartKeyframe and add/remove visual cues on the framereel.
  • Changes to the playback code have been made to see if a curStartKeyframe has been set.

My more detailed functionality plan:
Play Selection Plan.docx

In #116 I suggested an end selection button as well. However, this created a number of complications and made this feature a much more confusing UX.

@le717 I'd welcome your feedback on this. Thanks! :)

Edit: this has issues working after using the framereel's delete icons, although due to #114 I won't be attempting to fix this.

@le717
Copy link
Contributor

le717 commented Mar 1, 2016

Yea, you'll have to hold your horses on any work from me anytime soon. :(

https://twitter.com/_le717/status/704477544620417027

@charlielee
Copy link
Owner Author

@le717 Ah I'm sorry to hear that :(

@le717
Copy link
Contributor

le717 commented Mar 1, 2016

That said, this is a major change I want to review. If you can hold off on merging for a bit that would awesome. :)

@charlielee
Copy link
Owner Author

@le717 Sure thing :)

@le717 le717 changed the title Keyframes 1 fixes #116 Implement captured frames loop region playback Mar 2, 2016
@le717 le717 added this to the v0.7.x milestone Mar 2, 2016
@le717
Copy link
Contributor

le717 commented Mar 20, 2016

I feel so bad I can't do anything right now. I really want to look through all this and get it merged, but without my laptop I can't test anything, and testing is half of review. :(

@charlielee
Copy link
Owner Author

@le717 Don't feel bad, it's not your fault at all! :)

@le717 le717 mentioned this pull request Mar 23, 2016
@charlielee charlielee added the feature New or enhanced functionality for users label Mar 31, 2016
@charlielee charlielee force-pushed the keyframes-1 branch 3 times, most recently from 067d5aa to da1b6b4 Compare April 2, 2016 21:00
@charlielee
Copy link
Owner Author

@le717 Thanks for your comment, merging this was not right thing to do. Of course no one likes to be criticised or told they're wrong, but when done in a polite and constructive way (as you did) it is greatly appreciated and I'm very happy to listen. If there any other issues like this now or in the future feel free to speak up; I won't be offended. :)

I'll remove the merge commit from this branch and as you said, shortcuts can be reorientated once nwjs-013 is merged with master.

BTW I can't thank you enough for essentially mentoring me with this project. Your work here has been invaluable in making this a reality and helping me learn way more than I could of imagined in such a short space of time. I guess you've helped keep Boats Animator "a float"...

@le717
Copy link
Contributor

le717 commented Apr 5, 2016

BTW, my laptop is in the mail on the way back. Let's hope they actually fixed it this time.

@charlielee
Copy link
Owner Author

@le717 that's great to hear! :) I don't think I could last as long as you have without a computer.

@le717
Copy link
Contributor

le717 commented Apr 6, 2016

Oh, I have had a computer. I nabbed dad's Ultrabook and have been using it. I just have not had my computer, thus all my development stuff. :)

@le717
Copy link
Contributor

le717 commented Apr 11, 2016

*sigh* Loosing my laptop again for a third time because tech support creates problems. I am so, so sorry to keep all this waiting. I really want to review/merge everything but I can't without my dev environment (and I can't really set it up on another computer). :(

@charlielee
Copy link
Owner Author

This should be rebased before it is merged.

@charlielee
Copy link
Owner Author

I've finished updating this to work with the updated onion skin slider. The behaviour described in #71 has been fully implemented.

@le717
Copy link
Contributor

le717 commented Dec 20, 2016

I'm playing with this right now. :)

@le717 le717 self-requested a review December 20, 2016 18:04
Copy link
Contributor

@le717 le717 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, it was working. I went to confirm the behavior and it has suddenly broken. Onion skin is not working at all when a keyframe is selected. No errors thrown or anything, just flat out nothing. Not sure what's up. :\

That said, I do have an initial comment:

Why was the keyframe set to be added only on double-click? It does not make sense. It is more intuitive to click a frame once and set a keyframe that way to initiate playback starting point and onion skinning. I'd like it if just clicking a frame sets a keyframe and selects it in one go. It'd probably simplify the code a bit. Also, kill the blue inner border. Since it all goes in one action, the gray outline works.

Once I can get onion skinning working again, I'll leave more feedback.

@le717
Copy link
Contributor

le717 commented Dec 20, 2016

OK, so I've gone though and changed it to single click. Behavior is much more intuitive and straight-forward now. Hopefully I've not broken anything in the process. However, for whatever reason, onion skin is still broken. I've traced down two things (there may be more). I'll list them and let you figure it out.

  • When I add a keyframe, #onion-skinning-frame is not updated with the selected frame.
  • It would appear that the opacity change is doing nothing. It looks like the preview window is covering up the onion skin preview completely. However, I can't find anything that changes it.
  • The fact this started this morning, before my changes, and only worked for a few minutes, makes this even more confusing. :\

@charlielee
Copy link
Owner Author

That is very strange indeed, opacity change is working for me and never was broken.

Without the blue outline I don't really understand how the user can tell the current location of the key frame.

Also the key frame system is now broken:

  • When a user switches from playback to capture mode the keyframe is always set to the last frame captured - this means moving the onion skin slider to the left will always have the same effect as sliding it right which is the very behaviour this PR was supposed to change!
  • When a user begins playback from a keyframe, the current keyframe increments with each frame displayed. The keyframe system is supposed to allow you the begin playback from a set location multiple times and therefore should only change when the user explicitly says so.
  • When the last frame captured is reached in the frame reel, the play/pause and stop buttons do not begin playback from the beginning of the frame reel because a keyframe has been set to the last frame.

I have to admit I am rather confused with the changes you have made to keyframe system. I am not entirely sure that we both have the same idea in our minds of how it is supposed to work!

@le717
Copy link
Contributor

le717 commented Dec 20, 2016

I am terribly sorry. I was seeing it as more of an "advanced" form of frame selection... or something. IDK what, exactly. I've reset my changes and restored your code. I was trying to align it with Dragonframe's behavior, but I found it DF does not have this functionality so I was confused. The concept of "keyframes" is foreign to brickfilming (it's used in 3D animation), so that was throwing me and rioforce off.

That said, I've made two minor modifications after talking this over with rioforce:

  • The "Add keyframe" button is confusing, so it was removed
  • You should not be able to remove a keyframe by clicking it again

Here was the behavior rioforce said this should probably exhibit, again, since this is seemingly new behavior.

1.	Click on frame to make it gray
2.	Press play, it plays from frame to end and stops
3.	If looping, it goes to the beginning of the timeline
4.	If clicked another frame, it becomes gray
5.	Rinse and repeat

1.	Click on frame twice to make it blue
2.	Press play, it plays from frame to end and goes back to the frame
3.	If looping, it goes back to the blue frame then the end and the blue frame, etc etc.
4.	Press the “X” to remove the blue off the frame
5.	If clicked another frame once while a frame is blue, keep frame blue and still start from blue frame

We meet all this but still start from blue frame in the second half. Basically, when a "keyframe" is set, it overrides normal playback behavior, and the "gray" normal selection becomes a frame previewer.

Does this make sense or are both of us still confused?

@le717
Copy link
Contributor

le717 commented Dec 20, 2016

OK, I found the onion skinning behavior. It doesn't act as I nor rioforce expected, which is why I thought it broke. I'll write up how I was expecting it to work soon. Let's just start with the above issue and get it figured out.

Marking as approved for merging for the time being.

@charlielee
Copy link
Owner Author

Yes that makes sense and rioforce has got what I had in mind. I am of course open to a better and or radically different solution and since you two found this feature confusing, I would imagine the average user would find it even more so!

I'll hold off merging this until I see what you and rioforce were expecting of this. I haven't used Dragonframe enough to understand how this type of thing is handled there but I'll try and get round to downloading a trial.

@charlielee
Copy link
Owner Author

Also since I'd like to push forward with things I'm going to push this back to 0.8.x and krank out 0.7.4.

@charlielee charlielee modified the milestones: v0.8.x, v0.7.x Dec 20, 2016
@rioforce
Copy link
Contributor

rioforce commented Dec 21, 2016

Yes that makes sense and rioforce has got what I had in mind. I am of course open to a better and or radically different solution and since you two found this feature confusing, I would imagine the average user would find it even more so!

Are you speaking of the onion skinning or the playback-from-a-location feature? I think that the playing back from the location is a good idea, and returning to that set location is a good idea that not even Dragonframe has. Dragonframe allows the playback to start at the selected frame, but it will stop at the end of the timeline when the sequence is over and return to the beginning of the timeline if play is pressed again.

I took this quick video of the onion skinning functionality of Dragonframe earlier in case you need some help understanding how it's supposed to work. I'm afraid that I was super vague when I first described it all those ages ago. I'll need to check to see how the current functionality works, but for now, here's the video link if you need it. 😉

https://www.youtube.com/watch?v=JiuaDUKF-4Q

P.S. just to echo le717 here, the terminology "keyframe" was confusing to me when le was trying to describe to me the functionality of the program. It's not a big deal seeing as it's only internal, but I just found it to be somewhat misleading because keyframing seems quite different (at least in my mind 😛 )

@le717
Copy link
Contributor

le717 commented Dec 21, 2016

The onion skinning behavior in question is as so:

Current

  • When a frame is blue, you have to click live view to trigger onion skin.

Expected

  • When a frame is blue, you can drag the slider and it will begin onion skinning

Current

  • When a frame is blue, dragging the slider to the right overlays the last captured frame
  • When a frame is blue, dragging the slider to the left overlays the blue frame

Expected

  • When a frame is blue, dragging the slider to the right does nothing*
  • When a frame is blue, dragging the slider to the left overlays the blue frame

* This is how DF works, but it may be alright to keep the current behavior. We can have animators try it and tell us.

The first behavior is the most troublesome. The remaining code, skinning over the last captured when no frame is blue, works as expected.

Copy link
Contributor

@le717 le717 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Marking PR as unapproved so we can get everything cleared up.

@charlielee
Copy link
Owner Author

Thanks for the explanations.. :) I'm happy to rename the keyframe feature if anyone has suggestion for a clearer name for it.

In regards to onion skinning, while different from Dragonframe, would the following be suitable behaviour?

Action Capture mode Playback mode
Slide left If blue frame, onion skin with it, otherwise use last frame If blue frame, onion skin with it, otherwise do nothing
Slide right Onion skin with the last frame captured Do nothing

Also when playback occurs should onion skinning be paused?

Sorry for my continued confusion and please do correct me if I still haven't quite got this!

@le717
Copy link
Contributor

le717 commented Dec 22, 2016

You're fine. This is getting lots of discussion because we want it to right and we are trying to figure out what "right" it. :)

OK, let's backtrack. Let's leave the the slider alone ATM and focus on this one part right here:

Current
When a frame is blue, you have to click live view to trigger onion skin.
Expected
When a frame is blue, you can drag the slider and it will begin onion skinning

This is probably the biggest behavior issue right now. I do not want to have to mark a frame then go all the way back to the live button, click it, then begin onion skinning. I should be able to mark a frame and immediately begin. I've recorded a quick video of the current behavior, which, BTW, is not expected or intuitive.

https://www.youtube.com/watch?v=I9PtzTU4M6s

Does what I am saying and expecting here make sense?

@charlielee
Copy link
Owner Author

@le717 I've just made an update to the onion skinning behaviour. It now operates in both playback and capture modes.

@le717
Copy link
Contributor

le717 commented Dec 28, 2016

So you know how earlier this year I couldn't work on this because my laptop died?

Well, it died again. This time it's overheating and won't stay on. It's possible I'll be buying a new laptop.

Soooooo... yea. Sorry about that. 😞

@charlielee
Copy link
Owner Author

Oh no not again! I'm sorry to hear that 😞

@le717 le717 removed this from the v0.8.x milestone Dec 26, 2017
@charlielee
Copy link
Owner Author

This PR contains some interesting ideas but has been stale for so long that I think it should be closed and reexplored at a later stage

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New or enhanced functionality for users
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants