pyopenGL OBJ loader improvements

Question

Current OBJLoader:

Using pygame's obj loader as a base, I created a new obj loader for Python OpenGL. The loader includes the following functionality:

1. Collect vertices, normals, and texture to draw
2. Divide objects based on materials
3. Store each material's vertices, normals, colours and textures in separate VAO's
4. Have an instance vbo ready for each material
5. Call an appropriate shader for each material
6. Draw the whole Object as if it is one object

This means that you can call a single object to be drawn once, i.e:

 obj_m = ObjLoader()
 obj_m.load_model("C:/Projects/PythonScripts/GIS/obj_pins/KFC.obj")
 obj_KFC = obj_m.load_obj()

Or you can draw the object in multiple places using instancing, i.e:

 instance_list = [[0,0,0], [-10,0,0], [10,0,0]]
 obj_m = ObjLoader()
 obj_m.load_model("C:/Projects/PythonScripts/GIS/obj_pins/KFC.obj", *instance_list)
 obj_KFC = obj_m.load_obj()

Then you can call the object in your drawing loop with or without transformation, i.e:

 obj_m.draw_call(obj_KFC, view, projection, model)

or

 transform = glm.mat4(1.0)
 transform = glm.rotate(transform, glfw.get_time(), glm.vec3(0.0, 1.0, 1.0))
 transform = glm.scale(transform, glm.vec3(1.0, 1.0, 1.0))
 obj_m.draw_call(obj_KFC, view, projection, model, transform)

Output: Instanced Object

Current Drawbacks

I only recently started with OpenGL for fun and I'm not a programmer per se. Therefore I implemented the object loader's functionality as I went along. The code is therefore not "pretty", nor tested extensively and mostly without added error checking code. These are the known drawbacks:

1. The .obj file should be triangulated i.e using Blender:

   Triangulate object

2. The .obj, .mtl, and image (if a material contains a texture) needs to be in the same folder

3. The link to the texture in the .mtl folder should be relative, therefore containing only the name of the image i.e: map_Kd 3D_kfc.png

Improvements

I thought to share the code because as I stated, I'm not a programmer per se, but I think others can benefit from this especially when the code is "cleaner". So the questions:

1. Are there obvious structure improvements?
2. Are there obvious improvements to the code?
3. Are there any wanted improvements i.e lighting?

OBJLOADER CODE

