Hi everyone!
 

Im working on a tutorial / work portfolio that will try and cover not just a single topic ("animating a 3D model", "lighting", "collision detection"), but a whole asteroid-like game using C++ and OpenGL.
My goal is to try and cover all aspects of this game and how all the parts are connected to each other in a comprehensive and efficient way.

The game is pretty close to being finished and sits at gitlab, but my site covering it is still in progress. But I figured it would make sense to let others look at it and give me feedback on it as I work on the tutorial. Is there anyone who would be interested in helping out with reading through it, checking for misstakes or just general feedback? I would of course mention anyone who gives me feedback at a credits part of the tutorial (please put a credits section in your feedback (name, twitter account, website - what ever you want) if you want to get credited).

The work-in-progress site sits at http://165.22.53.246

All and any help would be very appreciated!

Nice job. I didn't build the project, but I read over what you have so far and took a look at some code.

This is all low-hanging fruit, but here are some comments.

Obviously having someone formally proofread and edit the tutorial would be good, but meanwhile I think you could probably clean up quite a few things with a careful pass over the text. Things I noticed were punctuation issues (missing periods and so on), spelling errors that spellcheck might not catch (e.g. bellow vs below), other possible spelling errors (I assume you have spellcheck available), your/you're errors, extra or misplaced words, and so on. I think with a careful pass you could probably fix a lot of the issues, leaving an easier task for other proofreaders (and having others proofread is definitely a good idea).

Regarding code formatting, it looks like you're using hard tabs rather than soft tabs (spaces). I'll refrain from offering an opinion on this as I know it's a contentious issue, but I'll just point out that using hard tabs can result in code displaying differently in different contexts. For example, in the setting in which I'm viewing the tutorial and code at the moment, the indentation seems excessive and makes the code a little hard to read. Again though, opinions differ on this, so I'm just making an observation here, not offering any sort of suggestion.

I looked at one source file pair, player.h/cpp. I realize C++-related best practices and idioms may not be your focus here, but given that people sometimes pick up habits from tutorials, it might be worth taking a look at some things.

With that in mind, here are some things I noticed. (Again, this is all low-hanging fruit - these are just standard things that often come up in the context of C++ code reviews.)

- You're using a preprocessor macro for the 'player depth' value, but in C++ it's typically preferred to use constants. There are various reasons for this (try searching for 'c++ const vs define' for more info).

- It's typical to pass arguments of non-primitive types (like glm::vec3) by constant reference to avoid unnecessary copying. In this context it probably doesn't really matter either way, but passing by constant reference is arguably more idiomatic.

- It looks like you're returning some values by non-constant reference (e.g. GetMoveSpeed()). I didn't look at the rest of the code to see how these functions are used, but I'm wondering what the purpose of the references is.

- In Render() you're using references, but they're non-constant. It would be atypical for a render function to modify matrices that are passed in, so I'm wondering if you meant for these to be constant. You have a similar non-constant reference in DoMove().

- You might also look into 'constant correctness' in C++ in general. Opinions differ on how broadly 'const' should be used, but it would certainly be idiomatic to use it more than you are currently.

- 'using namespace std;' in a source file isn't necessarily a problem, but it does dump a lot of stuff into (in this case) the global namespace. And, it looks like you're qualifying with std:: explicitly in some places anyway. Personally I'd drop the 'using' and just qualify explicitly.

- You might look into 'NULL' vs 'nullptr' in C++.

- Does 'Player' need an explicit non-default destructor?

- I don't think you need the '!m_projectileBalls.empty()' check.

I see some other minor things, but I'll stop there. Looks good - I hope this feedback will be helpful.

I find the idea of integrating the elements of a basic game into a whole nice. Am sure it will help the one or other.

While you're at it, because we have so many OpenGL 3.3 examples, i suggest to use 4.5, which by now is 5 years old as well.

On 6/20/2019 at 6:32 PM, JimmieJohnsson84 said:

Hi Zakwayda, Green_Baron.

Zakwayda:
This is very good feedback and what I was hoping to get.
I do intend to have good C++ code as well, as good as is possible. There are always certain subjects that tend to become a bit a matter of taste both in coding style and choosen patterns/design. I like getting feedback on that too, because I might find someone else has a great suggestion that I might want to go for that would improve on it.

