[quote name='SteveDeFacto' timestamp='1314146651' post='4853032']
[quote name='phantom' timestamp='1314135653' post='4852972']
[quote name='SteveDeFacto' timestamp='1314135264' post='4852969']
There are many many features not demonstrated in this test. Also it's loading an FBX file not a BSP file.

Yes, but that doesn't answer my question; what makes it 'next generation'?
[/quote]

It's not... Yet...
[/quote]

I see... *looks at 'about' box on website*
I see...

OK, so what planned features do you have which make it 'next generation'?
[/quote]

http://ovgl.org/view_topic.php?topic=91JL96IHFS

I have mixed feelings about this project.


On the one hand, it is certainly commendable that you are putting forth such consistent effort on such a large scale project. Most people don't have it in them to stick with things like this long enough to get results.

On the flip side, though, I kind of agree with Phantom, although I'm a little more understanding about your own opinion of your work. When I was a much younger and less experienced programmer, I also over-described my projects and over-inflated the complexity, richness, and polish of what I worked on. So I totally know what it feels like; you've done something significant, which by all means you should be proud of - but not too proud.

Realism is important, and humility is nothing short of vital, especially when you start opening your work to external criticism. Remember that you are in a position where you need feedback, not sales; overselling your work is going to harm you far more than it will help. What does it gain if people think your project is really cool at first, but never use it because it can't deliver on your promises? Conversely, what do you have to lose if people try your project with low expectations and are pleasantly surprised by what they find?

It is always better to underpromise and overdeliver than vice versa.


You cannot possibly compete with the large teams of people who are making actual next-generation game engines. And even if you could, you wouldn't have to post about it on GDNet - your work would speak so loudly for itself that we'd almost certainly be talking about it already. So by projecting this image like your lone-wolf operation is going to beat out id or Crytek, you're really just inviting a lot of skepticism and dismissal from the people you should be courting.


I haven't had a chance to look at the code itself, so my opinion may change for the better, but judging from your posting history around here I wouldn't exactly jump at the chance to commit my next title to your code base. Again, I don't want to minimize your accomplishment, or sound too critical; you've got a lot of good stuff done. But my gut is that there's still a huge amount of room for improvement.

Here's some accumulated thoughts after looking at your project:

• I hope to God your code is better than your web design. Seriously, find someone with some actual graphic design experience and at least get some pointers on how to improve your site. Remember, your site is going to be your first impression for a lot of people, and if that first impression is less than stellar, you're going to lose a lot of potential interest right then and there.
• Your downloads page is depressing. For claiming to be cross-platform, not even having Linux or Mac downloads is an instant loser in my book. If you can't actually support multiple platforms right now and prove it, then don't advertise as such.
• Never post links if they are "deprecated" or "outdated." If people shouldn't be downloading that code, don't link it, period. I should not have to wade through a list of links to find the "right thing to do." You should make the "right thing" the path of least resistance. I should click Downloads, Windows Snapshot, Install/Run. Nothing else. If I weren't committed to helping you by offering you a code review, I would have left your site already and made a mental note never to come back.
• Your Google Code page... uh... leaves a lot to be desired. Seriously, put some effort into this - it's what determines how many people will care about your project. Your code could be the best thing ever, but I'll never know as the average Joe stumbling across your site, because you look like you don't even care enough to properly describe your project on its own home page.
• Your examples exhibit a typical failure of example code. You put too much focus on brief, simple-looking demos and not enough focus on showing how a real project would use your library. Unless of course your "real" usages are full of poorly-factored functions, magic numbers, global variables, and other bad engineering practices - which I hope not, for your sake.
• I cherry-picked OvglMesh.cpp to look at, just because it seemed decently-sized and mesh handling is fairly core to any rendering engine. Following are a few shotgun observations from that file.
• Repeating your license at the top of every code file is just sloppy. Put it in trunk/License.txt and be done with it. If you ever decide to change license schemes, you're in for a headache. Also, as a reader of your code, I don't give a shit about your license. I want to know why OvglMesh.cpp exists and what I might find in there. This is probably the number one thing that people screw up with file-level documentation; don't give me legalese gibberish that I can find elsewhere, don't give me "@brief None" ever, and for the love of all things holy and sexy, please tell me what the point of the stinkin' file is in the first place.
• Your operator== and operator!= implementations look a bit bush-league. This is not a nice thing for me to see as the very first bit of your code that I review. Why not just return (memcmp(...) == 0); instead of the totally unnecessary if and else? This sort of verbose over-implementation of a very trivial code concept doesn't give me much hope for the rest of your code's quality. Now, I'm doing my best not to make snap judgments and just prematurely conclude that all of your code is this questionable, but the average reviewer may not be so gracious.
• Why do you explicitly qualify all of your namespaces instead of dropping in judicious using-declarations or explicitly wrapping the implementations in a namespace {} block? This is another newbie-level mistake in code organization and reflects poorly on your code quality once again.
• Your code formatting is inconsistent, particularly when it comes to spacing and indentation and brace styles. Pick a format and stick with it. This is probably the single biggest smell I detect in new programmers' work - if you aren't disciplined and consistent enough to format things the same way even in the same 20KB file, what else are you cutting corners on?
• Magic numbers are evil, and your code is littered with them. What does mesh->Simplify(100, 0) mean? That's just terrible style and, yet again, makes me question if I should even bother looking at the large-picture architectural design of your code. If the implementation is this sloppy, I'm probably not going to like what I find at a ten-thousand-foot perspective.
• Checking in code with commented-out lines to your version-control repository is the height of sloppy. If you don't need code, remove it. If you need it again in the future, pull it out of revision history. There is no excuse for commenting out large swaths of code and then committing that.
• You use std::vector<std::vector<Ovgl::Face>> (note the >> with no separating space) which is not strictly legal prior to C++11, and will make many compilers barf in odd situations.
• Your (lack of) naming conventions makes it extremely hard to tell what is a local variable versus a class member versus a global, just by glancing; I don't advocate Hungarian notation, but at least format names in a way that makes it easier to tell where they belong, e.g. lowercasenames for locals, CamelCaseNames for members, and UPPERCASENAMES for constants/macros/etc.
• Your code lacks even rudimentary exception safety, and the use of raw pointers and arrays is a sign of very poor C++ style, at best. At worst, it's a guarantee that your code is riddled with nasty and subtle bugs.
• Anyone who writes a C-style cast in a C++ program should be lashed repeatedly with a USB cable.
• Lots more magic numbers and no documentation. What the hell is the point of all that index shuffling going on in Mesh::Update()? Documentation is your friend!
• You use an awful lot of platform-dependent type definitions (e.g. DWORD) for claiming to be cross-platform code. Not good.
• For some reason I popped open OvglMesh.h to see something in there, and I'll switch to remarking on that next. I didn't finish the .cpp file because I figure you've got enough negativity to deal with already ;-)
• Your Clean enum has terribly named entries. Better hope you never need a different enum with similarly-named entries! Common convention on every team I've worked with is to prefix enum entries with the enum name itself, e.g. CLEAN_BROKEN_FACES.
• extern "C". Inside a namespace. Exporting classes with std::vector members. At this point, I give up; this is hopelessly broken code and it's a bloody miracle it works on Windows, and will certainly never work anywhere else.

I have reached the conclusion I was afraid of from the beginning: you're not nearly experienced enough as a programmer to be billing this as a next-generation game engine. Or a last-gen game engine. Or even a game engine, because it's barely a renderer wrapper for OpenGL. Your code is full of mistakes and poor practices and outright sins; you have a dearth of actual features; and nothing - from your demos, to your library implementations, to your website(s), to the design of the library itself - strikes me as anywhere close to production quality.


I don't want to be discouraging here. Think of it as tough love. Like I said before, you've done a lot of work and your results are certainly something to be proud of - to an extent. I believe that and stand by it.


But you have a really long way to go.

After reading Apoch's post I got curious and looked at the code. wow, he didn't exaggarate. Why in the world do you have "class Mesh" AND "class CMesh" ? That's just downright confusing. A few other hints:






Ovgl::Vector2 Ovgl::Vector2Set( float x, float y )
{
Ovgl::Vector2 out = {0};
out.x = x;
out.y = y;
return out;
}

This function (as well as all the similar ones for ever data type) are entirely superflous and inefficient. Look up "constructors."[font="Monaco,"] Also, no need to zero out the data if you are just about to overwrite it anyway, it's just wasted CPU cycles. [/font]
[font="Monaco,"]
[/font]
[font="Monaco,"]
Ovgl::Vector3 Ovgl::Vector3::operator * ( const Ovgl::Vector3& in ) const
{
Ovgl::Vector3 out = {0};
out.x = x * in.x;
out.y = y * in.y;
out.z = z * in.z;
return out;
}
[/font]
[font="Monaco,"]
[/font]
[font="Monaco,"]This will confuse every single person that uses your code, they will expect a cross product or a dot product (which is why, in my code, I never overload the * or / operator for vectors because it's just so ambiguous)....and basically everything else Apoch said about inconsistencies and inefficiencies. You may want to read a good book or two on C++ since you seem to be missing out on some very basic concepts (like constructors).[/font]

• I hope to God your code is better than your web design. Seriously, find someone with some actual graphic design experience and at least get some pointers on how to improve your site. Remember, your site is going to be your first impression for a lot of people, and if that first impression is less than stellar, you're going to lose a lot of potential interest right then and there.

[/quote]

Coded it in notepad, Made UI images in GIMP, and uploaded it with FileZiilla. Not going to work on it until Ovgl is at least in beta and has all major features implemented.

• Your downloads page is depressing. For claiming to be cross-platform, not even having Linux or Mac downloads is an instant loser in my book. If you can't actually support multiple platforms right now and prove it, then don't advertise as such.

[/quote]

How many open source projects do you know of actually keep up to date binaries on their downloads page? It would be a massive wast of time to compile and upload every single minor update. I'm talk 80% of my effort could literally go to maintaining up to date binaries...

• Never post links if they are "deprecated" or "outdated." If people shouldn't be downloading that code, don't link it, period. I should not have to wade through a list of links to find the "right thing to do." You should make the "right thing" the path of least resistance. I should click Downloads, Windows Snapshot, Install/Run. Nothing else. If I weren't committed to helping you by offering you a code review, I would have left your site already and made a mental note never to come back.

[/quote]

Those were posted almost a year ago. I've not updated them and don't plan to until the projects reaches beta.

• Your Google Code page... uh... leaves a lot to be desired. Seriously, put some effort into this - it's what determines how many people will care about your project. Your code could be the best thing ever, but I'll never know as the average Joe stumbling across your site, because you look like you don't even care enough to properly describe your project on its own home page.

[/quote]

Well I agree and I'm going to work on that soon.

• Your examples exhibit a typical failure of example code. You put too much focus on brief, simple-looking demos and not enough focus on showing how a real project would use your library. Unless of course your "real" usages are full of poorly-factored functions, magic numbers, global variables, and other bad engineering practices - which I hope not, for your sake.

[/quote]

Technically not examples. They are more like place holders right now.

• Repeating your license at the top of every code file is just sloppy. Put it in trunk/License.txt and be done with it. If you ever decide to change license schemes, you're in for a headache. Also, as a reader of your code, I don't give a shit about your license. I want to know why OvglMesh.cpp exists and what I might find in there. This is probably the number one thing that people screw up with file-level documentation; don't give me legalese gibberish that I can find elsewhere, don't give me "@brief None" ever, and for the love of all things holy and sexy, please tell me what the point of the stinkin' file is in the first place.

[/quote]

Every open source project I know of places the license in the code. A few examples I can pull off the top of my head are Bullet, Box2D, Recast and many many more.The apache license clearly states to place the license in your code as well.
@brief, @file, and others are tags for doxygen. Those are required to use doxygen to automatically generate documentation.

• Your operator== and operator!= implementations look a bit bush-league. This is not a nice thing for me to see as the very first bit of your code that I review. Why not just return (memcmp(...) == 0); instead of the totally unnecessary if and else? This sort of verbose over-implementation of a very trivial code concept doesn't give me much hope for the rest of your code's quality. Now, I'm doing my best not to make snap judgments and just prematurely conclude that all of your code is this questionable, but the average reviewer may not be so gracious.

[/quote]

Fixed

• Why do you explicitly qualify all of your namespaces instead of dropping in judicious using-declarations or explicitly wrapping the implementations in a namespace {} block? This is another newbie-level mistake in code organization and reflects poorly on your code quality once again.

[/quote]

Good point

• Your code formatting is inconsistent, particularly when it comes to spacing and indentation and brace styles. Pick a format and stick with it. This is probably the single biggest smell I detect in new programmers' work - if you aren't disciplined and consistent enough to format things the same way even in the same 20KB file, what else are you cutting corners on?

[/quote]

Fixed

• Magic numbers are evil, and your code is littered with them. What does mesh->Simplify(100, 0) mean? That's just terrible style and, yet again, makes me question if I should even bother looking at the large-picture architectural design of your code. If the implementation is this sloppy, I'm probably not going to like what I find at a ten-thousand-foot perspective.

[/quote]

void Ovgl::Mesh::Simplify( DWORD max_faces, DWORD max_vertices )
To me that speaks for it's self. Simplify mesh to have no more than 100 faces. No further explanation should be require...

• Checking in code with commented-out lines to your version-control repository is the height of sloppy. If you don't need code, remove it. If you need it again in the future, pull it out of revision history. There is no excuse for commenting out large swaths of code and then committing that.

[/quote]

This is true but it's honestly a pain to have to download and search through code every time I want to change something. Especially since all of the code I commented I was actively working on at the time. Those lines are already fixed in the version of the engine I have on my hard drive right now.

• You use std::vector<std::vector<Ovgl::Face>> (note the >> with no separating space) which is not strictly legal prior to C++11, and will make many compilers barf in odd situations.

[/quote]

Fixed

• Your (lack of) naming conventions makes it extremely hard to tell what is a local variable versus a class member versus a global, just by glancing; I don't advocate Hungarian notation, but at least format names in a way that makes it easier to tell where they belong, e.g. lowercasenames for locals, CamelCaseNames for members, and UPPERCASENAMES for constants/macros/etc.

[/quote]

Honestly have never heard that rule. I will work to fix that.

• Your code lacks even rudimentary exception safety, and the use of raw pointers and arrays is a sign of very poor C++ style, at best. At worst, it's a guarantee that your code is riddled with nasty and subtle bugs.
• Anyone who writes a C-style cast in a C++ program should be lashed repeatedly with a USB cable.

[/quote]

Can you show me a few examples of what you are talking about?

• Lots more magic numbers and no documentation. What the hell is the point of all that index shuffling going on in Mesh::Update()? Documentation is your friend!

[/quote]

• You use an awful lot of platform-dependent type definitions (e.g. DWORD) for claiming to be cross-platform code. Not good.

[/quote]

You are right and I've been trying to start changing my style so it will be easier to port the project.

• Your Clean enum has terribly named entries. Better hope you never need a different enum with similarly-named entries! Common convention on every team I've worked with is to prefix enum entries with the enum name itself, e.g. CLEAN_BROKEN_FACES.

[/quote]

Fixed

• extern "C". Inside a namespace. Exporting classes with std::vector members. At this point, I give up; this is hopelessly broken code and it's a bloody miracle it works on Windows, and will certainly never work anywhere else.

[/quote]

extern "C" should be on the outside of a namespace?

After reading Apoch's post I got curious and looked at the code. wow, he didn't exaggarate. Why in the world do you have "class Mesh" AND "class CMesh" ? That's just downright confusing. A few other hints:






Ovgl::Vector2 Ovgl::Vector2Set( float x, float y )
{
Ovgl::Vector2 out = {0};
out.x = x;
out.y = y;
return out;
}

This function (as well as all the similar ones for ever data type) are entirely superflous and inefficient. Look up "constructors."[font="Monaco,"] Also, no need to zero out the data if you are just about to overwrite it anyway, it's just wasted CPU cycles. [/font]
[font="Monaco,"]
[/font]
[font="Monaco,"]
Ovgl::Vector3 Ovgl::Vector3::operator * ( const Ovgl::Vector3& in ) const
{
Ovgl::Vector3 out = {0};
out.x = x * in.x;
out.y = y * in.y;
out.z = z * in.z;
return out;
}
[/font]
[font="Monaco,"]
[/font]
[font="Monaco,"]This will confuse every single person that uses your code, they will expect a cross product or a dot product (which is why, in my code, I never overload the * or / operator for vectors because it's just so ambiguous)....and basically everything else Apoch said about inconsistencies and inefficiencies. You may want to read a good book or two on C++ since you seem to be missing out on some very basic concepts (like constructors).[/font]

Lack of [color="#1c2837"]constructors and destructors are the thing I'm most ashamed of. You must realize I started with DirectX and Visual Basic. This exact engine has been built and rebuilt more times than I can count and the release and create functions you see are copy and pasted from my older versions of the engine. The structure of the engine is evolving from what I observe from other languages and APIs. This API has technically been in development for almost 10 years in some form or another. It's like a mash of all previous failed [color="#1c2837"]attempts.