-
Notifications
You must be signed in to change notification settings - Fork 279
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
Temporarily modify width and feather during Quick Sizing #1853
Conversation
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? |
@MrStevns Based on the Qt documentation for 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. |
@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.
There was a problem hiding this 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.
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.