Blank (white) Sprite -- can I pass a SubRect?

[tokiwotomare]

Hello friends,

My code's process is (in a nutshell) like so:
- a Player class declares and initializes an Animator class
- the Animator class holds said Player's sprite sheet, and is, among other things, responsible for passing back the appropriate sprite to draw at any given time
- the game requests said sprite whenever it needs to draw.

My sprite is showing up as a blank white box. My current theory is that the sprite's image is going out of scope at some point, but I can't figure out where.

Here are (I hope all of) the relevant bits (a bunch of fluff taken out unless requested):

mj_animator.h

Code: [Select]

class mj_animator
{
public:
    mj_animator(const std::string &Filename ... );
    mj_animator();
    sf::Sprite get_sprite();
private:
    sf::Sprite sprite_sheet;
    sf::Image img;
}


mj_animator.cpp

Code: [Select]

mj_animator::mj_animator(const std::string &Filename, ...) {
    if(! img.LoadFromFile(Filename)) {
        // error handling stuff
    }
    sprite_sheet.SetImage(img);
}

sf::Sprite mj_animator::get_sprite() {
    // some calculations to determine x1, y1, x2, y2 of an IntRect
    sprite_sheet.SetSubRect(sf::IntRect(x1, y1, x2, y2));
    return sprite_sheet;
}


mj_player.cpp

Code: [Select]

mj_player::mj_player(sf::Image sheet) {
    //animators is an array of mj_animators objects (in mj_player.h)
    animators[0] = mj_animator("file.jpg", ...);
    animators[1] = mj_animator("file.jpg", ...);
    current_animator = 1; //an int to track which animator currently being used
}

// ... some functions to determine which animator we need to have going at any given time, etc

mj_player::Draw(sf::RenderWindow &App) {
    // some calculations to determine current position
    sf::Sprite draw_sprite = animators[current_animator].get_sprite();
    draw_sprite.SetPosition(position.x, position.y);
    App.Draw(draw_sprite);
}


If there's anything you think you need, I'll put it -- I didn't include a lot of what I think is unrelated because it's already a fairly bloated post.

My best guess, as I said, is that either somewhere in the process, the image goes out of scope, or that I can't pass sprite_sheet back as I think I can.

Any guesses? Thanks.

edit:
I should note that through some debugging (read: a bunch of couts), I did determine that the math that I didn't include here is actually all correct -- that the SubRect is being correctly calculated, so it's not trying to draw some weird part of the image that doesn't exist or anything like that.

[Laurent]

You must define (or disallow) a copy constructor and an operator =. See the last paragraph of the tutorial about sprites to know why it matters.

[tokiwotomare]

Aha! Worked like a charm. Thanks for the response.

I started working on an image management class, figuring that I'd need one at some point anyway. I figured it'd be best to truly only ever have one sf::Image, so when I need to pass it around the program, I'll pass references to it.

Is there anything I should watch out for? Does Sprite.SetImage do wonky things with references, or should that be okay?

[Laurent]

Everything should be fine.

[Gibgezr]

Creating a new sf::Sprite every frame of your game for each animator object is a bad idea. I would design it with the animator class returning a pointer to its sprite, then just using the pointer to adjust the sprite's position and draw it.

I would also go so far as to make the pointer a public variable in the animator class; an accessor function really adds nothing but overhead to the class design in this case (why do we need data hiding for a sf::Sprite object?). As it is likely that the animator class could get used for drawing literally thousands of sprites per frame, removing even function-call overhead is a worthy optimization, in my mind at least.

While I am busy showing my old-school realtime C/Assembly programmer biases, another change I would make is to not bother passing the reference to the RenderWindow in to the player Draw() function...unless you actually are going to support multiple RenderWindows. It's not a real speed optimization, by why pass around a variable that actually remains static for the lifetime of the app? I'd just store a pointer to the RenderWindow in any object that needed it. That is just a style/design preference of mine, so I'm just kvetching at this point, but passing parameters means storing/loading/restoring registers. Where it gives you any flexibility that you might actually use, it's worth it, but for something with practically nil usage, why bother?

tldr; don't allocate new objects every frame of your game if you don't need to, and watch function-call overhead for things you may do hundreds/thousands of times per frame. And yes, I am an old buzzard.

[Nexus]

I would also go so far as to make the pointer a public variable in the animator class; an accessor function really adds nothing but overhead to the class design in this case [...] removing even function-call overhead is a worthy optimization, in my mind at least.

Breaking encapsulation because of assumptions is generally a bad idea. Today's compilers are good at inlining where appropriate, in these cases the overhead is zero. In the other cases, I would still use a profiler and see if there is an actual impact on performance, before practising premature optimization. Chances are very high that the function calls are not the critical part of the application.

but passing parameters means storing/loading/restoring registers. Where it gives you any flexibility that you might actually use, it's worth it, but for something with practically nil usage, why bother?

From a design point of view, there is also a third option: A class that stores the render window and has a function Draw(const Player&). Like this, you can decrease coupling between logical and graphical elements. In a good design, the player doesn't know how to draw himself. He is just an abstract object which interacts with game logics. He can of course still store rendering information, but he doesn't use them himself.

The advantage of this separation lies in the fact that the rendering parts of the program aren't spread over all classes. Rather, you have a central instance which is in charge of graphical representation and has the full control over it. This makes modifications in the rendering system a lot easier. You can overload the function for different types and work more generically. Besides, game elements become more lightweight when they don't bear any graphical responsibilities.

[Gibgezr]

From a design point of view, there is also a third option: A class that stores the render window and has a function Draw(const Player&). Like this, you can decrease coupling between logical and graphical elements. In a good design, the player doesn't know how to draw himself. He is just an abstract object which interacts with game logics. He can of course still store rendering information, but he doesn't use them himself.

The advantage of this separation lies in the fact that the rendering parts of the program aren't spread over all classes. Rather, you have a central instance which is in charge of graphical representation and has the full control over it. This makes modifications in the rendering system a lot easier. You can overload the function for different types and work more generically. Besides, game elements become more lightweight when they don't bear any graphical responsibilities.

This is an excellent concept. It's usually referred to as the Model-View-Controller paradigm (I always call it the Controller-Model-View paradigm, which seems like a more natural ordering of the program architecture to me, and is the way the folks I used to work with referred to it in the 80's).

A quote from Wikipedia:
"The goal of MVC is, by decoupling models and views, to reduce the complexity in architectural design and to increase flexibility and maintainability of code."

It really works, too.