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

Temporarily modify width and feather during Quick Sizing #1853

Merged
merged 2 commits into from
Jul 31, 2024

Conversation

khoidauminh
Copy link
Contributor

During Quick Sizing, Pencil2d tries to write changes to disk every time the width/feather changes. Some devices, particularly drawing tablets, send events at a high rate, causing Pencil2d to fail to keep up with with the events and hang for a moment.

This PR temporarily fixes the issue by introducing new methods that only changes the properties, then calling the original methods (which includes writing to disk) once we're done doing Quick Sizing.

@MrStevns
Copy link
Member

MrStevns commented Jul 24, 2024

Hi khoidauminh

Thanks for the contribution.

While the idea that we don't write to disk on every change is fine, I think the name of the function is misleading as it still changes the width property. I'd prefer that we didn't come up with a "temporary" bandaid solution but rather figure out how we can fix this properly.

As such, would you mind trying something?

I noticed that we call QSettings::sync() explicitly in all the tools after updating a setting, which might be overkill. Further more, this could also be a what's causing the needless amount of writes because sync will force an update every time it's called, where as normally setValue will simply add a write event to the event queue, and apply it sometime later. Theoretically that should prevent it from writing to disk too often.

What happens if you remove the sync calls, does the performance problem persist?
For example in PencilTool::setWidth, line: 77.

@scribblemaniac
Copy link
Member

@MrStevns Based on the Qt documentation for QSettings::sync, it gets called by the QSettings destructor., although the documentation for the destructor itself is less clear. While calling it explicitly might be overkill, I suspect it won't fix the performance issue here.

While a "proper" fix might address the possibility of this being an issue for all settings with some debouncing, I don't think this is a bandaid solution. It makes some sense to me that the width/feathering would not be applied by quick sizing until you release the cursor. If the property value is not set at all though, then you won't be able to see live updates in the options panel.

@khoidauminh
Copy link
Contributor Author

@MrStevns I've tested this and the main bottleneck is actually the QSettings::setValue() function. I think it tries to scan through the keys and set the value, which is very expensive.

We don't need to expose quick sizing behaviour to the ToolManager, since the value itself is not important, we just need to notify the UI, so the slider updates.
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.

Thanks for taking the initiative to fix this.

I've made some changes to simplify the logic.
Since the UI only had to be notified to update the slider, you could call ToolPropertyChanged from StrokeTool directly which avoids exposing the tempWidth logic to the manager.

@MrStevns MrStevns merged commit 7d1847e into pencil2d:master Jul 31, 2024
10 checks passed
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.

4 participants