Fix `wxGLCanvas` performance when using Wayland (PR #23554)
VZ
This should hopefully help with #23512, but I haven't actually tested it with Kicad itself (yet).
Note: when backporting to 3.2, the last commit should be omitted or constexpr in it changed.
You can view, comment on, or merge this pull request online at:
https://github.com/wxWidgets/wxWidgets/pull/23554
Commit Summary
- fca2f53 Don't use wxGLCanvasEGL::m_readyToDraw for X11 windows
- 4884727 Fix wxGLCanvasEGL refresh logic under Wayland
- 31fa446 Add some tracing to wxGLCanvasEGL code
File Changes
(1 file)
- M src/unix/glegl.cpp (37)
Patch Links:
- https://github.com/wxWidgets/wxWidgets/pull/23554.patch
- https://github.com/wxWidgets/wxWidgets/pull/23554.diff
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz pushed 3 commits.
- 194a7be Improve wxGLCanvasEGL refresh logic under Wayland
- f2214ff Don't draw on hidden X11 EGL windows
- 54d44fc Add some tracing to wxGLCanvasEGL code
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
There is not much time left for merging this and backporting it to 3.2 if we want this to be included in 3.2.3.
@imciner2 @kuon Please test this PR if you can.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Nicolas Goy
I'm sorry, I broke my leg and couldn't test this PR yet. It's on my list, I'll do it asap.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Arti Zirk
Hi @vadz, I have tested this PR by applying the patches to Arch Linux wxWindows 3.2.2.1 package. fca2f53 needed patch -F3 but other applied cleanly. I also built kicad-wayland that has patches that enable Wayland support. I am using Sway 1.8.1.
I can confirm that this PR fixes performance problems that happen when Cross-probing is enabled and one of the windows is hidden.
(Also KiCad under native Wayland is soo much smoother than under Xwayland)
If there is anything else I can build and test I will gladly help.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
Great, thanks a lot for testing!
I'm going to merge this in master now and will cherry-pick it to 3.2 too.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
Merged #23554 into master.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Arti Zirk
After thinking about this for a while I think I may have found a small oversight.
IsShownOnScreen() uses m_readyToDraw as one of the checks under Wayland. Before this PR, m_readyToDraw was set to true once after first wl_frame_callback_handler. After this PR now the m_readyToDraw is toggled after each frame.
IsShownOnScreen() probably shouldn't flap at 60hz?
I hacked it use gtk_window_is_visible() for IsShownOnScreen check and in my limited testing Kicad renders a bit better.
diff --git a/src/unix/glegl.cpp b/src/unix/glegl.cpp
index c35273caddaa..0f009e3f4fb8 100644
--- a/src/unix/glegl.cpp
+++ b/src/unix/glegl.cpp
@@ -697,12 +697,13 @@ bool wxGLCanvasEGL::SwapBuffers()
bool wxGLCanvasEGL::IsShownOnScreen() const
{
wxDisplayInfo info = wxGetDisplayInfo();
+ GdkWindow* const window = GTKGetDrawingWindow();
switch ( info.type )
{
case wxDisplayX11:
return GetXWindow() && wxGLCanvasBase::IsShownOnScreen();
case wxDisplayWayland:
- return m_readyToDraw && wxGLCanvasBase::IsShownOnScreen();
+ return gtk_window_is_visible(window) && wxGLCanvasBase::IsShownOnScreen();
default:
return false;
}
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
Thanks @artizirk, I think you're right and we shouldn't be using m_readyToDraw here, but I'm a bit confused as to how does it actually change anything for redrawing: we don't seem to use IsShownOnScreen() anywhere under Wayland (under X11 we use it in SwapBuffers()).
What am I missing here?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
ShalokShalom
@vadz Open another issue for that?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@ShalokShalom Yes, you're right, we need to fix this. It looks easy, but I still don't understand how does it change anything for KiCad which only uses XWayland AFAIK.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
ShalokShalom
They have a thread about Wayland support, and it looks like there is even an Arch package with it.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Manolo-ES
Sorry, I'm strongly against this PR (internally using eglSwapInterval). Other PRs I see affected are #23909 and #23512
Setting SwapInterval is a user decision, not a wx one. It may be good for some cases but not for others. See this Khronos wiki article and its discussion.
Be aware that any final user can override app's settings by directly selecting it at driver's configuration (some vendors provide it).
Any how, I think SwapInterval is a feature that wx should provide to the user.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@Manolo-ES I am not sure if you've followed the previous discussions and bug reports, but without this PR existing code working fine under the other platforms (Windows, Mac and even Linux with X11) makes the program completely unusable under Wayland, i.e. it's a fatal problem and not just something that can be ignored.
I don't think it's reasonable to require all wx programs using OpenGL to call eglSwapInterval() themselves and I think it's even less reasonable to expect all users of these programs to change some driver setting.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.