Hey there,

So awhile back, I wrote a post about a C++ question I had with SDL, and I think I've tracked down the problem, but it's a problem that makes no sense whatsoever.  So I'm thinking there must be some noobish little nano-detail I just never learned, specifically how to discover the cause of undefined behavior.

So the original problem was like this: I had a "game" class with a vector of game objects; this game class handles the main game loop in a "start" method.  The problem was, objects being created after calling "start" were not being drawn.  The game objects in the vector (instances of a "GameObject" class) have an SDL_Rect that's used for collision checking and other things.  This gets passed to an Animation object, which does the actual SDL stuff involved in drawing.

So my first idea was to find out what the objects' rectangles' values were.  So I had it print the x, y, w and h in each step of the game loop.  For the first game object, I would get what I expected, "468 544 32 32".  The object was at 468,544 and its width and height were 32.  But the second object seems to be generating random numbers: "4696288 1 7554358 10".  I passed in everything from references to the player object's x/y/w/h to hard-coded numbers like 32, 32, 32, 32.  Still random.

So I figured maybe it has something to do with the animation class, and did something similar.  The class maintains an "index" property (an int) which starts at 0 and goes up until it hits a pre-defined total, and then reset to 0; so I figured I'd have it print that.  With the first object (created before calling Game.start) this worked as expected.  The index started at 0, looped to the total, etc. as expected.  But with the second instance (created on key press at runtime) the index is 4741476 (again with the WTF...?!?!?! hair--) :lol:

So I'm clearly missing something stupidly simple, like maybe default values of ints that are properties of a class or something.  I initialize them to 0 in the constructors, but somehow they are getting set to other numbers, seemingly for the sake of just cuz.  Talk about undefined behavior.

Now I know that in general, code helps - but I really don't know of any way to give you code without just pasting in my entire program.  If you need it, I'll give it to you, but I'm thinking I must be missing something obvious.  If I'm not, please let me know and I'll paste away. Thanks.

Sounds like uninitialized values somehow.

Show how you create the GameObjects after "start" and add them to the vector. This probably also means showing the constructors you use for the relevant classes.

You said you checked the values of the SDL_Rects and got those bogus values --when? Inside the Animation object, or somewhere else?

Hey, thanks for the quick reply!  Yes, that's what I thought too... but in the constructors I set the values, and printing them to the console after initializing them shows the values I would expect - it's during the draw method that they seem to go random... could it be how SDL_Rect is initialized?  Maybe it isn't allowed as a class property or something (though I couldn't find that in the documentation anywhere).  Or maybe it should be a pointer to an SDL_Rect and created in the constructor with "new"?

Either way, here's what I'm guessing is the code involved:

// --------------------------------------------------------
// GameObject.h
// --------------------------------------------------------

#include "SDL.h"
#include "Animation.h"

#ifndef GAMEOBJECT_H_
#define GAMEOBJECT_H_

//////////////////////////////////////////////////////////////////////////////////////////////////////
//      DECLARATION
//////////////////////////////////////////////////////////////////////////////////////////////////////

class GameObject
{
public:
    int x, y, w, h;
    GameObject(int x, int y, int w, int h);
    void set_animation(Animation * a);
    void Draw(SDL_Surface * destination);
    bool collision(GameObject * other);
    void destroy();
protected:
    SDL_Surface * sprite;
    SDL_Rect rect;
    Animation * animation;
    bool _destroyed;
};

//////////////////////////////////////////////////////////////////////////////////////////////////////
//      DEFINITION / IMPLEMENTATION
//////////////////////////////////////////////////////////////////////////////////////////////////////

GameObject::GameObject(int x, int y, int w, int h)
{
    x = x;
    y = y;
    w = w;
    h = h;

    rect.x = x;
    rect.y = y;
    rect.w = w;
    rect.h = h;

    _destroyed = false;
}
void GameObject::Draw(SDL_Surface * destination)
{
    if (!_destroyed)
    {
        // Update the object's position
        rect.x = x;
        rect.y = y;

        // In an effort to figure out why the heck new objects added to the vector were not being drawn,
        // I created this code
        std::cout << rect.x << " ";
        std::cout << rect.y << " ";
        std::cout << rect.w << " ";
        std::cout << rect.h << std::endl;

        // And update the animation
        animation->update(destination, rect);
    }
}
void GameObject::set_animation(Animation * a)
{
    animation = a;
}
bool GameObject::collision(GameObject * other)
{
    return ((x > other->x && x < other->x + other->w && y > other->y && y < other->y + other->h)
            || (x + w > other->x && x + w < other->x + other->w && y > other->y && y < other->y + other->h)
            || (x > other->x && x < other->x + other->w && y + h > other->y && y + h < other->y + h + other->h)
            || (x + w > other->x && x + w < other->x + other->w && y + h > other->y && y + h < other->y + h + other->h));
}
void GameObject::destroy()
{
    _destroyed = true;
    delete this;
}

