Sign in to follow this  

Game Engine using SDL 2

This topic is 1109 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

Your math.h should have precomputed value :

const float DEG2RAD = 0.01745329251994329576923690768f;
const float RAD2DEG = 57.2957795130823208767981548141f;

inline float ToRadian( const float Degree )
{
  return ( Degree * DEG2RAD );
}

inline float ToDegree( const float Radian )
{
  return ( Radian * RAD2DEG );
}

You can clean your main.cpp, but it's personal comment, if you release, you have to clean, it's not a big code source so it's not a big effort.

You can optimize your vector2.h and vector3.h a lot using *= versus /= and avoiding return in CreateVector using reference as output.

I saw the same thing on different files who can be changed, output who has to be reference output and not returned.

This kind of optimization is important if the function is called each frame or often.

Now what I have to say about the all stuff is it's not a game engine for me but a framework actually.

But it can't be really used by other because you don't have any doc and it needs to be clean on different places.

Edited by Alundra

Share this post


Link to post
Share on other sites

Please, move the code out of the headers.  Every file that includes a header will include all the code as well.  That means you'll have multiple instances of the same code included over and over.

Just declare the classes and API's in the include files, and have the source code be available via a library, or at least separate .cpp source files.

Share this post


Link to post
Share on other sites

It's true, I didn't noticed that the first view. It's very bad coding style.

When you will move all the code, if you have function who is just "get" or "set", let them inline.

It's a hint for the compiler but you know these function will be inlined so it's ok to let them in .h inlined.

Share this post


Link to post
Share on other sites

You can remove a lot of new/delete usage from your classes, e.g.

class Global
{
public:
	Global(SDL_Window& window, SDL_Renderer& renderer, Logger& logger)
		: input()
		, screen(window, renderer)
		, logger(logger)
		, audio(logger)
		, gfx(window, renderer, logger)
	{
	}

	Input input;
	Screen screen;
	Logger& logger;
	AudioManager audio;
	GraphicsManager gfx;
};

This is exactly equivalent to your original code, but removes the need for you to remember to write a destructor that deletes everything (and the chance that you'll forget to add a matching delete for every new)... The member variables will be constructed automatically when a Global object is constructed, and will be destructed automatically when that object is destructed.

 

You're also not following the rule of three -- if your class has a destructor, then it must also have a copy constructor and an assignment operator (the above re-write avoids all this hassle by just not having an explicit destructor cool.png).

Edited by Hodgman

Share this post


Link to post
Share on other sites
If you use C++ go with SFML instead.

If he used SFML, the result would have been the same because it's a question of coding style more than API used.

Another point is SFML is more a game framework for OpenGL, SDL can be just used for windowing on D3D/OGL.

But after that, it's just a comment because SDL or SFML can be used for Unix version only if needed.

 

You're also not following the rule of three

I always like when Hodgman speaks about rule of three, because it's not always knew and should be.

Edited by Alundra

Share this post


Link to post
Share on other sites

Thanks everyone I almost finished all of them! I'm currently working on seperaing the code into .h and .cpp files, it's the last thing. I would like to hear further tips!

Edited by kovacsmarcell99

Share this post


Link to post
Share on other sites

Hey everyone! I've done some work on the engine in the last half year currently I am adding the .cpp files. Could you guys give some further tips? (Hint: I've redone the logging pls check it. Is it good?)

Share this post


Link to post
Share on other sites


Hey everyone! I've done some work on the engine in the last half year currently I am adding the .cpp files. Could you guys give some further tips? (Hint: I've redone the logging pls check it. Is it good?)

 

Sure, here's a few things I noticed:

	//TODO: This MAY cause problems!
	bool isOpen = (bool)(in);

See http://www.cplusplus.com/reference/fstream/ifstream/is_open/

	Logger() {};
	Logger(Logger const&){};
	Logger& operator=(Logger const&){};

If your compiler supports C++11, consider using = default; or = delete;  See http://en.wikipedia.org/wiki/C%2B%2B11#Explicitly_defaulted_and_deleted_special_member_functions

	static Logger* Instance();
	static void ResetInstance();

Consider making a templated Singleton class so you don't need to re-write this for every singleton you have.  Personally, I try to stay away from them but sometimes they're appropriate (ie. a Logger class is quite appropriate).

#define LOG(Message_) Logger::Instance()->LogLine(static_cast<std::ostringstream&>(std::ostringstream().flush() << Message_).str());

Using a preprocessor to wrap any debug/log statements is a great idea, as you can easily have the #define statement do nothing for certain builds.  However, consider utilizing the __FILE__ and __LINE__ macros to see where messages are popping up.  This will help in the long run.  (note that __LINE__ is an int).

