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

[pre-ll] Update to Glutin 0.21.0 #2737

Closed
goddessfreya opened this issue Apr 10, 2019 · 15 comments
Closed

[pre-ll] Update to Glutin 0.21.0 #2737

goddessfreya opened this issue Apr 10, 2019 · 15 comments

Comments

@goddessfreya
Copy link
Contributor

@goddessfreya goddessfreya commented Apr 10, 2019

Hey folks. Glutin v0.21.0 is out.

@alexheretic

This comment has been minimized.

Copy link
Contributor

@alexheretic alexheretic commented Apr 14, 2019

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

This comment has been minimized.

Copy link
Contributor Author

@goddessfreya goddessfreya commented Apr 14, 2019

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

This comment has been minimized.

Copy link
Contributor

@alexheretic alexheretic commented Apr 14, 2019

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

This comment has been minimized.

Copy link
Contributor Author

@goddessfreya goddessfreya commented Apr 14, 2019

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

This comment has been minimized.

Copy link
Contributor

@alexheretic alexheretic commented Apr 14, 2019

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

This comment has been minimized.

Copy link
Contributor Author

@goddessfreya goddessfreya commented Apr 19, 2019

@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

This comment has been minimized.

Copy link
Contributor

@alexheretic alexheretic commented Apr 19, 2019

Nice one

@goddessfreya goddessfreya changed the title Update to Glutin RC. Update to Glutin 0.21.0 Apr 20, 2019
@kvark

This comment has been minimized.

Copy link
Member

@kvark kvark commented Apr 22, 2019

Fixed by #2742

@kvark kvark closed this Apr 22, 2019
@goddessfreya

This comment has been minimized.

Copy link
Contributor Author

@goddessfreya goddessfreya commented Apr 22, 2019

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

@kvark kvark reopened this Apr 22, 2019
@psincf

This comment has been minimized.

Copy link

@psincf psincf commented Jun 25, 2019

@zegentzy @kvark
This issue seems to be similar to #2757 and have been resolved with #2824

@kvark kvark changed the title Update to Glutin 0.21.0 [pre-ll] Update to Glutin 0.21.0 Jun 25, 2019
@termhn

This comment has been minimized.

Copy link
Contributor

@termhn termhn commented Jun 26, 2019

Yeah this is resolved by #2824

@termhn termhn closed this Jun 26, 2019
@kvark

This comment has been minimized.

Copy link
Member

@kvark kvark commented Jun 26, 2019

@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?

@kvark kvark reopened this Jun 26, 2019
@termhn

This comment has been minimized.

Copy link
Contributor

@termhn termhn commented Jun 26, 2019

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

@kvark

This comment has been minimized.

Copy link
Member

@kvark kvark commented Jun 26, 2019

sorry, I got confused. Thanks for correction!

@kvark kvark closed this Jun 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.