#endif

// --------------------------------------------------------
// Animation.h
// --------------------------------------------------------

#ifndef ANIMATION_H_
#define ANIMATION_H_

#include "SDL.h"

#define ANIMATION_HORIZONTAL 0
#define ANIMATION_VERTICAL 1

//////////////////////////////////////////////////////////////////////////////////////////////////////
//      DECLARATION
//////////////////////////////////////////////////////////////////////////////////////////////////////

/**
 * This class handles the math and SDL stuff involved in looping through an animation
 * @todo Add events like pause(), stop() etc.  If possible, add a setter for speed, and
 * maybe a way to set the pic (for objects that will use multiple instances of this).
 */
class Animation
{
public:

    /**
     * Constructor: init instance variables
     * @param[in] The image file and/or path
     * @param[in] The width of a single pic in the animation
     * @param[in] The height of a single pic
     * @param[in] One of the constants #defined above - Tells the Update function whether to move the X or Y
     * @param[in] The total number of pics in the animation
     * @remarks This class assumes a single "strip" of images, either horizontal or vertical, with no gaps
     * between them (for now).  This is what the direction and total are for, so it "knows" how to run
     * the animation.  If either of these is off, the image will flicker as class loops through the pics.
     */
    Animation(const char * sprite_file, int width, int height, int direction, int total, int speed);

    /**
     * Shows the next pic in the animation
     * @param[in] The surface we're drawing to
     * @param[in] The rectangle where the animation should be positioned on the screen
     */
    void update(SDL_Surface * destination, SDL_Rect object_rect);

private:
    SDL_Surface * sprite;
    SDL_Rect rect;
    int direction, total, index, speed, next;
};

//////////////////////////////////////////////////////////////////////////////////////////////////////
//      DEFINITION / IMPLEMENTATION
//////////////////////////////////////////////////////////////////////////////////////////////////////

Animation::Animation(const char * sprite_file, int width, int height, int direction, int total, int speed)
{
    // Save references to the ints
    direction = direction;
    total = total;
    speed = speed;
    next = speed;     // Used in update

    // Set the starting index to 0;
    index = 0;

    // Init the rect property
    rect.x = 0;
    rect.y = 0;
    rect.w = width;
    rect.h = height;

    // Load the sprite
    sprite = NULL;
    sprite = SDL_LoadBMP(sprite_file);
    if (this->sprite == NULL)
        std::cout << "Oh crap, something went wrong.  Fight with this later.\n";
}
void Animation::update(SDL_Surface * destination, SDL_Rect object_rect)
{
    // Another part of the debugging attempt
    // When I create the first object, its index starts at 0, where I expect it
    // But when I create the second, the index is 4741476 (for only God and Bjourne Stroustrup know why)
    // So clearly it's undefined behavior, but why?  The index is initialized to 0 in the constructor, and
    // so is the total
    //std::cout << index << " " << std::endl;

    // Update the index
    index++;        // The debugger points to this line as the problem

    // I've tried commenting out all this - the problem is not here.
    if (index >= total * speed)
    {
        index = 0;
        rect.x = 0;
        rect.y = 0;
        next = speed;
    }
    else if (index == next)
    {
        next += this->speed;
        if (direction == ANIMATION_HORIZONTAL)
            rect.x += rect.w;
        else
            rect.y += rect.h;
    }

    // And update the animation
    SDL_BlitSurface(sprite, &rect, destination, &object_rect);
}

#endif

// --------------------------------------------------------
// Main.cpp
// --------------------------------------------------------

#include <iostream>
#include "Game.h"

#include <iostream>
#include "SDL.h"
#include "Window.h"
#include "GameObject.h"
#include "Game.h"
#include "Animation.h"
#include "Controller.h"

// Global variables
bool left_pressed = false, right_pressed = false, up_pressed = false, down_pressed = false;
Animation player_left("bin\\debug\\graphics\\player_left.bmp", 32, 32, ANIMATION_VERTICAL, 8, 10);
Animation player_right("bin\\debug\\graphics\\player_right.bmp", 32, 32, ANIMATION_VERTICAL, 8, 10);
Animation player_up("bin\\debug\\graphics\\player_up.bmp", 32, 32, ANIMATION_HORIZONTAL, 8, 10);
Animation player_down("bin\\debug\\graphics\\player_down.bmp", 32, 32, ANIMATION_HORIZONTAL, 8, 10);
GameObject player(468, 544, 32, 32);


