How to pass by reference properly

[freak1k]

Here is a stripped down version of what works, but isn't exactly what I want to do:

Code: [Select]


//--------------
//main.cpp
//--------------
#include "SettingsContainer.h"
#include <iostream>

int main()
{
    Blah::AllSettings settings;
    Blah::SettingsContainer container;

    container.init(settings);
    std::cout << settings.App->IsOpened();  // <- returns false because it is destroyed as soon as it is created

    while(settings.App->IsOpened())
    {
        while (settings.App->GetEvent(*settings.Event))
        {
            // Close window : exit
            if (settings.Event->Type == sf::Event::Closed)
                settings.App->Close();

            // Escape key : exit
            if ((settings.Event->Type == sf::Event::KeyPressed) && (settings.Event->Key.Code == sf::Key::Escape))
                settings.App->Close();


            // Resize event : adjust viewport
            if (settings.Event->Type == sf::Event::Resized)
                glViewport(0, 0, settings.Event->Size.Width, settings.Event->Size.Height);
        }
        settings.App->SetActive();
    }
    return EXIT_SUCCESS;
}

//--------------
//SettingsContainer.h
//--------------

#ifndef SETTINGSCONTAINER_H
#define SETTINGSCONTAINER_H

#include <SFML/Window.hpp>

namespace Blah
{
    struct AllSettings
    {
        sf::Window* App;
        sf::Event* Event;
    };

    class SettingsContainer
    {
        protected:
        private:
            AllSettings settings;
        public:
            SettingsContainer();
            virtual ~SettingsContainer();
            void init(AllSettings&);
    };
};
#endif // SETTINGSCONTAINER_H


//--------------
//SettingsContainer.cpp
//--------------

namespace Blah
{
    SettingsContainer::SettingsContainer()
    {
        //ctor
    }

    SettingsContainer::~SettingsContainer()
    {
        //dtor
    }

    void SettingsContainer::init(AllSettings& settings)
    {
        sf::Window App(sf::VideoMode(800, 600, 32), "SFML OpenGL");
        sf::Event Event;

        settings.App = &App;
        settings.Event = &Event;
    }
}

//---------------------


The problem is that I can't seem to get it to not be destroyed (which is what I believe is happening)  I would like to store the screen state in a struct that I can pass around instead of having a bunch of garbage in main().  I have added pointers in all sorts of crazy ways, but can not seem to get any of my implementations to work except for this one.  I even used new to allocate Blah::SettingsContainer, but to no avail because I could never get it to compile.

I know that this is more of a C++ question in general.  If anyone has suggestions, it would be much appreciated.  I am sure there that I have been vague in some way that I did not think of.  I am using Code::Blocks with MinGW 4.4.5 on a WinXP computer with an AMD processor if that matters.

Thank you

[Laurent]

You must clearly decide who is the owner of the window, ie. who will create, store and destroy it.
If you pass pointers and references around, but nobody really stores the window object in first place, you'll always end up with dangling pointers/references and crashes.

You should also think more object-oriented. Classes are not meant to encapsulate data, but rather services. So for example, low-level window handling (GetEvent, Display, ...) should be encapsulated in the same class that owns the window object, and forwarded to the outside with a more straight-forward API.

I don't know enough about your application to give you a solution, but you could for example have a Application class that has a sf::Window member, and provides functions such as init(), run() and cleanup().

In regular object-oriented applications (not samples or demos), main() should really just consist of instanciating some kind of Application class and let it run.

[freak1k]

Thank you for the quick response, but it did not answer my question.

I plan on having a true owner of the object, as it will be placed inside another class eventually.  I did this for clarity in the current setting to try and get the concepts worked out.

Then again, maybe my limited understanding of C++ is showing.  There are so many beginner tutorials out there, but nothing in the more advanced topics that seem to work with the way that I look at things.  I like C++, but lets face it, I am a procedural programmer that uses it just to keep things organized.

I only started messing with SFML today, and I already like the way it is laid out.  There are so many libraries that I have messed with that just don't fit what I am trying to do.  Maybe I will just have to figure out another way to do it.

edit: is there something that I need to clarify as to what I am attempting?  BTW, I usually do what you are suggesting and create run() or init() functions.  Thank you for that though.


Thank you though.

[Laurent]

is there something that I need to clarify as to what I am attempting?

Yes. If in your real application you store the window in an owner object, then what's your problem?

The piece of code that you posted showed a scope/ownership problem, and I answered that. If it's not your problem then you'll need to explain a little bit more

[freak1k]

Excuse me if it seems like I'm making too many replies, but the reason that I want a settings object that I can pass around is because if I controlled everything in one class, that class would be huge and unmanageable.

I've written part of this program a few times before (never finished) and I have found it so much easier to deal with as several classes.  One day I would like to make a finished product, but for now it is just fun coding it.  I want to spread out who does what, but in order for there to be communication among the various parts, it is better for me to have something that the parts can use to work together with.  And encapsulating that communication device also helps keep it manageable.

Or maybe I am just delusional.

