2

I have a Layer class which contains certain objects like buttons, sliders etc. I've built the Layer class so that it's capable of refreshing itself. Essentially, it deletes all/most of its objects and then recreates them. The reason for that is that I have various functionalities like changing font size or the theme of my UI and I want to have the changes be apparent immediately.

The only way to accomplish this consistently is to simply rebuild everything, because the themes and font sizes affect both the look and the layout of everything. This all works really well and I'm satisfied with the system.

However, issues (and by issues I mean crashes) arise when one the objects that belongs to the layer initiates the refreshing. For instance, I have a font size slider which is supposed to change the font size and then make the layer refresh. It has a callback to the refresh function of the layer, captured by a lambda, and stored in a member std::function object. What happens when the function is executed is that at some point, before the calling function is finished, the slider, of course, gets destroyed and destroys the function along with it. Thus the function that's currently running stops existing and the program crashes.

My question is what would be a good and elegant way of accomplishing this behavior that doesn't result in crashes via this paradoxical situation?

EDIT: Code

class MySlider : public MyNode
{
    //... various other class members

    std::function<void()> functionOnRelease = nullptr;

    MySlider::MySlider(/*constructor arguments*/)
    {
        this->preferenceKey = key;

        slider->addTouchEventListener([=](Ref* sender, ui::Widget::TouchEventType type)
        {
            if (type == ui::Widget::TouchEventType::ENDED)
                on_release();
        });

        //... remainder of constructor body
    }

    void MySlider::set_function_on_release(const std::function<void()>& functionOnRelease)
    {
        this->functionOnRelease = functionOnRelease;
    }

    void on_release()
    {
        if (preferenceKey.empty() == false)
            PREFM->set_preference(preferenceKey, slider->getPercent() + min);

        if (functionOnRelease)
            functionOnRelease();

        //the program crashes at this point when the slider gets destroyed
    }
}


//The layer in question and the function that creates its slider
void Options::create_font_size_slider()
{
    NodeVector sliders;

    //there's a static create function convention imposed by the engine    
    auto fontSizeSlider = MySlider::createWithPreferenceAutomatic("Font Size: ", "FontSizePercentage", 50, 150, false, sliders);

    fontSizeSlider->set_function_on_release([=] { this->refresh(); /*this kills the slider*/});
}

EDIT 2:

I've managed to avert the crash by a simple and ugly hack. I've forced the refresh() function to happen on a separate thread after a slight delay (0.05 seconds). This gives enough time for everything to happen without the delay being noticeable. I can live with the potential occasional crash in the event of a really slow device and strange thread scheduling since everything that needs to be done gets done before the crash can occur.