void GameLoop()
{
    // Update the player's position
    if (left_pressed)
        player.x -= 2;
    else if (right_pressed)
        player.x += 2;
    if (up_pressed)
        player.y -= 2;
    else if (down_pressed)
        player.y += 2;

    // And slow things down a sec
    SDL_Delay(10); //For now - I'll have to come up with another way to make characters move slowly later
}

void KeyDownEventHandler(int key)
{
    if (key == LEFT)
    {
        player.set_animation(&player_left);
        left_pressed = true;
    }
    else if (key == RIGHT)
    {
        player.set_animation(&player_right);
        right_pressed = true;
    }
    else if (key == UP)
    {
        player.set_animation(&player_up);
        up_pressed = true;
    }
    else if (key == DOWN)
    {
        player.set_animation(&player_down);
        down_pressed = true;
    }
}
void KeyUpEventHandler(int key)
{
    if (key == LEFT)
        left_pressed = false;
    else if (key == RIGHT)
        right_pressed = false;
    else if (key == UP)
        up_pressed = false;
    else if (key == DOWN)
        down_pressed = false;
    else if (key == SPACE)
    {
        // This is where I'm creating the instances at runtime - the player presses Space and creates a new GameObject instance
        // Same constructor, same Game object, different GameObject and Animation objects.
        Game * game = Game::Get();
        Animation player_left2("bin\\debug\\graphics\\player_left.bmp", 32, 32, ANIMATION_VERTICAL, 8, 10);
        GameObject test(32, 32, 32, 32);
        test.set_animation(&player_left2);
        game->AddObject(&test);
    }
}

int WinMain()   // I'm on Windows, and apparently SDL wants WinMain, as calling it "main" causes compiler tantrums
{
    Controller::OnKeyUp = KeyUpEventHandler;
    Controller::OnKeyDown = KeyDownEventHandler;

    Game * game = Game::Get();
    game->CreateWindow("Game Title", 800, 600);

    // Create our player animation
    player.set_animation(&player_left);
    game->AddObject(&player);

    game->Start(GameLoop);

    return 0;
}

So after doing all kinds of experiments with this code to try and guess out what possible reason there could be for the randomness, the only guess I got is maybe the object's SDL_Rect... can you see anything else it might be?  Thanks.

On 6/19/2017 at 4:21 PM, ElGeeko7 said:

GameObject::GameObject(int x, int y, int w, int h)

{
    x = x; //This is wrong.
    //...
}

 

Your constructor is wrong, and you are using uninitialized data.

When you draw, you're copying the uninitialized values from the member into the rect, and then you attempt to draw.

x = x will assign the value of argument x to itself -- it will do nothing.

To refer to the member's x value, you need to write "this->x".

This is because when you just write "x", there's no way for the compiler to guess which "x" you want to refer to. Because of this, it will always pick the values passed into the function.

Thus, you need to write "this->x = x;" -- and similar for the other lines.

In case you're wondering, all class and struct instances have a "this" pointer, which is what we're using to solve this problem.

Another solution would be to rename the argument values to something else. Either will work, but it's important to understand why the current code is broken.

Lactose is correct, but as an addendum you can do something similar to what you did (using the same variable name) if you utilize member initialization lists. So your constructor could be as follows:

GameObject::GameObject(int x, int y, int w, int h)
    : x(x), y(y), w(w), h(h), _destroyed(false)
{
    rect.x = x;
    rect.y = y;
    rect.w = w;
    rect.h = h;
}

Without getting into the merits of using the same name for the parameters and member variables, this is just to note that it's possible.

Other general comments:

• don't do `delete this`. That is just asking for trouble. Objects should be destroyed by their owners.
• Your test.set_animation call is completely dangerous because you pass a pointer to a local variable. That means the pointer is completely worthless once the KeyUpEventHandler function returns, meaning you are likely to see a crash.
• Exactly the same applies to the AddObject call below that.
• you don't really need the x, y, w, h AND the rect - they're both the same data, right?
• your _destroyed variable currently serves no purpose - if you've already deleted an object, then it is completely destroyed - there is no point trying to read it to see whether the object is dead or not. That value may not be correct, and it may not even be in memory you are allowed to access any more. This is why you need to have an object's owner destroy the object.