inline std::string GetClockTime(void)

Consider making global functions as static functions in a class so you don't pollute the global namespace, and I don't seem to see any namespaces anywhere.  Also, I assume this is C++ since you're using C++ tools, so remove (void) as it does nothing in C++ and only clutters things.

if (logNumber > 500)

500 is a magic number to me.  Consider creating a constant that makes more sense to an external viewer.

	if (!instance)
		instance = new Logger();

At first, I was a believer of the "no braces for a single statement if statement" but unfortunately, it can be error prone (an error may only come up 1 in 1000 of these situations, but even that small chance is worth the coding style change), so consider always wrapping statements in { } braces.

	instance = NULL;

Consider using nullptr. See http://en.wikipedia.org/wiki/C%2B%2B11#Null_pointer_constant

Logger* Logger::Instance()
{
	if (!instance)
		instance = new Logger();

	return instance;
}

Consider returning a reference for a function if it's guaranteed that the item it returns will always exist.  Only use a raw pointer if you cannot guarantee that the returned item will exist.

 

I also implemented my log/debug functions as functions that take a std::string as a parameter, but I probably should have just overloaded the << operator instead, so you may want to try that out as well.

 

I was just looking in TextureManager as well, and for your sanity, use typedefs or using = SomeC++11Type or using = SomePreC++11Type.  I'm referring to this:


#ifdef CPP11_SUPPORT
	std::unordered_map<std::string, SDL_Texture*> textures;

	std::unordered_map<std::string, bool> errorShown;
#else
	std::map<std::string, SDL_Texture*> textures;

	std::map<std::string, bool> errorShown;
#endif

I hope this helps.

Share this post


Link to post
Share on other sites

 



Hey everyone! I've done some work on the engine in the last half year currently I am adding the .cpp files. Could you guys give some further tips? (Hint: I've redone the logging pls check it. Is it good?)

 

Sure, here's a few things I noticed:

	//TODO: This MAY cause problems!
	bool isOpen = (bool)(in);

See http://www.cplusplus.com/reference/fstream/ifstream/is_open/

	Logger() {};
	Logger(Logger const&){};
	Logger& operator=(Logger const&){};

If your compiler supports C++11, consider using = default; or = delete;  See http://en.wikipedia.org/wiki/C%2B%2B11#Explicitly_defaulted_and_deleted_special_member_functions

	static Logger* Instance();
	static void ResetInstance();

Consider making a templated Singleton class so you don't need to re-write this for every singleton you have.  Personally, I try to stay away from them but sometimes they're appropriate (ie. a Logger class is quite appropriate).

#define LOG(Message_) Logger::Instance()->LogLine(static_cast<std::ostringstream&>(std::ostringstream().flush() << Message_).str());

Using a preprocessor to wrap any debug/log statements is a great idea, as you can easily have the #define statement do nothing for certain builds.  However, consider utilizing the __FILE__ and __LINE__ macros to see where messages are popping up.  This will help in the long run.  (note that __LINE__ is an int).

inline std::string GetClockTime(void)

Consider making global functions as static functions in a class so you don't pollute the global namespace, and I don't seem to see any namespaces anywhere.  Also, I assume this is C++ since you're using C++ tools, so remove (void) as it does nothing in C++ and only clutters things.

if (logNumber > 500)

500 is a magic number to me.  Consider creating a constant that makes more sense to an external viewer.

	if (!instance)
		instance = new Logger();

At first, I was a believer of the "no braces for a single statement if statement" but unfortunately, it can be error prone (an error may only come up 1 in 1000 of these situations, but even that small chance is worth the coding style change), so consider always wrapping statements in { } braces.

	instance = NULL;

Consider using nullptr. See http://en.wikipedia.org/wiki/C%2B%2B11#Null_pointer_constant

Logger* Logger::Instance()
{
	if (!instance)
		instance = new Logger();

	return instance;
}

Consider returning a reference for a function if it's guaranteed that the item it returns will always exist.  Only use a raw pointer if you cannot guarantee that the returned item will exist.

 

I also implemented my log/debug functions as functions that take a std::string as a parameter, but I probably should have just overloaded the << operator instead, so you may want to try that out as well.

 

I was just looking in TextureManager as well, and for your sanity, use typedefs or using = SomeC++11Type or using = SomePreC++11Type.  I'm referring to this:


#ifdef CPP11_SUPPORT
	std::unordered_map<std::string, SDL_Texture*> textures;

	std::unordered_map<std::string, bool> errorShown;
#else
	std::map<std::string, SDL_Texture*> textures;

	std::map<std::string, bool> errorShown;
#endif

I hope this helps.

 

Thank you very much!

Share this post


Link to post
Share on other sites

This topic is 1109 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this