Welcome, Guest. Please login or register. Did you miss your activation email?

Author Topic: Memory management  (Read 8976 times)

0 Members and 1 Guest are viewing this topic.

ScArL3T

  • Newbie
  • *
  • Posts: 32
    • View Profile
Memory management
« on: August 21, 2015, 05:36:08 pm »
This is not really a SFML-related post but I would really appreciate your help.

At the moment, I'm using a std::vector of std::unique_ptr.
I push state objects created with std::make_unique using std::move when I want to change state and then I erase the stack.

I'm testing things out using 2 states: one has some textures in it and the other one is empty.
What happens when I repeatedly switch between states is that memory usage increases per switch.

I also tried making the textures raw pointers. When I manually delete them before switching states there is no increase in memory. But if I try to make them shared pointers or unique pointers the memory will still increase.
I'm curious how should I handle these states and pointers.

I change states using this method:
void StateManager::changeState(std::unique_ptr<State> state)
{
    //If the vector isn't empty destroy the current states
    //Clear the vector
    if (states.empty() == false)
    {
        for (auto i = 0; i < states.size(); i++)
        {
            states[i]->destroy(*window);
        }
        states.clear();
    }

    //Move the new state inside the vector
    states.push_back(std::move(state));

    //Initiate the state
    if (states.empty() == false)
    {
        states[MAINSTATE]->init(*window); //MAINSTATE = 0
    }
}

eXpl0it3r

  • SFML Team
  • Hero Member
  • *****
  • Posts: 11030
    • View Profile
    • development blog
    • Email
AW: Memory management
« Reply #1 on: August 21, 2015, 08:06:31 pm »
Create a minimal and compilable example that reproduces the issue, otherwise it can be anything or nothing at all, we wouldn't know.
Official FAQ: https://www.sfml-dev.org/faq.php
Official Discord Server: https://discord.gg/nr4X7Fh
——————————————————————
Dev Blog: https://duerrenberger.dev/blog/

ScArL3T

  • Newbie
  • *
  • Posts: 32
    • View Profile
Re: Memory management
« Reply #2 on: August 21, 2015, 09:07:25 pm »
This is my State class:
#ifndef STATE_H
#define STATE_H

#include "Globals.h"

class State
{
public:
        virtual void init(sf::RenderWindow &window) = 0;
        virtual void update(sf::RenderWindow &window, float dt) = 0;
        virtual void draw(sf::RenderWindow &window) = 0;
        virtual void handleEvents(sf::RenderWindow &window, sf::Event &event) = 0;
        virtual void destroy(sf::RenderWindow &window) = 0;
};

#endif

And this is my MenuState class:
#include "MenuState.h"
#include "PlayState.h"

void MenuState::init(sf::RenderWindow &window)
{
        WIDTH = window.getSize().x;
        HEIGHT = window.getSize().y;
        std::cout << "MENUSTATE" << std::endl;
        playState = false;

        m_textureNormal = std::make_shared<sf::Texture>();
        m_textureNormal->loadFromFile("data/texture.png", sf::IntRect(0, 0, 64, 64));

        m_textureFlag = std::make_shared<sf::Texture>();
        m_textureFlag->loadFromFile("data/texture.png", sf::IntRect(128, 0, 64, 64));

        m_textureBomb = std::make_shared<sf::Texture>();
        m_textureBomb->loadFromFile("data/texture.png", sf::IntRect(192, 0, 64, 64));
       
        m_textureClicked = std::make_shared<sf::Texture>();
        m_textureClicked->loadFromFile("data/texture.png", sf::IntRect(256, 0, 64, 64));
       
}

void MenuState::update(sf::RenderWindow &window, float dt)
{
       
}

void MenuState::draw(sf::RenderWindow &window)
{

}

void MenuState::handleEvents(sf::RenderWindow &window, sf::Event &event)
{
        if (event.type == sf::Event::KeyPressed && event.key.code == sf::Keyboard::P)
                state_manager.changeState(std::unique_ptr<State>(std::make_unique<PlayState>()));
}

void MenuState::destroy(sf::RenderWindow &window)
{
        /*delete m_textureNormal;
        delete m_textureFlag;
        delete m_textureBomb;
        delete m_textureClicked;*/

       
}

I also have a PlayState but that class is empty.
If I switch to PlayState and then back to MenuState the RAM increases by ~5mb and keeps doing so. One quick fix was to use raw pointers for the textures and then delete them in the "destroy" method but I don't like the idea of using them because it is "bad practice".

