search

aseprite / laf

A C++ library to create desktop applications
https://aseprite.github.io/laf/
MIT License
276 stars 60 forks source link

Fix low performance over time with SetCursor (Fix Aseprite#2713) #29

Closed Enfyve closed 3 years ago

Enfyve commented 3 years ago

Instead of calling SetCursor(), swap the cursor reference on the window class. This aims to address https://github.com/aseprite/aseprite/issues/2713 Where SetCursor() results in decreased responsiveness as usage time progresses and persists even after closing and reopening the program.

I agree that my contributions are licensed under the Laf license, and agree to future changes to the licensing.

dacap commented 3 years ago

Hi @Enfyve, thanks for your patch. Actually I was thinking that the SetCursor() call is not necessary at all (because we call SetCursor() on the WM_SETCURSOR message). But after testing it doesn't work (and your patch has the same issue):

When the mouse is captured, the cursor is not changed/updated (it looks like Windows cached the last used cursor in SetCursor() and doesn't send more WM_SETCURSOR messages nor ask for the cursor to the window class).

A possible solution I'm thinking of is to call SetCursor() outside WM_SETCURSOR with some kind of timer (to group several mouse moves in one). But I'm not sure how well that could work.

Enfyve commented 3 years ago

@dacap Ah, I think I see what you mean. Whenever I was testing I altered the brush and moved my mouse which resulted in the cursor updating, but if I use key modifiers like ctrl/alt or change tool types without moving my mouse, the cursor won't be updated. I may have been overenthusiastic thinking such a simple solution would work 😅

I suppose setting the cursor on WM_KEYDOWN/WM_KEYUP in order to have a WM_SETCURSOR dispatch would be a naïve approach too..

I think the idea you had mentioned in the other repo regarding caching cursors may be the best approach. If I am understanding things correctly, moving the mouse causes a new bitmap to be generated, and while that process isn't unreasonably costly (including CreateIconIndirect()) it seems that the resulting HCURSOR is copied during SetCursor() and that copy is not freed by windows upon exiting or calling DeleteIcon()

dacap commented 3 years ago

That's right. I'm giving a chance to the cache approach. I guess it will be even better for theme cursors too (so the conversion between a os::Surface -> HCURSOR is done just one time instead of on every setNativeMouseCursor() call).