Issue with operations on 2 vectors in one loop

Question

When I use this loop, code doesn't work propertly :

for(int i = 0; i < INTRO_LOGO_COUNT; i++)
{
    char path_buffer[32]; // 32 = logo_xxx.png
    sprintf(path_buffer, "data/texture/intro/logo_%d.png", i);
    std::cout << path_buffer;

    m_vecTextures.push_back(sf::Texture());
    m_vecTextures.back().loadFromFile(path_buffer);

    m_vecSprites.push_back(sf::Sprite());
    m_vecSprites.back().setTexture(m_vecTextures[i]);

    // Center
    m_vecSprites.back().setPosition(
        SCREEN_WIDTH / 2 - m_vecSprites.back().getLocalBounds().width / 2,
        SCREEN_HEIGHT / 2 - m_vecSprites.back().getLocalBounds().height / 2);
    m_vecSprites.back().setColor(sf::Color(255, 255, 255, 0)); // Opacity : 100%
}

Textures got mixed, and only second texture work propertly. (INTRO_LOGO_COUNT is defined as 2) And first texture have valid size but image from second texture. Second texture have valid size and image. But after 1 change (spilt into 2 loops) :

for( int i = 0; i < INTRO_LOGO_COUNT; i++ )
{
    char path_buffer[32]; // 32 = logo_xxx.png
    sprintf(path_buffer, "data/texture/intro/logo_%d.png", i);
    std::cout << path_buffer;

    m_vecTextures.push_back(sf::Texture());
    m_vecTextures.back().loadFromFile(path_buffer);
}

for( int i = 0; i < INTRO_LOGO_COUNT; i++ )
{
    m_vecSprites.push_back(sf::Sprite());
    m_vecSprites.back().setTexture(m_vecTextures[i]);
    // Center
    m_vecSprites.back().setPosition(
        SCREEN_WIDTH / 2 - m_vecSprites.back().getLocalBounds().width / 2,
        SCREEN_HEIGHT / 2 - m_vecSprites.back().getLocalBounds().height / 2);
    m_vecSprites.back().setColor(sf::Color(255, 255, 255, 0)); // Opacity : 100%
}

Everything works fine. Where is problem ? I tried different ways, but always same effect. Thanks!

Answer 1

The reason is from the implementation of vector. When a vector is created, it allocates a fix size of memory, for example, the initialize size of the vector is A. When you push element into the vector and size is up to A, then vector will automatically re-allocate an other continuous memory block with size 2*A, if it can't re-allocate with a size 2*A memory block at the same address, it will allocate a new memory block and copy the content in the old memory to the new one.

So in your first loop:

m_vecSprites.back().setTexture(m_vecTextures[i]);

From the link you provided, the definition of function "setTexture' is:

void Sprite::setTexture(const Texture& texture, bool resetRect)
{
    // Recompute the texture area if requested, or if there was no valid texture & rect before
    if (resetRect || (!m_texture && (m_textureRect == sf::IntRect())))
        setTextureRect(IntRect(0, 0, texture.getSize().x, texture.getSize().y));

    // Assign the new texture
    m_texture = &texture;
}

And the member variable 'm_texture' is pointer type

the functon 'setTexture' gets a reference to m_vecTextures[i], but since the vector 'm_vecTextures' will still increase, if it reaches the initial size, the vector will resize and move the memory block, and the reference could become valid.

if the vector is initialize with size INTRO_LOGO_COUNT, the first loop will work fine.

Answer 2

UPDATING BASED ON INFO GLEANED FROM API

First off, you shouldn't be using sprintf for this. sprintf is horribly unsafe and you could easily buffer overflow if you aren't careful. If are you going to insist on using C functions, at least use snprintf. Secondly, you are not cleaning the string up after yourself, nor are you putting a NULL terminator anywhere. This could easily lead to undefined behavior.

You are in C++ so use stringstream and spare your selfsome the effort

I would write this as, although personally i would use a move semantic, or copy constructor and not keep editing the object at "back"... but that's just me

Secondly, the fact that it is kept as a reference and that you aren't using pointers means our pointer can't change... So Vector can't be used unless you can guarantee that it wont be resized later.... if you want to use a vector beware

m_vectTextures.reserve(INTRO_LOGO_COUNT); //reserves size so resize won't occur
for( int i = 0; i < INTRO_LOGO_COUNT; i++)
{
    sf::Color color(255, 255, 255, 0); // Opacity : 100%

    const char* prefix = "data/texture/intro/logo_";
    const char* suffix = ".png";
    std::stringstream ostr;
    ostr << prefix << i << suffix;
    std::string path( ostr.str() )
    std::cout << path;

    // if the above doesnt cut it, consider the same idea but using snprintf to read the 3 characters you need instead of an entire buffer

    m_vecTextures.push_back( sf::Texture() );
    sf::Texture& texture = m_vecTextures.back();
    texture.loadFromFile( path.c_str() ); //if you need a string

    m_vecSprites.push_back( sf::Sprite() );
    sf::Sprite& sprite = m_vecSprites.back();
    sprite.setTexture(texture);

    // Center
    sprite.setPosition(
        ( SCREEN_WIDTH - sprite.getLocalBounds().width ) / 2,
        ( SCREEN_HEIGHT - sprite.getLocalBounds().height ) / 2 );
    sprite.setColor( color );

    //since you still aren't sure and I don't know a better way to check
    std::cout << "sprite " << i << " is using texture of " << i
              << " which had a name of " << path << std::endl;
    //if you have a getter to check any of this now would be a good time.
}

Alternative

1. Use a vector of pointers
2. Use a list or a dequeue instead of a vector