import numpy as np
from OpenGL.GL import *
from OpenGL.GL.shaders import compileProgram, compileShader
import os
import glm
from PIL import Image
class ObjLoader:
 def __init__(self):
 self.vert_coords = []
 self.text_coords = []
 self.norm_coords = []
 self.object_parts = {}
 self.setup_index()
 self.obj_shaders()
 def setup_index(self):
 self.material_index = []
 self.vertex_index = []
 self.texture_index = []
 self.normal_index = []
 self.model = []
 self.material_index = []
 self.vertex_index = []
 self.texture_index = []
 self.normal_index = []
 def load_model(self, file, *args):
 for line in open(file, 'r'):
 if line.startswith('#'): continue
 values = line.split()
 if not values: continue
 if values[0] == 'v':
 self.vert_coords.append(values[1:4])
 if values[0] == 'vt':
 self.text_coords.append(values[1:3])
 if values[0] == 'vn':
 self.norm_coords.append(values[1:4])
 if values[0] == 'mtllib':
 self.mtl = values[1]
 matarials = self.mat_in_file(file)
 m_counter = 0
 for line in open(file, 'r'): # open(file, 'r'):
 if line.startswith('#'): continue
 values = line.split()
 if not values: continue
 if values[0] == 'usemtl':
 if values[1] != matarials[m_counter][0]:
 self.define_i_t_u(file, matarials[m_counter], args)
 m_counter += 1
 self.setup_index()
 if values[0] == 'f':
 face_i = []
 text_i = []
 norm_i = []
 for v in values[1:4]:
 w = v.split('/')
 face_i.append(int(w[0]) - 1)
 text_i.append(int(w[1]) - 1)
 norm_i.append(int(w[2]) - 1)
 self.vertex_index.append(face_i)
 self.texture_index.append(text_i)
 self.normal_index.append(norm_i)
 self.define_i_t_u(file, matarials[m_counter], args)
 def define_i_t_u(self, file, mat, args):
 self.vertex_index = [y for x in self.vertex_index for y in x]
 self.texture_index = [y for x in self.texture_index for y in x]
 self.normal_index = [y for x in self.normal_index for y in x]
 for i in self.vertex_index:
 self.model.extend(self.vert_coords[i])
 for i in self.texture_index:
 self.model.extend(self.text_coords[i])
 for i in self.normal_index:
 self.model.extend(self.norm_coords[i])
 self.model = np.array(self.model, dtype='float32')
 glUseProgram(mat[1])
 texid = self.MTL(file)
 l_vao = self.vao_creator(self.vertex_index, self.model, mat[1], mat[2], texid, args)
 self.object_parts["{0}".format(mat[0])] = l_vao
 def MTL(self, filename):
 filename = os.path.splitext(filename)[0] + '.mtl'
 filepath = os.path.dirname(filename)
 base = os.path.basename(filename)
 file_only_name = os.path.splitext(base)[0]
 contents = {}
 mtl = None
 image = None
 texid = None
 surf = None
 values = []
 for line in open(filename, "r"):
 if line.startswith('#'): continue
 values = line.split()
 if not values: continue
 if values[0] == 'newmtl':
 mtl = contents[values[1]] = {}
 elif mtl is None:
 raise ValueError("mtl file doesn't start with newmtl stmt")
 elif values[0] == 'map_Kd':
 self.object_shader = self.shader_obj_text
 # load the texture referred to by this declaration
 mtl[values[0]] = values[1]
 texid = mtl['texture_Kd'] = glGenTextures(1)
 glBindTexture(GL_TEXTURE_2D, texid)
 glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER,
 GL_LINEAR)
 glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER,
 GL_LINEAR)
 image = Image.open(os.path.join(filepath, mtl['map_Kd']))
 flipped_image = image.transpose(Image.FLIP_TOP_BOTTOM)
 img_data = flipped_image.convert("RGBA").tobytes()
 glTexImage2D(GL_TEXTURE_2D, 0, GL_RGBA, image.width, image.height, 0, GL_RGBA, GL_UNSIGNED_BYTE,
 img_data)
 return texid
 def vao_creator(self, vertex_index, model, shader, defuse_col, texid, args):
 texture_offset = len(vertex_index) * 12
 VAO = glGenVertexArrays(1)
 glBindVertexArray(VAO)
 VBO = glGenBuffers(1)
 glBindBuffer(GL_ARRAY_BUFFER, VBO)
 glBufferData(GL_ARRAY_BUFFER, model.itemsize * len(model), model, GL_STATIC_DRAW)
 # position
 glVertexAttribPointer(0, 3, GL_FLOAT, GL_FALSE, model.itemsize * 3, ctypes.c_void_p(0))
 glEnableVertexAttribArray(0)
 # texture
 glVertexAttribPointer(1, 2, GL_FLOAT, GL_FALSE, model.itemsize * 2, ctypes.c_void_p(texture_offset))
 glEnableVertexAttribArray(1)
 # Normals
 glVertexAttribPointer(2, 3, GL_FLOAT, GL_FALSE, model.itemsize * 3, ctypes.c_void_p(12))
 glEnableVertexAttribArray(2)
 # Instancing
 if len(args) == 0:
 amount = 1
 vert_instance = np.array([[0.0, 0.0, 0.0], [1.0, 1.0, 1.0]])
 else:
 amount = len(args)
 vert_instance = np.array(args)
 modelMatrices = []
 # Storring the objects in a list
 for i in range(0, amount):
 mod_list = []
 model_o = glm.mat4(1.0)
 model_o = glm.translate(model_o, glm.vec3(vert_instance[i][0], vert_instance[i][1], vert_instance[i][2]))
 # model_o = glm.scale(model_o, glm.vec3(0.00005, 0.00005, 0.00005))
 for i in range(4):
 for j in range(4):
 mod_list.append(model_o[i][j])
 modelMatrices.append(mod_list)
 modelMatrices = np.array(modelMatrices, dtype="f")
 instanceVBO = glGenBuffers(1)
 glBindBuffer(GL_ARRAY_BUFFER, instanceVBO)
 glBufferData(GL_ARRAY_BUFFER, amount * glm.sizeof(glm.mat4), modelMatrices, GL_STATIC_DRAW)
 # Bind each vertex attrib array of matrices (4 vectors in Matrix)
 glEnableVertexAttribArray(3)
 glVertexAttribPointer(3, 4, GL_FLOAT, GL_FALSE, glm.sizeof(glm.mat4), ctypes.c_void_p(0))
 glEnableVertexAttribArray(4)
 glVertexAttribPointer(4, 4, GL_FLOAT, GL_FALSE, glm.sizeof(glm.mat4), ctypes.c_void_p(glm.sizeof(glm.vec4)))
 glEnableVertexAttribArray(5)
 glVertexAttribPointer(5, 4, GL_FLOAT, GL_FALSE, glm.sizeof(glm.mat4),
 ctypes.c_void_p((2 * glm.sizeof(glm.vec4))))
 glEnableVertexAttribArray(6)
 glVertexAttribPointer(6, 4, GL_FLOAT, GL_FALSE, glm.sizeof(glm.mat4),
 ctypes.c_void_p((3 * glm.sizeof(glm.vec4))))
 # Set instancing
 glVertexAttribDivisor(3, 1)
 glVertexAttribDivisor(4, 1)
 glVertexAttribDivisor(5, 1)
 glVertexAttribDivisor(6, 1)
 glBindVertexArray(0)
 return ([VAO, len(vertex_index), shader, defuse_col, texid, len(vert_instance)])
 def mat_in_file(self, filename):
 filename = os.path.splitext(filename)[0] + '.mtl'
 mtl = None
 contents = {}
 mat_list = []
 for line in open(filename, "r"):
 if line.startswith('#'): continue
 values = line.split()
 if not values: continue
 if values[0] == 'newmtl':
 object_shader = self.shader_obj
 mtl = contents[values[1]] = {}
 matname = values[1]
 elif mtl is None:
 raise ValueError("mtl file doesn't start with newmtl stmt")
 elif values[0] == 'Kd':
 defuse_col = [float(values[1]), float(values[2]), float(values[3])]
 elif values[0] == 'd':
 defuse_col = defuse_col + [float(values[1])]
 mat_list.append([matname, object_shader, defuse_col])
 elif values[0] == 'map_Kd':
 object_shader = self.shader_obj_text
 mat_list.pop()
 mat_list.append([matname, object_shader, defuse_col])
 return mat_list
 def load_obj(self):
 obj_list = [self.object_parts[ind] for ind in self.object_parts]
 return obj_list
 def draw_call(self, vao_list, view, projection, model, *transf):
 if len(transf) == 0:
 transform = glm.mat4(1.0)
 else:
 transform = transf[0]
 for key in vao_list:
 VAO = key[0]
 vertex_len = key[1]
 shader = key[2]
 d_colour = key[3]
 texid = key[4]
 len_instanced = key[5]
 glUseProgram(shader)
 glBindTexture(GL_TEXTURE_2D, texid)
 view_loc = glGetUniformLocation(shader, "view")
 proj_loc = glGetUniformLocation(shader, "projection")
 model_loc = glGetUniformLocation(shader, "model")
 col_loc = glGetUniformLocation(shader, "mycolor")
 glUniformMatrix4fv(view_loc, 1, GL_FALSE, view)
 glUniformMatrix4fv(proj_loc, 1, GL_FALSE, projection)
 glUniformMatrix4fv(model_loc, 1, GL_FALSE, model)
 glUniform4fv(col_loc, 1, glm.value_ptr(glm.vec4(d_colour)))
 transformLoc = glGetUniformLocation(shader, "transform")
 glUniformMatrix4fv(transformLoc, 1, GL_FALSE, glm.value_ptr(transform))
 glBindVertexArray(VAO)
 glDrawArraysInstanced(GL_TRIANGLES, 0, vertex_len, len_instanced)
 glBindVertexArray(0)
 def obj_shaders(self):
 vertex_obj = """
 # version 330
 in layout(location = 0) vec3 position;
 in layout(location = 1) vec2 textureCoords;
 in layout(location = 2) vec3 normalCoords;
 in layout (location = 3) mat4 instanceMatrix;
 uniform mat4 transform;
 uniform mat4 view;
 uniform mat4 model;
 uniform mat4 projection;
 out vec2 newTexture;
 out vec3 newNorm;
 void main()
 {
 gl_Position = projection * view *instanceMatrix *model * transform *vec4(position, 1.0f);
 newTexture = textureCoords;
 newNorm = normalCoords;
 }
 """
 fragment_obj = """
 #version 330
 in vec2 newTexture;
 in vec3 newNorm;
 uniform vec4 mycolor;
 out vec4 outColor;
 uniform sampler2D samplerTexture;
 void main()
 {
 outColor = mycolor;
 }
 """
 fragment_obj_text = """
 #version 330
 in vec2 newTexture;
 in vec3 newNorm;
 in vec3 out_colours;
 uniform vec4 mycolor;
 out vec4 outColor;
 uniform sampler2D samplerTexture;
 void main()
 {
 outColor = texture(samplerTexture, newTexture);
 }
 """
 self.shader_obj = compileProgram(compileShader(vertex_obj, GL_VERTEX_SHADER),
 compileShader(fragment_obj, GL_FRAGMENT_SHADER))
 self.shader_obj_text = compileProgram(compileShader(vertex_obj, GL_VERTEX_SHADER),
 compileShader(fragment_obj_text, GL_FRAGMENT_SHADER))

