Dynamic number of houses in the parent and child tutorial

General JUCE discussion
1

Based on the parent and child component tutorial, I’m trying to generate a dynamic number of houses as a C++ exercise since I’m still fairly new to the language. I attempted this by editing the StreetComponent class as follows:

class StreetComponent : public juce::Component {
public:
    StreetComponent(int numHouses) {
        this->numHouses = numHouses;
        housePtr = new HouseComponent[numHouses];
        for (int i = 0; i < numHouses; i++) {
            addAndMakeVisible(housePtr);
            housePtr++;
        }
    }

    ~StreetComponent() {
        delete[] housePtr;
    }

    void resized() override {
        for (int i = 0; i < numHouses; i++) {
            housePtr->setBounds(50 + 100*i, 175, 50, 50);
            housePtr++;
        }
    }

private:
    HouseComponent* housePtr;
    std::vector<std::unique_ptr<HouseComponent>> housePtrVec;
    int numHouses;
};

And then in the SceneComponent Class, calling the StreetComponent constructor as follows to generate three houses:

class SceneComponent    : public juce::Component
{
public:
    SceneComponent() : street(3)
    {
        addAndMakeVisible (floor);
        addAndMakeVisible(street);
        addAndMakeVisible(sun);
    } ......

However, this is generating an exception that claims a window is being resized during a paint method, so I suppose something in my code is attempting to resize a parent window. Other than these changes, the code is the same as the final version available to download on the tutorial. If anyone can point out what’s going wrong that’d be fantastic, and I can provide more info if needed.

2

Reading this code, I guess you should start with some C++ memory management best practice. First of all, in modern C++ you should never deal with calling delete manually, neither should you use raw pointers to store owned memory. Instead always use smart pointers like std::unique_ptr. Although the error you describe doesn’t really fit into the piece of code you posted, I see some things here that will immediately lead to a crash here.

If we look into this loop

        housePtr = new HouseComponent[numHouses];
        for (int i = 0; i < numHouses; i++) {
            addAndMakeVisible(housePtr);
            housePtr++;
        }

We see that after assigning a new allocated array of houses to your housePtr member variable, you are iterating over numHouses and increasing your member variable housePtr with each iteration. This means, at the end of the constructor, your housePtr will point the the first memory location behind your array. So, all your subsequent calls to housePtr will be on some invalid memory location. And with each resize (and you should have in mind that resize will be called a lot of time when moving or resizing your window, not only once) you start right away with accessing out of bound memory. Once you have reached the destructor (if you even will reach it without a crash after all this) you then try to free some completely different memory location then the one you assigned your array to.

The memory management best practices mentioned above taken aside, you should use indexing here, e.g.

for (int i = 0; i < numHouses; ++i)
            addAndMakeVisible (housePtr[i]); // add and make visible has an overload taking references

for (int i = 0; i < numHouses; ++i) 
            housePtr[I].setBounds (50 + 100 * i, 175, 50, 50);

But as I said above, you can even do better, which is not doing manual memory management. To do so, using a container class and a smart pointer will lead to much better and safer code. Now looking at your code I see that you already added a std::vector<std::unique_ptr<HouseComponent>> which is exactly what we need! So let’s use this and get rid of new, delete and a raw pointer member. Here is a refactored version of that class how I would write it with some comments added.

class StreetComponent : public juce::Component 
{
public:
    StreetComponent (int initialNumHouses) 
      : numHouses (initialNumHouses) // since num houses doesn't change, declare it const & set it via initializer list
    {
        for (int i = 0; i < numHouses; ++i) 
        {
             // Create a HouseComponent managed by a std::unique_ptr
            auto newHouse = std::make_unique<HouseComponent>();
            
             // Add it as child
            addAndMakeVisible (*newHouse);
            
            // Let the unique ptr vector take ownership of it by moving the new house into it
            housePtrVec.push_back (std::move (newHouse));
        }
    }

    // We can also simply delete it now, all freeing is done automatically
    ~StreetComponent() = default;

    void resized() override 
    {
        // Let's use a range based loop
        int xOffset = 0;
        for (auto& house : housePtrVec) 
        {
            house->setBounds (50 + xOffset, 174, 50, 50);
            xOffset += 100;
        }
    }

private:
    std::vector<std::unique_ptr<HouseComponent>> housePtrVec;
    const int numHouses;
};
1 Like
3

Thanks for letting me know how to fix this! I’m quite a bit new to C++ (if you couldn’t already tell). I just have a couple questions if you don’t mind;

  1. For smart pointers, do you want to use them pretty much always, or only when you would reserve a block of memory with “new”? For example, would something like a circular buffer be implemented with raw pointers still?
  2. Why do you use auto in the “resized” method if we know what type we’re iterating over? Just to make it more generic/robust?
4

Smart pointers basically tie a delete call to the scope of your pointer, so you should use them in all cases where you would have needed to call delete on a raw pointer that points to heap allocated memory. There is nothing wrong in using raw pointers pointing to stack or heap memory managed somewhere else (e.g. as you said with a ring buffer etc.). But as raw pointers are the most “dangerous” thing in C++ just use them as little as possible. E.g. if you want to pass a big object to a function or a class always use a reference instead of a pointer if you don’t need the possibility to re-assign it to another object.

I personally (and many experienced C++ coders out there) follow the “almost always auto” paradigm :wink: The idea is that in case the compiler can figure out the type, use auto. There are multiple motivations behind that, one is that you are forced to write code in a way that you don’t need information on the type in order to understand it (e.g. think about good variable naming choices) and with that you usually end up with better readable code since often type names exceed the length of auto, especially if they are templated or nested ones. Another aspect is that it makes your code better maintainable in case you rename types or change the return type of a function – you don’t need to change every occurrence and deal with unintended implicit casts, since the compiler will always chose the type suited best. One exception here is that you should explicitly mark references as such, since otherwise the compiler will chose to create a copy. In this context I also tend to mark pointers explicitly, e.g. I write auto if I want to go by value, auto& if I want to go by reference and auto* if it is a pointer. But that are my personal code styling decisions, there might be others out there with other styles and you will find your own style that grows with your experience as a C++ programer :slight_smile: