python: move refcounting in GL types also to holders.

Much cleaner now, yay.

src/Magnum/CMakeLists.txt

@@ -29,7 +29,11 @@ if(WITH_PYTHON)
     install(FILES Python.h DESTINATION ${MAGNUM_INCLUDE_INSTALL_DIR})
 endif()
 
-find_package(Magnum COMPONENTS SceneGraph)
+find_package(Magnum COMPONENTS GL SceneGraph)
+
+if(Magnum_GL_FOUND)
+    add_subdirectory(GL)
+endif()
 
 if(Magnum_SceneGraph_FOUND)
     add_subdirectory(SceneGraph)

src/Magnum/GL/CMakeLists.txt

@@ -0,0 +1,30 @@
+#
+#   This file is part of Magnum.
+#
+#   Copyright © 2010, 2011, 2012, 2013, 2014, 2015, 2016, 2017, 2018, 2019
+#             Vladimír Vondruš <mosra@centrum.cz>
+#
+#   Permission is hereby granted, free of charge, to any person obtaining a
+#   copy of this software and associated documentation files (the "Software"),
+#   to deal in the Software without restriction, including without limitation
+#   the rights to use, copy, modify, merge, publish, distribute, sublicense,
+#   and/or sell copies of the Software, and to permit persons to whom the
+#   Software is furnished to do so, subject to the following conditions:
+#
+#   The above copyright notice and this permission notice shall be included
+#   in all copies or substantial portions of the Software.
+#
+#   THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+#   IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+#   FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+#   THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+#   LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+#   FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+#   DEALINGS IN THE SOFTWARE.
+#
+
+if(WITH_PYTHON)
+    add_custom_target(MagnumGLPython SOURCES Python.h)
+    set_target_properties(MagnumGLPython PROPERTIES FOLDER "Magnum/Python")
+    install(FILES Python.h DESTINATION ${MAGNUM_INCLUDE_INSTALL_DIR}/GL)
+endif()

src/Magnum/GL/Python.h

@@ -1,5 +1,5 @@
-#ifndef magnum_PyGL_h
+#ifndef Magnum_GL_Python_h
-#define magnum_PyGL_h
+#define Magnum_GL_Python_h
 /*
     This file is part of Magnum.
 
@@ -25,29 +25,37 @@
     DEALINGS IN THE SOFTWARE.
 */
 
-#include <pybind11/pybind11.h>
+#include <memory> /* :( */
 #include <vector>
-#include <Magnum/GL/Mesh.h>
+#include <pybind11/pybind11.h>
-#include <Magnum/GL/Framebuffer.h>
+#include <Magnum/GL/GL.h>
+
+#include "Magnum/Python.h"
 
