Collision using rgb

Started by
17 comments, last by Heelp 8 years, 2 months ago


There's also a problem with deleting the objects, search for codewords 'new' and 'delete' and see if you see something wrong because I tried all day and still wont compile

What compiler errors are you getting? Also, I'm confused about what the issue is with the file loading. It sounded like a bug, but then you say the code doesn't compile which implies you haven't run the code. If it is a bug, what is the behavior you are seeing?

C++: A Dialog | C++0x Features: Part1 (lambdas, auto, static_assert) , Part 2 (rvalue references) , Part 3 (decltype) | Write Games | Fix Your Timestep!

Advertisement

Oh, sorry, I'm new and I confuse the terms quite often. My mistake, sry. It compiles but it returns 1. The thing is that all my functions return 1 when there is an error, I wanted to make a different comment that is printed on screen for every error in every function but someone told me I needed SDL 2.0 for this, and I'm using SDL 1.2, so now when something is wrong, I made all the functions to return 1, But I don't know exactly what function went wrong because all of them return 1 without any comment.

And when I compile, it returns 1 on console, that's the problem.

A very simple thing you can do that should work is return different numbers for the different errors instead of using the same number. Also, you should learn how to use a debugger. Vimeo is blocked at work so I can't check how useful it is, but Google suggested this video.

C++: A Dialog | C++0x Features: Part1 (lambdas, auto, static_assert) , Part 2 (rvalue references) , Part 3 (decltype) | Write Games | Fix Your Timestep!

Yes, I did it now. I have mistakes in set_tiles() function, and in the int main() when showing the tiles on screen, and maybe in my show() function.

nobodynews Thanks so much, man. I found my mistake, I'm showing too much tiles on screen, TOTAL_TILES is set to 441, and the maximum number of tiles which can be shown without getting error is 121, if I try to show 122 tiles, it returns 3, If i try with anything lower than 122, it works. Maybe my memory is not big enough or something, I don't know. But thanks again.

I had a quick look and below are my first findings.

In general, code looks quite neat.

I got it to compile, but it gave me a warning (always turn on as many warnings in the compiler as possible, anything it find saves you the trouble of debugging it)


prog.cpp: In function 'bool set_tiles(Tile**)':
prog.cpp:129:1: warning: no return statement in function returning non-void [-Wreturn-type]
 }

This means 'set_tiles' may return any value, including '0' (false) even if loading went well.

Easily solved with "return true;" at the end of the function.

Running it crashed immediately. It happened in "tiles[t]->show();" and this takes some steps to explain. Just going from top to bottom through that routine.


     FILE* filePointer;
-    filePointer=fopen("Pics/pacmanMap.txt", "r");
+    filePointer = fopen("Pics/pacmanMap.txt", "rt");
+    if (filePointer == nullptr) return false;

I used 'diff', a program to output changes between files. Lines starting with a '-' are in your file but I removed them, lines starting with '+' were added by me.

A changed line gets a -, and then a +, so you can easily compare both versions.

So above I edited the "filePointer=fopen" line, adding some spaces around the =, and a "t" with the "r" (since text files are always opened with a "t" modifier).

I also added a line that checks the return value of the 'fopen' call (always check things for errors, it will save you hours of debugging).


             counter ++;
+// You don't check whether tiles[counter] exists. That can cause memory
+// corruption!
+            if (counter == TOTAL_TILES) break; // Abort reading if all tiles read.

If you reach the end of an array, tiles[counter] will appear to continue to work, but actually corrupts other values. Better check boundaries :)

This solves the problem of reading too many tiles (if your 0/1 input file is too big for some reason). My crash was however reading too few digits. That is solved as follows


     Tile *tiles[ TOTAL_TILES ];
 
+    // Here 'tiles' is random garbage, it's better to initialize it.
+    // (I may be wrong here, maybe C++ does initialize it. However, better safe than sorry.)
+    for (int t = 0; t < TOTAL_TILES; t++) tiles[t] = nullptr;

