Skip to content

Only update the dirty portion of the canvas #1761

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 9 commits into from
Jun 3, 2023
Merged

Only update the dirty portion of the canvas #1761

merged 9 commits into from
Jun 3, 2023

Conversation

MrStevns
Copy link
Member

@MrStevns MrStevns commented Apr 23, 2023 •
edited
Loading

This PR is the first of a series #1760 of improvements to address our declining performance issues.

In order to keep the scope as small as possible, I've only applied the changes to the CanvasPainter for now. The same could be done to the CameraPainter as well as the other Painter classes.

A summary of the changes:

  • Added blitRect to CanvasPainter, as well as implemented logic so that only the dirty area is cleared.
  • Cleaned up update logic in all the different tools. This is now done a bit smarter by applying the update directly in the drawXX method that was called anyway, instead of a separate method.
  • Rename invalidateCaches -> InvalidatePainterCashes
  • Removed ScribbleArea::paintBitmapBufferRect because it applies the change to the target image instead of buffer, defeating the purpose of the buffer entirely and probably causing unforeseen issues should it not be that we cleared the canvas pixmap on every draw call.
  • Fixes a memory leak in paintVectorFrame operation.

closes #1763

MrStevns added 4 commits April 23, 2023 12:19
@MrStevns
CanvasPainter painting improvements
b8f8823 
- Split painting for current frame and onion skinning
- Introduce blitRect for updating only the dirty portion of the frame
- Optimization: Only do an expensive fill when the size changes, otherwise rely on the blitRect to clear the dirty portion.
@MrStevns
Only update the dirty potion
0999f48 
Aside from cleaning up various extra update methods that are no longer necessary, I've made a minor change in vectorImage, in order to get it's correct dirty region.
@MrStevns
Fix vector selection being slightly wider than the overall bounds
36d9b2e 
This was a necessary change in order to calculate the correct bounds that wouldn't draw artifacts.
@MrStevns
Merge branch 'master' into painting-blitting-improvements
3ee714f
@MrStevns MrStevns mentioned this pull request Apr 23, 2023
Open
6 tasks
@J5lx J5lx added this to the v0.6.7 milestone Apr 23, 2023
@J5lx J5lx self-assigned this Apr 23, 2023
@MrStevns
Fix stroke placement for vector polyline
e41218d 
This also fixes some inconsistency in the CanvasPainter that I couldn't make sense of..
@MrStevns
Copy link
Member Author

Found a small bug regarding the polyline on the vector layer that should be fixed now. This also allowed a bit of cleanup regarding some inconsistent transform logic.

@MrStevns MrStevns requested a review from J5lx May 7, 2023 12:37
@MrStevns MrStevns mentioned this pull request May 27, 2023
Closed
J5lx
J5lx requested changes May 30, 2023
View reviewed changes
Copy link
Member

@J5lx J5lx left a comment

Choose a reason for hiding this comment

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

Hi @MrStevns. First of all, sorry for the delay in reviewing this. I thought I’d be able to finish this quickly, but evidently I failed quite spectacularly. On the plus side though, I’m happy to report I don’t have much to report. I only noticed one curious little bug which causes the current frame to also be drawn as next onion skin if it is the last keyframe on the layer, which looks like this:

image

Otherwise, these changes look pretty good to me. Well done!

MrStevns added 2 commits May 31, 2023 07:45
@MrStevns
Fix next onion skin being drawn on current frame when it's the last
85a9c1c
@MrStevns
Re-add loadFile if the bitmap image hasn't been loaded yet
8d56750 
Although i don't like that this exists in our painter, I believe we added it because for some reason the image hasn't been loaded yet, so let's leave it for now... ideally though i've really like to get rid of such weird mutable things that has little to do with painting.
@MrStevns
Copy link
Member Author

MrStevns commented May 31, 2023 •
edited
Loading

Hi Jacob

Thank you for your thorough review, I appreciate it!

I had some time this morning to look into the issue. After tracing over the old code again I noticed that we previously used getLastBitmapImageAtFrame or getBitmapImageAtFrame depending on the context. the onion skin logic previously used the latter so the fix was fortunately trivial.

PR is ready for review again

@MrStevns MrStevns requested a review from J5lx June 3, 2023 11:30
J5lx
J5lx approved these changes Jun 3, 2023
View reviewed changes
Copy link
Member

@J5lx J5lx left a comment

Choose a reason for hiding this comment

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

Bug confirmed fixed, looking good now! Feel free to merge if the change I made meets with your approval.

@MrStevns
Copy link
Member Author

MrStevns commented Jun 3, 2023

Changes looks good to me! I will merge the PR 😄

@MrStevns MrStevns merged commit c215a60 into pencil2d:master Jun 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@J5lx J5lx J5lx approved these changes

Assignees

@J5lx J5lx

Labels
Canvas
Projects
Enhancement Priority
Status: No status
Pull Request Priority
Status: Merged
Milestone
0.7.0
Development

Successfully merging this pull request may close these issues.

Memory leak when editing Stable Build files with Nightly Build editor.
2 participants
@MrStevns @J5lx