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

Merges context creation code. #1029

Merged
merged 11 commits into from Sep 11, 2018
Merged

Conversation

@goddessfreya
Copy link
Member

goddessfreya commented Jun 13, 2018

Signed-off-by: Hal Gentz zegentzy@protonmail.com

Does #1028

Currently only 4 platforms need testing and one platform needs to get working.

Compiles with the following backends:

  • Linux | X11 | EGL
  • Linux | X11 | GLX
  • Linux | Wayland | EGL
  • Linux | OSMesa
  • Windows | WGL
  • Windows | EGL
  • Web | Emscripten
  • macOS
  • iOS
  • Android

Has been tested with the following backends:

  • Linux | X11 | EGL
  • Linux | X11 | GLX
  • Linux | Wayland | EGL
  • Linux | OSMesa
  • Windows | WGL
  • Windows | EGL
  • Web | Emscripten
  • macOS
  • iOS
  • Android
@goddessfreya goddessfreya changed the title Merges context creation code. [WIP] [Help Requested] Merges context creation code. Jun 13, 2018
@goddessfreya goddessfreya force-pushed the goddessfreya:merge-contexes branch from 2f70648 to 99b10a0 Jun 13, 2018
@keringar

This comment has been minimized.

Copy link

keringar commented Jun 13, 2018

I have a windows desktop and android device so I can help. My finals are almost over so I'll be able to help soon.

@goddessfreya goddessfreya force-pushed the goddessfreya:merge-contexes branch from 99b10a0 to e0b3d6d Jun 17, 2018
@goddessfreya goddessfreya force-pushed the goddessfreya:merge-contexes branch 2 times, most recently from 2b5ba81 to 76754c8 Jun 29, 2018
@kvark

This comment has been minimized.

Copy link
Contributor

kvark commented Jul 6, 2018

@zegentzy your PR is not configured for me to edit, so here is the iOS fix - kvark@5eb2598
Not tested, just built.

@goddessfreya

This comment has been minimized.

Copy link
Member Author

goddessfreya commented Jul 6, 2018

Thank you @kvark!

@francesca64 Unless someone wants to volunteer for testing these changes on the remaining platforms, I think this is as complete as it can get. Can you review/merge it?

@francesca64

This comment has been minimized.

Copy link
Collaborator

francesca64 commented Jul 6, 2018

