I made this Renderer class recently, to simplify the user interface of my library's API. I would like to ask advice about the move semantics (copy constructor, std::unique_ptr, ...), or any other things you can point out to improve my code.

As you can see, I take in parameters in RenderObject in the form of a pointer. This is simply because Material uses a Texture class, whose assignment operators are disabled. It only has a move constructor (C++11), but Material cannot use it.

2 Answers
2

Resource management:

I general, the code seems alright to me, but one thing you continue to do, if I recall from your previous questions, is to use raw pointers and manual resource/memory management. So I'd like to urge you to look into the standard smart pointers and start using them.

The basic usage is:

Resource is shared between instances of different objects: Frequent case for things like textures, materials and shaders. Use a shared_ptr for them.

Resource is explicitly owned by an instance of an object: In a renderer, this could apply to many types of things, from light sources to object transforms. Use a unique_ptr for those.

A few other things:

RenderObject is just a data container with a bunch of accessors. When you get to this point, it might be worth considering just making it a plain struct with all fields public, since there is no difference regarding encapsulation when you can freely get/set all fields. In such cases, the accessors only add a level of unnecessary indirection. However, you might also consider a redesign altogether, since the heavy use of accessors indicate that all the algorithms involving a RenderObject are outside the class, which in turn indicate that it is probably strongly coupled with other areas of the code and systems.

So, if it is not necessary, as you say, why is it there? It would be safer indeed if the destructor of Texture did the cleanup, so you should probably avoid exposing a public Dispose() method anyways, to prevent making the mistake of having a disposed/invalid object in your hands. This is specially true for library code, make it as hard as possible for the user to shoot himself in the foot (dedicated users will still manage, but don't facilitate ;)).

The fact that the constructor of Material is templated for a texture type puzzles me:

template<typename T>
Material(T&& texture, float shininess, ...

The member m_texture is of type Texture, so unless there's some polymorphic conversion going on here, the template parameter should just be a Texture &&.

Don't this-> qualify member variables, ever! We have a very amusing example right here on CR of how using it can lead to some pretty hilarious bugs.

Avoid writing the empty constructors/destructors and let the compiler do its job providing the empty defaults. But in particular, don't do this:

~ForwardRenderer(){}///default dtor

Yep, anyone that has been programming in C++ for more than two days knows what a destructor looks like. Unless you're using the comment for trolling purposes[*], trim it down.

std::vector<RenderObject>* m_pobjects in ForwardRenderer seems like it could be declared by value. Then change the constructor to move the parameter or take it by && move-reference. The less pointer chasing the better when it comes to performance critical applications like real-time rendering. Make good use your data cache.

[*]: I recall once reading a comment just above a class destructor that said something like "If you don't know what this is, then you're probably going to get fired..." -- Obviously the author was being funny, and this kind of internal jokes can help bootring the morale of a team, but otherwise, never comment explaining obvious aspects of the language.

\$\begingroup\$Actually, Texture has a Dispose() method, which gets called automatically called by the destructor.... but, the user can call it manually, and then, the destructor will not call Dispose() twice ! -- This is basically usefull if the user wants to delete a big object (like a Shader) before the variable goes out of scope.\$\endgroup\$
– MattMattJul 26 '15 at 8:18

\$\begingroup\$2) About smart pointers... All my opengl classes have their assignment operators deleted; but they have move constructors. So the question would be that should I typedef all my opengl classes to a unique_ptr ? like typedef shared_ptr<__classMesh> Mesh; this would be ton of work though...\$\endgroup\$
– MattMattJul 26 '15 at 8:20

Otherwise, some maintainer like me is going to come behind you and remove what looks like dead code.

Now, there seems to be some debate about whether or not to declare empty finalizers, but I found a great analysis of the topic that you may be interested in. The TL;DR version of that piece is below.

To be able to quickly decide whether a class needs an empty non-inline destructor definition I condensed the above analysis into a short checklist. When designing a class interface, ask yourself the following three questions:

Do you need to anchor the vtbl (doesn’t apply to class templates)?

Does proper destruction of data members require additional declarations or functionality that is not available in the class interface? Does the destruction need to be done consistently with construction (e.g., using the same runtime)?

Do you need to define a stable interface and chances are that later you may have to add some functionality to the destructor?

If the answers to all these questions are “No”, then let the compiler provide the default implementation of the destructor.

\$\begingroup\$Hehe, I realize this is an extended comment to my answer, but bare in mid that I was explicitly talking about the constructor/destructor pair. If it was a normal method instead left empty, the comment would be just fine. I shall read the link you've provided in more detail. Thanks for bringing it up ;)\$\endgroup\$
– glampertJul 26 '15 at 2:14

\$\begingroup\$Actually, I started to write this before I saw your answer @glampert. Lol\$\endgroup\$
– RubberDuckJul 26 '15 at 2:15

\$\begingroup\$Oh. @glampert my point wasn't bad comments, but that if there's a reason to have an empty method, you have to leave a comment explaining why. Otherwise, someone like me will see this and go "Wtf? Dead code." And then poof it's gone.\$\endgroup\$
– RubberDuckJul 26 '15 at 2:17

1

\$\begingroup\$Yep, I also got ahead of myself :P, but good point, for sure.\$\endgroup\$
– glampertJul 26 '15 at 2:19