RenderEngine

[Kamjam66xx]

Im working on a render engine and eventually a complete game engine. I put my project on github, and im ready for a code review if anyone wants to give me input!

 

Thanks in advance 

https://github.com/Kamjam21xx/KameronsRenderEngine/tree/master/src/OpenGlCourseApp

[Unimportant]

1 hour ago, Kamjam66xx said:

• 28 minutes ago, Kamjam66xx said:

Im working on a render engine and eventually a complete game engine. I put my project on github, and im ready for a code review if anyone wants to give me input!

 

Thanks in advance 

https://github.com/Kamjam21xx/KameronsRenderEngine/tree/master/src/OpenGlCourseApp

Did a quick review, and there's some problems. Some minor and some very serious.

• Some of your classes that try to manage resources actually don't. Look up the rule of 3/5/0. Here's an example from Skybox:

  //Skybox.h
  private:
  	Mesh *skyMesh;
  	Shader *skyShader;
  
  //Skybox.cpp
  SkyBox::SkyBox(std::vector<std::string> faceLocations)
  {
  	// shader
  	skyShader = new Shader();

  You've got some raw member pointers for which you then allocate memory with new. What happens when a Skybox instance gets copied? The default copy constructor will simply copy those raw pointers so now you've got 2 skyboxes pointing to the same resources. When one does some modifications to those resources, the results will be reflected to both, leading to weird bugs. Furthermore, when one skybox is destroyed and frees it's memory (you've commented out the deletes in the destructor for some reason? - now you're leaking memory) the other skybox is pointing to deleted resources. The rule of 3 (pre C++11) used to say that if your class manages resources like this you must write:

  • Copy constructor and assignment operators that perform a deep copy. That is, allocate their own memory and copy the contents over so now both instances have their own resources.

  • A destructor that frees the resources.

 

This then later became the rule of 5 in C++11 as move semantics where added. If applicable you should now also add a move constructor and assignment operator that can steal the source's resources cheaply.

The rule of 0, which I subscribe to, states that classes that manage resources, and thus have custom copy/move constructors/assignment should deal exclusively with the ownership and management of their resource. Other classes can then use these RAII objects as members so they themselves don't need to worry about any of it and need 0 custom copy/move operators.

 

So, in short, you should probably wrap the management of skyMesh and skyShader into another class that's solely responsible for managing it.

 

One could argue that there only ever needs to be a single Skybox and it won't be copied. In that case you should delete the copy constructor and assignment operator to prevent accidental copies. Even then you should wrap those pointers into a std::unique_ptr that manages their lifetime for you so you can't leak. You should not be using naked new and delete.

 

You make similar resource management errors throughout your code, here's another example:

//Texture.h
private:
	//<snip>
	const char* fileLocation;

//Texture.cpp
Texture::Texture(const char* fileLoc)
{
	//<snip>
	fileLocation = fileLoc;
	//<snip>
}

bool Texture::LoadTextureA() {
	// loads image. one and done.
	unsigned char *texData = stbi_load(fileLocation, &width, &height, &bitDepth, 0);

You give a C style string with the file location to the constructor of this class which it then stores. A C style string is nothing but a char pointer pointing to the actual string which lives somewhere outside the Texture class. Then, possibly at a much later time, you use this pointer in your member functions. What if the string is already gone by then and the pointer is dangling ? For small aggregations like this it makes no sense to have the caller worry about the string lifetime. Simply make fileLocation a std::string so each Texture has it's own file location stored safely inside, not depending on the outside world. 

• There's lots more places in your code where you use C style strings. You should convert them all to std::string to minimize the chance of similar bugs. If you need a C style char pointer to pass to the GL functions use std::string::c_str to get a C style string pointer from a std::string at the very last moment.

• signed/unsigned interrelations. for example:

  for (size_t i = 0; i < (MAX_POINT_LIGHTS + MAX_SPOT_LIGHTS); i++)

  MAX_POINT_LIGHTS and MAX_SPOT_LIGHTS are signed int, so the result of adding them is also signed int, which is then compared to a size_t which is unsigned. Your compiler should emit a warning for this. Signed and unsigned interrelating can lead to different results then what you were expecting. <more info>. It's probably not a problem here because the numbers are so small but it is a general code smell you should watch out for. My way of doing things is to always use signed, for everything (math related, bit manipulation is something different), even things that can't be negative. It's much better to catch some value that can't be negative being negative in the debugger then see it as some huge overflown positive value that might escape detection. If you really need the extra bit, use a larger datatype. size_t is a abomination that should never have been unsigned. (altough understandable, it's a carry-over from C from decades ago when that extra bit really was required). When you get a size_t from some function call, convert it to signed asap. I've my custom made template function that assigns the value of a size_t to a given signed variable and throws when it won't fit. In this case you can offcourse just change to "int i = 0".

There's probably more things to find but I guess this is enough to get you busy refactoring for a while.

[Kamjam66xx]

@Unimportant Thanks! I really appreciate that. I definitely got my work cut out for me. But it pays off to make sure things are done right. Especially considering the functionality ill be adding in the future. 

 

Its a learning process. Ive been programming for like 6 months, and wasted a lot of time learning java, python, etc. Im trying to progress and specialize as fast as i can now!

 

I actually didnt know you could get the pointer to the member char* via "std::string::c_str"

[Unimportant]

@Kamjam66xx Had some more time this afternoon, some more things:

• const correctness. For example:

  //GL_Window.h
  GLint getWindowWidth() { return width; }

  Member functions that do not modify the class they belong to should be marked const:

  GLint getWindowWidth() const { return width; }

  This protects us from accidentally modifying the object anyway, the compiler will now shout at us if we do, but more importantly it allows invoking the member function on a const instance/reference/pointer to const. For example, say I write a function:

  int
  GetWindowSurface(const GL_Window& window)
  {
  	return window.getWindowWidth() * window.GetWindowHeight();  
  }

  We pass a reference to the window because a window is potentially a heavy object and we don't want to copy it just to inspect its width and height. The reference is const because we don't want to accidentally modify the window. This will however result in a compilation error in your case. Because you did not mark getWindowWidth and getWindowHeight const we can not call them on a const reference to a window. It's important to start using const correctness as soon as possible because having to add it later results in a game of "chase the const" where every time you add a const it'll require adding more consts deeper down. If you simply google "const correctness" you should find lots of articles to read.

• Very long parameter lists might indicate some code smells:

  DirectionalLight::DirectionalLight(GLfloat shadowWidth, GLfloat shadowHeight,
                                     GLfloat red,        GLfloat green,      GLfloat blue, 
  				   GLfloat aIntensity, GLfloat dIntensity,				 
  				   GLfloat xDir, GLfloat yDir, GLfloat zDir)

  (Those backslashes are not needed btw, only for multi line C style macro's, which you should almost never use in C++ anyway). If you have such long lists of parameters you should ask yourself if the class in question is not taking on too much responsibilities that should be further divided down or if some of those parameters could not be grouped into further abstractions that could be perhaps re-usable. In this case "red", "green" and "blue" probably represent a color, and "xDir", "yDir" and "zDir" might represent a 3D point or vector ? Consider creating color and 3D point/vector classes that can over time grow into usefull entities in their own right.  If you find your concepts to be correct and you really need so many parameters, consider grouping them in a struct, that makes the code cleaner and easier to follow. Consider:

  DirectionalLight fooLight(12.0, 10.0, 101.0, 0.0, 50.0, 25.0, 25.0, 1001.0, 5000.0, 300.0);
  
  //Versus...
  
  DirectionalLightConfig lightConfig;
  config.shadowWidth = 12.0;
  config.shadowHeight = 10.0;
  config.red = 101.0;
  config.green = 0.0;
  config.blue = 50.0;
  config.aIntensity = 25.0;
  config.dIntensity = 25.0;
  config.xDir = 1001.0;
  config.yDir = 5000.0;
  config.zDir = 300.0;
  DirectionalLight barLight(lightConfig);

  Which would you rather be debugging in 6 months ?

• Breaking encapsulation trough the back gate:

  //Light.h
  class Light
  {
  public:
  	
  	//<snip>
  	ShadowMap* GetShadowMap() { return shadowMap; }
  	//<snip>
  protected:
  	//<snip>
  	ShadowMap *shadowMap;
    	//<snip>
    
    

  We've already covered the part where shadowMap should be wrapped in a std::unique_ptr because Light owns the resource and is responsible for it. Simply giving the pointer away trough the GetShadowMap function breaks encapsulation. A caller can simply get the pointer out and start doing anything with it, even freeing the memory! If callers only need to be able to see the ShadowMap but not modify it, you can have GetShadowMap return a const reference to the ShadowMap:

  const ShadowMap&
  GetShadowMap() const { return *shadowMap; }

  Now callers can get a const reference to the ShadowMap and inspect it all they want, but they cannot modify it. Note that we also marked the function const because it's absolutely certain now it does not modify the Light object. Also note that the caller can now only call functions that are marked const on the ShadowMap object we return, so you need to mark the approriate ShadowMap member functions const aswell. This is what I meant with "chase the const". If callers do need to be able to modify the ShadowMap you should carefully consider your object hierarchy. This particular ShadowMap is part of a Light. The only operations on this ShadowMap that make sense are those in direct context of a Light. Thus, the Light is the (only) one that should be making any modifications to the ShadowMap, probably as part of a more complex operation supported by Light. Also consider that the shadowMap pointer is protected, which means derived classes can access it. This is also a form of breaking encapsulation. I simply have to derive a class and add my own member function that returns the pointer and I'm right back at it. Consider making the pointer private and provide some protected member functions for derived classes to perform operations on it.

• Use resource handles wherever you can:

  void Mesh::CreateMesh(GLfloat *vertices, unsigned int *indices, 
  		      unsigned int numOfVertices, unsigned int numOfIndices, 
  		      GLenum drawType
  ) 

  vertices and indices should be replaced with std::vectors holding the vertices and indices. And since a std::vector knows it's own size you'd no longer need the numOfVertices and numOfIndices parameters. Resulting code will be less error prone because you can use range based for loops on vectors and as already mentioned there's no chance to get the size wrong. You can always get at the vector's buffer with std::vector::data if you need to pass it to the GL functions. Most compilers also support some debug flags that let you enable range checks on std::vector. This falls in the same category as using std::string in stead of raw C strings we already covered before. Use resources handles everywhere and only get the raw pointers out at the very last moment if you need them.

Now I'm out of time again.