I can test on macOS, iOS, and Android (though that's part of review anyway). I'll start working my way through this tomorrow.

@@ -1,4 +1,6 @@
# Unreleased
- ***BREAKING*** Headless contexts have been removed. Please instead use

This comment has been minimized.

Copy link
@francesca64

francesca64 Jul 8, 2018

Collaborator
  • There should be a newline between the header and the entry list
  • This BREAKING doesn't conform to the style precedent set by winit's CHANGELOG, which is Breaking:
  • This sounds a bit misleading, as in, it makes it sound like the functionality has been removed, rather than that the separate API has been removed
  • Adding an example program makes API migrations nicer for everyone

(whoops, I was supposed to "Start a review" instead of commenting... well, enjoy this early preview)

Copy link
Collaborator

francesca64 left a comment

Alright, spending 3 hours reviewing this should be enough for one day...

So far I've only tested on X11 and Android. X11 is broken, but Android appears fine. However, I didn't test any headless stuff.

I can't test things definitively until you've created a suitable example program.

src/lib.rs Outdated
//!
//! - `window` allows you to create regular windows and enables the `WindowBuilder` object.
//! - `headless` allows you to do headless rendering, and enables
//! the `HeadlessRendererBuilder` object.
//!
//! By default only `window` is enabled.

This comment has been minimized.

Copy link
@francesca64

francesca64 Jul 8, 2018

Collaborator

The use of the word "only" no longer makes sense.

unsafe {
match self.context {
GlContext::Egl(_) | GlContext::Glx(_) => {
assert_eq!((self.display.xlib.XResizeWindow)(

This comment has been minimized.

Copy link
@francesca64

francesca64 Jul 8, 2018

Collaborator

This assertion fails when running the window example! I can tell you neither researched nor tested this, since Xlib functions return 1 upon success: https://github.com/mirror/libX11/blob/78851f6a03130e3c720b60c3cbf96f8eb216d741/src/ChWindow.c#L53

Anyway, why are you resizing the window? resize is supposed to be called to resize the context after the window itself has been resized by other means.

Also, please include trailing commas after all match arms. It's also preferable for unsafe blocks to be tighter, since this encloses more than it needs to.

This comment has been minimized.

Copy link
@goddessfreya

goddessfreya Jul 9, 2018

Author Member

This was "tested" by seeing if this panics: gfx-rs/gfx@5399982 . I've now discovered that it incorrectly doesn't call the resize function. This was "researched" by referring to http://xfree86.org/current/XResizeWindow.3.html, which doesn't make it clear that it will return a 1 upon success. I assumed 0 upon success because, well, isn't that the standard?

I was under the impression that resize is what you call to resize the window.

Edit: Anyways, with this info I know that X11 doesn't need a resize function.

This comment has been minimized.

Copy link
@francesca64

francesca64 Jul 9, 2018

Collaborator

Sorry if I was mean about this. I tend to take mistakes in PRs personally.

match *self {
Context::X(ref _ctxt) => (),
Context::X11(ref ctxt) => ctxt.resize(window.get_xlib_window().unwrap(), width, height),

This comment has been minimized.

Copy link
@francesca64

francesca64 Jul 8, 2018

Collaborator

Why is a no-op no longer adequate?

This comment has been minimized.

Copy link
@goddessfreya

goddessfreya Jul 8, 2018

Author Member

I (incorrectly) added resizing support. Not actually necessary to be part of this PR.

Context::Wayland(ref ctxt) => ctxt.get_pixel_format()
Context::X11(ref ctxt) | Context::X11Context(_, ref ctxt) => ctxt.get_pixel_format(),
Context::Wayland(ref ctxt) | Context::WaylandContext(_, ref ctxt) => ctxt.get_pixel_format(),
Context::OsMesa(ref _ctxt) => panic!(),

This comment has been minimized.

Copy link
@francesca64

francesca64 Jul 8, 2018

Collaborator

What is the purpose of this panic?

This comment has been minimized.

Copy link
@goddessfreya

goddessfreya Jul 8, 2018

Author Member

OSMesa doesn't expose a swap_buffers. OSMesa's get_pixel_format panics. OSMesa has no resize function. I just realized the headless ones should be panicking the same.

pixel_format: PixelFormat,
}

pub struct HeadlessContext {
context: id,

This comment has been minimized.

Copy link
@francesca64

francesca64 Jul 8, 2018

Collaborator

This context appears to never be release'd (meaning, it leaks). Using IdRef instead would fix that.

This comment has been minimized.

Copy link
@goddessfreya

goddessfreya Jul 9, 2018

Author Member

Neither was the old one. I guess the headless one received less scrutiny. Will fix.

src/lib.rs Outdated
/// surface is resized.
///
/// The easiest way of doing this is to call this method for each `Resized` window event that
/// is received with the width and height given by the event.

This comment has been minimized.

Copy link
@francesca64

francesca64 Jul 8, 2018

Collaborator

This is also a good opportunity to update this line, since these days you take the LogicalSize from Resized, convert it into a PhysicalSize, and pass that.

src/lib.rs Outdated
let gl_attr = gl_attr.map_sharing(|ctxt| &ctxt.context);
platform::Context::new_context(el, &pf_reqs, &gl_attr, shareable_with_windowed_contexts)
.map(|context| Context {
context: context,

This comment has been minimized.

Copy link
@francesca64

francesca64 Jul 8, 2018

Collaborator

I'd prefer Context { context } for maximum cleanliness.

src/lib.rs Outdated
/// out of memory, etc.
/// The generated gl context is of course shareable with other contexts
/// made from this function. It is possible to also share it with a headless
/// context made with the shareable_with_windowed_contexts flag set to true.

This comment has been minimized.

Copy link
@francesca64

francesca64 Jul 8, 2018

Collaborator

I'd prefer "gl" be changed to "OpenGL", and both shareable_with_windowed_contexts and true should be in backticks.

I'm not sure how appropriate "of course" is here, since it seems to take the feature for granted. Considering that context sharing is never explained, the implication that this functionality is inherently expected could seem odd to readers.

pub trait GlContext {
pub trait GlContext
where
Self: Sized,

This comment has been minimized.

Copy link
@francesca64

francesca64 Jul 8, 2018

Collaborator

This added constraint is a breaking change, so must be mentioned in the CHANGELOG.

//! # Features
//!
//! This crate has two Cargo features: `window` and `headless`.
//! This crate has one Cargo feature: `window`.

This comment has been minimized.

Copy link
@francesca64

francesca64 Jul 8, 2018

Collaborator

The removal of a feature can cause build failures, so it must be mentioned in the CHANGELOG.

@goddessfreya goddessfreya force-pushed the goddessfreya:merge-contexes branch from 223b282 to 58b9296 Jul 9, 2018
@goddessfreya

This comment has been minimized.

Copy link
Member Author

goddessfreya commented Jul 9, 2018

swap_buffers, get_pixel_format and resize, under my understanding, should be panicking for headless contexts. It's nonsensical to, for example, call eglSwapBuffers when there is no visible native window to swap with, even if valid.

(In egl's case, if surface isn't a native window, the function silently does nothing.)

@kvark

This comment has been minimized.

Copy link
Contributor

kvark commented Jul 9, 2018

@francesca64

Alright, spending 3 hours reviewing this should be enough for one day...

Thank you for such a detailed and thoughtful review, as well as testing. Hope you aren't getting burned by this, and it the issues are to be resolved nicely.

@francesca64

This comment has been minimized.

Copy link
Collaborator

francesca64 commented Jul 9, 2018

@zegentzy I get what you're saying about the panics, but at bare minimum these panics would have to print explanations and have accompanying documentation notes. Though, what this tells me is that this API design is fairly problematic. I don't think it's reasonable for the API to misrepresent the context's capabilities.

@kvark I won't deny that I'm pushing myself harder than I should, but no one will want to contribute to glutin if PRs don't get reviewed. Just make sure I don't try to become the maintainer of any more of tomaka's repos!

@goddessfreya

This comment has been minimized.

Copy link
Member Author

goddessfreya commented Jul 9, 2018

I'd like to note that users can only call those three functions from GlWindow, which they won't have if they used a headless context. Anyways, I'll add messages and notes to the documentation.

@francesca64

This comment has been minimized.

Copy link
Collaborator

francesca64 commented Jul 10, 2018

Alright, that sounds reasonable then. If it's outright impossible for a GlWindow to be constructed with a headless context, then you don't even need to mention it in the docs, and it could be unreachable.

@goddessfreya goddessfreya force-pushed the goddessfreya:merge-contexes branch 2 times, most recently from 68a0c71 to 83d0768 Jul 10, 2018
@goddessfreya

This comment has been minimized.

Copy link
Member Author

goddessfreya commented Jul 10, 2018

@francesca64 Ok, I've replaced the panics with unreachable and haven't touched the docs.

@goddessfreya

This comment has been minimized.

Copy link
Member Author

goddessfreya commented Jul 23, 2018

I just noticed you added iOS support to the CI, @francesca64. Thank you! I'll be sure to fix the compile failure.

@goddessfreya goddessfreya force-pushed the goddessfreya:merge-contexes branch 2 times, most recently from 3b115ad to 880bca2 Jul 24, 2018
@goddessfreya

This comment has been minimized.

Copy link
Member Author

goddessfreya commented Jul 24, 2018

@francesca64 I'm like 98% certain the x11-dl build failure isn't caused by me. Can you check it out?

@francesca64

This comment has been minimized.

Copy link
Collaborator

francesca64 commented Jul 25, 2018

Yup: erlepereira/x11-rs#88

Is this PR considered complete now? I would still like to see an example program added.

@goddessfreya goddessfreya changed the title [WIP] [Help Requested] Merges context creation code. [Help Requested] Merges context creation code. Jul 26, 2018
Signed-off-by: Hal Gentz <zegentzy@protonmail.com>
@goddessfreya goddessfreya force-pushed the goddessfreya:merge-contexes branch from fba1575 to a2626df Aug 5, 2018
@@ -1,5 +1,9 @@
# Unreleased

- ***Breaking*** The entire api for Headless contexts have been removed. Please instead use `Context::new()` when trying to make a context without a visible window. Also removed `headless` feature.

This comment has been minimized.

Copy link
@francesca64

francesca64 Aug 5, 2018

Collaborator

"api" should be capitalized, "Headless" should be lowercase.

This comment has been minimized.

Copy link
@felixrabe

felixrabe Aug 5, 2018

Contributor

... contexts [have -> has] been removed ...

This comment has been minimized.

Copy link
@goddessfreya

goddessfreya Aug 5, 2018

Author Member

Thanks.

@@ -1,5 +1,9 @@
# Unreleased

- ***Breaking*** The entire api for Headless contexts have been removed. Please instead use `Context::new()` when trying to make a context without a visible window. Also removed `headless` feature.
- ***Breaking*** Structs implementing the `GlContext` trait must now be sized.

This comment has been minimized.

Copy link
@francesca64

francesca64 Aug 5, 2018

Collaborator

Structs aren't the only thing that can implement traits, so "Types" is probably better. Changing "sized" to Sized could also be clearer.

src/lib.rs Outdated
) -> Result<Self, CreationError>
{
let ContextBuilder { pf_reqs, gl_attr } = context_builder;
let gl_attr = gl_attr.map_sharing(|ctxt| panic!());

This comment has been minimized.

Copy link
@francesca64

francesca64 Aug 5, 2018

Collaborator

Shouldn't this panic! have a nice little message? Ideally referring the user to the docs.

This comment has been minimized.

Copy link
@francesca64

francesca64 Aug 5, 2018

Collaborator

Also, ctxt is an unused var. You should go with _ctxt so everyone doesn't see a warning.

This comment has been minimized.

Copy link
@goddessfreya

goddessfreya Aug 5, 2018

Author Member

Thanks


fn main() {
let mut events_loop = glutin::EventsLoop::new();
let mut size = glutin::dpi::PhysicalSize {

This comment has been minimized.

Copy link
@francesca64

francesca64 Aug 5, 2018

Collaborator

Is there a reason you chose this over PhysicalSize::new?

This comment has been minimized.

Copy link
@goddessfreya

goddessfreya Aug 5, 2018

Author Member

No, I didn't check if a PhysicalSize::new function exists. Updated.

src/lib.rs Outdated
pub fn new(
window_builder: WindowBuilder,
context_builder: ContextBuilder,
events_loop: &EventsLoop,
) -> Result<Self, CreationError>
{
let ContextBuilder { pf_reqs, gl_attr } = context_builder;
let gl_attr = gl_attr.map_sharing(|ctxt| panic!());

This comment has been minimized.

Copy link
@francesca64

francesca64 Aug 5, 2018

Collaborator

ctxt is unused and generates a warning.

src/lib.rs Outdated
unsafe {
platform::Context::new(window_builder, events_loop, &pf_reqs, &gl_attr)
.map(|(window, context)| GlWindow {
window: window,

This comment has been minimized.

Copy link
@francesca64

francesca64 Aug 5, 2018

Collaborator

window: window -> window (the same goes for the context: context below)

@@ -10,13 +10,12 @@ use winit;
use Api;

This comment has been minimized.

Copy link
@francesca64

francesca64 Aug 5, 2018

Collaborator

Api, ContextError, and PixelFormat are all unused.

@francesca64

This comment has been minimized.

Copy link
Collaborator

francesca64 commented Aug 5, 2018

I'm starting to go through and give this a final round of testing on all platforms. So far this looks pretty much ready, though it seems like the EGL module has several unused items now.

Signed-off-by: Hal Gentz <zegentzy@protonmail.com>
Signed-off-by: Hal Gentz <zegentzy@protonmail.com>
@goddessfreya goddessfreya mentioned this pull request Aug 15, 2018
13 of 14 tasks complete
src/lib.rs Outdated
context: Context { context },
})
}
platform::Context::new(window_builder, events_loop, &pf_reqs, &gl_attr)

This comment has been minimized.

Copy link
@goddessfreya

goddessfreya Aug 16, 2018

Author Member

@francesca64 Not all the platforms are safe. Some platforms have unsafe "new" functions (the platforms which support context sharing). Now it won't compile.

@goddessfreya

This comment has been minimized.

Copy link
Member Author

goddessfreya commented Aug 16, 2018

@francesca64 The platforms which currently support context sharing had their "new" functions marked unsafe. Your latest commit stops this PR from compiling.

@francesca64

This comment has been minimized.

Copy link
Collaborator

francesca64 commented Aug 16, 2018

Oh, I see. Having that be inconsistent seems rather surprising; is there a better way you can handle that?

@goddessfreya

This comment has been minimized.

Copy link
Member Author

goddessfreya commented Aug 17, 2018

@francesca64 We can mark them all unsafe or we could disable the warning. The second is probably better.

@francesca64

This comment has been minimized.

Copy link
Collaborator

francesca64 commented Aug 17, 2018

I'm in favor of disabling the warning (assuming I'm correct in understanding that this doesn't affect the public API) with the proviso that a comment also be added explaining why the warning was disabled.

@goddessfreya goddessfreya mentioned this pull request Aug 26, 2018
1 of 1 task complete
francesca64 and others added 2 commits Aug 15, 2018
Signed-off-by: Hal Gentz <zegentzy@protonmail.com>
@goddessfreya goddessfreya force-pushed the goddessfreya:merge-contexes branch from e197feb to fcc75e8 Aug 26, 2018
@goddessfreya

This comment has been minimized.

Copy link
Member Author

goddessfreya commented Aug 26, 2018

@francesca64 Done.

@goddessfreya

This comment has been minimized.

Copy link
Member Author

goddessfreya commented Sep 9, 2018

@francesca64 What's the progress on getting this merged?

@francesca64

This comment has been minimized.

Copy link
Collaborator

francesca64 commented Sep 11, 2018

Super sorry for the repeated delays! I was going through a tough time emotionally for a while, and I've only just recently returned to being active on glutin and winit. I think we're ready to merge now; thanks so much for all the hard work!

@francesca64 francesca64 merged commit 209ab92 into rust-windowing:master Sep 11, 2018
5 checks passed
5 checks passed
ci/circleci: android-test Your tests passed on CircleCI!
Details
ci/circleci: asmjs-test Your tests passed on CircleCI!
Details
ci/circleci: wasm-test Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.