Improving performance under Windows #180
Conversation
|
Here are the screenshots i offered. Solid backgrounds work fine and theme colors are respected. Win 7 Aero Theme
Win 7 Classic Theme
Win 7 High Contrast Theme
Win 7 with Custom Theme
|
|
Very happy to see some solutions popping up here. One quick question about the last screen shot: is the label font color not white due to the theme? Looks kind of weird as the forms elements are white. |
|
Good catch! I'll see what's up with the label text. There is a chance I messed the WM_CTLCOLORSTATIC up. |
nptr
force-pushed
the
container_performance
branch
2 times, most recently
from
bdd953b
to
cd1c95b
Compare
|
Seems like there is an expectation that the text color is set directly to the controls device context in the WM_CTLCOLORSTATIC handler. MSDN doesn't really say that this is important but only portraits it as an option in my opinion. I added the Its better now: |
Win11 Light Theme
|
windows/container.cpp
Outdated
| @@ -50,25 +69,46 @@ static LRESULT CALLBACK containerWndProc(HWND hwnd, UINT uMsg, WPARAM wParam, LP | |||
| mmi->ptMinTrackSize.x = minwid; | |||
| mmi->ptMinTrackSize.y = minht; | |||
| return lResult; | |||
|
|
|||
| // GDI doesn't support transparency. The Win32 controls way to achieve a similar effect | |||
There was a problem hiding this comment.
Nit: trailing whitespace here and below (GitHub UI doesn't detect this currently)
180.patch:57: trailing whitespace.
// GDI doesn't support transparency. The Win32 controls way to achieve a similar effect
180.patch:62: trailing whitespace.
// other containers. So we have to dynamically find the next control that is not a
warning: 2 lines add whitespace errors.
There was a problem hiding this comment.
Looks very clean to me. More lines removed than added - that's how I like things.
I tested things on my Windows 7 VM. I did not notice any regressions. Painting is a lot faster. Sadly still not instant, I guess that has to do with the delay of all the parent traversal and individual painting of controls?
Wine seems to work too.
A few things I noted that should probably be addressed in another PR but that are related and important enough to mention here:
- The
windowClassOflooks kind of wrong to me, asL"button"is used for many controls besidesuiGroup, like for example actual buttons. This seemingly doesn't seem to matter though? Which makes me wonder: how much of this code is actually needed at all? - Another existing problem that seems to persist with these patches is the
uiComboboxnot getting drawn off the bat. Page4 and 16 highlight this quite nicely. It only happens when you first open the application and gets drawn once you move the mouse over it, resize the window, etc.
windows/container.cpp
Outdated
| r = ps.rcPaint; | ||
| paintContainerBackground(hwnd, dc, &r); | ||
| EndPaint(hwnd, &ps); | ||
| if(!dc) break; |
There was a problem hiding this comment.
| if(!dc) break; | |
| if(dc == NULL) | |
| break; |
For consistency
There was a problem hiding this comment.
Done. I squashed the changes with the previous whitespace fixes.
There was a problem hiding this comment.
About the uiCombobox: I was hoping it vanishes too but it didn't. Possibly a bad combination of window styles that result in bad ordering of the draw calls? On a second glance, it could also be a Z-ordering problem.
There was a problem hiding this comment.
About the
uiCombobox: I was hoping it vanishes too but it didn't. Possibly a bad combination of window styles that result in bad ordering of the draw calls? On a second glance, it could also be a Z-ordering problem.
I've tracked this issue down somewhat.. nothing to do with uiCombobox specifcally.
Its an existing bug in uiTab page control impl: tab pages are implemented as reparented dialogs on Windows.
See the old issue with test case to reproduce: andlabs/ui#380 (comment)
There was a problem hiding this comment.
Anything left before this PR can be merged?
There was a problem hiding this comment.
Nope I think its ready to go, thanks @nptr
| parent = parentOf(parent); | ||
| // skip groupboxes; they're (supposed to be) transparent | ||
| // skip uiContainers; they don't draw anything | ||
| cls = windowClassOf(parent, L"button", containerClass, NULL); |
There was a problem hiding this comment.
I know this is from existing code and doesn't seem to affect things but L"button" is used for a lot more than uiGroup and looks wrong to me. No need for action.
There was a problem hiding this comment.
That would be pushbuttons, radiobuttons, checkbox, commandlink and groupbox if I am not mistaken. Only groupboxes can be parents in libui and possibly in general. I also had to look at this code quite a bit, but its fine i think. Just Win32 common controls madness.
nptr
force-pushed
the
container_performance
branch
from
e125787
to
359c4b6
Compare
Improving performance under Windows by dropping support for arbitrary backgrounds.
I was asked to open a PR for my patch i offered in issue #161.
libuihas acontainercontrol that is (seemingly) used to provide the basic layout mechanisms. It is supposed to be transparent.Unfortunately GDI doesn't support real transparency and things get really hacky because of this. The Win32 controls way to achieve a similar effect is to ask the parent control for the appropriate background brush (WM_CTLCOLORSTATIC) or ask to the parent to render the background for it (WM_PRINTCLIENT).
The original author chose the second route, double buffered too. This works with every background (images, gradients etc.). It hurts performance pretty hard however.
Note: I am not 100% sure if there isn't another reason why rendering is so slow. There seems to be some overdraw for example but I don't have the time to dig deeper.
The WM_CTLCOLORSTATIC route only supports solid color backgrounds. Since Microsoft doesn't use themed backgrounds since XP and most user themes also don't use gradients etc, the first approach seems to be a sufficient. This is what this PR is about. Performance is much better and there is no visual difference under the common Windows themes (Classic, Aero Basic, Aero Win7, Aero Win10).
Please have a look and let me know if it works for you just as good. I can also post some screenshots in the next few days.