Example code

import glfw
import pyrr
from ObjLoader import *
def window_resize(window, width, height):
 glViewport(0, 0, width, height)
def main():
 # initialize glfw
 if not glfw.init():
 return
 w_width, w_height = 800, 600
 window = glfw.create_window(w_width, w_height, "My OpenGL window", None, None)
 if not window:
 glfw.terminate()
 return
 glfw.make_context_current(window)
 glfw.set_window_size_callback(window, window_resize)
 instance_list = [[0,0,0], [-10,0,0], [10,0,0]]
 obj_m = ObjLoader()
 obj_m.load_model("C:/Projects/PythonScripts/GIS/obj_pins/KFC.obj", *instance_list)
 obj_KFC = obj_m.load_obj()
 glClearColor(0.2, 0.3, 0.2, 1.0)
 glEnable(GL_DEPTH_TEST)
 view = pyrr.matrix44.create_from_translation(pyrr.Vector3([0.0, 0.0, -20.0]))
 projection = pyrr.matrix44.create_perspective_projection_matrix(65.0, w_width / w_height, 0.1, 100.0)
 model = pyrr.matrix44.create_from_translation(pyrr.Vector3([0.0, 0.0, 0.0]))
 while not glfw.window_should_close(window):
 glfw.poll_events()
 glClear(GL_COLOR_BUFFER_BIT | GL_DEPTH_BUFFER_BIT)
 #obj_m.draw_call(obj_KFC, view, projection, model)
 transform = glm.mat4(1.0)
 transform = glm.rotate(transform, glfw.get_time(), glm.vec3(0.0, 1.0, 1.0))
 transform = glm.scale(transform, glm.vec3(1.0, 1.0, 1.0))
 obj_m.draw_call(obj_KFC, view, projection, model, transform)
 glfw.swap_buffers(window)
 glfw.terminate()