I will look into the #define vs const. I've used them at some place like the Z depth for some objects - I intend to move all that code into something else. As is now, ducks are spawned in the main loop for example and this Z depth as well as world boundaries are fetched from defines and hardcoded in multiple places. I want to move all that into some sort of container, not quite sure how it should look yet.
 

I will also have a look over on constant. I think in most places, Im ok with the reference to vectors not being copied since its a reference to the object. But it would make more sense that for example the "Scene" object has a constant ref handle, since it should never be able to update the content of these vectors. That is a good point that I will try and fix.

I will look through your feedback and try and improve on things going forward. I appreciate it and Im very glad that you took the time to read through both the blog and the code - thank you!

Green_Baron:

Thanks, I'm glad to hear that. Im hoping that integrating all these different parts like you say into a whole thing is good. So I want to try and do that as good as possible and share it. Im hoping that posting it online and getting others involved will make it even better, cause I'm sure there is a good community out there with a lot of skills and knowledge that will help improving things that I've missed and so on. I might try and use some more modern OpenGL techniques from 4.5 as you say. First, I feel like I have to try and finish the tutorial with the code I have thought. I got a lot of more stuff to cover yet

I got feedback from a very talented developer working in a game studio so right now Im trying to make a couple of improvements that he suggested. After that I will try to write about the particle system and then move on over to the collision detection, the quadtree for the collision detection, some scenario tests that you can build to verify that the collision system is working as expected. I got an A* path finding algorithm in there that I need to implement into a special enemy that will follow the player. I also got to finish up some more game mechanics, a game menu etc. I will make posts here as I make some progress and Im always happy to hear opinons and feedback.

/Jimmie

Hi All,

I've made a small update that is posted to the tutorial, the beginning of the particle system. It covers geometry instancing and explains a way to create a sort of "explosion" particle system. It is still pretty "early" and not finished yet, but I would love to hear any feedback on it. Is there something that is unclear? Have I made some misstake? Perhaps I could have done something in a better way. Let me know

Input or rather random thought. Don't be disappointed that it is only critique ?

I realize you schlepp all kind of uniforms locations around in the Model3D class, while i would expect them to be somewhere higher level, or part of the shader-class or a shader-interface-class (uniform buffers come to mind if you expect all these matrices to change quickly).

Also, i would suggest to at least mention to calculate normals in the vertex shader, which is probably much more performant than pre-calculation and loading from disk and copying through memory and you don't need a normal matrix (or am i wrong ? It is actually the first time i heard about it :-)) and have up-to-date normals each frame. And that way you only have positions to pass into the pipeline.

Nitpicking: C++14 knows uniform initialization.

Oh, and the std::vector can be pre-sized on declaration (edit: and .resize()ed making room for a number of instances), that way you don't need to store pointers in it but can actually store all the instances, leaving the cleanup to the vector and at the same time avoiding copying of the vector each time something is added. Just wanted to clear that, because the number of particles is probably fixed ? ?

'nother thing, the random numbers in c++, i just learned that the other week while fooling around with a ray tracer tutorial:

Choose and seed the random number generator and set the distribution (one time):

std::random_device seed;
std::minstd_rand yourGenerator( seed() );
std::uniform_real_distribution<float> randomFloat0To1( 0.0f, 1.0f );

... from now on, each time you call 'randomFloat0To1( yourGenerator )' you get a number between [0.0f..1.0f) (open interval). See <random> docu for other distris/generators. It's cool.

Hope that wasn't total nonsense ?

Hi Green_Baron,

Thank you for your input.

Getting critiqued is the only way to improve
I have been thinking about fixing the random number generator, this is the old way and I know there is a newer one. I will dig into it. I primarly want to understand eventual performance costs that using a new number generator could incur. My gut feeling is that there should be none.

I will come back to read these comments and look over the code and tutorial again, perhaps make some modifications. I got quite a lot of ground to clear yet though - collision detection/managment, quad trees and an A* path finding code that needs to be put, scenario tests and performance tests. And probably something more
I really want to get a PBR-lighting shader in there as well, as the current one is a very old version of lighting.

But I'm looking at all comments here and will at some point try to address them all. So thanks for the input!
 

I wanted to get it online and get more eyes on it as soon as possible in case I made some unreasonable design decisions or missed out on something that could obviously be done in a better way. So far it seems like I have'nt done anything to crazy

Again, thanks for the input!

