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

[Qt] multiple window/docking support #55

Open
tlambert03 opened this issue Feb 4, 2025 · 43 comments
Open

[Qt] multiple window/docking support #55

tlambert03 opened this issue Feb 4, 2025 · 43 comments
Labels
bug Something isn't working

Comments

@tlambert03
Copy link
Contributor

it looks like the call to winId() here in rendercanvas:

def _get_surface_ids(self):
if sys.platform.startswith("win") or sys.platform.startswith("darwin"):
return {
"window": int(self.winId()),
}
elif sys.platform.startswith("linux"):
if False:
# We fall back to XWayland, see _coreutils.py
return {
"platform": "wayland",
"window": int(self.winId()),
"display": int(get_alt_wayland_display()),
}
else:
return {
"platform": "x11",
"window": int(self.winId()),
"display": int(get_alt_x11_display()),
}
else:
raise RuntimeError(f"Cannot get Qt surface info on {sys.platform}.")

causes problems with the dock widget system, such that when you drag them, they don't "go with you" ... It's hard to explain so here's a visual:

before adding pygfx canvas, when you click and drag a dock widget it goes with you:

Screen.Recording.2025-02-03.at.7.48.41.PM.mov

after adding pygfx canvas, it does indeed float... but then it "stays put" until you go back and click on it again

Untitled.mov

really not sure who should be "responsible" for this one (pygfx or the end user). But so far, I haven't yet been able to find a way to recover normal dock widget activity after having instantiated a QtRenderCanvas.

@almarklein
Copy link
Member

almarklein commented Feb 4, 2025

Thanks for the report! I can indeed reproduce this with this minimal example. It does not even need a rendercanvas; creating a widget and calling winId() on it is enough to trigger the issue:

from PySide6 import QtWidgets, QtCore
# from rendercanvas.qt import QRenderWidget
# from rendercanvas.utils.cube import setup_drawing_sync


class Main(QtWidgets.QMainWindow):
    def __init__(self):
        super().__init__()
        self.resize(800, 600)

        # self.canvas = QRenderWidget( update_mode="continuous")
        # draw_frame = setup_drawing_sync(self.canvas)
        # self.canvas.request_draw(draw_frame)

        self.canvas = QtWidgets.QWidget()
        self.canvas.winId()

        self.dock1 = QtWidgets.QDockWidget("Dock 1", self)
        self.dock2 = QtWidgets.QDockWidget("Dock 2", self)
        self.dock3 = QtWidgets.QDockWidget("Dock 3", self)

        for dock in (self.dock1, self.dock2, self.dock3):
            dock.setFeatures(QtWidgets.QDockWidget.DockWidgetFeature.DockWidgetMovable)
            self.addDockWidget(QtCore.Qt.DockWidgetArea.LeftDockWidgetArea, dock)

        self.dock1.setWidget(QtWidgets.QPushButton("A button"))
        self.dock2.setWidget(QtWidgets.QPushButton("Another button"))
        self.dock3.setWidget(QtWidgets.QWidget())

        self.setCentralWidget(self.canvas)


app = QtWidgets.QApplication.instance()
if app is None:
    app = QtWidgets.QApplication()
m = Main()
m.show()
app.exec()

Not a huge problem, since users can still move the dockwidget, although its annoying.

I tested this on MacOS, are you aware of other platforms where this happens?

This looks like this bug report: https://bugreports.qt.io/browse/QTBUG-90976, and some others that it links to. They all seem closed though, so I'm not sure of the current status.

@tlambert03
Copy link
Contributor Author

Also tested on macOS…. Haven’t tried others yet. Thanks for pasting the mre (sorry I should have pasted the little simple example I also made while testing)

@tlambert03
Copy link
Contributor Author

Feel free to close this. I’m not sure how actionable it is from your side. I dont yet know enough about the underpinnings to know whether there could be a reasonable workaround to calling winId().

@Korijn
Copy link
Contributor

Korijn commented Feb 4, 2025

This looks like this bug report: https://bugreports.qt.io/browse/QTBUG-90976, and some others that it links to. They all seem closed though, so I'm not sure of the current status.

This bug (and the related bugs) are all from 2021 or older, I believe, and they seem to relate to Qt 5 as well. There might be a regression in Qt 6 now.

@tlambert03 I think the best action you could take is create an account and file a bug report with the PySide developers here: https://bugreports.qt.io/projects/PYSIDE/issues/

You can share Almar's MRE with them.

