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

#1105 Added checks on timelinecells, so scrub is tracked #1257

Merged

Conversation

davidlamhauge
Copy link
Contributor

@davidlamhauge davidlamhauge commented Aug 26, 2019

This PR see to it, that the scrubber always is visible during playback.
It is a rather simple solution, since we shouldn't do too complicated things on the timeline, while we wait for the Timeline rewrite.
This one has bugged me (and @Jose-Moreno) and possible others for some time, so here is a solution.
1105_scrubPlaybackTracking

Copy link
Member

@MrStevns MrStevns left a comment

Functionality wise I have no issues, it's only code complaints I'd like to see fixed before this can be merged. Timelinecells is already filled with spaghetti code all over the place, I see no reason to further the mistake.

core_lib/src/interface/timelinecells.cpp Outdated Show resolved Hide resolved
core_lib/src/interface/timelinecells.cpp Outdated Show resolved Hide resolved
Copy link
Member

@MrStevns MrStevns left a comment

LGTM! Great improvement David 👍

@MrStevns
Copy link
Member

MrStevns commented Aug 26, 2019

Since the number of changes here are small, we can merge this right away, however to give the chance of someone else to review it, I'll wait till tomorrow before merging.

@davidlamhauge
Copy link
Contributor Author

davidlamhauge commented Aug 26, 2019

It is indeed small changes, but let it rest a day if someone else wants to review it. Thanks!

@Jose-Moreno
Copy link
Member

Jose-Moreno commented Aug 26, 2019

Awesome work David, once this is merged Github will automatically close the relevant issue. Thanks!

This closes #1105

@scribblemaniac scribblemaniac added Enhancement Timeline UX Related to the way users interact with the program labels Aug 26, 2019
Copy link
Member

@scribblemaniac scribblemaniac left a comment

I personally think that the timeline should move for any scrubbing actions, not just playback. For instance, if you are moving it with the arrows keys or mouse, you want to see where you are moving it to, even if that's off the current screen. I've submitted a PR to your repo for this, but I'd like to hear if there are any thought on this idea first. Otherwise this looks good.

@MrStevns
Copy link
Member

MrStevns commented Aug 26, 2019

I agree that would be useful.

…king

Update timeline position for all scrub movements, not just playback
@Jose-Moreno
Copy link
Member

Jose-Moreno commented Aug 27, 2019

I also agree that should be helpful since currently using the arrow keys or the period / comma keys do not update the timeline. It'd be nice that probably the Next / previous keyframe actions update the timeline as well. 👌

@davidlamhauge
Copy link
Contributor Author

davidlamhauge commented Aug 27, 2019

Two small errors fixed.
When moving left, the scrubber went out of sight.
If you used the horizontal scrollbar to move the scrubber out of sight, and then pressed Play, the scrubber would remain out of sight, until it reached the place you scrolled to.
Now everything should work as expected, whether you Play, Scrub the timeline, or move to previous/next frame or keyframe.
Feel free to test it!

Copy link
Member

@MrStevns MrStevns left a comment

Reviewing after latest code changes: looks fine to me.

A question though, right now the behaviour differs between going forward and backwards, going backwards only pushes the timeline a frame at a time, while going forward pushes you x frames forward... I think this should be consistent but which behaviour is best? the former gives you absolute control of the situation, while the latter throws you x frames forward, so if you wanted to only scrub 3 frames or so, then you'll be required to scrub forwards afterwards.

I'd also like to see the scrubber being paused temporarily while moving the timeline scrollbar or at least allow free roaming. Right now you're fixed to the scrubber visibility wise.

@davidlamhauge
Copy link
Contributor Author

davidlamhauge commented Aug 28, 2019

I have made so it behave likewise forwards and backwards.
I don't understand your last paragraph @candyface , about pausing the scrubber, so I haven't addressed that.
I meant this PR to be a quick-fix for an old problem, while we're waiting for the new timeline, but I'll of course be ready to make more changes, if they are wanted.

@scribblemaniac scribblemaniac self-requested a review Aug 31, 2019
Copy link
Member

@scribblemaniac scribblemaniac left a comment

Looks good. I like that it still jumps halfway during playback rather than staying at the end as it means less full timeline updates during the time-sensitive playback. I might even be tempted to go a step further and have it jump to the beginning when the scrubber goes off the screen during playback (ie mFrameOffset = mEditor->currentFrame())

@davidlamhauge
Copy link
Contributor Author

davidlamhauge commented Aug 31, 2019

It is less timeline updates, that's the reason for jumping halfway back, when it goes of screen. I thought that it would be too much if it went all the way over to the left.
But - I am ok with letting it jump to the far left, if we agree on that.

Copy link
Member

@MrStevns MrStevns left a comment

I am satisfied with the changes. Imo. we can merge now.

@scribblemaniac
Copy link
Member

scribblemaniac commented Sep 1, 2019

@candyface Do you have any comments on the discussion about how far back the scrubber should go during playback?

@MrStevns
Copy link
Member

MrStevns commented Sep 1, 2019

@candyface Do you have any comments on the discussion about how far back the scrubber should go during playback?

Not really, I think it's fine to scrub to the middle of the view.

@MrStevns
Copy link
Member

MrStevns commented Sep 3, 2019

Two developers has approved the changes, so I'm merging. If there's a need to further discuss the functionality, a new issue can be created.

@MrStevns MrStevns merged commit 5341b85 into pencil2d:master Sep 3, 2019
@chchwy chchwy added this to the 0.6.5 milestone Nov 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Timeline UX Related to the way users interact with the program
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants