Improved the Linux version#1
Conversation
|
Wow, I hadn't expected anyone had even seen this project yet. Let alone had a use for it and even a contribution. NICE. By the way, I re-wrote the history (2 commits) really recently to fix an incorrect author email. Hopefully that doesn't mess things up. |
| src/engine.cpp | ||
| src/font.cpp | ||
| src/main.cpp | ||
| src/sdl_utils.cpp |
There was a problem hiding this comment.
The newline change creates too much noise to see the actual changes.
I just committed a fix for the new-lines, hadn't noticed because my editor just hid this.
The project is an old project that has kind of tumbled along over time, so it's got some cruft!
Anyway, please rebase from the newline commmit so we can look at these changes in better isolation.
|
It took me a bit to realize everything going on here. I think you can squash this whole commit and rebase it on the newlines I've fixed per you bringing to my attention. This will make your sdl_util much easier to integrate. Thanks. |
|
It's kind of fun to me that I got a contribution this early. I'll give you some additional critiquing now even though it's kinda hard in the current state. |
| #include "sdl_gl.h" | ||
| #include <iostream> | ||
|
|
||
| void sdl_application_abort(const char * msg) |
There was a problem hiding this comment.
I like my function names to be camel case, starting in upper case.
In this case, I would suggest something like: AbortSDLApplication. However, you'll see in a comment below, the engine is actually handling this via destructor which is triggered from uncaught exception, (or intentionally thrown exception). And doing that makes this not really SDL related at all.
So, perhaps this would be better moved to the engine.cpp as a helper called:
AbortApplication(const char *msg, int errorCode = 1);
or something like that. Wdyt?
There was a problem hiding this comment.
You can update the Init failure cases to use that too.
| void sdl_application_abort(const char * msg) | ||
| { | ||
| std::cout << SDL_GetError() << std::endl; | ||
| SDL_Quit(); |
There was a problem hiding this comment.
take a look at main.cpp.
I don't normally use exceptions, but the main application has a final catch_all for exceptions which shuts the engine down properly.
I recommend:
throw(1);
Like in the initialization error checks, which the main app will catch and shutdown the engine including "SDL_Quit();"
| exit (-1); | ||
| } | ||
|
|
||
| void getInfo(void) |
There was a problem hiding this comment.
How about "PrintSDLInfo" (I don't want to prefix with "SDL" like SDLFunctionName because it's easily confused with the library functions).
And also we're not really "getting" it, as it's not returned.
It's fine as printf for now, with stderr, or cerr <<
But frankly I should add a logging method, so that messages can be printed with a specific level of verbosity (Debug, Info, Warning, Error) etc, which filter based on a flag indicating desired verbosity.
| SDL_VERSION(&compiled); | ||
| SDL_GetVersion(&linked); | ||
|
|
||
| fprintf(stdout, "We compiled against SDL version %d.%d.%d ...\n",compiled.major, compiled.minor, compiled.patch); |
| fprintf(stdout, "And we are linking against SDL version %d.%d.%d.\n", linked.major, linked.minor, linked.patch); | ||
|
|
||
| int glMajor = 0; | ||
| int glminor = 0; |
There was a problem hiding this comment.
use consistent naming, glMajor, glMinor
| int glminor = 0; | ||
| glGetIntegerv(GL_MAJOR_VERSION, &glMajor); | ||
| glGetIntegerv(GL_MINOR_VERSION, &glminor); | ||
| std::cout << "Current OpenGL version : " << glMajor << "." << glminor << std::endl; |
There was a problem hiding this comment.
use consistent print method, stick with printf or cout in the same function
| std::cout << "Current OpenGL version : " << glMajor << "." << glminor << std::endl; | ||
| } | ||
|
|
||
| void getTextureInfo( SDL_Texture * texture) |
There was a problem hiding this comment.
How about: "PrintSDLTextureInfo"?
No space before first parameter.
I know everyone has their own style about declaring pointers:
Var name;
Var name;
Var * name;
etc. I guess I won't nit pick this, at least it's self consistent in this file. It just differs from other files.
| int anErr = 0; | ||
| int count = 0; | ||
|
|
||
| if ( count % loop_value == 0 ) |
There was a problem hiding this comment.
Copy and paste error or left over debug? Please cleanup the unused looping logic, this isn't looping.
There appears to be no point to any of this "count" or "loop_value" stuff.
Every time this function is called, it'll initialize the variables (they aren't static), so the (0 % 100 == 0) is always true.
2 participants
Hi,
First, thanks for sharing your code.
On my side, I fixed the Linux build + added some features, e.g. to show the current OpenGL version (add #ifdef DEBUG ... #endif to shut it up and to avoid verbose application)
FYI, the abort() method was missing, and I added mine. Feel free to use whatever code you want, and use the license you need.
And the best for 2018 :-)