From 843160f58aea65cc6d17ba81b25a890690eca17a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Mon, 16 Sep 2019 18:17:59 +0200 Subject: [PATCH] python: throw instead of returning a falsy value in shader APIs. --- doc/python/magnum.gl.rst | 9 +++++++ src/python/magnum/gl.cpp | 34 ++++++++++++++++++++++--- src/python/magnum/test/test_gl_gl.py | 37 +++++++++++++++++++++++++--- 3 files changed, 72 insertions(+), 8 deletions(-) diff --git a/doc/python/magnum.gl.rst b/doc/python/magnum.gl.rst index 08db132..1acedbb 100644 --- a/doc/python/magnum.gl.rst +++ b/doc/python/magnum.gl.rst @@ -38,6 +38,15 @@ to have class members initialized before a GL context is present, but in Python there's no such limitation so these don't make sense. +.. py:function:: magnum.gl.AbstractShaderProgram.link + :raise RuntimeError: If linking fails +.. py:function:: magnum.gl.AbstractShaderProgram.uniform_location + :raise ValueError: If there's no uniform of that name +.. py:function:: magnum.gl.AbstractShaderProgram.uniform_block_index + :raise ValueError: If there's no uniform block of that name +.. py:function:: magnum.gl.Shader.compile + :raise RuntimeError: If compilation fails + .. py:class:: magnum.gl.Mesh TODO: remove this once m.css stops ignoring the first caption on a page diff --git a/src/python/magnum/gl.cpp b/src/python/magnum/gl.cpp index ad14eff..be641af 100644 --- a/src/python/magnum/gl.cpp +++ b/src/python/magnum/gl.cpp @@ -308,7 +308,14 @@ void gl(py::module& m) { .def("add_file", [](GL::Shader& self, const std::string& filename) { self.addFile(filename); }, "Add shader source file") - .def("compile", static_cast(&GL::Shader::compile), "Compile shader"); + .def("compile", [](GL::Shader& self) { + /** @todo log redirection -- but we'd need assertions to not be + part of that so when it dies, the user can still see why */ + if(!self.compile()) { + PyErr_SetString(PyExc_RuntimeError, "compilation failed"); + throw py::error_already_set{}; + } + }, "Compile shader"); } /* Abstract shader program */ @@ -363,14 +370,33 @@ void gl(py::module& m) { /* Somehow the overload static_casts don't work and it complains it can't bind a protected function, have to use lambdas */ .def("link", [](GL::AbstractShaderProgram& self) { - return static_cast(self).link(); + /** @todo log redirection -- but we'd need assertions to not be + part of that so when it dies, the user can still see why */ + if(!static_cast(self).link()) { + PyErr_SetString(PyExc_RuntimeError, "linking failed"); + throw py::error_already_set{}; + } }, "Link the shader") .def("uniform_location", [](GL::AbstractShaderProgram& self, const std::string& name) { - return static_cast(self).uniformLocation(name); + /** @todo log redirection -- but we'd need assertions to not be + part of that so when it dies, the user can still see why */ + const Int location = static_cast(self).uniformLocation(name); + if(location == -1) { + PyErr_Format(PyExc_ValueError, "location of uniform '%s' cannot be retrieved", name.data()); + throw py::error_already_set{}; + } + return location; }, "Get uniform location") #ifndef MAGNUM_TARGET_GLES2 .def("uniform_block_index", [](GL::AbstractShaderProgram& self, const std::string& name) { - return static_cast(self).uniformBlockIndex(name); + /** @todo log redirection -- but we'd need assertions to not be + part of that so when it dies, the user can still see why */ + const UnsignedInt index = static_cast(self).uniformBlockIndex(name); + if(index == 0xffffffffu) { + PyErr_Format(PyExc_ValueError, "index of uniform block '%s' cannot be retrieved", name.data()); + throw py::error_already_set{}; + } + return index; }, "Get uniform block index") #endif .def("set_uniform", setUniform, "Set uniform value") diff --git a/src/python/magnum/test/test_gl_gl.py b/src/python/magnum/test/test_gl_gl.py index 52084ba..3b2a260 100644 --- a/src/python/magnum/test/test_gl_gl.py +++ b/src/python/magnum/test/test_gl_gl.py @@ -64,7 +64,7 @@ void main() { } """.strip()) - self.assertTrue(vert.compile()) + vert.compile() a.attach_shader(vert) if magnum.TARGET_GLES2: @@ -87,15 +87,33 @@ void main() { color = vec4(0.0); } """.strip()) - self.assertTrue(frag.compile()) + frag.compile() a.attach_shader(frag) a.bind_attribute_location(0, "position") - self.assertTrue(a.link()) + a.link() location = a.uniform_location("transformationProjectionMatrix") self.assertGreaterEqual(location, 0) a.set_uniform(location, Matrix4()) + def test_link_fail(self): + a = gl.AbstractShaderProgram() + # Link of an empty shader will always fail + with self.assertRaisesRegex(RuntimeError, "linking failed"): + a.link() + + def test_uniform_fail(self): + a = gl.AbstractShaderProgram() + with self.assertRaisesRegex(ValueError, "location of uniform 'nonexistent' cannot be retrieved"): + a.uniform_location("nonexistent") + # Asking for uniform on a non-linked program is an error, eat it so the + # setup/teardown checks don't complain + self.assertEqual(gl.Renderer.error, gl.Renderer.Error.INVALID_OPERATION) + + if not magnum.TARGET_GLES2: + with self.assertRaisesRegex(ValueError, "index of uniform block 'nonexistent' cannot be retrieved"): + a.uniform_block_index("nonexistent") + class Buffer(GLTestCase): def test_init(self): a = gl.Buffer() @@ -228,7 +246,18 @@ class Shader(GLTestCase): gl_Position = vec4(0.0); } """) - self.assertTrue(a.compile()) + a.compile() + + def test_compile_fail(self): + if magnum.TARGET_GLES2: + a = gl.Shader(gl.Version.GLES200, gl.Shader.Type.VERTEX) + elif magnum.TARGET_GLES: + a = gl.Shader(gl.Version.GLES300, gl.Shader.Type.VERTEX) + else: + a = gl.Shader(gl.Version.GL300, gl.Shader.Type.VERTEX) + a.add_source("error!!!!") + with self.assertRaisesRegex(RuntimeError, "compilation failed"): + a.compile() class Texture(GLTestCase): def test_minification_filter(self):