I've done this before and they were quite responsive and eager to help!

@Korijn Korijn closed this as completed Feb 4, 2025
@tlambert03
Copy link
Contributor Author

Yeah I’ve filed bugs there as well, (can’t say it’s always met with eagerness though, so generally look for workarounds first). I think it’s lower than pyside/shiboken though, as it happens with pyqt too

@Korijn
Copy link
Contributor

Korijn commented Feb 4, 2025

Well, in that case, I hope we get lucky :)

@almarklein
Copy link
Member

... whether there could be a reasonable workaround to calling winId().

I was going to suggest that you set QRenderWidget(..., present_method="bitmap"), which will render to an offscreen texture, download it, and pass the bitmap to qt. However, as it is now, winId() is called regardless of the present_method. This is easy to fix though, and that would provide a route to avoid this issue. However, it will be at the cost of performance, which may be noticeable if the canvas size is large.

Other than that, if we want to render to the portion of the screen for that Qt widget, we really need that winid :)

@tlambert03
Copy link
Contributor Author

fwiw, it doesn't seem to affect windows.

@tlambert03
Copy link
Contributor Author

tlambert03 commented Mar 10, 2025

Actually, I think the issue of calling winId is a bigger deal than just dock widgets on macos. I can say that I've noticed a lot of weird little bugs over the past few months. There was this issue of dragging dock windows on macos (which we just decided to live with since macos wasn't our immediate target for this project). Then there is this issue in ndv of weird positioning of a dropdown: which doesn't happen with vispy, and only happens once we let rendercanvas call winId. I'm now also seeing pygfx play poorly with qt-advanced-docking-system in pymmcore-plus/pymmcore-gui#51 ... there too, things are fine up until i create a pygfx canvas with rendercanvas... after which all rendering gets wonky (title bars are cut off and resizing leads to glitches).

So, for us at the moment, this issue is creeping into a lot of little annoying bugs, leaving us trying to determine whether it's worth pursuing one of those workarounds that @almarklein proposed above, or, unfortunately, possible not going full steam into pygfx as I had proposed in pyapp-kit/ndv#128.

I am unfortunately pretty useless in this at the moment, since I don't yet understand the need for, and implications of, winId. But just wanted to bump this now-closed issue and say that it's still a rather big issue for us

@tlambert03 tlambert03 changed the title bug(ish): calling winId() may break dockwidgets bug(ish): calling winId() may break many other Qt widgets Mar 10, 2025
@tlambert03
Copy link
Contributor Author

tlambert03 commented Mar 10, 2025

one question @almarklein... since vispy doesn't seem to have this issue, I went back to see what was done there. I see that the the EGL-backed canvas does use winId... but the "regular" CanvasBackendDesktop does not:

        if QT5_NEW_API or PYSIDE6_API or PYQT6_API:
            # Qt5 >= 5.4.0 - sharing is automatic
            QGLWidget.__init__(self, p.parent, hint)

            # Need to create an offscreen surface so we can get GL parameters
            # without opening/showing the Widget. PyQt5 >= 5.4 will create the
            # valid context later when the widget is shown.
            self._secondary_context = QtGui.QOpenGLContext()
            self._secondary_context.setShareContext(self.context())
            self._secondary_context.setFormat(glformat)
            self._secondary_context.create()

            self._surface = QtGui.QOffscreenSurface()
            self._surface.setFormat(glformat)
            self._surface.create()
            self._secondary_context.makeCurrent(self._surface)

is that code above similar in spirit to what you were describing in #55 (comment). that is, does it suffer from the same performance hit you'd expect with present_method="bitmap"? Or, does it actually benefit in this case from direct support of OpenGL by qt?

Either way, it would indeed be great if we had an option here, using rendercanvas and pygfx, to completely avoid using winId

@Korijn
Copy link
Contributor

Korijn commented Mar 10, 2025

I don't know enough about this to help you solve your problems, but I can explain that creation of a graphics context (the link between your application and the graphics card & screen) always requires a window Id, no matter if the window Id is handled "visibly" in Python code or "invisibly" inside Qt C++ code.

It's possible that in more advanced window management scenarios the Window ID is invalidated or multiple Window IDs come into play, and pygfx goes out of sync. This kind of challenge is not applicable when using vispy's CanvasBackendDesktop exactly because it delegates context (and therefore Window ID) management to Qt.

Unfortunately, if you want to delegate control to Qt, that means you are limited to the kinds of contexts Qt supports, which is only GL.

