Implement access of raw context handles #939

ebkalderon · 2017-10-03T23:04:15Z

Added

Add GlContextExt trait, which grants type-safe access to the underlying OpenGL context (fixes Getting raw pointer to platform GL context #935).

Fixed

Minor formatting changes, i.e. fixing some spacing, reordering imports, etc.
Fix small doc comment typo.

CC @tomaka @kvark

tomaka · 2017-10-04T08:08:14Z

Thanks for the PR.

I haven't read the PR in details yet, but I have three remarks:

I don't think the method should be named as_mut_ptr, because that supposes that it returns a pointer.
I'd prefer if the code in the platform module didn't import anything from the os module. The retrieval methods should be directly implemented on the context, and not through a trait.
The GlContext trait implementations shouldn't use types that aren't integers or void pointers. I don't know if it's the case right now in your PR, but for example returning an opaque object or a pointer to an opaque object wouldn't be good.

ebkalderon · 2017-10-04T11:49:37Z

I appreciate the helpful feedback!

Sounds good. Perhaps raw_handle() or raw() or some similar permutation be a better name? Or do you have another preference in mind?
Will do. Shall I implement GlContextExt directly on the top-level glutin::Context instead?
I've already made sure of that, but please double-check in case of mistakes.

tomaka · 2017-10-04T12:03:12Z

Sounds good. Perhaps raw_handle() or raw() or some similar permutation be a better name?

I'd say raw_handle looks good.

Will do. Shall I implement GlContextExt directly on the top-level glutin::Context instead?

The GlContextExt trait shouldn't be used or mentionned anywhere, except in the files of the os module. In other words it should be totally isolated in this module.

It's not necessarily a hard-written rule, but that's how all other traits of os work right now, and it's better when everything uses the same design.

ebkalderon · 2017-10-04T13:55:25Z

The GlContextExt trait shouldn't be used or mentionned anywhere, except in the files of the os module. In other words it should be totally isolated in this module.

I believe you might have misunderstood my original question, but thanks for the helpful guideline. I was asking whether I should implement GlContextExt for glutin::Context in the same manner that WindowExt is implemented for winit::Window (relevant source).

tomaka · 2017-10-04T14:22:11Z

I believe you might have misunderstood my original question, but thanks for the helpful guideline. I was asking whether I should implement GlContextExt for glutin::Context in the same manner that WindowExt is implemented for winit::Window (relevant source).

Well, the answer is yes. Just move the impl ... for ... to the various submodules of os.
I'm confused by your question because implementing GlContextExt on Context is the whole point of the PR!

ebkalderon · 2017-10-04T14:59:01Z

Sorry if I wasn't being clear then, haha. I meant implementing for glutin::Context in contrast to implementing it against the platform::Contexts. Give me a break, I have been running very low on coffee these past few days! 😂

tomaka · 2017-10-04T17:59:33Z

src/api/osmesa/mod.rs

@@ -186,6 +190,11 @@ impl OsMesaContext {
    pub fn get_pixel_format(&self) -> PixelFormat {
        unimplemented!();
    }
+
+    #[inline]
+    pub unsafe fn raw_handle(&self) -> osmesa_sys::OSMesaContext {


That's not a void pointer.

Isn't OSMesaContext a mutable pointer to an opaque struct meant to be used identically to *mut c_void?

They are equivalent, but using a c_void makes it more semver-friendly. Sorry for not going into details.
You should return a *mut c_void and cast the OSMesaContext to it with as.

Okay, will do.

EDIT: Should I use a *mut libc::c_void or a *mut std::os::raw::c_void? The two types aren't compatible with each other, unfortunately. See rust-lang/libc#180. I'm assuming the former, since you have libc as a dependency.

No, please use the std::os::raw one.
The libc dependency should be removed eventually, but since doing so requires a major version bump, I never think about doing it.

Got it, std::os::raw it is!

tomaka · 2017-10-04T18:01:06Z

src/api/wgl/mod.rs

@@ -198,6 +197,11 @@ impl Context {
    pub fn get_pixel_format(&self) -> PixelFormat {
        self.pixel_format.clone()
    }
+
+    #[inline]
+    pub unsafe fn raw_handle(&self) -> winapi::HDC {


Not a void pointer.

Good catch! Thank you, I should have made it into an HGLRC instead.

tomaka · 2017-10-04T18:02:28Z

src/os/macos.rs

+use os::GlContextExt;
+
+impl GlContextExt for Context {
+    type Handle = id;


Not a void pointer.

What type should I use instead? I am not too familiar with OpenGL on Mac OS X.

A *mut c_void. https://github.com/servo/cocoa-rs/blob/master/src/base.rs#L17

Gotcha. Same rules as with OSMesaContext, then.

tomaka · 2017-10-05T11:52:08Z

Thanks!

ebkalderon changed the title ~~Implement GlContextExt for getting raw context handles~~ Implement access of raw context handles Oct 3, 2017

Implement GlContextExt for getting raw context handles (fixes #935)

90b188b

ebkalderon added 2 commits October 4, 2017 13:34

Rename all methods to raw_handle(), move trait impls to os submodules

e36f678

Add missing comma in match statement

d2bc54f

tomaka suggested changes Oct 4, 2017

View reviewed changes

ebkalderon added 2 commits October 4, 2017 14:27

Return an HGLRC instead of HDC for WGL contexts

f62bd0b

Cast certain handles as void pointer, minor reformatting

6b196fb

tomaka approved these changes Oct 5, 2017

View reviewed changes

tomaka merged commit f7e4e1a into rust-windowing:master Oct 5, 2017

ebkalderon mentioned this pull request Oct 5, 2017

Update as_mut_ptr to raw_handle and remove GlContextExt from iOS #940

Merged

francesca64 mentioned this pull request Nov 28, 2018

Add a way to retrieve the EGL display #1082

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement access of raw context handles #939

Implement access of raw context handles #939

ebkalderon commented Oct 3, 2017 •

edited

Loading

tomaka commented Oct 4, 2017

ebkalderon commented Oct 4, 2017

tomaka commented Oct 4, 2017

ebkalderon commented Oct 4, 2017

tomaka commented Oct 4, 2017

ebkalderon commented Oct 4, 2017 •

edited

Loading

tomaka Oct 4, 2017

ebkalderon Oct 4, 2017

tomaka Oct 4, 2017

ebkalderon Oct 4, 2017 •

edited

Loading

tomaka Oct 4, 2017

ebkalderon Oct 4, 2017

tomaka Oct 4, 2017

ebkalderon Oct 4, 2017

tomaka Oct 4, 2017

ebkalderon Oct 4, 2017

tomaka Oct 4, 2017

ebkalderon Oct 4, 2017

tomaka commented Oct 5, 2017

Implement access of raw context handles #939

Implement access of raw context handles #939

Conversation

ebkalderon commented Oct 3, 2017 • edited Loading

Added

Fixed

tomaka commented Oct 4, 2017

ebkalderon commented Oct 4, 2017

tomaka commented Oct 4, 2017

ebkalderon commented Oct 4, 2017

tomaka commented Oct 4, 2017

ebkalderon commented Oct 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebkalderon Oct 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomaka commented Oct 5, 2017

ebkalderon commented Oct 3, 2017 •

edited

Loading

ebkalderon commented Oct 4, 2017 •

edited

Loading

ebkalderon Oct 4, 2017 •

edited

Loading