-#include "magnum/bootstrap.h"
+namespace Magnum { namespace GL {
 
-namespace magnum {
+/* Stores additional stuff needed for proper refcounting of buffers owned by
+   a mesh. For some reason it *has to be* templated, otherwise
+   PYBIND11_DECLARE_HOLDER_TYPE doesn't work. Ugh. */
+template<class T> struct PyMeshHolder: std::unique_ptr<T> {
+    static_assert(std::is_same<T, GL::Mesh>::value, "mesh holder has to hold a mesh");
 
-struct PyMesh: GL::Mesh {
+    explicit PyMeshHolder(T* object): std::unique_ptr<T>{object} {}
-    explicit PyMesh(GL::MeshPrimitive primitive): GL::Mesh(primitive) {}
-    explicit PyMesh(MeshPrimitive primitive): GL::Mesh(primitive) {}
-    explicit PyMesh(GL::Mesh&& mesh): GL::Mesh(std::move(mesh)) {}
 
-    std::vector<py::object> buffers;
+    std::vector<pybind11::object> buffers;
 };
 
-struct PyFramebuffer: GL::Framebuffer {
+template<class T> struct PyFramebufferHolder: std::unique_ptr<T, PyNonDestructibleBaseDeleter<T, std::is_destructible<T>::value>> {
-    explicit PyFramebuffer(const Range2Di& viewport): GL::Framebuffer{viewport} {}
+    static_assert(std::is_same<T, GL::Framebuffer>::value, "framebuffer holder has to hold a framebuffer");
 
-    std::vector<py::object> attached;
+    explicit PyFramebufferHolder(T* object): std::unique_ptr<T, PyNonDestructibleBaseDeleter<T, std::is_destructible<T>::value>>{object} {}
+
+    std::vector<pybind11::object> attachments;
 };
 
-}
+}}
+
+PYBIND11_DECLARE_HOLDER_TYPE(T, Magnum::GL::PyMeshHolder<T>)
+PYBIND11_DECLARE_HOLDER_TYPE(T, Magnum::GL::PyFramebufferHolder<T>)
 
 #endif

src/python/magnum/gl.cpp

@@ -41,10 +41,23 @@
 
 #include "Corrade/Python.h"
 #include "Magnum/Python.h"
+#include "Magnum/GL/Python.h"
 
 #include "corrade/EnumOperators.h"
 #include "magnum/bootstrap.h"
-#include "magnum/PyGL.h"
+
+namespace magnum { namespace {
+
+/* Otherwise pybind yells that `generic_type: type "Framebuffer" has a
+   non-default holder type while its base "Magnum::GL::AbstractFramebuffer"
+   does not` -- we're using PyFramebufferHolder for it */
+template<class T> struct NonDefaultFramebufferHolder: std::unique_ptr<T, PyNonDestructibleBaseDeleter<T, std::is_destructible<T>::value>> {
+    explicit NonDefaultFramebufferHolder(T* object): std::unique_ptr<T, PyNonDestructibleBaseDeleter<T, std::is_destructible<T>::value>>{object} {}
+};
+
+}}
+
+PYBIND11_DECLARE_HOLDER_TYPE(T, magnum::NonDefaultFramebufferHolder<T>)
 
 namespace magnum {
 
@@ -290,7 +303,7 @@ void gl(py::module& m) {
         .value("STENCIL", GL::FramebufferClear::Stencil);
     corrade::enumOperators(framebufferClear);
 
-    PyNonDestructibleClass<GL::AbstractFramebuffer> abstractFramebuffer{m,
+    py::class_<GL::AbstractFramebuffer, NonDefaultFramebufferHolder<GL::AbstractFramebuffer>> abstractFramebuffer{m,
         "AbstractFramebuffer", "Base for default and named framebuffers"};
 
     abstractFramebuffer
@@ -301,10 +314,10 @@ void gl(py::module& m) {
         .def("read", static_cast<void(GL::AbstractFramebuffer::*)(const Range2Di&, const MutableImageView2D&)>(&GL::AbstractFramebuffer::read),
             "Read block of pixels from the framebuffer to an image view");
 
-    PyNonDestructibleClass<GL::DefaultFramebuffer, GL::AbstractFramebuffer> defaultFramebuffer{m,
+    py::class_<GL::DefaultFramebuffer, GL::AbstractFramebuffer, NonDefaultFramebufferHolder<GL::DefaultFramebuffer>> defaultFramebuffer{m,
         "DefaultFramebuffer", "Default framebuffer"};
 
-    PyNonDestructibleClass<PyFramebuffer, GL::AbstractFramebuffer> framebuffer{m,
+    py::class_<GL::Framebuffer, GL::AbstractFramebuffer, GL::PyFramebufferHolder<GL::Framebuffer>> framebuffer{m,
         "Framebuffer", "Framebuffer"};
 
     py::class_<GL::Framebuffer::ColorAttachment>{framebuffer, "ColorAttachment", "Color attachment"}
@@ -324,15 +337,17 @@ void gl(py::module& m) {
     framebuffer
         .def(py::init<const Range2Di&>(), "Constructor")
         .def_property_readonly("id", &GL::Framebuffer::id, "OpenGL framebuffer ID")
-        .def("attach_renderbuffer", [](PyFramebuffer& self, GL::Framebuffer::BufferAttachment attachment, GL::Renderbuffer& renderbuffer) {
+        .def("attach_renderbuffer", [](GL::Framebuffer& self, GL::Framebuffer::BufferAttachment attachment, GL::Renderbuffer& renderbuffer) {
             self.attachRenderbuffer(attachment, renderbuffer);
 
             /* Keep a reference to the renderbuffer to avoid it being deleted
                before the framebuffer */
-            self.attached.emplace_back(pyObjectFromInstance(renderbuffer));
+            pyObjectHolderFor<GL::PyFramebufferHolder>(self).attachments.emplace_back(pyObjectFromInstance(renderbuffer));
         }, "Attach renderbuffer to given buffer")
 
-        .def_readonly("attached", &PyFramebuffer::attached, "Renderbuffer and texture objects referenced by the framebuffer");
+        .def_property_readonly("attached", [](GL::Framebuffer& self) {
+            return pyObjectHolderFor<GL::PyFramebufferHolder>(self).attachments;
+        }, "Renderbuffer and texture objects referenced by the framebuffer");
 
     /* An equivalent to this would be
         m.attr("default_framebuffer") = &GL::defaultFramebuffer;
@@ -362,12 +377,12 @@ void gl(py::module& m) {
         #endif
         ;
 
-    py::class_<PyMesh>{m, "Mesh", "Mesh"}
+    py::class_<GL::Mesh, GL::PyMeshHolder<GL::Mesh>>{m, "Mesh", "Mesh"}
         .def(py::init<GL::MeshPrimitive>(), "Constructor", py::arg("primitive") = GL::MeshPrimitive::Triangles)
         .def(py::init<MeshPrimitive>(), "Constructor")
         .def_property_readonly("id", &GL::Mesh::id, "OpenGL vertex array ID")
         .def_property("primitive", &GL::Mesh::primitive,
-            [](PyMesh& self, py::object primitive) {
+            [](GL::Mesh& self, py::object primitive) {
                 if(py::isinstance<MeshPrimitive>(primitive))
                     self.setPrimitive(py::cast<MeshPrimitive>(primitive));
                 else if(py::isinstance<GL::MeshPrimitive>(primitive))
@@ -376,25 +391,27 @@ void gl(py::module& m) {
             }, "Primitive type")
         /* Have to use a lambda because it returns GL::Mesh which is not
            tracked (unlike PyMesh) */
-        .def_property("count", &GL::Mesh::count, [](PyMesh& self, UnsignedInt count) {
+        .def_property("count", &GL::Mesh::count, [](GL::Mesh& self, UnsignedInt count) {
             self.setCount(count);
         }, "Vertex/index count")
 
         /* Using lambdas to avoid method chaining getting into signatures */
 
-        .def("add_vertex_buffer", [](PyMesh& self, GL::Buffer& buffer, GLintptr offset, GLsizei stride, const GL::DynamicAttribute& attribute) {
+        .def("add_vertex_buffer", [](GL::Mesh& self, GL::Buffer& buffer, GLintptr offset, GLsizei stride, const GL::DynamicAttribute& attribute) {
             self.addVertexBuffer(buffer, offset, stride, attribute);
 
             /* Keep a reference to the buffer to avoid it being deleted before
                the mesh */
-            self.buffers.emplace_back(pyObjectFromInstance(buffer));
+            pyObjectHolderFor<GL::PyMeshHolder>(self).buffers.emplace_back(pyObjectFromInstance(buffer));
         }, "Add vertex buffer", py::arg("buffer"), py::arg("offset"), py::arg("stride"), py::arg("attribute"))
-        .def("draw", [](PyMesh& self, GL::AbstractShaderProgram& shader) {
+        .def("draw", [](GL::Mesh& self, GL::AbstractShaderProgram& shader) {
             self.draw(shader);
         }, "Draw the mesh")
         /** @todo more */
 
-        .def_readonly("buffers", &PyMesh::buffers, "Buffer objects referenced by the mesh");
+        .def_property_readonly("buffers", [](GL::Mesh& self) {
+            return pyObjectHolderFor<GL::PyMeshHolder>(self).buffers;
+        }, "Buffer objects referenced by the mesh");
 
     /* Renderer */
     {

src/python/magnum/meshtools.cpp

@@ -30,7 +30,6 @@
 #include <Magnum/Trade/MeshData3D.h>
 
 #include "magnum/bootstrap.h"
-#include "magnum/PyGL.h"
 
 namespace magnum {
 
@@ -46,10 +45,10 @@ void meshtools(py::module& m) {
 
     m
         .def("compile", [](const Trade::MeshData2D& data) {
-            return PyMesh{MeshTools::compile(data)};
+            return MeshTools::compile(data);
         }, "Compile 2D mesh data")
         .def("compile", [](const Trade::MeshData3D& data) {
-            return PyMesh{MeshTools::compile(data)};
+            return MeshTools::compile(data);
         }, "Compile 3D mesh data");
 }