If you want to use wgpu, at least today, that means we have to implement context management ourselves. And that means we have to work with what Qt offers in its API.

It could be there is some kind of event system that we can subscribe to, to allow pygfx to handle all of the window management updates.

@tlambert03
Copy link
Contributor Author

tlambert03 commented Mar 10, 2025

yeah: that much does make sense, thank you :) I think this does pretty quickly get down to some qt-specific details. Qt itself tells you to be careful about it:

WId QWidget::winId() const

Returns the window system identifier of the widget.

Portable in principle, but if you use it you are probably about to do something non-portable. Be careful.


I do understand that our hands are tied here a bit. But, considering that this is a pretty major hicup... would you consider at least opening this issue again?

@tlambert03
Copy link
Contributor Author

tlambert03 commented Mar 10, 2025

It's possible that in more advanced window management scenarios the Window ID is invalidated or multiple Window IDs come into play, and pygfx goes out of sync ... It could be there is some kind of event system that we can subscribe to, to allow pygfx to handle all of the window management updates.

Each of my issues does indeed get triggered as soon as there is more than one window at play. So i agree that there is likely some room for improvement in the context management implementation

@Korijn Korijn reopened this Mar 10, 2025
@Korijn
Copy link
Contributor

Korijn commented Mar 10, 2025

So i agree that there is likely some room for improvement in the context management implementation

The primary question, I think, is if we can do it with the Qt API surface in Python, or if we have to go C++.

@Korijn Korijn changed the title bug(ish): calling winId() may break many other Qt widgets [Qt] multiple window support Mar 10, 2025
@Korijn Korijn added the bug Something isn't working label Mar 10, 2025
@Korijn Korijn changed the title [Qt] multiple window support [Qt] multiple window/docking support Mar 10, 2025
@Korijn
Copy link
Contributor

Korijn commented Mar 10, 2025

Yeah, alright, so in the case of docking widgets, it is indeed possible that Qt creates a new OS native window, with its own window ID. There is a signal QDockWidget.topLevelChanged that can be used to detect this.

We would have to subscribe to the event, get the new Window ID, and create a new graphics context (and destroy the old one).

Note that this also means all rendering context needs to be reinitialized, including reuploading any buffers and such.

Note: I figured this out by asking chatgpt. The Qt docs seem to indicate it's true.

You could try subscribing to the event to see if it is indeed triggered when your bug occurs.

@tlambert03
Copy link
Contributor Author

I'll try to have a look soon to see if the implementation here could be improved to be aware of changing ids

@tlambert03
Copy link
Contributor Author

There is a signal QDockWidget.topLevelChanged that can be used to detect this.

side-note: just fyi. I'm actually no longer even using QDockWidget. While that was the first thing that made me notice this issue, the issue extends well beyond dock widgets.

@Korijn
Copy link
Contributor

Korijn commented Mar 10, 2025

There is a signal QDockWidget.topLevelChanged that can be used to detect this.

side-note: just fyi. I'm actually no longer even using QDockWidget. While that was the first thing that made me notice this issue, the issue extends well beyond dock widgets.

Well, any reproduction scripts you can share will prove useful! We'll probably have to cover more than one scenario.

@Korijn
Copy link
Contributor

Korijn commented Mar 10, 2025

Just a note relating to the original report, I think this bug matches your reproduction: https://bugreports.qt.io/browse/QTBUG-98035 and there's a note about it in the docs here too: https://doc-snapshots.qt.io/qt6-dev/qdockwidget.html#appearance

@Korijn
Copy link
Contributor

Korijn commented Mar 10, 2025

Anyway, for rendercanvas, I guess we need to implement QEvent::WinIdChange

@Korijn
Copy link
Contributor

Korijn commented Mar 10, 2025

is that code above similar in spirit to what you were describing in #55 (comment). that is, does it suffer from the same performance hit you'd expect with present_method="bitmap"? Or, does it actually benefit in this case from direct support of OpenGL by qt?

To help you see the relationship with pygfx situation:

A "trick" to maintain the graphics context regardless of windowing changes, is to use an offscreen canvas, and copy the rendering results out of that framebuffer over to whatever other window you want to actually display in. The offscreen canvas also requires a window id, but because it's invisible it is much less susceptible to change. This does add overhead since you need to copy data out of the gpu and then back onto the gpu just to move it between two window contexts.

I imagine this is more or less what vispy does.

Note that there's not really anything special about these problems and solutions, all graphics applications deal with them in the same way. It's nothing particular about pygfx or vispy.