if __name__ == "__main__":
 main()

• Mast: Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers . Feel free to post a new question instead (after waiting for a day, perhaps more answers are coming in), with a link to this question for additional context. Thanks!

Answer 1

Hello and welcome to CodeReview!

A random list of observations:

Type hints

For your members,

 self.vert_coords = []

can be (assuming it's a list of floats)

self.vert_coords: List[float] = []

and so on. This helps for a few reasons - smart IDEs will notice if you're using the member with an unexpected type and can warn you; autocomplete is better; and this is clearer to other programmers (including you in six months).

Typo

matarials = materials

Generation/comprehension

This:

 face_i = []
 text_i = []
 norm_i = []
 for v in values[1:4]:
 w = v.split('/')
 face_i.append(int(w[0]) - 1)
 text_i.append(int(w[1]) - 1)
 norm_i.append(int(w[2]) - 1)

can be expressed a number of different ways; one example:

parts = [
 [
 int(w) - 1 for w in v.split('/')
 for w in v.split('/')
 ]
 for v in values[1:4]
]
self.vertex_index.append([part[0] for part in parts])
self.texture_index.append([part[1] for part in parts])
self.normal_index.append([part[2] for part in parts])

Or you could also make a function that yields:

def get_vertices(values):
 for v in values[1:4]:
 yield int(v.split('/')[0]) - 1
...
self.vertex_index = list(get_vertices(values))

Nomenclature

Re. def MTL - usually methods in Python are lower-case.

Also, this:

modelMatrices

is typically in snake_case, i.e. model_matrices

Array definition

This:

 vert_instance = np.array([[0.0, 0.0, 0.0], [1.0, 1.0, 1.0]])

is more legible as

vert_instance = np.array([
 [0.0, 0.0, 0.0],
 [1.0, 1.0, 1.0],
])

Computers are good at loops

 glVertexAttribDivisor(3, 1)
 glVertexAttribDivisor(4, 1)
 glVertexAttribDivisor(5, 1)
 glVertexAttribDivisor(6, 1)

can become

for i in range(3, 7):
 glVertexAttribDivisor(i, 1)

Indentation

Your vertex_obj string has a large amount of indentation. I'm not sure why this is, but either way, that string is better off living in a file that you read in when you need it. It's more maintainable that way.

• Kempie: Thanks. I edit my code with your suggestions. It even pointed out an error where I tested 2 instances for vert_instance and never changed the code back to a single instance. Also had duplicate variable declarations.
• Reinderien: Please do not edit existing code based on suggestions. Policy is to open a new question.

Answer 2

Let's start with the high-level stuff. I find it a little bit odd that an object loader also interfaces with OpenGL - I would instead recommend that you write the loader as part of a module, and that you render in a separate application. What you have right now is not an object loader but rather a minimal renderer that consists of an object loader class. Cohesion and coupling is definitely the biggest issue in your code.

I also think you'd benefit more from looking at the implementation in something like pywavefront over the one in pygame: https://github.com/pywavefront/PyWavefront.

Separate minor things

Setup/initialisation

You're duplicating several declarations in ObjLoader.setup_index, but more importantly why even have that method when it's called upon in __init__? Sure, you're also calling on this method from elsewhere, but that seems to be more of a symptom of the coupled functions of the class.

Opening files

Right now you're doing stuff like:

for line in open(file, 'r'):

Beware that you're opening file here without closing it. You could either make sure you close it explicitly, or you can use contextmanagers to deal with that for you (with open(file, 'r') as f:).

Formatting

Readability counts. Don't do in-line stuff like this if line.startswith('#'): continue while also having no blank lines - it's quite easy to miss.

This is also really confusing self.vertex_index = [y for x in self.vertex_index for y in x] - what are you trying to accomplish? Don't use variable names like x and y unless you're dealing with plots or the like.

One more thing: def MTL(self, filename): - try to follow PEP8 and don't capitalise methods.

Imports

from OpenGL.GL import *

This is generally disliked since it hides what you're importing and from where. I'd recommend trying to import everything you use, but if you want to be a bit lazy it would be preferred if you at least did from OpenGL import GL as gl.

It's also considered good style to order your imports: first standard library, then 3rd party libraries, then your own modules. In your case that would mean something like this:

import os
import numpy as np
from PIL import Image
from OpenGL import GL as gl
from OpenGL.GL.shaders import compileProgram, compileShader
import glm

Versioning

Finally, be a bit careful with requirements: your shaders are written to use glsl 3.3, but nowhere have you written that OpenGL 3.3 is a requirement, and you don't even specify to use (at least) 3.3 in your example.

Answer 3

Two things that I can see that I think would improve your structure a lot:

• Keep the shader sources in separate files (I like to have them in /res/shaders with a .glsl extension) and load them in. You could have a class called ShaderProgram with methods load_shader and that also links up the shader program and removes the compiled shaders when linked.

• Having a class also for the VAO would allow you to access members, which means that you could transform this:

   for key in vao_list:
   ...
   shader = key[2]
   ...
   glUseProgram(shader)
  

  to something like this:

   for VAO in vao_list:
   glUseProgram(VAO.shader)
  

Smaller improvements

You're currently dealing with a lot of lists and manual indexes where you could use classes and list comprehensions. This:

 mod_list = []
 for i in range(4):
 for j in range(4):
 mod_list.append(model_o[i][j])

could be written as mod_list = [model_o[i][j] for j in range(4) for i in range(4)]. List comprehensions are faster and easier to read. And speaking of lists, you may want to have a look at Collections.deque (a bit faster), and you may want to rethink some of your lists since you probably are using a lot of performance there.

---

Use context managers. You're opening files without making sure that they're closed, e.g. here: for line in open(file, 'r'):.

"Bugs"

You're overwriting a lot of your variables here:

 def setup_index(self):
 self.material_index = []
 self.vertex_index = []
 self.texture_index = []
 self.normal_index = []
 self.model = []
 self.material_index = [] # duplicate
 self.vertex_index = [] # duplicate
 self.texture_index = [] # duplicate
 self.normal_index = [] # duplicate

I also think I found a "bug", where you do:

 mtl = None
 ...
 if values[0] == 'newmtl':
 ...
 mtl = contents[values[1]] = {}
 ...
 elif mtl is None:
 raise ValueError("mtl file doesn't start with newmtl stmt")

It would only evaluate that else-if if the first if is False, and the else-if will only be True if the first if is False. Based on your message in the raised error, I would think that you just want if values[0] == "newmtl": <do-this> else: raise <error>.