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

Defer redraw of timeline content #1737

Merged
merged 3 commits into from
Jan 8, 2023

Conversation

J5lx
Copy link
Member

@J5lx J5lx commented Oct 31, 2022

Based on my findings from when I profiled the performance issue in #1724, I changed the timeline to redraw its contents only in response to a paint event rather than every single time in updateContent() (which is triggered by all sorts of operations throughout the program) and also made the existing caching mechanism a bit more aggressive while I was at it.

Additional changes:

  • Fixed an issue where the current frame border was not updated while playing
  • Fixed an issue where the current frame border did not take the length of sound frames into account
  • Fixed an issue that allowed frames to become selected after dragging the timeline via MMB
  • Moved the timeline and related classes from core_lib to app because that’s where all the other dock widgets live
  • Delayed showing the dock widgets until they have been docked to prevent them from briefly appearing in an undocked state before the main window is shown

@MrStevns
Copy link
Member

MrStevns commented Nov 6, 2022

I will start reviewing this now

Copy link
Member

@MrStevns MrStevns left a comment

Choose a reason for hiding this comment

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

Overall the changes looks good, although with additional cache logic now, we're definitely getting to the point where the next time we're touching the timelinecells class, we should refactor it beforehand.. it's getting to the point where it's difficult to understand changes and the control flow.

I noticed a bug which was introduced with the new cache logic

  • When right-clicking on the camera keyframe, the menu appears but the frame is not filled with white, which indicates that the frame is highlighted. The fix seems to be to add mRedrawContent to true in the showCameraMenu, so the repaint happens.

The rest of the changes looks good, and nice to see some well deserved clean up too 👍
I'd like to see the bug fixed but otherwise I only have one comment regarding the clearCache method being removed.

app/src/timelinecells.cpp Outdated Show resolved Hide resolved
@J5lx
Copy link
Member Author

J5lx commented Jan 4, 2023

I addressed both of your remarks, PTAL

we're definitely getting to the point where the next time we're touching the timelinecells class, we should refactor it beforehand

Agreed.

@J5lx J5lx requested a review from MrStevns January 4, 2023 00:11
@MrStevns
Copy link
Member

MrStevns commented Jan 8, 2023

Changes looks good to me now, merging.

@MrStevns MrStevns merged commit 16da92d into pencil2d:master Jan 8, 2023
@J5lx J5lx deleted the enhancements/defer-timeline-draw branch February 18, 2023 00:24
@MrStevns MrStevns added this to the v0.6.7 milestone Jul 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

2 participants