The most ideal solution would be to not use pointers at all but even if I don't use raw/smart pointers for the textures the RAM still increases.

The only solution as of right now is to use raw pointers. I'm asking if there is any other fix.

eXpl0it3r

  • SFML Team
  • Hero Member
  • *****
  • Posts: 11030
    • View Profile
    • development blog
    • Email
AW: Memory management
« Reply #3 on: August 21, 2015, 09:17:50 pm »
Well the code is neither minimal nor compilable. So I still can't say what the issue could be. ;)
Official FAQ: https://www.sfml-dev.org/faq.php
Official Discord Server: https://discord.gg/nr4X7Fh
——————————————————————
Dev Blog: https://duerrenberger.dev/blog/

ScArL3T

  • Newbie
  • *
  • Posts: 32
    • View Profile
Re: Memory management
« Reply #4 on: August 21, 2015, 09:38:17 pm »
I'm pretty sure this is compilable:
https://github.com/ScArL3T/Leak

You can take a look at the State header, StateManager class and then MenuState. Basically just the code I put up there.

Hiura

  • SFML Team
  • Hero Member
  • *****
  • Posts: 4321
    • View Profile
    • Email
Re: Memory management
« Reply #5 on: August 22, 2015, 09:05:28 am »
SFML / OS X developer

GraphicsWhale

  • Full Member
  • ***
  • Posts: 131
    • View Profile
Re: Memory management
« Reply #6 on: August 22, 2015, 02:33:47 pm »
It appears as if your memory leak is caused by the lack of a virtual destructor in the State class. Your derived classes can't generate a constructor that can be called, which means your `shared_ptr<sf::Texture>`s aren't being properly deleted. Your other members might not be deleted properly as well, but I can't tell as easily.

Also:

1. You don't need to call std::make_unique if you're using std::unique_ptr's constructor. Use either std::unique_ptr<MyClass> myptr(new MyClass(myparam)); or std::unique_ptr<MyClass> myptr = std::make_unique<MyClass>(myparam);.

2. Prefer for (std::unique_ptr<State>& state : states) { ... } to for (int i = 0; i < vec.size(); i++) { ... }.

3. Don't use shared_ptr to store your textures unless you have to.

4. Prefer if (!states.empty()) { ... } to if (states.empty() == false) { ... }.

5. Why would you ever use win32 functions in an SFML application? Maybe message boxes, but definitely not to draw things.

6. State managers typically use a state-stack. You're drawing and updating all of your states at once. Why?

7. std::vectors have these neat functions called std::vector::front and std::vector::back. Please learn to use them.

8. If you're going to embed Lua into your game (to anyone who hasn't seen the source, there's a todo-list comment), please get an actual working game first. Embedding a scripting language is one of the things you do to a working engine, not a barely-made game that has a memory leak. Scripting languages are difficult to embed, and you're not going to get any benefits from them until you know exactly where you need them. If you want to venture into the land of Lua, go ahead, but it's a fast way to nuke your project.
« Last Edit: August 22, 2015, 05:11:07 pm by GraphicsWhale »

ScArL3T

  • Newbie
  • *
  • Posts: 32
    • View Profile
Re: Memory management
« Reply #7 on: August 22, 2015, 05:58:56 pm »
I know my so-called "engine" has many flaws but it's just a prototype to just test things out for now. I was going to update them all because I thought I could make it easier if for example I have an InventoryState and then I push it in the vector. That way the animations from the game will still play, but only the Inventory's events will be managed. But again, that's just an "idea".

I implemented the virtual destructor and everything works fine now. Thank you!

I know I shouldn't be using pointers for my textures or any other class members, only if I have to. I was testing things out to see if any of the smart pointers worked properly, but it seems the problem was the virtual destructor.

About the Win32 functions. I think you're referring to the splashscreen class. I found that implementation on google a while ago (not too sure but I guess there was a topic about that on SFML's forum). Basically it allows transparency. Since it's impossible to do that with SFML I only found some solutions using OpenGL and this one that I use.

Also, I don't know why would I prefer the "new" keyword over the "std::make_unique" since that's a nice feature C++14 brings. Maybe I'm missing someting, I will research more about it. Thank you again for your time!
« Last Edit: August 22, 2015, 06:04:29 pm by ScArL3T »

GraphicsWhale

  • Full Member
  • ***
  • Posts: 131
    • View Profile