Make sure tiles[] is fully initialized. Also


         for( int t = 0; t < TOTAL_TILES; t++ )
         {
-            tiles[ t ]->show();
+            // And here is another problem, what if you didn't load enough tiles??
+            // Either check 'counter' after fully loading the file, or be more careful here:
+            if (tiles[t] != nullptr) tiles[t]->show();

Just skip if you run into a 'nullptr'. Alternatively, you can just 'break', and terminate the loop, since there are not going to be any more tiles to show.

After this, the window stayed up, but crashed again when typing ESC. Perhaps some other unchecked call????

Other stuff I ran into:

I hate '=' in a condition (I once spend 45 minutes finding one), so I do:


-    //While going through all the characters...
-    while( ( t = getc( filePointer ) )!= EOF )
+//    //While going through all the characters...
+//    while( ( t = getc( filePointer ) )!= EOF )
+
+// I don't like assignments inside conditions, so I do the following instead:
+    while (true)
     {
+        t = getc(filePointer);
+        if (t == EOF) break;

The '0' case and the '1' case while loading look very similar, you can merge them with


-        if( t == '0' )
+        if( t == '0' || t == '1' )
         {
             //Make the type of the tile = 0
-            tileType = 0;
+            tileType = t - '0';
             tiles[ counter ] = new Tile( x, y, tileType );

and deleting the '1' case.

About your error reporting. You can use a string as error message, for example



-bool init()
+// You can also pass a constant string back to the caller indicating what went wrong:
+const char *init()
 {
     //Initialize all SDL subsystems
     if( SDL_Init( SDL_INIT_EVERYTHING ) == -1 )
     {
-        return false;
+        return "SDL_init failed";
     }

     ......

     //If everything initialized fine
-    return true;
+    // C++11 and higher uses 'nullptr' instead of NULL.
+    return nullptr;
 }

The function returns a "const char *" instead of a boolean, and you can catch and print it with


-    if( init() == false )
-    {
+    const char *return_value = init();
+    if (return_value != nullptr) {
+        // Make sure you add a \n at the end of the string that you print otherwise you don't get a line.
+        // I use 'stderr', as it is for error messages.
+        // The normal 'printf' uses stdout, which is for regular output.
+        fprintf(stderr, "Error: %s\n", return_value);
         return 1;
     }

However, something like below should also work:


     //Load the files
     if( load_files() == false )
     {
+        printf("load_files() failed\n"); // <-- should also work
         return 1;
     }

I don't what platform you use, but Windows is notorious for not showing the stdout and stderr streams in the GUI.

std::string is passed differently, normally:


+// std::strings are normally passed by const reference, to avoid copying the object:
- SDL_Surface * load_image( std::string filename )
+// SDL_Surface * load_image( const std::string &filename )

     //While the user hasn't quit
+    // Booleans can also be tested directly instead of comparing with false or true,
+    // like "while (!quit)"
     while( quit == false )

         {
             SDL_Delay( 1000 / FRAMES_PER_SECOND  - fps.get_ticks() - 5 );
         }
+        // Does the "- 5" miss in the condition above? (which was chopped off by diff, sorry about that)
+        // Perhaps compute the value instead, and then test?
+        // int delay = 1000 / FRAMES_PER_SECOND  - fps.get_ticks() - 5;
+        // if (delay > 0) SDL_Delay(delay);

Last but not least,


Tile *tiles[ TOTAL_TILES ];   // Lots of very small allocations, why not do

Tile tiles[TOTAL_TILES]; // One big array containing all 'Tile' objects.
//Tile::Tile() can be empty, move initialization eg to Tile::load(x, y, tiletype), so you can do

tiles[counter].load(x, y, tileType);

Hope this helps.

Alberth, Thank you so much. You don't know how much stuff you did for me here, thanks a lot. The funny thing is that I pay 9000 pounds per semester and when I ask my professor to see my code, he just tells me how he did it and shows me HIS code, not actually seeing what are my mistakes in MY code. And you here, took the time to actually see all my code and point all my mistakes, and you did it for free. Thanks again, so much, everything is working now. There's no way I could have fixed all this by myself, especially the part where you check the tiles[t] for nullptr before showing it on screen, and actually this was the reason why return 3 was showing on console.

for( int i=0; i<=1billion; i++)
{
printf("THANKS MAN \n" );
}

You're welcome :)

Peer review is a very powerful way to learn. Find one or more other persons eg fellow students, and read and comment on each other code, point out other (better?) ways to solve a problem. Different people have a different views, and different ideas how to do things, and usually they are all valid. Above all however, have fun doing it :)

//Post deleted

This topic is closed to new replies.

Advertisement