1

So I am new to C++ and I am trying to use an inner class like this:

struct PreviousButton {
    SelectionScreen* ss;

    PreviousButton(SelectionScreen* sss) {
        ss = sss;
    }

    void ClickAction() {
        //images is a vector with images. in here it is empty
        ss->images;
    }
};

This inner class is inside the class SelectionScreen and I need to perform a click action and need some of the variables of the selectionscreen.

SelectionScreen:

class SelectionScreen {
public:
    void AddImage(Image img);
    std::vector<Image> images;

How I fill the vector:

Image* image = new Image{ };
AddImage(*image2);

AddImage method:

void SelectionScreen::AddImage(Image img)
{
    images.push_back(img);
}

But when I try to use the selectionscreen inside the class all of the variables are empty. But when I look into selectionscreen all of the variables are set.

The way I pass the SelectionScreen:

PreviousButton* previousButton = new PreviousButton(*ren, this);

How SelectionScreen gets initialized: (this method is called from the main)

int Program::Render() {
    bool quit = false;
    MenuScreen m = SelectionScreen{ Sdl_Renderer };
    // change current screen to selectionscreen
    ScreenController::GetInstance().ChangeMenu(m);

    while (!quit) {
    // handle user input and repaint
    }

    // delete all windows if quit is true
    SDL_DestroyRenderer(Sdl_Renderer);
    SDL_DestroyWindow(Sdl_Window);
    SDL_Quit();
    return 0;
}

Does anyone knows why my variables are empty?

Jelle van Es
  • 613
  • 6
  • 20
  • 3
    Apart from the fact that I see too many pointers, have you initialized `ss` inside `PreviousButton`s constructor? – Paolo M Oct 12 '15 at 09:33
  • 3
    There are no inner classes here. Perhaps you mean a data member of a class type. Also please describe what you mean by empty variable. – n. m. could be an AI Oct 12 '15 at 09:34
  • Sounds like he's talking about subclassing AbstactUIComponent. It's a not an inner class, it is PreviousButton's base class. – Neil M Oct 12 '15 at 09:36
  • @PaoloM I did not initialize ss, but if im right it is a pointer and it doesn't need to be initialized? – Jelle van Es Oct 12 '15 at 09:36
  • @n.m. with empty variable I mean. I have a vector in my selectionscreen class and I put 2 images in the vector. When I try to get an image out of the vector inside PreviousButton. The vector is empty, so it doesn't contain any image – Jelle van Es Oct 12 '15 at 09:37
  • @JellevanEs is this question about `SDL_Renderer& ren`? That's the only reference that I see in your code. – eerorika Oct 12 '15 at 09:43
  • @user2079303 O sorry I mistyped something I guess. Its about the SelectionScreen, passed as "this". Those variables are empty. – Jelle van Es Oct 12 '15 at 09:45
  • 1
    @JellevanEs well, if this question is not about references, then i recommend you come up with a more descriptive title and get rid of the irrelevant tag. You should also create a [mcve](http://stackoverflow.com/help/mcve). – eerorika Oct 12 '15 at 09:47
  • You never mention `vector` (the single most relevant detail) anywhere in your post and, most importantly, anywhere in your code. Please try to include relevant details in your question. – n. m. could be an AI Oct 12 '15 at 09:49
  • @n.m. I updated my question I hope it is clear enough now. – Jelle van Es Oct 12 '15 at 09:57
  • @JellevanEs When you say the `vector` is empty, how do you know this? From the debugger? Or by printing `images.size()`? – atkins Oct 12 '15 at 10:05
  • @atkins I used the debugger to check what objects were in the vector. – Jelle van Es Oct 12 '15 at 10:07
  • You say "I look here... I look there". It's not clear how exactly you look. Please describe what you actually do (e.g. "I type `foobar` in the debugger") and what actually happens (e.g. "the debugger prints `barfoo`"). – n. m. could be an AI Oct 12 '15 at 10:15
  • Unrelated but important: `mage* image = new Image{ }; AddImage(*image2);` looks like a memory leak (unless you use the pointer later on). – n. m. could be an AI Oct 12 '15 at 10:18
  • @JellevanEs There's nothing obviously wrong with `struct PreviousButton`. I would suggest showing us more details of `class SelectionScreen`, especially around the calls to `AddImage` and the creation of `previousButton` (e.g. what do you do with `previousButton` after you created it?). – atkins Oct 12 '15 at 10:21
  • @atkins i don't do anything relevant with the `previousButton`. The only thing I do after I create it is: `previousButton->SetDimensions(100, 10, 75, 75); previousButton->SetLocation(100, 282); AddUIComponent(previousButton);` This are just some things I have to do to make sure it gets drawed by SDL – Jelle van Es Oct 12 '15 at 10:56
  • @JellevanEs Actually, probably more important is how you create and store the `ScreenSelection` - can you show us that code? The problem you are seeing suggests the `ScreenSelection` has been deleted between you creating it and invoking `PreviousButton::ClickAction()`. – atkins Oct 12 '15 at 11:36
  • @atkins I added a code snippet I hope this gives you enough information. If not please let me know – Jelle van Es Oct 12 '15 at 11:44
  • @JellevanEs Yes, I think it does - see my updated answer. – atkins Oct 12 '15 at 11:53
  • @JellevanEs Sorry, think my original update was not correct. There's a further update now. Fortunately, the solution is the same, but I think my original explanation was wrong. – atkins Oct 12 '15 at 12:04
  • @atkins Thank you for your help so far. Right now I can't edit my code. As soon as I am home I will test your answer. I will let you know what happens. – Jelle van Es Oct 12 '15 at 12:07

1 Answers1

3

This isn't addressing the immediate issue you describe, but you have a serious problem here:

  ss->currentImageIndex = ss->images.capacity() - 1;

images.capacity() - 1 is not guaranteed to return a valid index into your vector. You should use images.size() - 1.

std::vector::capacity() tells you the storage space currently allocated for the vector, not the number of elements it actually contains.

UPDATE

When you assign a SelectionScreen to a MenuScreen like this

MenuScreen m = SelectionScreen{ Sdl_Renderer }; 

you "slice" the SelectionScreen object (see What is object slicing?).

This line creates a new MenuScreen object, m, by copy-constructing it from a temporary SelectionScreen object. The new MenuScreen object, m, is not a SelectionScreen. It is also a completely distinct object from the temporary SelectionScreen created on the right-hand side of this expression.

Your PreviousButton, which I assume is created in the constructor of SelectionScreen, holds a pointer to the temporary SelectionScreen, which the compiler is free to delete once this line has completed.

To fix this, you could use this initialisation instead:

MenuScreen& m = SelectionScreen{ Sdl_Renderer };

In this case, m is a reference to your (temporary) SelectionScreen - this is good because (a) references are polymorphic, so it still knows that it's really a SelectionScreen, not just a MenuScreen and (b) it's a reference to the exact same SelectionScreen object created on the right-hand side, which your PreviousButton has a pointer to. I've also bracketed the "temporary" bit now, because taking a local reference to a temporary guarantees it will exist for as long as the reference does (see e.g. Does a const reference prolong the life of a temporary?).

Note that I am assuming here that you only make use of this SelectionScreen within the while (!quit) loop that follows, i.e. the screen can safely be deleted at the end of this int Program::Render() method.

Community
  • 1
  • 1
atkins
  • 1,963
  • 14
  • 27
  • Thanks for pointing this out. Till now it worked great but I didn't know this. I will change it. – Jelle van Es Oct 12 '15 at 10:09
  • Thank you very much your solution works. Just one more question. `MenuScreen& m = SelectionScreen{}` works. But does it also work if I just do `SelectionScreen m = SelectionScreen{}` ? – Jelle van Es Oct 12 '15 at 16:02
  • 1
    That will "almost certainly" also work. When the type on the left of `=` is the same as the type constructed on the right the compiler will is allowed to use "copy elision" which means no temporary will be constructed (I'll refer you to my question on that subject here! http://stackoverflow.com/q/33086346/995218). However, if you want to be absolutely sure you could just write `SelectionScreen m{Sdl_Renderer};`, to make it explicit that you don't expect a copy constructor to be called. – atkins Oct 12 '15 at 17:19
  • 1
    By the way, I also discovered today that `MenuScreen& m = SelectionScreen{ Sdl_Renderer };` is not supported by most compilers - I think only Visual Studio will allow it. Sounds like it worked for you, but if you want to be sure your code is portable you should use `const MenuScreen& m = SelectionScreen{ Sdl_Renderer };` i.e. use a `const` reference. However, that may cause a problem if `ScreenController::ChangeMenu()` requires a non-const reference. Anyway, `SelectionScreen m{Sdl_Renderer};` solves all these problems, so I would just go with that. – atkins Oct 12 '15 at 17:24