Support for creating child windows

Boscop commented 7 years ago

It would be very useful to have support for creating child windows with a given parent handle. I implemented support for child windows on Windows in my glutin fork like half a year ago: https://github.com/Boscop/glutin/blob/master/src/window.rs#L205 https://github.com/Boscop/glutin/blob/master/src/api/win32/init.rs#L76-L84 https://github.com/Boscop/glutin/blob/master/src/api/win32/callback.rs#L23 https://github.com/Boscop/glutin/blob/master/src/api/win32/mod.rs#L471-L473 (Child windows have to run in the same thread as the parent window, so I'm using a HashMap with HWND as key for event dispatching of all the windows in the same thread.)

It would also be useful to have support for timers on the window. Either via callback: https://github.com/Boscop/glutin/blob/master/src/window.rs#L508-L516 Or events: https://github.com/Boscop/glutin/blob/master/src/events.rs#L61 https://github.com/Boscop/glutin/blob/master/src/api/win32/callback.rs#L342-L346

I implemented this because it's necessary to create a child window for writing GUIs for VST plugins. The plugin host creates a window and passes it to the plugin, which has to create a child window on that for it's GUI and register a timer on the HWND so that it refreshes it's GUI every few milliseconds. This is what I do in the plugin:

impl<'a> Editor for MyPlugin<'a> {
    fn open(&mut self, parent: *mut c_void) {
        let mut display = WindowBuilder::new().with_dimensions(WINDOW_WIDTH, WINDOW_HEIGHT).with_parent(WindowID::new(parent)).build_glium().unwrap();
        {
            let window = display.get_window().unwrap();
            unsafe { user32::SetWindowLongA(window.platform_window() as winapi::HWND, winapi::winuser::GWLP_USERDATA, self as *mut _ as winapi::LONG); }
            window.set_timer(WIN_GUI_FRAME_TIMER, 25 /* ms */, Some(handle_timer)).unwrap();
        }
// ...
const WIN_GUI_FRAME_TIMER: winapi::UINT_PTR = 4242;
unsafe extern "system" fn handle_timer(hwnd: winapi::HWND, _: winapi::UINT, timer_id: winapi::UINT_PTR, elapsed_ms: winapi::DWORD) {
    if timer_id != WIN_GUI_FRAME_TIMER {
        return;
    }
    let plugin = &mut *(user32::GetWindowLongA(hwnd, winapi::winuser::GWLP_USERDATA) as *mut MyPlugin);
    plugin.do_ui();
}

Unfortunately I only know how to do this on windows. . It would be very useful to have this in winit, maybe even in a cross-platform way. So that winit with conrod can be used to write GUIs for VST plugins, completely in Rust :)

Btw, at the time, conrod didn't have a winit (or glium) backend, so I also forked piston so I could pass the parent all the way down from my plugin to PistonWindow in conrod down to glutin. Btw, here is a screenshot of an old version of one of my plugins: https://i.imgur.com/4BQ1tCQ.gifv In the future I would like to use conrod with winit/glium backend for this.

monomadic commented 7 years ago

@Boscop I can successfully do this on mac os, using cocoa-rs, but haven't started on winit yet, just making raw cocoa framework calls at the moment. Can we share knowledge? I've come to similar conclusions - PR on winit to accept child windows + conrod is the best way to get all of this going.

conundrumer commented 6 years ago

fwiw, this functionality isn't that niche:

Boscop commented 6 years ago

Rust sciter has it too. (Qt can also do it btw.)

monomadic commented 6 years ago

Can winit give us some official stance on this? Many of us at the rust-dsp group have had hacked/forked versions of winit for a while now to get this going but it's become very tiresome keeping up with the changes.

If winit is willing to accept a "parent" functionality, could you provide us with some kind of api guideline (eg. what the calls should look like, an interface) and we will PR the feature.

I think the main point of confusion for us is not knowing if it's worth the trouble of PR-ing, if it is unlikely to be wanted by the winit project. I'll put the time into the mac side (I've already done it many times before for previous winit versions, though the latest makes it more complicated).

What do you guys think?

Boscop commented 6 years ago

Yes, I would like to have this in winit itself.. I added this feature to my glutin fork before winit existed, then again to winit because it had changed so much, but now it has changed again.. Other people (who aren't using winit for VST plugins but still need child window functionality) would also benefit from this feature in winit :) If tomaka agrees to merge a PR that implements this for mac and windows, I'll do the windows part.

monomadic commented 6 years ago

@tomaka - myself and @Boscop would be willing to work together on a PR for this (we've done this before on earlier versions), but we'd need your input as to:

whether it's even wanted in winit
what the api / interface would look like

francesca64 commented 6 years ago

I don't see any reason why this wouldn't be accepted, so long as it doesn't cause issues with the existing functionality/ergonomics or have a prohibitive hit on complexity/maintainability. I'd even be willing to implement this on X11, time permitting.

As far as what the API should be, that's hard to determine without me knowing what's required. Since both of you understand what you need, that's a good starting point to work with. Once it's implemented, we'll know exactly how things differ between platforms, and what impact the implementation has on existing structure. From there, it would be much easier to develop an effective interface than it would be to try to develop one now. In the short term, WindowBuilderExt and WndowExt are always safe bets, since they temporarily absolve you from having to think about it.

I should mention there's a caveat here. We have a shortage of reviewers, so depending on how extensive/intrusive the changes would be, it could take a while to get them merged.

monomadic commented 6 years ago

Given it's quite a bit of work, I'd really want someone to weigh in with merge permissions before we started this. If the project slips too far, particularly eyeing off PR https://github.com/tomaka/winit/pull/237 then the entire codebase changes significantly and we'd basically have to start again.

I think the safest way to do this is to bypass any kind of Builder pattern and just request a Window object directly from the Window class, supplying a handle as a c_void pointer.

The problem with supplying builder objects is that a lot of it becomes kind of redundant. Setup stuff isn't required as the window is already constructed and has attributes.

The other primary issue with winit is the way the mac codebase will implement Drop to free up the pointer to the window after the window closes. This is not desired at all and is too heavy handed for any applications wishing to just 'attach' to an unowned window. It will crash any host software you're trying to run under.

This may be a radical suggestion - but is winit actually becoming too complicated? Window creation and event loop management are so opinionated but separate that combining them in my opinion is creating a fairly bloated little environment. I know this might be the wrong place to raise this point, but to me, this whole issue being so complicated is a code smell of a god-problem with the library - it knows and controls a little too much and it doesn't really have to.

francesca64 commented 6 years ago

@robsaunders well, I do have merge permissions. You can wait for someone else if you'd like, but I have the most free time.

I've been thinking more about this, and no real guarantees can be made about whether or not a PR that doesn't exist yet will be merged. You're giving me the impression that the changes required are quite substantial (the more you elaborate on this, the easier it is to for an assessment to be made). If it were merged, would you be around to fix any regressions reported, and advise other contributors about how to understand the new code if needed, including on how to rebase existing PRs?

Bypassing the builder makes sense, right. As far as Drop goes, I'm somewhat averse to making the lifetime management of windows more complex.

For your decoupling suggestion, I'd recommend creating a thorough proposal about 1) the intended API, 2) the implementation/architectural changes required, and 3) the pros and cons for both all users (but especially the average user) and for maintainers/contributors (basically, the impact it would have on Rust's graphical application story as a whole).

monomadic commented 6 years ago

@francesca64 perfect! That's basically what I was asking, without trying to be rude :) thank you for putting the time into this issue.

The rust-dsp community (https://github.com/rust-dsp) reeeeally want this merged as it's kind of halting a lot of progress for us as we continually attempt to re-invent the wheel for a graphics solution, so I hope I'm portraying this correctly, but I want to be as explicit and honest as possible about what's required.

The scope of the issue is primarily hampered by the assumptions winit makes about windows you'll be managing, mainly on the MacOS side though. The Linux and Windows sides seem to be a much more straightforward implementation. This is because MacOS's event delegation system creates more choices for how to 'listen' to events, and because MacOS has an ARC for any created objects which must be catered for.

I don't foresee huge problems as long as we can bypass Builder and somehow remove the automatic ability to nuke the window (and call NSApp:terminate). A simple unowned_window parameter could alert winit to treat the contexts it does not own as read-only.

Fundamentally, I agree that we would not want to change winit in a way that any user not desiring these features would even notice they are there.

Just quickly on the last point about decoupling. I would foresee a library that concentrates purely on window generation, which winit could then depend upon. So users of winit would be completely unaware, but perhaps users like us would be able to interact with the smaller module directly.

This last point may actually solve both of our problems and also allow for new plugin/host architecture systems to use winit. For example, I could use a library that sits on top of this window library that wraps the window in a WebKit or WebView control (depending on OS) and rely on the smaller library purely for window generation, which as of now I have to roll my own and understand how the Rust/FFI/ObjC bridge works - I've gone this route and it is HEAVY learning, no normal user would want to do this.

Winit "takes over" so you can't do the above scenario at all, and then it provides an event loop that I have ZERO use for as these subsystems have their own eventing and callbacks.

Or, I could introduce a library that purely, for example, wraps some MacOS control such as a NSTimer or system control, and users could bundle together the support they needed to build a simple app without having to drag around the event management stuff of winit.

The winit system has chosen to abstract away events for a good reason - it becomes platform agnostic even to game systems like OpenGL, which is where it is most useful. But for regular desktop use or libraries that don't wish to go down this path, there's really no options in rust-land for them and it's holding them back in my opinion.

But again, even if the project were split out, I think users of winit should be largely unaffected as winit would just rely on this lib.

francesca64 commented 6 years ago

@robsaunders alright, unowned_window sounds pretty reasonable. I don't have any objections at this point, and I'll have faith in your statement that it won't balloon complexity or anything.

There are some pending API changes / reworks to be mindful of:

434 will hopefully be done soon (though it looks like the scope could be expanding) and I imagine it will touch some of the same code as your changes.
You're already aware of #237. I don't think it's likely to merge any time soon, but ideally your changes won't be fundamentally obstructive to its design. I don't know if that needs to be said (since I don't actually know what saying that entails in this case), but I feel I'd be remiss if I didn't say it.
There's also #445, relevant for @Boscop. It doesn't look like it will merge soon, since the unresolved issues are pretty significant and there's not any discussion happening.

On the decoupling issue, I can definitely see the merits in the idea. However, I have some reasons for why I don't think it can be a priority at present:

The average Rustacean already doesn't seem to know the difference between glutin and winit. With the addition of a 3rd layer, I can vividly visualize this scenario: report issue to glutin -> "sorry, this should actually be reported to winit" -> report issue to winit -> "actually, this is an issue with \<insert name of our super cool window creation crate here>". I know this isn't a very satisfying reason for impeding progress, but I do want to emphasize that while we can abstract this complexity away in the API, it can still lead to confusion.
It would be very difficult to mobilize a change of this scope. Making changes across multiple platforms is always an undertaking, and the current situation with reviewers and dedicated contributors per platform is as much of a blocking issue as anything else.
winit still has a lot of unresolved issues, both in implementation and API. Many of these have a much more immediate impact on whether or not applications can effectively be developed on top of winit. My feeling is that it's best to focus most of our limited resources on these issues.

My current objective is to try to improve project health, and I think that when winit becomes healthier, a proposal like this is much more realistic.

On that note, how would you feel about being pinged to review macOS PRs? It would be a great help.

monomadic commented 6 years ago

@francesca64 I can review MacOS PR's, sure. If you check my github you'll see I've done a lot of work in this area (Rust-MacOS FFI stuff). Would be happy to help out where I can, time permitting.

Decoupling, I did assume this was the reason so far. I think breaking off into libraries can simplify testing and merging PRs though. As libraries grow they often decouple to modularise teams and complexity. But I do see that it's no simple feat.

I do think that one approach would be for someone to actually write this smaller library, and perhaps once it reaches a level of stability and maturity, winit could consider consuming it in a future version and shedding its internal complexity on window generation and act as a wrapper for that plus eventing and the other benefits it provides.

I will need some time to fully go through those issues you linked, I am aware of them but I think I will chat with @Boscop and come up with a general game plan.

francesca64 commented 6 years ago

I'll give you some updates. CloseRequested/Destroyed is pretty much wrapped up, and it's very unintrusive, so you don't have to worry about it.

For the 2 PRs I linked, this is where discussion on the underlying issues is moving: https://github.com/tomaka/winit/issues/459#issuecomment-382405917

l0calh05t commented 5 years ago

As I'm interested in writing audio plugins in Rust, I very much hope that this becomes a reality sooner rather than later. However, the current coupling of event handling and the EventLoop cannot work in this scenario, as applications already have their own event loop. I believe it is absolutely necessary to separate the two and give each Window an onEvent handler instead of passing a general one to EventLoop.run. Otherwise, I see no way of making it optional in a clean fashion. Obviously, this would be a fairly big API change and control flow would have to be handled differently, but for this to succeed I don't see any way around it.

goddessfreya commented 5 years ago

Okay, so I just skimmed this and I'm not sure what exactly is wanted....

Based on these two things that @conundrumer linked, can the RawContextExts solve this problem?

https://docs.rs/glutin/0.22.0-alpha1/x86_64-pc-windows-msvc/glutin/platform/windows/trait.RawContextExt.html https://docs.rs/glutin/0.22.0-alpha1/glutin/platform/unix/trait.RawContextExt.html

goddessfreya commented 5 years ago

If not, what exactly is missing? If I understand this issue correctly, you folks are looking to separate the window handling from the opengl context creation code it self. This, afaik, is handled by that glutin extension.

This extension, of course, assumes the window is externally managed by some other library. (The window can even be managed by winit itself, fwiw.)

If the goal is to have the externally managed window somehow also partially managed by winit, can you explain to me what parts you want managed?

l0calh05t commented 5 years ago

@ZeGentzy I don't believe it would, no. We don't have control over the parent HWND (on Windows at least) or the application's event loop, so even if creating an OpenGL context in the HWND worked (and that is a big if as the parent window might not have CS_OWNDC set!), we'd still need a way to actually get events etc.

l0calh05t commented 5 years ago

No, OpenGL context creation is a separate issue. I may not event want to use OpenGL. The goal isn't really to partially manage an external window with winit, but rather to create a child window within an externally managed window for which we can get events, draw into, etc. In win32 terms, we want to create a child HWND with it's own window class and window proc.

Maybe have a look at what IPlug2 does in OpenWindow in https://github.com/iPlug2/iPlug2/blob/master/IGraphics/Platforms/IGraphicsWin.cpp

goddessfreya commented 5 years ago

Well, if CS_OWNDC is not set, I don't think this SFML function would work either, would it?

https://www.sfml-dev.org/documentation/2.4.2/classsf_1_1Window.php#a6d60912633bff9d33cf3ade4e0201de4

RawContextExt on windows shouldn't take control of the HWND, so if it does, I fucked up when implementing it. The RawContext generated will not allow you to receive events/do any of the windowing stuff usually available. It's only a glutin context, that's it.

l0calh05t commented 5 years ago

@ZeGentzy Depends on how it is implemented. It may not. I think @conundrumer examples are a bit misleading wrt this winit issue.

goddessfreya commented 5 years ago

No, OpenGL context creation is a separate issue. I may not event want to use OpenGL. The goal isn't really to partially manage an external window with winit, but rather to create a child window within an externally managed window for which we can get events, draw into, etc. In win32 terms, we want to create a child HWND with it's own window class and window proc.

Maybe have a look at what IPlug2 does in OpenWindow in https://github.com/iPlug2/iPlug2/blob/master/IGraphics/Platforms/IGraphicsWin.cpp

Ah, okay, that SFML link threw me off, as I'm 99% sure it doesn't make a child window, but just an OGL context on a window handle.

So I just skimmed this link, and I'm wondering, how exactly do you expect to receive events? If winit isn't managing the eventloop I'm not sure if that is possible? Presumably what is managing it will be the one passing you the events, no?

Anyways, I think a good first step is writing an PR with the new high level API you want, so that people can look at it and discuss it.

l0calh05t commented 5 years ago

So I just skimmed this link, and I'm wondering, how exactly do you expect to receive events? If winit isn't managing the eventloop I'm not sure if that is possible? Presumably what is managing it will be the one passing you the events, no?

Anyways, I think a good first step is writing an PR with the new high level API you want, so that people can look at it and discuss it.

As I mentioned in my first comment, the current EventLoop.run approach cannot work in this case. The solution on windows is to have your own window class and window proc that, via GWLP_USERDATA to get a pointer to your window instance, call your own event handling methods.

A new PR or shouldn't it maybe be an "RFC"-style issue?

l0calh05t commented 5 years ago

https://github.com/rust-dsp/rtb-rs/blob/master/examples/window.rs seems to be going in such a direction, but the implementation is currently mostly non-existent

goddessfreya commented 5 years ago

A new PR or shouldn't it maybe be an "RFC"-style issue?

Either works. The PR doesn't have to actually implement the backends, it just needs to show what new API you want and come with a little explanation.

crsaracco commented 5 years ago

vst-rs has, from what I can tell, two requirements that are not currently covered by winit (although I will admit that I'm not 100% familiar with the winit ecosystem, so let me know if I'm wrong and this is actually possible):

1:

Plugins need to attach to a parent window. The VST API/"protocol" does this by giving you the parent window's handle:

X11 window id (u32) on X11 systems
*mut winapi::HWND__ on Windows
Cocoa... something (?) on OS X (I'm not an apple person :D)

What happens under the hood is that the host (DAW) is creating the window for you, which you need to attach to in order for the DAW-plugin interaction to work correctly. I have an implementation for X11 here, and here is the specific example of attaching to the parent window in X11 systems. (Sorry for the messy, probably incorrect code; I'm not really a GUI dev. This code does work well enough to spawn a window in Linux and get events back to vst-rs, though.)

I think it was designed this way because of some limitations in OS X needing to handle all events in the parent window (main thread?)?

2:

(This part I'm super fuzzy on, because Linux handles it differently than the other two OSes, and again I'm not really a GUI dev so I have no idea what I'm doing. I'm sure I'm going to say something wrong here.)

The system needs to use the parent window's event loop, instead of creating its own (again, I think because of OS X?). Nevertheless, we need to be able to handle the events from within the plugin, somehow.

It's been a while since I touched my vst2-window code (and even longer since I touched rtb-rs), but I think the idea was going to end up being to pass in your own event handler so that the parent window can call that to handle events.

In the linux world, you can spawn the entire child window context (and event handler) in its own thread, and let it handle events that way. So the idea of passing in your own event handler to the hypothetical attach_child_window() function would work here, too.

There's also a little context in our wiki under the GUI section.

I'm really kicking myself for not keeping the vst2-window tester VST around for context =/

I'll follow this and #1044 and see if I can contribute anywhere.

l0calh05t commented 5 years ago

@crsaracco Part 1 seems to be partially done, as WindowBuilderExtWindows offers with_parent_window. WindowBuilderExtMacOS and WindowBuilderExtUnix don't appear to have anything similar (the latter has with_x11_visual and with_x11_screen but I don't think that's the right thing).

Wrt Part 2: From https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/Multithreading/ThreadSafetySummary/ThreadSafetySummary.html#//apple_ref/doc/uid/10000057i-CH12-123427

The NSView class is generally not thread-safe. You should create, destroy, resize, move, and perform other operations on NSView objects only from the main thread of an application.

So, yeah, this would appear to be a restriction on OS X (or is that macOS now?), as you get an NSView from the host application if I'm not mistaken (not an Apple person either, at least not wrt development)

In windows an HWND can be created from another thread in theory. But the consensus around the web seems to be you shouldn't. Haven't tried to do that yet personally.

crsaracco commented 4 years ago

@l0calh05t Ah, yep, indeed. Seems like the Unix and MacOS versions doesn't have with_parent yet.

Osspial commented 4 years ago

@crsaracco Does #1508 satisfy requirement 2 on Windows?

Fredemus commented 4 years ago

Hey @Osspial Very cool to see some progress on this! A lot of people in RustAudio are very excited to start making some gui's for our vst's.

I'm trying to get a vst running using #1508, but I'm getting following error:

thread '' panicked at 'InvalidateRgn should be used instead of UpdateWindow', C:\Users\rasmu\Documents\RustProjects\winit\src\platform_impl\windows\event_loop.rs:880:17

I don't think this is my fault, but the code for the vst is here if that's useful:

Apologies if this isn't the right place for this

perlindgren commented 4 years ago

Hey @Osspial Very cool to see some progress on this! A lot of people in RustAudio are very excited to start making some gui's for our vst's.

I'm trying to get a vst running using #1508, but I'm getting following error:

thread '' panicked at 'InvalidateRgn should be used instead of UpdateWindow', C:\Users\rasmu\Documents\RustProjects\winit\src\platform_impl\windows\event_loop.rs:880:17

I don't think this is my fault, but the code for the vst is here if that's useful: https://gist.github.com/Fredemus/af69619c3fbc79b91ce7471d7372ed7e#file-winit-vst-rs-L72-L107

Apologies if this isn't the right place for this

Hmm, the link above seems dead.

Fredemus commented 4 years ago

Sorry, fixed it now

Osspial commented 4 years ago

Good to know that this is a step in the right direction!

I'm trying to get a vst running using #1508, but I'm getting following error:

thread '' panicked at 'InvalidateRgn should be used instead of UpdateWindow', C:\Users\rasmu\Documents\RustProjects\winit\src\platform_impl\windows\event_loop.rs:880:17

I don't think this is my fault, but the code for the vst is here if that's useful:

Don't worry, that indeed is not your fault. There's actually some commented-out code right next to that panic that handles its branch properly - I think the assumption was that only Winit developers would ever see that panic - so I've uncommented said code and removed the panic in #1496. That should be fixed in #1508 once #1496 gets merged and I rebase #1508 onto master.

crsaracco commented 4 years ago

@crsaracco Does #1508 satisfy requirement 2 on Windows?

Sorry Osspial for sleeping on this for a while, I sorta ragequit working on VST GUIs for a while 😅.

I feel my interest coming back around a little, so I might look into this again pretty soon.

Although I will say, I'm mainly interested in Linux right now, so we'll see I guess.

ales-tsurko commented 4 years ago

Just to (semi-)clarify the current state of this issue for those, who are watching for it: the first part (https://github.com/rust-windowing/winit/issues/159#issuecomment-511124405) seems to be possible on macOS too. WindowExtMacOS::ns_view (or maybe HasRawWindowHandle) allows you to make it a subview of another view using NSView::addSubview method from Cocoa. But I'm not sure what should be done and what happens with the window containing this view 😄

ales-tsurko commented 4 years ago

I made an example for the "child window" on macOS. It uses winit and Iced. There're some issues though. Contributions are very welcome. For VST you would use EventLoop::run_return inside the idle function.

t-sin commented 2 years ago

In Linux X11, I could create a "child window" with egui for my VST3 plugin. It's used the fork repositories include changes equivalent to #2246. Using the result of that PR like this,

soyboy-window

kchibisov commented 2 years ago

X11 support was added for that.

daxpedda commented 11 months ago

Is this not completely covered by WindowBuilder::with_parent(), which is only missing Wayland (is it possible there?).

If yes we could close this and open a new tracking issue where we clarify what's missing. (apologies in advance, I didn't read through all that)

kchibisov commented 10 months ago

It all boils down to what kind of child windows we want here, it's either subviews (the ones constrained inside the parent) or when you just attach on-top of arbitrary window which may not even come from winit.

In general, both are not complete(subviews are not present at all). And both possible on Wayland, though child window is a bit limited there(not suvbiews).

jgcodes2020 commented 9 months ago

Is this not completely covered by WindowBuilder::with_parent(), which is only missing Wayland (is it possible there?).

If yes we could close this and open a new tracking issue where we clarify what's missing. (apologies in advance, I didn't read through all that)

If you're on the same wl_display connection, you can establish the surface as a wl_subsurface. I actually need this functionality for writing an emulator, as the library I'm using (mupen64plus) seems to need its own surface.

rust-windowing / winit

Support for creating child windows #159

434 will hopefully be done soon (though it looks like the scope could be expanding) and I imagine it will touch some of the same code as your changes.