Gfx: [pre-ll] Update to Glutin 0.21.0

[goddessfreya]

Hey folks. Glutin v0.21.0 is out.

[alexheretic]

I've migrated pre-ll with #2742. It's quite a breaking set of changes, but it does seem overall an improvement.

I would recommend adding some migration notes & explanations to the changelog or somewhere as it isn't totally obvious how to migrate. Take for example:

Removed new_windowed and new_headless from WindowedContext and Context, respectively.

This should mention what you should use now, ie ContextBuilder::build_windowed. It's particularly annoying here as these removed methods only just appeared last month.

I fully support breaking APIs to improve them, but breaking APIs is still a pain downstream. It would be nice to see justifications and as simple as possible migration steps accompanying the breaking changes.

I thought the NotCurrent & PossiblyCurrent type restrictions may actually break my usage in Robo Instructus where I have event processing in a compute thread and rendering in a different thread. I had the WindowedContext in an Arc so the compute thread had read access to some attributes. This usage isn't possible any more as the former must be moved into the latter, and the latter is not Send.

However, I found I was only really accessing get_hidpi_factor() in my compute and I can just save this and keep it up to date with HiDpiFactorChanged. So it doesn't break my compute/render thread usage yet.

[goddessfreya]

I apologize, @alexcrichton, for I haven't really gone around to writing a migration guide, and I should probably do that.

Anyways, I don't think it really makes sense to put a WindowedContext in an Arc (alone). For one, the window is not always Send + Sync (MacOS comes to mind), and so you'd have to first call .split on it (and only put the RawContext into the Arc).

Now, if the RawContext is PossiblyCurrent, it can't be moved between threads, and you should be using Rc, and well, I don't see any reason to ever make it not current, unless you got other contexts, but I guess you could use Rc<Cell<Option<..>>>> for that, or Rc<Cell<ContextEnum>>, where ContextEnum is something like enum ContextEnum { PC(RawContext<PossiblyCurrent>), NC(RawContext<NotCurrent>) }

If it is NotCurrent, you should be using Arc<Mutex<Option<RawContext<NotCurrent>>>>, and running .lock and .take before running .make_current, so that other threads don't get a chance to see it as current, and then .make_not_current before placing it back.

[alexheretic]

Well certainly I'd agree that it doesn't make much sense putting the window in and Arc _now_. I don't need to share the opengl context among threads, I just use some window properties (ie dpi) in my compute thread event processing.

Robo Instructus has a compute thread handling events and looping the game state unconnected to rendering, which happens on another thread that has the opengl context and it's own render loop.

winit already starting closing the walls on this technique some time ago when the EventsLoop became !Send, so this needs to be created in the compute thread. It's still ok because I do that & use it to create the WindowedContext<NotCurrent> which I _can_ send to my render thread where it can be made current.

Like I said losing the Arc (or rather losing Sync on the window) I no longer have any access to the window getter methods in my compute thread but I can work around that. I'm just trying to explain my usage here, and what I need to keep it working. Which after these changes comes down to "Please keep WindowedContext<NotCurrent> supporting Send".

[goddessfreya]

I'm afraid the Window part of the WindowedContext<T> cannot be Send on some platforms. Glutin, however, exposes the .split function if you intend to share the Context between threads.

Sending WindowedContext<NotCurrent> (or more precisely, the contained Window) to other threads is just incorrect on some platforms. If code which does that compiles on MacOS (and other platforms that don't allow that, although I only know of MacOS), then you should file a bug with winit.

[alexheretic]

Hmm it's documented that Window is Send. In fact the winit docs specifically mention this:

Note that the EventsLoop cannot be shared accross threads (due to platform-dependant logic forbiding it), as such it is neither Send nor Sync. If you need cross-thread access, the Window created from this EventsLoop can be sent to an other thread, and the EventsLoopProxy allows you to wakeup an EventsLoop from an other thread.

But anyway I'll keep in mind sending the context split off from the window as the next workaround when I get around to macos support.

[goddessfreya]

Well, that is wrong then :)

https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/Multithreading/ThreadSafetySummary/ThreadSafetySummary.html#//apple_ref/doc/uid/10000057i-CH12-123364

[goddessfreya]

@alexheretic Just wanted to inform you that I typed up a migration guide. Once some others eyeballs proof read it for me, I plan to release 0.21.0.
https://gentz.rocks/posts/glutin-v0-21-0-migration-guide/

[alexheretic]

Nice one

[kvark]

Fixed by #2742

[goddessfreya]

That only updated pre-II. gfx-hal is still on 0.19.

[psincf]

@ZeGentzy @kvark
This issue seems to be similar to https://github.com/gfx-rs/gfx/issues/2757 and have been resolved with https://github.com/gfx-rs/gfx/pull/2824

[termhn]

Yeah this is resolved by #2824

[kvark]

@termhn the master got updated, but pre-ll has not, and that's what @alexheretic is interested in, so I think we should keep this open?

[termhn]

@kvark I think #2742 updated pre-ll a little while ago, no? We originally reopened after that merge because master wasn't updated

[kvark]

sorry, I got confused. Thanks for correction!