-
-
Notifications
You must be signed in to change notification settings - Fork 76
Conversation
Thanks ! Can you squash your first commit please ? cc @gkoz |
Sure, think I did that right? |
Yup, that's good ! Thanks. :) |
Cargo.toml
Outdated
@@ -5,6 +5,8 @@ authors = ["The Gtk-rs Project Developers"] | |||
|
|||
[dependencies] | |||
libc = "0.1" | |||
gl = { version = "^0", optional = true } | |||
glutin = { version = "^0", optional = true } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be a good idea to clamp down on the allowed versions a bit? What if glutin 0.5
removes WindowBuilder
?
Please add this patch to make travis build the non-dummy version of the example: --- a/build.sh
+++ b/build.sh
@@ -5,7 +5,7 @@ set -e
if [ "$GTK" = latest ]; then
BUNDLE="gtk-3.18.1-2"
- FEATURES=gtk_3_10
+ FEATURES="gtk_3_16 opengl"
fi
if [ -n "$BUNDLE" ]; then |
…on required versions for OpenGL dependencies, add comment explaining glutin use in glarea example
Sure, hopefully this is better. I believe that the travis build failed as the example requires the modified GLArea stuff from the PR I submitted to the gtk repo (gtk-rs/gtk#185) |
The build succeeds now. @tomaka has hinted on |
So here's the explanation from @tomaka:
He also said that there was no "official" way to do it currently. :) |
Yeah I agree it's a hack, not sure about the chances of breakage though. Looked through the libepoxy source, their approach basically comes down to picking between Either way to me it looks like GtkGLArea creates a context for you, but relies on you to correctly infer the correct way to load and use OpenGL functions after initialization, so the same type of logic with the same type of assumptions probably needs to go somewhere to make this usable. Would you all rather I put in a large "HACKS AND BROKENNESS" warning, look at implementing libepoxy-style choosing between variants of Somewhat of a side-note, what do you all / @tomaka think should be the way forward with regards to glium integration? I started looking at what would need to happen for that to work, looks like one would need to create a |
@mjkoo We'd be very happy if you investigated proper (or at least more reliable) ways of integrating |
If we don't find a "perfect" / "official" solution, I'm okay with merging this PR if we add a complete explanation of what's going on in order to avoid losing people if some are interested in using opengl in GTK. |
@GuillaumeGomez I think there is a better solution, as it stands this version isn't really suitable as it leads to conflicts between |
src/glarea.rs
Outdated
|
||
let window = Window::new(gtk::WindowType::Toplevel).unwrap(); | ||
|
||
let cell = Rc::new(RefCell::new(GLArea::new().unwrap())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any GObject
already is a refcounted shared reference so you don't need to add another layer of that. Cloning will just add to refcount and all methods take &self
.
There will be a way to downcast objects once the object reform lands. I'll also look into making |
Cool thanks, that makes sense now, |
The amount of unsafe code makes me nervous. How much of it is due to the lack of |
Pretty much all of it, the |
src/glarea.rs
Outdated
let shader = Gl.CreateShader(ty); | ||
// Attempt to compile the shader | ||
let csrc = ffi::CString::new(src).unwrap().as_ptr(); | ||
Gl.ShaderSource(shader, 1, &csrc, ptr::null()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, it seems you're not required to use CString
here because the function takes an array of lengths?
I'm somewhat allergic to CString
because it's a popular source of accidental dangling pointers (not that you did anything wrong here).
src/glarea.rs
Outdated
if status != (epoxy::TRUE as GLint) { | ||
let mut len = 0; | ||
Gl.GetShaderiv(shader, epoxy::INFO_LOG_LENGTH, &mut len); | ||
let mut buf = vec![0u8; len as usize - 1]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't len
"the size of the character buffer required to store the information log)" making len - 1
too short?
Regarding the shader compiling functions, are the errors "static" in the sense that broken code won't compile and that's it, or can they depend on the hardware and the driver? In the latter case we might want to handle the error more gracefully than panicing. Also it appears |
…shader failure messages on panic
src/glarea.rs
Outdated
let mut len: GLint = 0; | ||
Gl.GetProgramiv(program, epoxy::INFO_LOG_LENGTH, &mut len); | ||
let mut buf = Vec::with_capacity(len as usize); | ||
buf.set_len((len as usize) - 1); // Skip trailing null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd still be cautious when writing past the "official" length. And I just don't see why buf = vec![0; len as usize]
would be inadequate. You can ignore the null byte later by taking a slice or letting CStr
take care of both that and utf8 compliance:
CStr::from_ptr(buf.as_ptr()).to_string_lossy()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay that sounds good, will make that change. In general writing to a Vec within its allocated capacity should be "okay" though (for some definition of okay)?
Fixed the issue with the shader compilation error fetching and printing, I think I originally was having issues compiling that that caused me to change it, not 100% sure why but the previous version definitely was off by one (good catch!) and just ate the actual shader compilation error. Brought it back to be in line with the implementation from the Shader compilation will depend on the hw and driver, not sure what would be a better way of handling it for this example application though? That error message appears to be from libepoxy itself (https://github.com/anholt/libepoxy/blob/master/src/dispatch_common.c#L674), perhaps we could do a check against |
An ideal way could be along the lines of using Panicing in a signal handler (which is a C callback) should be avoided at all cost because currently we just ignore the issue of unwinding across the FFI boundary ;) |
This might be too complex for its own good. If we defer the error message then the |
00dbf43
to
1ba0d7e
Compare
@mjkoo: Hi! The gtk crate has well evolved. Are you interested in updating this PR to the last version so we can merge it? Having an opengl example in gtk-rs would be really awesome! |
Sure, I'll take a look at it this weekend. |
Great! Thanks a lot! |
d4e9930
to
d45da29
Compare
Closing due to inactivity. Don't hesitate to reopen once updated. :) |
Added example of using a GtkGLArea from gtk-rs, currently the method of loading the required OpenGL functions is somewhat hacky (create an invisible window with glutin and use it's get_proc_address function to do the required name resolution). C examples tend to use a library called epoxy which does platform-independent GL function loading itself, having a similar functionality from within glutin would probably be better so you can still use the platform detection and library loading stuff without needing to actually construct a context/window. Unsure if this is possible with glutin as it stands currently, but after briefly perusing the code it does not seem to be.