edit: I will clarify by posting the broken code, I just need a few minutes to rewrite it back to the broken version.

[freak1k]

well crap, now I just feel stupid.  It appears that I had a * in the wrong spot.  After all that I may as well post what I did (the correct code not my error)  But it still does not work the way that I want.

the problem occurs after the 4th line in main() where it says "container->init(settings);"

settings->App->IsOpened() returns false the line after I try to create and store a pointer to the window.  That is the problem that I am having and trying to work through.

I know the problem is in my part of the code, not SFML itself.

Code: [Select]


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

int main()
{
    Blah::AllSettings* settings = new Blah::AllSettings;
    Blah::SettingsContainer* container = new Blah::SettingsContainer;

    container->init(settings);
   
    std::cout << settings->App->IsOpened();  // <- it is destroyed as soon as it is created

    while(settings->App->IsOpened())
    {
        while (settings->App->GetEvent(*settings->Event))
        {
            // Close window : exit
            if (settings->Event->Type == sf::Event::Closed)
                settings->App->Close();

            // Escape key : exit
            if ((settings->Event->Type == sf::Event::KeyPressed) && (settings->Event->Key.Code == sf::Key::Escape))
                settings->App->Close();


            // Resize event : adjust viewport
            if (settings->Event->Type == sf::Event::Resized)
                glViewport(0, 0, settings->Event->Size.Width, settings->Event->Size.Height);
        }

        // Set the active window before using OpenGL commands
        // It's useless here because active window is always the same,
        // but don't forget it if you use multiple windows or controls

        settings->App->SetActive();

        //some openGL calls
       
        // Finally, display rendered frame on screen
        settings->App->Display();
    }

    delete container;
    container = NULL;

    delete settings;
    settings = NULL;

    return EXIT_SUCCESS;
}


//--------------
//SettingsContainer.h
//--------------

#ifndef SETTINGSCONTAINER_H
#define SETTINGSCONTAINER_H

namespace Blah
{
    struct AllSettings
    {
        sf::Window* App;
        sf::Event* Event;
    };

    class SettingsContainer
    {
        protected:
        private:
            AllSettings settings;
        public:
            SettingsContainer();
            virtual ~SettingsContainer();
            void init(AllSettings*&);
    };
};
#endif // SETTINGSCONTAINER_H


//--------------
//SettingsContainer.cpp
//--------------

namespace Blah
{
    SettingsContainer::SettingsContainer()
    {
        //ctor
    }

    SettingsContainer::~SettingsContainer()
    {
        //dtor
    }

    void SettingsContainer::init(AllSettings*& settings)
    {
        sf::Window App(sf::VideoMode(800, 600, 32), "SFML OpenGL");
        sf::Event Event;

        settings->App = &App;
        settings->Event = &Event;
    }
}



I hope that clarifies the situation.

Nevertheless I appreciate your patience.

[Laurent]

We're back to the very first problem, and I'm sorry to give you the same answer: your sf::Window object is local to the init function, is destroyed immediately, and therefore the "settings" variable will point to invalid data.

If you really have an owner for the window in your project, that stores it for the application lifetime, you should never face this problem.

[freak1k]

Ahh, then I misunderstood your post before.  Thank you, maybe you've put me on the right track...(I had though that it belonged to the instance of SettingsContainer and that it would stick around until I decided to destroy it.  I see the problem now)

But don't get your hopes up.  

SOLVED:

changed:

Code: [Select]


    void SettingsContainer::init(AllSettings& settings)
    {
        sf::Window App(sf::VideoMode(800, 600, 32), "SFML OpenGL");
        sf::Event Event;

        settings.App = &App;
        settings.Event = &Event;
    }


to:

Code: [Select]


    void SettingsContainer::init(AllSettings& settings)
    {
        settings->App = new sf::Window(sf::VideoMode(800, 600, 32), "SFML OpenGL");
        settings->Event = new sf::Event;
    }


Woot! you were a big help, Laurent.

[Laurent]

I had though that it belonged to the instance of SettingsContainer

Nop, it belongs to the scope where it is declared, and in your code this is the init function. To make it belong to the SettingsContainer, you would have to declare it as a member variable of the class.

I think you're writing code that is too complicated for what you want to achieve. Define a class that owns the window, and pass it (by reference) wherever you need to access the window. That's it.

Code: [Select]

class Application
{
public:

    Application()
    {
        window.Create(sf::VideoMode(800, 600), "SFML OpenGL");
    }

    sf::Window window;
};

int main()
{
    Application app;

    // your code as before, except it is using app.window
}

Of course this is very bad OO design, but it sticks with what you want to do -- just a container for your application-wide variables. However I hope you'll start thinking "object", and see how this code can be improved

[Laurent]

SOLVED

This is not 100% solved, your window is never deleted and copy of SettingsContainer instances is not safe. Never use dynamic allocation (new) unless you really have to.

[freak1k]

I agree it's not 100% as it is posted, I did put in the cleanup afterwards.  I will consider your advice.