One wild idea to get around (or encapsulate the impact of) winId calls is to use pygfx with an offscreen canvas to do your rendering, and then display to the screen using Qt's native open GL context.

Just to be clear though, I don't recommend this due to the performance overhead and complexity of the setup. I hope we can just make it work reliably by handling WinIdChange events.

@tlambert03
Copy link
Contributor Author

Thanks, I learned a lot more today, but didn’t have time to update here. I do think the problem is actually deeper … and while it may not be about pygfx vs vispy directly, it is likely about maturity of various implementations at the c level.

I no longer think it’s as simple as monitoring the winIdChange event (though I began there too). It appears the damage is done simply by forcing qt to create the native window id in the first place.

I understand the general concepts at play here. I’m as interested at this point in determining just how much work it’s going to be for me to be able to use wpgu in a rich qt context (even if conceptually, they’re not that different). I haven’t yet looked at the c implementation of qt to see why using a qopengl context would circumvent this… but that’s the next thing I would like to answer

I’m traveling for a seminar this week though so won’t have any more info soon

@almarklein
Copy link
Member

almarklein commented Mar 11, 2025

The offscreen canvas also requires a window id

For the record, an offscreen canvas uses a simple texture, there's no winid needed.

And indeed Vispy does not have this problem, because it uses OpenGL, and Qt provides an OpenGL context. We don't have that luxury with wgpu, so we need that raw winid.

One wild idea to get around (or encapsulate the impact of) winId calls is to use pygfx with an offscreen canvas to do your rendering, and then display to the screen using Qt's native open GL context.

Right now it uses painter.drawImage. Using OpenGL might make it faster, but I think the biggest bottleneck right now is us downloading the image from the GPU.

... which is what I spent some time on today. I noted my findings on the issue that's about offscreen rendering:
#40 (comment)

I also made a pr in wgpu-py that should increase your fps by about 20% when using the bitmap present method: pygfx/wgpu-py#680

I will make a change in rendercanvas so that winid() is not called when in bitmap mode. That way, you should have a workaround that should be solid. Might result in somewhat lower fps if you use a large window, but that will improve in the future.

I will also have a look at QEvent::WinIdChange

@tlambert03
Copy link
Contributor Author

I will make a change in rendercanvas so that winid() is not called when in bitmap mode

