GNOME Bugzilla – Bug 752314
Add Gtk::GLArea and example to gtkmm-demo
Last modified: 2015-07-23 19:43:17 UTC
Created attachment 307336 [details] [review] Adds Gtk::GLArea and a demo Sorry it's currently against 3.16 - that's what I've found on my machine to work with - but it's perhaps a good place to start to see if I'm going in the right direction. Adds the necessary GdkGLContext + enum + GtkGLArea bits plus a C++ equivalent of the C glarea example. Builds and runs, but I've probably got the lifecycle of some of these things wrong - on exit I get the following warning that perhaps someone with better knowledge of lifecycle can help me with: (lt-gtkmm-demo:28487): Gtk-CRITICAL **: gtk_gl_area_make_current: assertion 'gtk_widget_get_realized (widget)' failed It seems that the actual widget unrealize is being called before we get a chance to do what we need to clean up the GL bits. Any help here would be appreciated. (The original C glarea demo doesn't emit this error but has the same logic). In addition, I'm not confident in any of the "const" decisions for the wrapped methods / properties. If you'd prefer to have a patch against HEAD you can perhaps give me a little hint what's the best way to set up an environment for that (I'm on Arch L). So, one other thing to mention - this has only been run on Linux. I don't have a windows box to check it out I'm afraid - so possibly msvc might be unhappy with the c++ in the gtkmm-demo.
Review of attachment 307336 [details] [review]: It looks good. Many thanks. It just needs some simple improvements before we can put it in git master. Is the demo based on one in GTK+ in C? Also, we'll need to make this more C++11 like, in a separate commit, now that we use C++11 in gtkmm. But I can do that for you if you can't use git master. ::: .gitignore @@ +26,3 @@ +# /build +compile + These changes are unrelated. ::: configure.ac @@ -92,3 @@ # The extra cairomm-1.0 check is because gdkmm requires a newer cairomm than pangomm requires. # The extra gdk-pixbuf-2.0 check is because gdkmm requires a newer gdk-pixbuf than gtk+-3.0 requires. -AC_SUBST([GDKMM_MODULES], ['giomm-2.4 >= 2.44.0 pangomm-1.4 >= 2.27.1 gtk+-3.0 >= 3.16.0 cairomm-1.0 >= 1.9.2 gdk-pixbuf-2.0 >= 2.26.0']) Why do we need to explicitly depend on epoxy? Isn't that just something that GTK+ uses? ::: demos/gtk-demo/example_glarea.cc @@ +19,3 @@ + +enum { + X_AXIS, The other demo/example files use 2 spaces for indentation. Please do that here too. @@ +38,3 @@ + virtual ~Example_GLArea(); + + void on_axis_value_change( int axis, Glib::RefPtr<Gtk::Adjustment> adj ); Please use const & for the RefPtr. And this should be protected or private. @@ +50,3 @@ + Gtk::Button m_Button; + + Gtk::Box * create_axis_slider_box( int axis ); Please put the * next to the type. @@ +72,3 @@ +Example_GLArea::Example_GLArea() : + m_VBox(Gtk::ORIENTATION_VERTICAL, false ), + m_GLArea(), We don't need to specify the default constructor. @@ +81,3 @@ + + set_title("GL Area"); + set_default_size( 400, 600 ); Please remove the spaces inside the () here and elsewhere. I only add them where it makes the code clearer, for instance where there's a big sigc::mem_fun() part. @@ +171,3 @@ +} + +Gtk::Box * Example_GLArea::create_axis_slider_box( int axis ) Please put the * next to the type, instead of separating it by a space. ::: gdk/src/glcontext.ccg @@ +1,1 @@ +/* Copyright 1998-2015 The gtkmm Development Team The copyright year should be 2015. ::: gdk/src/glcontext.hg @@ +1,1 @@ +/* Copyright (C) 2002 The gtkmm Development Team The copyright year should be 2015. @@ +28,3 @@ +{ + +class GLContext : public Glib::Object This needs documentation based on the C documentation. And it needs a newin line. @@ +36,3 @@ + +public: + _WRAP_METHOD(Glib::RefPtr<Display> get_display(), gdk_gl_context_get_display, refreturn) This, and other getters that return objects, should have a const version. You can find similar methods by looking for "constversion". @@ +39,3 @@ + _WRAP_METHOD(Glib::RefPtr<Window> get_window(), gdk_gl_context_get_window, refreturn) + _WRAP_METHOD(Glib::RefPtr<GLContext> get_shared_context(), gdk_gl_context_get_shared_context, refreturn) + _WRAP_METHOD(void get_version(int& major, int& minor), gdk_gl_context_get_version) This should be const. @@ +41,3 @@ + _WRAP_METHOD(void get_version(int& major, int& minor), gdk_gl_context_get_version) + _WRAP_METHOD(void set_required_version(int major, int minor), gdk_gl_context_set_required_version) + _WRAP_METHOD(void get_required_version(int& major, int& minor), gdk_gl_context_get_required_version) This should be const. @@ +44,3 @@ + _WRAP_METHOD(void set_debug_enabled(bool enabled), gdk_gl_context_set_debug_enabled) + _WRAP_METHOD(bool get_debug_enabled(), gdk_gl_context_get_debug_enabled) + _WRAP_METHOD(void set_forward_compatible(bool compatible), gdk_gl_context_set_forward_compatible) This should have and "= true" default value. @@ +45,3 @@ + _WRAP_METHOD(bool get_debug_enabled(), gdk_gl_context_get_debug_enabled) + _WRAP_METHOD(void set_forward_compatible(bool compatible), gdk_gl_context_set_forward_compatible) + _WRAP_METHOD(bool get_forward_compatible(), gdk_gl_context_get_forward_compatible) This should be const. ::: gdk/src/types.hg @@ +59,2 @@ _WRAP_ENUM(ByteOrder, GdkByteOrder) +_WRAP_ENUM(GLError, GdkGLError) Can't this be in the glcontext.hg file? ::: gtk/src/glarea.ccg @@ +1,2 @@ +/* + * Copyright 1998-2002 The gtkmm Development Team The copyright year should be 2015. ::: gtk/src/glarea.hg @@ +1,2 @@ +/* + * Copyright (C) 1998-2002 The gtkmm Development Team The copyright year should be 2015. @@ +26,3 @@ +{ + +/** A widget providing an opengl rendering area This needs a full stop (period) at the end of the sentence. Also, this needs a @newin line. @@ +37,3 @@ + _CTOR_DEFAULT + + _WRAP_METHOD(Glib::RefPtr<const Gdk::GLContext> get_context(), gtk_gl_area_get_context, refreturn) This should have a const version. @@ +41,3 @@ + _WRAP_METHOD(void queue_render(), gtk_gl_area_queue_render) + _WRAP_METHOD(void attach_buffers(), gtk_gl_area_attach_buffers) + _WRAP_METHOD(void set_error(const GError *), gtk_gl_area_set_error) Let's not wrap this until we figure out how to do it without using the GError* C type. @@ +42,3 @@ + _WRAP_METHOD(void attach_buffers(), gtk_gl_area_attach_buffers) + _WRAP_METHOD(void set_error(const GError *), gtk_gl_area_set_error) + _WRAP_METHOD(GError* get_error(), gtk_gl_area_get_error) Let's not wrap this until we figure out how to do it without using the GError* C type. @@ +43,3 @@ + _WRAP_METHOD(void set_error(const GError *), gtk_gl_area_set_error) + _WRAP_METHOD(GError* get_error(), gtk_gl_area_get_error) + _WRAP_METHOD(void set_has_alpha(bool has_alpha), gtk_gl_area_set_has_alpha) This needs an "= true" default value. @@ +44,3 @@ + _WRAP_METHOD(GError* get_error(), gtk_gl_area_get_error) + _WRAP_METHOD(void set_has_alpha(bool has_alpha), gtk_gl_area_set_has_alpha) + _WRAP_METHOD(bool get_has_alpha(), gtk_gl_area_get_has_alpha) This should be const. @@ +45,3 @@ + _WRAP_METHOD(void set_has_alpha(bool has_alpha), gtk_gl_area_set_has_alpha) + _WRAP_METHOD(bool get_has_alpha(), gtk_gl_area_get_has_alpha) + _WRAP_METHOD(void set_has_depth_buffer(bool has_depth_buffer), gtk_gl_area_set_has_depth_buffer) This needs an "= true" default value. @@ +46,3 @@ + _WRAP_METHOD(bool get_has_alpha(), gtk_gl_area_get_has_alpha) + _WRAP_METHOD(void set_has_depth_buffer(bool has_depth_buffer), gtk_gl_area_set_has_depth_buffer) + _WRAP_METHOD(bool get_has_depth_buffer(), gtk_gl_area_get_has_depth_buffer) This should be const. @@ +48,3 @@ + _WRAP_METHOD(bool get_has_depth_buffer(), gtk_gl_area_get_has_depth_buffer) + _WRAP_METHOD(void set_has_stencil_buffer(bool has_stencil_buffer), gtk_gl_area_set_has_stencil_buffer) + _WRAP_METHOD(bool get_has_stencil_buffer(), gtk_gl_area_get_has_stencil_buffer) This should be const. @@ +49,3 @@ + _WRAP_METHOD(void set_has_stencil_buffer(bool has_stencil_buffer), gtk_gl_area_set_has_stencil_buffer) + _WRAP_METHOD(bool get_has_stencil_buffer(), gtk_gl_area_get_has_stencil_buffer) + _WRAP_METHOD(void set_auto_render(bool auto_render), gtk_gl_area_set_auto_render) This needs an "= true" default value. @@ +51,3 @@ + _WRAP_METHOD(void set_auto_render(bool auto_render), gtk_gl_area_set_auto_render) + _WRAP_METHOD(bool get_auto_render(), gtk_gl_area_get_auto_render) + _WRAP_METHOD(void get_required_version(int& major, int& minor), gtk_gl_area_get_required_version) This should be const.
> I've probably got the lifecycle of some of these things wrong - on exit I get > the following warning ... g_signal_connect() connects before the default signal handler. Glib::SignalProxyn<>::connect() by default connects after. You need m_GLArea.signal_unrealize().connect( sigc::mem_fun( this, &Example_GLArea::unrealize ), false ); > If you'd prefer to have a patch against HEAD you can perhaps give me a little > hint what's the best way to set up an environment for that (I'm on Arch L). A patch against HEAD is preferable. If you do that, you should use the resource file in the demo, like gtk+ does. Gtkmm demos did not do that in 3.16. To build from a git clone, use jhbuild: https://wiki.gnome.org/Projects/Jhbuild It takes quite a while to do it the first time. _WRAP_ENUM(GLError, GdkGLError) GdkGLError is not just an ordinary enum. It's a set of error codes. It shall be wrapped with _WRAP_GERROR(GLError, GdkGLError, GDK_GL_ERROR) preferably in glcontext.hg. _WRAP_METHOD(void make_current() const, gdk_gl_context_make_current, constversion) You can't use the constversion parameter if there isn't a corresponding non-const version. The code compiles, but it won't run correctly. Is make_current() a method that should be const? The name suggests that it may change the GLContext object.
Q+A: ---- > Is the demo based on one in GTK+ in C? Yep, it's a transliteration of the C example put into a C++ class and should (hopefully) function identically. > Why do we need to explicitly depend on epoxy? Isn't that just something that GTK+ uses? You're right the gtkmm library indeed doesn't need to depend on epoxy. There aren't any calls to GL symbols so I've removed it from the library deps. However, the gtkmm-demo example needs to compile against something with GL headers and link against something that resolves the symbols. Due to the wildly different ways in which GL can be included/linked against, the solution that Gtk have chosen (using epoxy) seems like a fair one. For now I've made the dependency on epoxy purely exist for the gtkmm-demo - there's a separate section in the configure.ac and a custom rule for the binary. If you'd prefer something else just let me know. > Lets not wrap the GError * methods set_error and get_error until.... I've left them in for the moment - simply because the example_glarea uses them to detect if there were errors during the make_current call. I'm unsure what is preferable here. Remove them and remove (comment out) the error checking perhaps? > lifecycle issues (::connect() by default connects after default s.h.) Marvellous, that does indeed resolve it. Additional changes: ------------------- * Removed use of g_malloc and g_free, instead fetch the GL shader/linking error logs into the buffer of a preallocated string. * g_warning -> cerr * Add the shaders into a "make dist" Outstanding issues: ------------------- * Mentioned GError* issues * Patch against HEAD / use resource file I'll look into jhbuild, but it might be a little while for me to get up to speed.
Created attachment 307427 [details] [review] Adds Gtk::GLArea and a demo
1. If Gdk::GLContext and Gtk::GLArea will be added to gtkmm soon, they will be @newin{3,18} It's too late to add anything to gtkmm 3.16. 2. 'make check' after configuration with --enable-warnings=fatal: gtk-demo/example_glarea.cc:152:65: error: unused parameter ‘context’ [-Werror=unused-parameter] bool Example_GLArea::render(const Glib::RefPtr<Gdk::GLContext>& context) cc1plus: all warnings being treated as errors Change to bool Example_GLArea::render(const Glib::RefPtr<Gdk::GLContext>& /* context */) or bool Example_GLArea::render(const Glib::RefPtr<Gdk::GLContext>&) 3. Some set-functions with a bool parameter have no default value: _WRAP_METHOD(void set_debug_enabled(bool enabled), gdk_gl_context_set_debug_enabled) _WRAP_METHOD(void set_has_stencil_buffer(bool has_stencil_buffer), gtk_gl_area_set_has_stencil_buffer) 4. These includes are unnecessary in glcontext.hg: #include <glibmm/value.h> #include <gdk/gdk.h> To be on the safe side, you could add #include <gdk/gdk.h> to glcontext.ccg. GLContext compiles without it, but only because gdkmm/window.h includes gdk/gdk.h. 5. get/set_error(): set_error() is easy: _CONVERSION(`const Glib::Error&',`const GError*',__FR2P) _WRAP_METHOD(void set_error(const Glib::Error& error), gtk_gl_area_set_error) void unset_error(); void GLArea::unset_error() { gtk_gl_area_set_error(gobj(), nullptr); } get_error() is also easy, if we accept to get a dynamically allocated Glib::Error instance: Glib::Error* get_error() const; _IGNORE(gtk_gl_area_get_error) Glib::Error* GLArea::get_error() const { GError* gerror = gtk_gl_area_get_error(const_cast<GtkGLArea*>(gobj())); return gerror ? new Glib::Error(gerror, true) : 0; } That's not very good. A slightly better alternative, but still not very good: bool has_error() const; void throw_if_error() const; _IGNORE(gtk_gl_area_get_error) bool GLArea::has_error() const { return gtk_gl_area_get_error(const_cast<GtkGLArea*>(gobj())); } void GLArea::throw_if_error() const { GError* gerror = gtk_gl_area_get_error(const_cast<GtkGLArea*>(gobj())); if (gerror) ::Glib::Error::throw_exception(g_error_copy(gerror)); }
Created attachment 307514 [details] [review] Add Gtk::GLArea and a demo Should now be a patch against HEAD. Includes a first attempt at resource usage and hopefully resolves some of the mentioned issues. Things I'd appreciate some thought on: * I went for has_error() and get_error(Glib::Error& out_error) - you may dislike this and I'll change it - consider it just testing the water * Using @newin marks the object as gtkmm3.18, but all of the member functions and properties are still marked gtkmm3.16 - presumably it's pulling this from gtk+ - I'm guessing it will need individual annotations to override those Thank you for your time on these reviews, by the way.
Oops, have been playing with it and have just noticed the lack of const on the has_error() and get_error() members. I'll add it the list of changes for next time around.
Created attachment 307582 [details] [review] Add Gtk::GLArea and example to gtkmm-demo Update that resolves the missing const issues on has_error and get_error. In addition, I've attempted to put in place the necessary documentation for the bits that will need the @newin{3,18} annotations, but they come with some little issues: * I can't work out how to override the "upstream" documentation bits on properties, signals and the GLError - the custom docs I added appear but the underlying ones do too * For properties there's a second issue - I get the "double docs" above for the regular property proxy - but for the PropertyProxy_ReadOnly they don't get the overridden docs at all (e.g. see GLArea::property_has_alpha const version) (Also, I'm pretty new to this whole docs thing, so feel free to tell me scrap that, do it this way)
Created attachment 307611 [details] [review] Adds Gtk::GLArea and a demo Small changes to use C++11 auto and universal initialisation. Unsure if you'll like the formatting. Removed some unnecessary code in the model/view/projection calc. Added error fetch + logging in the (un)realize() and render functions. Still outstanding issues: * Previously mentioned documentation override issues (double docs, missing on const property proxies)
Created attachment 307617 [details] [review] Adds Gtk::GLArea and a demo I discovered the use of g*k_docs_override.xml. Moved the necessary @newin{3,18} documentation into the docs_override where possible. This patch resolves (I believe) outstanding issues and is perhaps a good version to review. Now I'm off to the pub for a pint .-)
> I discovered the use of g*k_docs_override.xml. What a pity that you did! If all you want to change in the documentation is the @newin, there's a better way, provided you use a new enough version of gmmproc (which is part of glibmm). If you have pulled glibmm from git master, it's certainly new enough. You need glibmm 2.45.2 or newer. Use the optional newin parameter in _WRAP_METHOD, _WRAP_SIGNAL, _WRAP_PROPERTY, _WRAP_GERROR. Like so: _WRAP_METHOD(bool get_has_alpha() const, gtk_gl_area_get_has_alpha, newin "3,18") _WRAP_SIGNAL(void resize(int width, int height), "resize", newin "3,18") _WRAP_PROPERTY("auto-render", bool, newin "3,18") _WRAP_GERROR(GLError, GdkGLError, GDK_GL_ERROR, newin "3,18") Wrapping gtk_gl_area_get_error() in a good way is more complicated than it may seem at first. The GError struct is used for all types of errors. GError contains GQuark domain. In glibmm and gtkmm each domain is wrapped in a separate subclass of Glib::Error, e.g. Gdk::GLError if domain == GDK_GL_ERROR. We would like a method in Gtk::GLArea that can get an instance of the correct subclass of Glib::Error. Glib::Error::throw_exception() creates an instance of the correct type, but it also throws that instance.
Created attachment 307757 [details] [review] Adds Gtk::GLArea and a demo Thanks Kjell - I figured there had to be a better way than replicating the documentation itself in the source file - but I fell on doc_overrides rather than checking in mm_ bits. C'est la vie. Here's a patch that uses the extra newin parameter for the documentation where appropriate and removes the custom doc_overrides bits. In addition it has a regenerated gdk_docs.xml as upstream has a bug fix for one of the gdkglcontext property docs. Still unresolved: ----------------- Interface + mechanism for (set/get)_error. If it's ok I'll move that discussion to the mailing list rather than add lots of text here.
Created attachment 307823 [details] [review] Adds Gtk::GLArea and a demo Remove get_error and use throw_if_error on Gtk::GLArea. Update demo to reflect slight difference.
Now it's almost finished. In doxygen comments, references to methods are usually written "method_name()", instead of "@ref method_name". Doxygen understands that it's a reference, and the trailing parentheses show the reader that it's a method or function. A more serious point: In throw_if_error() you must change Glib::Error::throw_exception(error); to Glib::Error::throw_exception(g_error_copy(error)); Glib::Error::throw_exception() takes ownership of the GError object, but gtk_gl_area_get_error() does not create a new object for the caller, contrary to functions with a trailing GError** parameter, which is what throw_exception() is made for. If you want to test it, temporarily add m_GLArea.set_error(Gdk::GLError(Gdk::GLError::NOT_AVAILABLE, "Testing")); before one of the try-blocks in the demo. I don't know if the check for existence of epoxy in configure.ac is ok. I leave the decision to Murray. Is it possible (without too much trouble) to make it conditional, such that it's possible to run 'make' and 'make install' without epoxy?
> I don't know if the check for existence of epoxy in configure.ac is ok. I > leave > the decision to Murray. Is it possible (without too much trouble) to make it > conditional, such that it's possible to run 'make' and 'make install' without > epoxy? Doesn't the GTK+ build already depend on epoxy?
Created attachment 307917 [details] [review] Adds Gtk::GLArea and a demo Updated patch to fix missing g_error_copy and remove use of @ref in documentation. Sorry for missing the g_error_copy - you did point it out when you suggested it. I see the issue by setting an error as you suggested. Makes sense. RE: Configure time dependency on epoxy Looks like gtk+ has a hard private dependency on epoxy (https://git.gnome.org/browse/gtk+/tree/configure.ac#n1324) - but it's not exposed via the pkgconfig file. Which makes sense - some of the functionality in glarea relies on gl symbols - I spotted that the default resize() handler for glarea by default calls glViewport to adjust the viewport (https://git.gnome.org/browse/gtk+/tree/gtk/gtkglarea.c#n360) - which requires GL headers and symbols. One possibility - what could be done would be to introduce a "--enable-demos" style switch to configure which would then introduce the dependency + checks and build the demos. Just a thought.
The epoxy dependency is fine (and correct) as it is in your patch. Epoxy is needed to build GTK+ so it's fine to need it to build gtkmm from source. I'll push this soon.
Pushed to master. Thanks for this really useful contribution and thanks for being so responsive to all our nitpicking.