Re: Memory management
« Reply #8 on: August 22, 2015, 07:16:35 pm »
I know my so-called "engine" has many flaws but it's just a prototype to just test things out for now. I was going to update them all because I thought I could make it easier if for example I have an InventoryState and then I push it in the vector. That way the animations from the game will still play, but only the Inventory's events will be managed. But again, that's just an "idea".


What you could possibly do instead is add some pause and resume functions to your state, and give your states access to your state manager (pass a reference on each function call). What this could allow you to do is check if the newest state is an inventory state (via using dynamic_cast on it's pointer), and rather that halting the gameplay (which you could do if the state is something else), you could call some special function your inventory state has to pass a reference to your in-game state and have your inventory manually update your in-game state itself, but under special conditions (such as disabling input to your player while in the inventory menu, darkening things around the inventory menu, or whatever).

It's a bit of a "hack", don't recall ever trying it, so I might be missing something.

About the Win32 functions. I think you're referring to the splashscreen class. I found that implementation on google a while ago (not too sure but I guess there was a topic about that on SFML's forum). Basically it allows transparency. Since it's impossible to do that with SFML I only found some solutions using OpenGL and this one that I use.

The win32 API isn't portable. Even if you're only planning on using it on Windows, adjusting something like transparency isn't worth giving up platform-independent code.

