From c28f36e04a5d92379360195c934c8ec52dbacac6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Sat, 27 Jul 2019 19:56:55 +0200 Subject: [PATCH] python: move refcounting in GL types also to holders. Much cleaner now, yay. --- src/Magnum/CMakeLists.txt | 6 ++- src/Magnum/GL/CMakeLists.txt | 30 +++++++++++++ .../magnum/PyGL.h => Magnum/GL/Python.h} | 40 ++++++++++------- src/python/magnum/gl.cpp | 45 +++++++++++++------ src/python/magnum/meshtools.cpp | 5 +-- 5 files changed, 92 insertions(+), 34 deletions(-) create mode 100644 src/Magnum/GL/CMakeLists.txt rename src/{python/magnum/PyGL.h => Magnum/GL/Python.h} (52%) diff --git a/src/Magnum/CMakeLists.txt b/src/Magnum/CMakeLists.txt index d166fc6..c721189 100644 --- a/src/Magnum/CMakeLists.txt +++ b/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) diff --git a/src/Magnum/GL/CMakeLists.txt b/src/Magnum/GL/CMakeLists.txt new file mode 100644 index 0000000..14ab648 --- /dev/null +++ b/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š +# +# 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() diff --git a/src/python/magnum/PyGL.h b/src/Magnum/GL/Python.h similarity index 52% rename from src/python/magnum/PyGL.h rename to src/Magnum/GL/Python.h index 49d8e25..1271f49 100644 --- a/src/python/magnum/PyGL.h +++ b/src/Magnum/GL/Python.h @@ -1,5 +1,5 @@ -#ifndef magnum_PyGL_h -#define magnum_PyGL_h +#ifndef Magnum_GL_Python_h +#define Magnum_GL_Python_h /* This file is part of Magnum. @@ -25,29 +25,37 @@ DEALINGS IN THE SOFTWARE. */ -#include +#include /* :( */ #include -#include -#include +#include +#include + +#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 struct PyMeshHolder: std::unique_ptr { + static_assert(std::is_same::value, "mesh holder has to hold a mesh"); -struct PyMesh: GL::Mesh { - 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)) {} + explicit PyMeshHolder(T* object): std::unique_ptr{object} {} - std::vector buffers; + std::vector buffers; }; -struct PyFramebuffer: GL::Framebuffer { - explicit PyFramebuffer(const Range2Di& viewport): GL::Framebuffer{viewport} {} +template struct PyFramebufferHolder: std::unique_ptr::value>> { + static_assert(std::is_same::value, "framebuffer holder has to hold a framebuffer"); - std::vector attached; + explicit PyFramebufferHolder(T* object): std::unique_ptr::value>>{object} {} + + std::vector attachments; }; -} +}} + +PYBIND11_DECLARE_HOLDER_TYPE(T, Magnum::GL::PyMeshHolder) +PYBIND11_DECLARE_HOLDER_TYPE(T, Magnum::GL::PyFramebufferHolder) #endif diff --git a/src/python/magnum/gl.cpp b/src/python/magnum/gl.cpp index a46b5df..c33eb5b 100644 --- a/src/python/magnum/gl.cpp +++ b/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 struct NonDefaultFramebufferHolder: std::unique_ptr::value>> { + explicit NonDefaultFramebufferHolder(T* object): std::unique_ptr::value>>{object} {} +}; + +}} + +PYBIND11_DECLARE_HOLDER_TYPE(T, magnum::NonDefaultFramebufferHolder) namespace magnum { @@ -290,7 +303,7 @@ void gl(py::module& m) { .value("STENCIL", GL::FramebufferClear::Stencil); corrade::enumOperators(framebufferClear); - PyNonDestructibleClass abstractFramebuffer{m, + py::class_> abstractFramebuffer{m, "AbstractFramebuffer", "Base for default and named framebuffers"}; abstractFramebuffer @@ -301,10 +314,10 @@ void gl(py::module& m) { .def("read", static_cast(&GL::AbstractFramebuffer::read), "Read block of pixels from the framebuffer to an image view"); - PyNonDestructibleClass defaultFramebuffer{m, + py::class_> defaultFramebuffer{m, "DefaultFramebuffer", "Default framebuffer"}; - PyNonDestructibleClass framebuffer{m, + py::class_> framebuffer{m, "Framebuffer", "Framebuffer"}; py::class_{framebuffer, "ColorAttachment", "Color attachment"} @@ -324,15 +337,17 @@ void gl(py::module& m) { framebuffer .def(py::init(), "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(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(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_{m, "Mesh", "Mesh"} + py::class_>{m, "Mesh", "Mesh"} .def(py::init(), "Constructor", py::arg("primitive") = GL::MeshPrimitive::Triangles) .def(py::init(), "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(primitive)) self.setPrimitive(py::cast(primitive)); else if(py::isinstance(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(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(self).buffers; + }, "Buffer objects referenced by the mesh"); /* Renderer */ { diff --git a/src/python/magnum/meshtools.cpp b/src/python/magnum/meshtools.cpp index 9fc38de..0e450a5 100644 --- a/src/python/magnum/meshtools.cpp +++ b/src/python/magnum/meshtools.cpp @@ -30,7 +30,6 @@ #include #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"); }