As part of my ongoing development of a 3D game engine based on PyOpenGL, I am trying to do some refactoring to make working with textures and framebuffers as painless as possible. I feel this refactoring is needed so that my texture class can handle all of these (and more!) use-cases seamlessly:Loading in image data from a fileLoading in generated pixel dataApplying a texture to a sprite/model in my sceneAccept data in a variety of color formats.Accept not only color data, but also depth and stencil data.Rendering the framebuffer to a texture for post-processing effects/passes.I have created a simple demo that illustrates my code. There are three files: texture.py for my texture class (the main concern), framebuffer.py for handling creating framebuffers besides the default screen buffer, and main.py, which tests these two classes without relying on the rest of my engine (just renders a full-screen quad to an off-screen texture). Here is that minimal code that works to cover these uses:texture.pyimport numpy as npfrom OpenGL.GL import *from OpenGL.GL.ARB.texture_float import *from OpenGL.GL.EXT.framebuffer_object import *from PIL import Image_valid_formats = {    RGB: GL_RGB,     RGBA: GL_RGBA,     FLOAT: GL_FLOAT,    RGB_FLOAT: GL_RGB32F_ARB,    RGBA_FLOAT: GL_RGBA32F_ARB,}_valid_filters = [GL_NEAREST, GL_LINEAR]class Texture(object):    def __init__(self):        self._id = glGenTextures(1)    def get_data(self):        self.bind()        raw_data = glGetTexImage(GL_TEXTURE_2D, 0, GL_RGBA, GL_UNSIGNED_BYTE)        image = Image.frombytes(RGBA, (self.width, self.height), raw_data)        image = image.transpose(Image.FLIP_TOP_BOTTOM)        data = np.array(image)        self.unbind()        return data    def get_id(self):        return self._id    def load_from_url(self, asset_url, linear_filter=True, texture_unit=0):        asset = Image.open(asset_url)        data = np.array(asset, dtype=np.uint8).flatten()        width, height = asset.size        item_size = len(data)/(width * height)        image_format = RGBA if item_size == 4 else RGB        self.load_from_data(width, height, image_format, data, linear_filter, texture_unit)    def load_from_data(self, width, height, image_format, data=None, linear_filter=True, texture_unit=0):        self.width = width        self.height = height        self.image_format = image_format        self.linear_filter = linear_filter        self.texture_unit = texture_unit        gl_format = _valid_formats[self.image_format]        self.bind()        glTexParameterf(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, _valid_filters[self.linear_filter])        glTexParameterf(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, _valid_filters[self.linear_filter])        glTexImage2D(GL_TEXTURE_2D, 0, gl_format, self.width, self.height, 0, gl_format, GL_UNSIGNED_BYTE, data)        self.unbind()    def bind(self):        glActiveTexture(GL_TEXTURE1 + self.texture_unit)        glBindTexture(GL_TEXTURE_2D, self._id)    def unbind(self):        glActiveTexture(GL_TEXTURE0 + self.texture_unit)        glBindTexture(GL_TEXTURE_2D, 0)        glActiveTexture(GL_TEXTURE0)framebuffer.pyimport numpy as npfrom OpenGL.GL import *from OpenGL.GL.ARB.texture_float import *from OpenGL.GL.EXT.framebuffer_object import *from PIL import Imagefrom pyorama.graphics.texture import Texture_valid_attachments = {    COLOR_0: GL_COLOR_ATTACHMENT0,     COLOR_1: GL_COLOR_ATTACHMENT1,     COLOR_2: GL_COLOR_ATTACHMENT2,     COLOR_3: GL_COLOR_ATTACHMENT3,     COLOR_4: GL_COLOR_ATTACHMENT4,     COLOR_5: GL_COLOR_ATTACHMENT5,     COLOR_6: GL_COLOR_ATTACHMENT6,     COLOR_7: GL_COLOR_ATTACHMENT7,     COLOR_8: GL_COLOR_ATTACHMENT8,     COLOR_9: GL_COLOR_ATTACHMENT9,     COLOR_10: GL_COLOR_ATTACHMENT10,     COLOR_11: GL_COLOR_ATTACHMENT11,     COLOR_12: GL_COLOR_ATTACHMENT12,     COLOR_13: GL_COLOR_ATTACHMENT13,     COLOR_14: GL_COLOR_ATTACHMENT14,     COLOR_15: GL_COLOR_ATTACHMENT15,    DEPTH: GL_DEPTH_ATTACHMENT,    STENCIL: GL_STENCIL_ATTACHMENT,}class Framebuffer(object):    def __init__(self):        self._id = glGenFramebuffersEXT(1)    def bind(self):        glBindFramebufferEXT(GL_FRAMEBUFFER_EXT, self._id)    def attach_texture(self, attachment, texture):        self.bind()        glFramebufferTexture2DEXT(GL_FRAMEBUFFER_EXT, _valid_attachments[attachment], GL_TEXTURE_2D, texture.get_id(), 0)        self.unbind()    def unbind(self):        glBindFramebufferEXT(GL_FRAMEBUFFER_EXT, 0)    def detach_texture(self, texture):        self.bind()        glFramebufferTexture2DEXT(GL_FRAMEBUFFER_EXT, GL_DEPTH_ATTACHMENT, GL_TEXTURE_2D, 0, 0)        self.unbind()main.pyfrom pyorama.graphics.texture import Texturefrom pyorama.graphics.framebuffer import Framebufferimport pygameimport syswidth, height = (800, 600)pygame.init()pygame.display.set_mode((width, height), pygame.OPENGL | pygame.DOUBLEBUF)f = Framebuffer()t = Texture()t.load_from_data(width, height, RGBA)f.attach_texture(COLOR_0, t)while True:    for event in pygame.event.get():        if event.type == pygame.QUIT:            pygame.quit()            sys.exit()    f.bind()    glClear(GL_COLOR_BUFFER_BIT)    glClear(GL_DEPTH_BUFFER_BIT)    glBegin(GL_QUADS)    glColor(1, 0, 0, 1)    glVertex(-1, -1)    glVertex(-1, 1)    glVertex(1, 1)    glVertex(1, -1)    glEnd()    f.unbind()    print t.get_data()[0][0]    pygame.display.flip()Somehow, while this code works, I can't help but feel that there are many different ways that the user can break the texture class and trigger some cryptic OpenGL errors. Here is what I am worried about:I have tried to abstract away lots of OpenGL constants. Should these constants be placed at the module level, or should I place them at the class level instead? I also marked them as internal with a leading underscore. Is that all I need to do to dissuade users from modifying these constants and breaking behavior? It does not seem clear from the name of load_from_data() that data can be null (in order to accept incoming framebuffer data). Is there a better name for this function that makes it clear that I can either pass in data for rendering to the screen or leave it blank so that data can be populated in by rendering to the offscreen framebuffer later? Furthermore, with the default of data=None, it is not even clear that a 3D numpy array is required if you need to pass in data. I have lots of properties available to the user, such as width, height, texture_unit, etc. If the user creates a texture, loads some data, and then changes the width of the image later on, the data is not actually updated. What can I do to keep these user parameters and the actual OpenGL data in sync? Should I be doing some sort of validation of these texture parameters?I am sure that despite all of the considerations, there are even more issues that I can run into later down the line (e.g. cube-mapping with 6 textures comes to mind). What improvements can I make to the texture class to address all of the concerns above and make it flexible for future modifications? Should this texture class be split into multiple classes to handle, say, regular rendering versus framebuffer data or color versus depth data? Most other engines online I have seen are built on statically-typed languages and have method overloading and therefore do not seem to have to worry about passing in the wrong data or deal with missing arguments like in Python.
Designing a Robust Texture and Framebuffer class for Game Engine
python;game;opengl
I'm just going to review texture.py.There's no documentation. What kind of thing does an instance of the Texture class represent? How am I supposed to create and use one? What arguments do I pass to the load_from_data method? What does the get_data method return? Do I have to call the methods in a particular order for things to work? And so on.Image formats are represented as strings like RGBA. This makes it tricky to statically check that code is correct  tools like Pylint won't spot typos like RBGA. If the code used an enum the it would be easier to check the correctness of code.Image filters are represented by a Boolean: True represents bilinear and False represents nearest-neighbour. This representation makes code hard to understand (there's no particular reason why True represents bilinear rather than nearest-neighbour) and it seems like asking for trouble because in the future you may want to support mipmaps and so GL_TEXTURE_MIN_FILTER will need to take GL_NEAREST_MIPMAP_NEAREST or whatever. I recommend using an enumeration for filters.The Texture class has load_from_url and load_from_data methods that suggest that users might want to reload an existing Texture instance with a new image. This seems contrary to the way that objects are normally used in Python (where if you wanted a new texture, you'd make a new Texture). So I would make load_from_url and load_from_data into class constructors.The Texture class does not ensure that its instances are valid. For example it is an error to create a Texture object and then immediately call bind (you get a NameError). It is a good idea for classes to ensure that their instances are valid: this makes it harder to accidentally misuse them. In this case I would write:def __init__(self, width, height, image_format, data=None,             filter=TextureFilter.LINEAR, texture_unit=0):    ... docstring explaining the arguments ...    self._id = glGenTextures(1)    self.width = width    self.height = height    # ... and so on ...@classmethoddef from_url(cls, asset_url, filter=TextureFilter.LINEAR, texture_unit=0):    ... docstring explaining the arguments ...    # ... implementation here ...    return cls(width, height, image_format, data, filter, texture_unit)It looks as if the bind method must always be followed by unbind. But the code does not ensure this happens  if the code between bind and unbind raises an exception, then unbind is not called. This looks like a job for a context manager, for example:import contextlib@contextlib.contextmanagerdef bind(self):    ... docstring explaining the method ...    glActiveTexture(GL_TEXTURE1 + self.texture_unit)    glBindTexture(GL_TEXTURE_2D, self._id)    try:        yield    finally:        glActiveTexture(GL_TEXTURE0 + self.texture_unit)        glBindTexture(GL_TEXTURE_2D, 0)        glActiveTexture(GL_TEXTURE0)and then in load_from_data you'd write:with self.bind():    glTexParameterf(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, self.filter)    glTexParameterf(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, self.filter)    glTexImage2D(GL_TEXTURE_2D, 0, gl_format, self.width, self.height, 0, gl_format, GL_UNSIGNED_BYTE, data)