I'm not sure exactly what your goals were, but to adjust an image's transparency, use this function (sorry if it doesn't work properly, just quickly wrote it with little testing):

/*
   Takes an image and sets it's transparency relative to the parameter "level".

   For parameter "level":
   0.0f means no transparency, 1.0f means normal transparency, 2.0f means double transparency

   This is non-reversible if set to 0. Setting transparency to 0 will make it invisible and will no amount
   of multiplication can bring it back.
*/

void setImageTransparancy(sf::Image& image, float level)
{
    for (unsigned x = 0; x < image.getSize().x; x++)
    {
        for (unsigned y = 0; y < image.getSize().y; y++)
        {
            sf::Color pixel_color = image.getPixel(x, y); //Retrieve the pixel color
            float fval = (pixel_color.a / 255.0f); //Convert the 0-255 range integer to a 0-1 range float
            fval *= level; //Scale the float alpha with the level
            fval = fval < 0.0f ? 0.0f : fval > 1.0f ? 1.0f : fval; //Ensure the new value isn't below 0 or above 1
            pixel_color.a = (uint8_t)(fval * 255.0f); //Convert it back to a 0-255 integer and set it as the alpha
            image.setPixel(x, y, pixel_color); //Change the pixel to it's new color
        }
    }
}
 

However, I'd recommend simply drawing an sf::RectangleShape and setting it's fill color to whatever transparency you need, as it wont actually change the texture itself.

Also, I don't know why would I prefer the "new" keyword over the "std::make_unique" since that's a nice feature C++14 brings. Maybe I'm missing someting, I will research more about it.

IIRC std::make_unique can allocate the smart pointer (I believe the reference counter variable, not the smart pointer itself, which is on the stack) and your object in the same allocation, possibly providing better performance. But for small objects, the stack is always the best option. For things like textures, while storing them on the stack is not necessarily a good idea (if it'd even fit), I'm pretty sure SFML allocates it's memory for them on the heap, avoiding this problem.

ScArL3T

  • Newbie
  • *
  • Posts: 32
    • View Profile
Re: Memory management
« Reply #9 on: August 22, 2015, 07:57:10 pm »
What you could possibly do instead is add some pause and resume functions to your state, and give your states access to your state manager (pass a reference on each function call). What this could allow you to do is check if the newest state is an inventory state (via using dynamic_cast on it's pointer), and rather that halting the gameplay (which you could do if the state is something else), you could call some special function your inventory state has to pass a reference to your in-game state and have your inventory manually update your in-game state itself, but under special conditions (such as disabling input to your player while in the inventory menu, darkening things around the inventory menu, or whatever).

That's an interesting idea, thanks for sharing I will look into it!

I'm not sure exactly what your goals were

I was aiming for window transparency. Sorry if what I said earlier wasn't clear enough. Basically I know that SFML can't do that (correct me if I'm wrong) so the only solutions I had at the moment were using OpenGL or win32.


So, going back to the std::unique_ptr.
Basically if I have this method:
void foo(std::unique_ptr<int> a, std::unique_ptr<int> b);

I should use one of the following:
foo(std::unique_ptr<int>(new int(5)), std::unique_ptr<int>(new int(42)));
foo(std::make_unique<int>(5), std::make_unique<int>(42));

Is that what you were trying to say?

GraphicsWhale

  • Full Member
  • ***
  • Posts: 131
    • View Profile
Re: Memory management
« Reply #10 on: August 22, 2015, 08:32:39 pm »
I was aiming for window transparency.

Why exactly would you want to do that?


So, going back to the std::unique_ptr.
Basically if I have this method:
void foo(std::unique_ptr<int> a, std::unique_ptr<int> b);

I should use one of the following:
foo(std::unique_ptr<int>(new int(5)), std::unique_ptr<int>(new int(42)));
foo(std::make_unique<int>(5), std::make_unique<int>(42));

Is that what you were trying to say?

What I was saying is using std::make_unique is always better than not as it may enable memory allocation optimizations, whereas giving it a pointer to an already-allocated object cannot.

But preferably, lay off the use of dynamically-allocated memory. In most scenarios, it's not required. It's slower than stack memory, less cache friendly, more prone to mistakes. std::shared_ptrs are even worse because they use reference counting, so every time one is copy-constructed, exits the scope, or assigned, it must do additional work, and that's not even mentioning circular references.
« Last Edit: August 22, 2015, 08:35:56 pm by GraphicsWhale »

ScArL3T

  • Newbie
  • *
  • Posts: 32
    • View Profile
Re: Memory management
« Reply #11 on: August 22, 2015, 08:44:47 pm »
I was aiming for window transparency.

Why exactly would you want to do that?


I though it would be cool to use a transparent splashscreen.
If I ever need my program to be cross-platform, I'll find another solution or simply delete the class.

But preferably, lay off the use of dynamically-allocated memory.

Well, I kind of need pointers in my case because the State class is an abstract class and I read that references are not recommended (?). I find std::unique_ptr to be the best solution for me ATM since it can take care of the clean-up itself and the vector will be the only one that can access it once it is moved.

2. Prefer for (std::unique_ptr<State>& state : states) { ... } to for (int i = 0; i < vec.size(); i++) { ... }.

I cannot use the range-based for loop since the iterator must be updated. I tested this out, when I try to enter the PlayState for example, I get an error when the state is getting pushed back: Vector iterator not incrementable. The only way for this to work is to use:
for (auto i = 0; i < states.size(); i++)
Or:
for (auto it = states.begin(); it != states.end();  ++it )

EDIT: The second for loop gives the same error as the range-based. So the first for is the only one that's working.
« Last Edit: August 22, 2015, 10:32:56 pm by ScArL3T »

Hapax

  • Hero Member
  • *****
  • Posts: 3379
  • My number of posts is shown in hexadecimal.
    • View Profile
    • Links
Selba Ward -SFML drawables
Cheese Map -Drawable Layered Tile Map
Kairos -Timing Library
Grambol
 *Hapaxia Links*

ScArL3T

  • Newbie
  • *
  • Posts: 32
    • View Profile

GraphicsWhale

  • Full Member
  • ***
  • Posts: 131
    • View Profile
Re: Memory management
« Reply #14 on: August 23, 2015, 06:37:41 am »
Well, I kind of need pointers in my case because the State class is an abstract class and I read that references are not recommended (?). I find std::unique_ptr to be the best solution for me ATM since it can take care of the clean-up itself and the vector will be the only one that can access it once it is moved.

But that wasn't the only thing you were using it for.

If an object:

- Is too large for the stack (e.g. a 100x100 array of ints)
- Is polymorphic (e.g. your State class)
- Needs to live outside of the scope (e.g. returning an array)

Then dynamic memory is fine. Otherwise, it belongs on the stack. And even if it is fine, remember it still is a lot more expensive than stack allocations and is very cache-unfriendly (however, using it for a state class might not be too expensive because often-used allocations are much more likely to be cached, but the virtual functions still have a cost).

If you need to pass a function a pointer, it's fine to pass it the pointer using std::unique_ptr::get instead of the smart pointer itself. It makes the code a whole lot less "ugly".

And shared_ptrs should be reserved for when you must have the same object referenced from across various parts of the program but you're not quite sure who will be the last one to use it. So very rarely.

I know you probably knew at least some of that, but it's very important to not go throwing pointers at everything.

I cannot use the range-based for loop since the iterator must be updated. I tested this out, when I try to enter the PlayState for example, I get an error when the state is getting pushed back: Vector iterator not incrementable. The only way for this to work is to use:
for (auto i = 0; i < states.size(); i++)
Or:
for (auto it = states.begin(); it != states.end();  ++it )
EDIT: The second for loop gives the same error as the range-based. So the first for is the only one that's working.

Would you mind giving some context to this error? Because that's no supposed to happen.