user8145466
  • 88
  • 9
  • 1
    Recreating every object just to refresh seems a bit excessive. Can't you use something like an `update()` function on each object? – Rakete1111 Apr 07 '18 at 17:54
  • I could if the changes weren't envisioned to impact the UI as a whole. For instance, if I change the theme, the new theme has a different font and can have different sprite sizes. That means that the sizes of all objects also have to be updated which also means that their relative positions need to updated. Additionally, there are many different classes which behave in different ways so I'd have to individually tailor all the update functions. In theory, I could build such a system, but it would be extremely complicated and error prone. Rebuilding everything gets the job done perfectly. – user8145466 Apr 07 '18 at 17:58
  • I don't understand what you mean by the slider destroys the function along with it. I think you need to show us the relevant code. It does not seem very logical to me that the function that refreshes / recreates the entire layer be somehow owned by one of the classes of controls that are contained within the layer. – o_weisman Apr 07 '18 at 18:03
  • I would basically end up just recreating the current behavior in everything but name with the only benefit being that the objects never cease to exist. It would solve this particular issue with the functions, but almost the same amount of work would be executed and I'd have a huge amount of new code to maintain. – user8145466 Apr 07 '18 at 18:05
  • The function itself is not owned by the object. The object owns a pointer to it in the form of an std::function. Specifically the function call is wrapped in a lambda which also captures the pointer of the layer. This lambda gets stored in the std::function. When the object gets destroyed, so does the std::function which got called and that causes the crash. I'll post some code in the question now. – user8145466 Apr 07 '18 at 18:10
  • I'm currently employed to fix a design like this. I couldn't fix it and had to just throw it out and start over. Which GUI library are you using? – Clearer Apr 07 '18 at 18:30
  • I'm regretfully using the cocos2d-x game engine. Which is the reason why I can't really easily implement any obvious solution like threading. I thought of a workaround just now that I'll try to do. Maybe it will help you. I'll override the destroying function so it doesn't destroy the object immediately, but just set a flag. Then at very the end of function that calls the callback, if the flag to true is set I'll call the real destruction function from the parent class. Hopefully ensuring the object gets destroyed only when it's done with all it has to do. – user8145466 Apr 07 '18 at 18:37
  • Try removing the const from the std:: function parameter in the set() property and are you sure capture by value is appropriate here. I would prefer a std:: function& as a member variable. – seccpur Apr 07 '18 at 18:44
  • It's a const reference so the temporary lambda just gets moved into the member std::function so it's essentially the same as doing it by copy just without the copying. I did try it though, just in case, and it crashes just the same. The capture by value is ok because it's just capturing the Options layer pointer which does not change. All that aside, this arrangement works perfectly fine in all other cases where there's no suicide involved. – user8145466 Apr 07 '18 at 18:53
  • What does `MySlider::on_release()` want to do after it has called `functionOnRelease()`? It seems committing suicide should be the last thing it does. And if that is the case you should be OK. – Chris Drew Apr 07 '18 at 19:37
  • That's the worst part, it doesn't have to do anything. By the time it crashes, everything I wanted done was done and I couldn't care less if the object exists or not. The issue is that it doesn't commit suicide directly. It calls the refresh() function of the Layer that owns it. That function kills all of the layer's children and that's exactly what I want to happen. However it also kills the slider but the slider is still in the function call when that happens. – user8145466 Apr 07 '18 at 19:47
  • I can only reproduce a crash if I try to access a member variable after committing suicide. Otherwise it [seems to work fine](https://wandbox.org/permlink/61uddYUOKzjIvUba). – Chris Drew Apr 07 '18 at 19:48
  • I would imagine that's because you're a normal person that uses RAII and unique_pointers. My engine is based on Objective-C and doesn't do that. However, your example's been quite illuminating because it shows that C++ itself behaves as I would expect in this situation and the problem then lies in the engine which presumably tries accessing some of the destroyed stuff. So thank you, I now know in which direction to look for a solution. – user8145466 Apr 07 '18 at 19:56
  • On second viewing your example doesn't behave like my situation. Your destructor gets called after std::cout << "Slider finished doing something\n";, but my slider should get destroyed immediately (that's not the way I'd ideally want it to be, but it's the way it is). Once again, it's RAII in action, but all I can use are naked pointers and static create functions. – user8145466 Apr 07 '18 at 20:00
  • No, it gets destroyed before. There are two versions of `Slider` in my example. I've tried to [update the example to make it more clear](https://wandbox.org/permlink/kZGZ2UJcb1BdfNj5). – Chris Drew Apr 07 '18 at 20:09
  • My bad. That's very interesting then. However I've followed the debugger and determined the crash to happen in functional.h where I get an exception that the function no longer exists. So I'm not really sure what's happening. I'd blame it on msvc++ but it also happens with clang when I compile for Android. – user8145466 Apr 07 '18 at 20:10
  • Seems to be OK in [clang](https://wandbox.org/permlink/npCqx3cXeSmctDvA) and [msvc](http://rextester.com/ZINEC51868) too. I suggest you attempt to incrementally cut down your code to an [mcve](https://stackoverflow.com/help/mcve) that still crashes and in the process you might discover the problem. – Chris Drew Apr 07 '18 at 20:15
  • That would be difficult because I need the engine, the scene and layer and the classes in question in even the most minimal form. However, I've managed to hack a solution that I can live with and added an update in the question itself. Obviously I won't be posting it as an answer because it's retarded. Thank you and everyone else very much for the help! – user8145466 Apr 07 '18 at 20:19
  • Why not sending a custom event which destroys and create the UI again? – Ripi2 Apr 07 '18 at 20:25
  • That would be ideal, but I originally designed this whole refreshing thing for a different purpose entirely. The refresh() function was never meant to be called directly, instead only when coming back from other scenes. However, I'm apparently very prone to feature creep and I'm also still very much a beginner and this whole project's a learning process. One thing I've learned for sure is that cocos is a horrible engine and I just want to finish this game as painlessly as possible (thus avoid any large refactorings, though I've done plenty so far) and move on to green pastures. – user8145466 Apr 07 '18 at 20:45
  • Possible duplicate of [Deleting a std::function object within itself](https://stackoverflow.com/questions/22998364/deleting-a-stdfunction-object-within-itself) – Chris Drew Apr 07 '18 at 20:50
  • This is in my opinion a design flaw. Your slider object shouldn't call its own destructor even if it is indirectly. In fact, the slider itself shouldn't be "aware" that it should be refreshed and obviously the on_release function is called from somewhere else which is probably a better candidate for activating the refresh function of the Layer. Even if you do decide (and I think that is wrong!) that it has to be the slider that initiates the refresh, it should only raise an event which should be handled at the Layer class not via a callback. – o_weisman Apr 08 '18 at 05:15

1 Answers1

0

You could tackle this by reference counting your objects (as COM does, for example). Basically:

Retain () bumps the reference count of an object.

Release () decrements the reference count of an object, and, if it is now zero, deletes it.

An object starts life with a reference count of 1 and you never explicitly delete a reference counted object (protect the destructor).

Now, when an object wants to trigger a refresh, it can call Retain on itself first. When control returns to the object, it can call Release and then immediately return. The call to Release might delete the object, but that doesn't matter so long as it doesn't touch its instance data subsequently.

Reference counting can tackle a lot of object lifetime issues and you get to call delete (this). Fun times. See also:

http://en.cppreference.com/w/cpp/memory/shared_ptr

Paul Sanders
  • 24,133
  • 4
  • 26
  • 48