You probably know. Since you're using GLEW and GLFW, how about debug context. Source glfw doc and GLSL Cookbook. Yeah, i confess :-)). But context must 4.3 ! Or .2 ? Whatever ...

In your window object or function or somesuch:

	glfwWindowHint( GLFW_OPENGL_DEBUG_CONTEXT, GLFW_TRUE );
/*
 * open window and such
*/
// Initialize debug output
	GLint flags;
	glGetIntegerv( GL_CONTEXT_FLAGS, &flags );
	if( flags & GL_CONTEXT_FLAG_DEBUG_BIT ) {
		glEnable( GL_DEBUG_OUTPUT );
		// Envoke callback directly in case of error
	    glEnable( GL_DEBUG_OUTPUT_SYNCHRONOUS );
	    glDebugMessageCallback( glDebugOutput, nullptr );
	    glDebugMessageControl( GL_DONT_CARE, GL_DONT_CARE, GL_DONT_CARE, 0, nullptr, GL_TRUE );
	    Logbook::getInstance().logMsg( Logbook::WINDOW, Logbook::INFO, "Debug context created." );
	} else
		Logbook::getInstance().logMsg( Logbook::WINDOW, Logbook::WARNING,
				"Debug context not created. Continuing without debug messages." );

Declarations:

    static void errorCallback( int error, const char *msg );

    static void APIENTRY glDebugOutput( GLenum source, GLenum type, GLuint id, GLenum severity,
    		GLsizei length, const GLchar *message, const void *userParam );

... and definitions (just replace my logbook stuff)

// static
void GlfwWindow::errorCallback( int error, const char *msg ) {
	std::string s{ " [" + std::to_string(error) + "] " + msg };
	Logbook::getInstance().logMsg( Logbook::WINDOW, Logbook::ERROR, s );
	throw std::runtime_error( "GLFW window error, see logbook" );
}

// static
void APIENTRY GlfwWindow::glDebugOutput( GLenum source, GLenum type, GLuint id, GLenum severity,
		GLsizei length, const GLchar *message, const void *userParam ) {

    std::string s{ "Debug message (" + std::to_string(id) + "): " };
    switch( source ) {
        case GL_DEBUG_SOURCE_API:             s += "Source: API"; break;
        case GL_DEBUG_SOURCE_WINDOW_SYSTEM:   s += "Source: Window System"; break;
        case GL_DEBUG_SOURCE_SHADER_COMPILER: s += "Source: Shader Compiler"; break;
        case GL_DEBUG_SOURCE_THIRD_PARTY:     s += "Source: Third Party"; break;
        case GL_DEBUG_SOURCE_APPLICATION:     s += "Source: Application"; break;
        case GL_DEBUG_SOURCE_OTHER:           s += "Source: Other"; break;
    }
    s += "; ";

    switch( type ) {
        case GL_DEBUG_TYPE_ERROR:               s += "Type: Error"; break;
        case GL_DEBUG_TYPE_DEPRECATED_BEHAVIOR: s += "Type: Deprecated Behaviour"; break;
        case GL_DEBUG_TYPE_UNDEFINED_BEHAVIOR:  s += "Type: Undefined Behaviour"; break;
        case GL_DEBUG_TYPE_PORTABILITY:         s += "Type: Portability"; break;
        case GL_DEBUG_TYPE_PERFORMANCE:         s += "Type: Performance"; break;
        case GL_DEBUG_TYPE_MARKER:              s += "Type: Marker"; break;
        case GL_DEBUG_TYPE_PUSH_GROUP:          s += "Type: Push Group"; break;
        case GL_DEBUG_TYPE_POP_GROUP:           s += "Type: Pop Group"; break;
        case GL_DEBUG_TYPE_OTHER:               s += "Type: Other"; break;
    }
    s += "; ";

    switch( severity ) {
        case GL_DEBUG_SEVERITY_HIGH:         s += "Severity: high"; break;
        case GL_DEBUG_SEVERITY_MEDIUM:       s += "Severity: medium"; break;
        case GL_DEBUG_SEVERITY_LOW:          s += "Severity: low"; break;
        case GL_DEBUG_SEVERITY_NOTIFICATION: s += "Severity: notification"; break;
    }
    s += "; ";
    s += message;

    Logbook::getInstance().logMsg( Logbook::RENDERER, Logbook::LOG, s );
}	// glDebugOutput()