that would be great thanks. A workaround would be welcome, (since at the moment it's more or a less a full blocker)

@Korijn
Copy link
Contributor

Korijn commented Mar 11, 2025

Worst case this workaround may have to become the default for Qt :/

@tlambert03
Copy link
Contributor Author

from what I've been learning, it looks like Qt itself, in QOpenGLWidget also renders to an offscreen frame buffer, and no longer uses native child window IDs, as it was determined to cause issues. It creates an offscreen surface, and frame buffer objects, and binds them to the context. the render() method renders the scene to that fbo, calling paintGL(), and then the content is composited into the widget (which is ultimately triggered by QWidgetRepaintManager::flush)

so, while implementing that in an efficient way will be important, it doesn't look like it's perhaps as much of a "workaround" as we thought for Qt... but perhaps rather the standard approach

@Korijn
Copy link
Contributor

Korijn commented Mar 11, 2025

Interesting. That sounds like good news.

@tlambert03
Copy link
Contributor Author

Yeah, good news in the sense that I guess most Qt users will already be accustomed to that level of nerfed performance 😂. I can say that I’ve never been particularly unhappy with, say, vispy performance when used in Qt. I do think it will be nice to keep the winId approach around for those who want the best possible performance (particularly with larger canvases), but given the potential damage it can do when compositing any other widgets in the same compound window (and given how sneaky and difficult to debug those issues can be), I’m thinking the default for qt should indeed be an offscreen buffer approach, with an opt in for direct window id (if you’re all ok with that)

@Korijn
Copy link
Contributor

Korijn commented Mar 11, 2025

Yeah, good news in the sense that I guess most Qt users will already be accustomed to that level of nerfed performance 😂. I can say that I’ve never been particularly unhappy with, say, vispy performance when used in Qt. I do think it will be nice to keep the winId approach around for those who want the best possible performance (particularly with larger canvases), but given the potential damage it can do when compositing any other widgets in the same compound window (and given how sneaky and difficult to debug those issues can be), I’m thinking the default for qt should indeed be an offscreen buffer approach, with an opt in for direct window id (if you’re all ok with that)

It makes sense to me. The tricky part I guess is that for "quick viz" the winId approach is a better default. Ah well. I think you are right.

Like you say, "nerfed performance" is already the default experience for Qt applications. Applications like games which require absolute top of the line performance are typically not engineered with Qt anyway. They tend to go with GLFW or SDL.

@almarklein
Copy link
Member

from what I've been learning, it looks like Qt itself, in QOpenGLWidget also renders to an offscreen frame buffer,

Thats ... interesting.

The tricky part I guess is that for "quick viz" the winId approach is a better default. Ah well. I think you are right.

We can use the bitmap approach by default, and use the winid method when the QRenderWidget is instantiated through window-level wrapper QRenderCanvas (and when we know the system supports it).

One problem is that our bitmap-based approach is relatively slow, until we find a way to avoid waiting for the buffer to be mapped, either via async, or maybe with a callback.

@almarklein
Copy link
Member

[...] and then the content is composited into the widget (which is ultimately triggered by QWidgetRepaintManager::flush)

Have you also found what the actual composing looks like?

@Korijn
Copy link
Contributor

Korijn commented Mar 12, 2025

Thats ... interesting.

Well, it makes sense, if you think of Qt as primarily being a GUI framework. It's just easier to support the myriad of use cases they need to handle (e.g. moving around widgets in a docking scenario, but also showing the same rendered image in multiple detached windows, etc etc).

@almarklein
Copy link
Member

I wonder whether they need to actually download the rendered image from the GPU, or whether the compositing happens in OpenGL as well, just exchanging the FBO texture ...

@Korijn
Copy link
Contributor

Korijn commented Mar 12, 2025

There is some code in qt core related to a shared context. They may be able to skip that step indeed. But I can't tell for sure.

@Korijn
Copy link
Contributor

Korijn commented Mar 12, 2025

Reading into it a bit more, I believe the "common method" is to use opengl's context resource sharing to allow the window's opengl context to read from the FBO texture that the offscreen context is rendering to. So the texture backing the offscreen FBO is shared with the other contexts.

@Korijn
Copy link
Contributor

Korijn commented Mar 12, 2025

For example GLFW has a short docs section dedicated to it: https://www.glfw.org/docs/3.3/context_guide.html ("Context object sharing").

Supposedly it's even possible to mix drivers (e.g. render to the FBO texture with OpenGL, and read from the texture with DirectX).

I guess this also enables some fancy multi threading or even processing scenarios.

@almarklein
Copy link
Member

@tlambert03 Except from the docked-window needing a click before a drag, were the other issues only on MacOS?

@tlambert03
Copy link
Contributor Author

No the other issues are on windows. They’re a bit hard to give you a MRE. But let me know what I can do to help.

@Korijn
Copy link
Contributor

Korijn commented Mar 12, 2025

So here's an idea; what if you share our context with a Qt open gl context and use that to render to the screen, bypassing both GPU download overhead and winId calls?

@almarklein
Copy link
Member

what if you share our context with a Qt open gl context

That's currently not possible. WGPU does not have a notion of a context. Well, GPUDevice kinda plays that role. I'm not even sure if a Vulkan/Metal texture can be shared with OpenGL. But even if it can, the wgpu abstraction makes that impossible. Also see gfx-rs/wgpu#4067

@almarklein
Copy link
Member

A summary of the current state:

  • The extra click required to drag a canvas in a QDockWidget is a known issue.
  • Other issues are observed on Windows when winId() is called.
  • Both problems can (after #60) be avoided by using render_method='bitmap', however this lowers your FPS somewhat.

Done so far:

Future:

  • Keep an eye out for winid issues in React to changing winId? #62.
  • I found that we can improve the bitmap-present performance a lot more, but this requires changes to the scheduling / and usage of the async wgpu api.

@Korijn
Copy link
Contributor

Korijn commented Mar 13, 2025

what if you share our context with a Qt open gl context

That's currently not possible. WGPU does not have a notion of a context. Well, GPUDevice kinda plays that role. I'm not even sure if a Vulkan/Metal texture can be shared with OpenGL. But even if it can, the wgpu abstraction makes that impossible. Also see gfx-rs/wgpu#4067

Really?? It is definitely possible to share textures between contexts, I think between all drivers. It would be super lame if wgpu is the only driver that can't do it. :/

@almarklein
Copy link
Member

Well, they're working on it. I suspect a big part of the difficulty is to find an API to expose these handles for the different drivers that wgpu abstracts over.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants