Welcome, Guest. Please login or register. Did you miss your activation email?

Author Topic: Check out my Engine!  (Read 10758 times)

0 Members and 1 Guest are viewing this topic.

AndrewB

  • Newbie
  • *
  • Posts: 33
    • View Profile
Check out my Engine!
« on: July 25, 2013, 09:36:32 am »
Hey Guys,

I've decided to scrap what I have on my current project and start again. I'm making the new project open source to hopefully force myself to think about other developers and keep my code neat and readable.

Here is a link to the GitHub project: https://github.com/AndrewBerry/R9/
First commit of the engine working: https://github.com/AndrewBerry/R9/tree/cd4a3547cec9e1312b9c35db66a7ba3838ed6850

Any feedback is greatly appreciated, I've looked over this code a million times -- it would really help to get a second pair of eyes on it.

Thanks, Andrew

Grimshaw

  • Hero Member
  • *****
  • Posts: 631
  • Nephilim SDK
    • View Profile
Re: Check out my Engine!
« Reply #1 on: July 25, 2013, 10:47:12 am »
Hmm, can we see some screenshots of what it can do instead? :D

Nexus

  • SFML Team
  • Hero Member
  • *****
  • Posts: 6287
  • Thor Developer
    • View Profile
    • Bromeon
Re: Check out my Engine!
« Reply #2 on: July 25, 2013, 12:15:48 pm »
Some tips:
  • c class prefixes are questionable -- simply omit them, the class names are descriptive enough
  • Use constructor initializers list instead of assignments
  • Perform initialization in constructor instead of public Initiate() method
  • Use RAII (std::unique_ptr) instead of manual memory management (new/delete)
  • Consider the rule of three (no need to declare destructor with RAII)
  • Methods don't need to end with semicolon
  • Identifiers like __ENGINE_HPP__ are reserved (two underscores or underscore + uppercase letter is forbidden)
  • Don't include whole <SFML/Graphics.hpp> header, rather the specific parts you need
  • Some headers are not used, e.g. <iostream>
  • Use forward declarations to reduce compile-time dependencies
  • For better encapsulation, make variables private instead of protected; use protected methods
  • void in empty parameter lists is unusual in C++ (it's a relict from C)
  • Avoid static variables in functions if you actually need member variables
  • Types are not consistently named (exitData_t vs. cEngine) -- I wouldn't try to follow the standard library with _t postfixes
  • Use type inference: replace std::map< std::string, cGenericScene* >::iterator with auto
  • Avoid C and function-style casts, especially where no conversion is necessary: exitData = cGenericScene::exitData_t( cGenericScene::EXIT_CONTINUE );
Zloxx II: action platformer
Thor Library: particle systems, animations, dot products, ...
SFML Game Development:

AndrewB

  • Newbie
  • *
  • Posts: 33
    • View Profile
Re: Check out my Engine!
« Reply #3 on: July 25, 2013, 01:03:58 pm »
Some tips:
  • c class prefixes are questionable -- simply omit them, the class names are descriptive enough
  • Use constructor initializers list instead of assignments
  • Perform initialization in constructor instead of public Initiate() method
  • Use RAII (std::unique_ptr) instead of manual memory management (new/delete)
  • Consider the rule of three (no need to declare destructor with RAII)
  • Methods don't need to end with semicolon
  • Identifiers like __ENGINE_HPP__ are reserved (two underscores or underscore + uppercase letter is forbidden)
  • Don't include whole <SFML/Graphics.hpp> header, rather the specific parts you need
  • Some headers are not used, e.g. <iostream>
  • Use forward declarations to reduce compile-time dependencies
  • For better encapsulation, make variables private instead of protected; use protected methods
  • void in empty parameter lists is unusual in C++ (it's a relict from C)
  • Avoid static variables in functions if you actually need member variables
  • Types are not consistently named (exitData_t vs. cEngine) -- I wouldn't try to follow the standard library with _t postfixes
  • Use type inference: replace std::map< std::string, cGenericScene* >::iterator with auto
  • Avoid C and function-style casts, especially where no conversion is necessary: exitData = cGenericScene::exitData_t( cGenericScene::EXIT_CONTINUE );
Thanks Nexus, I've read over what you've said and changed what I have.

https://github.com/AndrewBerry/R9/tree/580c69f6e885e07567a180407235aec5475b2cc5
How's that look?

zsbzsb

  • Hero Member
  • *****
  • Posts: 1409
  • Active Maintainer of CSFML/SFML.NET
    • View Profile
    • My little corner...
    • Email
Re: Check out my Engine!
« Reply #4 on: July 25, 2013, 01:31:11 pm »
You still need to consider dropping the new and delete statements. For example change this in your tengine...

private:
    sf::RenderWindow *renderWindow;
public:
tEngine::tEngine() :
    currentScene( nullptr ),
    renderWindow( new sf::RenderWindow( sf::VideoMode( 800, 600 ), "R9" ) )
{
};
 

to

private:
    sf::RenderWindow renderWindow;
public:
tEngine::tEngine() :
    currentScene( nullptr ),
    renderWindow( sf::RenderWindow( sf::VideoMode( 800, 600 ), "R9" ) )
{
};
 
Motion / MotionNET - Complete video / audio playback for SFML / SFML.NET

NetEXT - An SFML.NET Extension Library based on Thor

Nexus

  • SFML Team
  • Hero Member
  • *****
  • Posts: 6287
  • Thor Developer
    • View Profile
    • Bromeon
Re: Check out my Engine!
« Reply #5 on: July 25, 2013, 01:36:59 pm »
You misunderstood me with the prefix, "t" is not any better than "c". You need no prefix at all, types are already uniquely recognizable because they are written in uppercase letters. Functions have the same naming convention, but are recognizable because of the parentheses. Therefore, simply use Engine, GenericScene, etc.

You still use manual memory management. Use automatic variables, and -- where necessary for polymorphism -- std::unique_ptr. Raw pointers should never own their objects. You can do it as follows. If you want, create a typedef for the unique pointer.
class Engine {
    ...
private:
    std::map< std::string, std::unique_ptr<GenericScene> > scenes;
    sf::RenderWindow                                       renderWindow;
    std::unique_ptr<GenericScene>                          currentScene;
};

...
Engine::Engine() :
    currentScene( nullptr ),
    renderWindow( sf::VideoMode( 800, 600 ), "R9" )
{
}

There are still some suggestions not applied yet (e.g. reserved identifiers, rule of three, static...), maybe read the list carefully again :)
« Last Edit: July 25, 2013, 01:41:13 pm by Nexus »
Zloxx II: action platformer
Thor Library: particle systems, animations, dot products, ...
SFML Game Development:

FRex

  • Hero Member
  • *****
  • Posts: 1848
  • Back to C++ gamedev with SFML in May 2023
    • View Profile
    • Email
Re: Check out my Engine!
« Reply #6 on: July 25, 2013, 02:50:13 pm »
Quote
Identifiers like __ENGINE_HPP__ are reserved (two underscores or underscore + uppercase letter is forbidden)
This is surprisingly common, Doom 3, Irrlicht, Ogre, I'm sure there is much more.
Back to C++ gamedev with SFML in May 2023

eXpl0it3r

  • SFML Team
  • Hero Member
  • *****
  • Posts: 11017
    • View Profile
    • development blog
    • Email
Re: Check out my Engine!
« Reply #7 on: July 25, 2013, 03:11:23 pm »
This is surprisingly common, Doom 3, Irrlicht, Ogre, I'm sure there is much more.
So is a lot of other bad practice in programming with C++. ;)
Official FAQ: https://www.sfml-dev.org/faq.php
Official Discord Server: https://discord.gg/nr4X7Fh
——————————————————————
Dev Blog: https://duerrenberger.dev/blog/

Nexus

  • SFML Team
  • Hero Member
  • *****
  • Posts: 6287
  • Thor Developer
    • View Profile
    • Bromeon
Re: Check out my Engine!
« Reply #8 on: July 25, 2013, 03:48:48 pm »
eXpl0it3r is right. Common doesn't imply correct.

I don't know who initially used __MACROS_LIKE_THIS__, but it seems like bad ideas spread much easier than good ones. Same for Hungarian Notation, manual memory management, performance myths, STL myths, wheel reinventions, singletons, ...

As already mentioned elsewhere, Irrlicht and Ogre are not necessarily good examples for clean code. At least not in 2013. C++ is one of the few programming languages where paradigms and idioms have fundamentally changed during the last decade.
Zloxx II: action platformer
Thor Library: particle systems, animations, dot products, ...
SFML Game Development:

FRex

  • Hero Member
  • *****
  • Posts: 1848
  • Back to C++ gamedev with SFML in May 2023
    • View Profile
    • Email
Re: Check out my Engine!
« Reply #9 on: July 25, 2013, 05:00:04 pm »
I know but many people(including cs teachers and students) don't care(or know) about things like that, it works here(ie. on one compiler, one system), the end, passed the class, hooray I'm a c++ programmer! Eh.. I've seen someone take out all deletes(because of course the course can't just go with RAII instead of state-of-the-art-of-90s c++98 and c89 and manual memory managment) out of his program to get under passing time threshold. I have to admit that's a killer idea that never crossed my mind... Of course everything is self cooked, often even STL is discouraged, because it's to test student's knowledge, not the fact they can use standard library or other libraries.. Maybe I just had bad experiences but no one checks students code for semi-arcane voodoo bullshit or they check it in a bitching way about lack of comments(seriously? that actually was about my reimplementation of std::vector(there was 50% points cut for using STL), I WONDER WHAT DOES getSize() and pushBack() do.. ffs..), style(padding with spaces or not), naming conventions(ANY is good as long as it's consistent within entire codebase) etc. On the plus side, hopefully c++11 and 16 will slowly keep growing and in the end there will be tons of sub standard programmers that write comments like (int i=0;//new local int i, initialized to zero) and can't use literally any library and self cook inefficient unsafe solutions, which will make us the gurus of the time. ;D
I get daydreamy lately.. :)
Back to C++ gamedev with SFML in May 2023

Atani

  • Newbie
  • *
  • Posts: 30
    • MSN Messenger - atanisoft@hotmail.com
    • AOL Instant Messenger - AtaniSYSOP
    • Yahoo Instant Messenger - atanisoft
    • View Profile
    • Email
Re: Check out my Engine!
« Reply #10 on: July 25, 2013, 05:45:22 pm »
On the plus side, hopefully c++11 and 16 will slowly keep growing and in the end there will be tons of sub standard programmers that write comments like (int i=0;//new local int i, initialized to zero) and can't use literally any library and self cook inefficient unsafe solutions, which will make us the gurus of the time. ;D

You must be new here ... :)  It has already started.. If you look at the general code quality produced today by recent grads vs seasoned devs you will notice a serious lack of standards.. However, I would love to see comments like that vs none at all!  At least that comment would show the dev knows something about what their program is going to do.

Isn't the pattern of:
#ifndef FOO
#define FOO
... code ...
#endif
in headers a bit of a relic of C days that was replaced with patterns like:
#pragma once
... code ...

binary1248

  • SFML Team
  • Hero Member
  • *****
  • Posts: 1405
  • I am awesome.
    • View Profile
    • The server that really shouldn't be running
Re: Check out my Engine!
« Reply #11 on: July 25, 2013, 05:51:19 pm »
Testing a student's aptitude and teaching them how to program efficiently should not be done at the same time. At least at my university, we have at least 5 modules on programming/software development, each refining the knowledge of the previous one. Going higher and higher through the layers of abstraction. Squeezing all that content into a single module leads to that famous "implement your own list even though STL has one" scenario. We got to implement our own lists too, but in the context of C not C++ or any other OOP language the provides them already. Care was taken to also make sure no bad habits are created, penalizing us for not free()ing memory in C etc. although it would not have made a difference to a lazy programmer.

I don't know whether I just got lucky to be at my university, but I assume that given enough effort and good will, even with the crappiest courses you can become a pretty decent software developer if you care enough. The reverse is also true, even if you have the best courses in the world but don't give a damn about clean code, god help you.

The funny thing at universities is that, in the courses for students, they preach all the good stuff, but if you look at the code they use as part of their research you can hardly believe it was them who wrote it ::). Must be the time/financial pressure :P. This is probably also the reason all the bad stuff you listed made it into those other libraries. They wanted to get something out the door fast, no matter what. And since refactoring takes time away from "productive development" nobody ever cares once the codebase is there. Yes, management often doesn't care about RAII, and don't even try convincing them, you are better off refactoring yourself without their consent.

Isn't the pattern of:
#ifndef FOO
#define FOO
... code ...
#endif
in headers a bit of a relic of C days that was replaced with patterns like:
#pragma once
... code ...
That is a tricky one, because technically, #pragma once isn't standardized so if you want to get code to compile on some arcane (but standards compliant, like that even exists) compiler, you are safer using the first variant.
SFGUI # SFNUL # GLS # Wyrm <- Why do I waste my time on such a useless project? Because I am awesome (first meaning).

eXpl0it3r

  • SFML Team
  • Hero Member
  • *****
  • Posts: 11017
    • View Profile
    • development blog
    • Email
Re: Check out my Engine!
« Reply #12 on: July 25, 2013, 05:57:41 pm »
... cs teachers and students ... course ... student's ...
Yep you should get back on ground, the whole world does not evolve around univerity classes. I'd claim that only a small percentage of the notable programmers have learned actual programming in the uni. The job of CS classes is not to teach you how to program for the industry, but it's a way to get back around to the theoretical parts, so you understand what's going on and getting into the whole "thinking in binary".
Sure it would be nice if the profs would up their style and knowledge so that the students could fully profit from the course, but it's necessary for the given task.
Besides that, it's impossible to teach C++ in a good manner while keeping up with the actual topic at hand in one semester, unless you'll have a "Introduction to programming" course for every single programming language ;)

Isn't the pattern of:
#ifndef FOO
#define FOO
... code ...
#endif
in headers a bit of a relic of C days that was replaced with patterns like:
#pragma once
... code ...
I have to disagree! While the #ifndef #define #endif pattern is accepted by the C++ standard #pragma once is not and you'll have no guarantee that your compiler is supporting that feature. True it's much nicer to see and write and as a matter of fact I'm using it as well, but it's no where near an indicator for better written code. ;)
Official FAQ: https://www.sfml-dev.org/faq.php
Official Discord Server: https://discord.gg/nr4X7Fh
——————————————————————
Dev Blog: https://duerrenberger.dev/blog/

Atani

  • Newbie
  • *
  • Posts: 30
    • MSN Messenger - atanisoft@hotmail.com
    • AOL Instant Messenger - AtaniSYSOP
    • Yahoo Instant Messenger - atanisoft
    • View Profile
    • Email
Re: Check out my Engine!
« Reply #13 on: July 25, 2013, 07:44:50 pm »
I have to disagree! While the #ifndef #define #endif pattern is accepted by the C++ standard #pragma once is not and you'll have no guarantee that your compiler is supporting that feature. True it's much nicer to see and write and as a matter of fact I'm using it as well, but it's no where near an indicator for better written code. ;)
By far, it does not make the code any different between the two options outside of 2+ fewer lines.
That is a tricky one, because technically, #pragma once isn't standardized so if you want to get code to compile on some arcane (but standards compliant, like that even exists) compiler, you are safer using the first variant.


Now, re support of this in the compiler/standards..  It is not part of the standard (I thought they put it on a list for Cx10/11 but its not there so oh well)... However, a lot of the compilers out there support it... I would advocate using it over the header guards since the compiler can optimize away recompilation of the unit (not all compilers do that optimization though).

netrick

  • Full Member
  • ***
  • Posts: 174
    • View Profile
Re: Check out my Engine!
« Reply #14 on: July 26, 2013, 02:52:46 pm »
Some tips:
  • Use constructor initializers list instead of assignments
Why? Consider this C++11 example.

class example
{
public:
void foo();
private:
int a = 0;
bool b = false;
std::string name = "default";
};
 
Faster and easier solution than initializers lists. C++11 is a big step forward. Using that, if you want to check/change the value you simply look at the variable declaration. And you don't produce unnecessary boilerplate code.

Of course there are cases when you must use initializers list, but generally I think that assignments are better.
« Last Edit: July 26, 2013, 03:00:59 pm by netrick »