Crash when auto resize a custom Widget

Started by Nafffen, 21 December 2022, 16:25:46

Nafffen

Hello,
I have a custom Widget:
class ItemContainerWidget : public tgui::WidgetI want to have this widget auto resize one of its edge when the user call setSize.
For example:
auto itemContainerW = ItemContainerWidget::create(ItemContainerWidget::AlignmentSide::VERTICAL, 2, m_listItemStack);
itemContainerW->setSize(800, tgui::bindHeight(m_scrollPanel)-m_scrollPanel->getScrollbarWidth());
800 here is no relevant as I want the x size to be auto detect in setSize method of ItemContainerWidget

That's the beginning of the method
void ItemContainerWidget::setSize(const tgui::Layout2d& size){

    cout << "before" << endl;
    cout << "size.getValue(): " << size.getValue() << endl;

    if(m_alignmentSide == AlignmentSide::HORIZONTAL){
        m_sizeItemRect = size.getValue().x/m_nbItemPerAlignmentSide;
    }else if(m_alignmentSide == AlignmentSide::VERTICAL){
        m_sizeItemRect = size.getValue().y/m_nbItemPerAlignmentSide;
    }
    cout << "m_sizeItemRect: " << m_sizeItemRect << endl;

    unsigned nbItemPerNonAlignmentSide = std::ceil(m_listIS.size()/(float)m_nbItemPerAlignmentSide);
    cout << "nbItemPerNonAlignmentSide: " << nbItemPerNonAlignmentSide << endl;

    //update size accordingly to m_nbItemPerAlignmentSide
    tgui::Layout2d newSize = size;
    if(m_alignmentSide == AlignmentSide::HORIZONTAL){
        newSize.y = nbItemPerNonAlignmentSide*m_sizeItemRect;
    }else if(m_alignmentSide == AlignmentSide::VERTICAL){
        newSize.x = nbItemPerNonAlignmentSide*m_sizeItemRect;
    }
    cout << "newSize: " << newSize.toString() << endl;
    Widget::setSize(newSize);

    .... change pos and size of some stuff ....

So it's seems to be ok, but my application crashes sometimes when I resize window, sometimes the behavior is what I expect it to be.
Even my VScode debug mode can't find the function that's crashing.
I didn't find any crash pattern, it seems to be totally random for me.

In the above example, I want the height of itemContainerW  to be proportional to another widget's height, so I use bindHeight, and the width auto set by setSide.
Then setSide just change the size according to AlignementSide variable before send it to parent Widget.

Did I miss somehting with this layout utilisation ?

update: It seems to crash more often if I resize the window smaller at first
If I resize it larger, it seems to be working as long as I do not go with a window too small...

Thank you

texus

The only reason for the layout system to crash that I am aware of, is if you create a circular dependency. For example if the height of m_scrollPanel is changed in your setSize function or is bound to the ItemContainerWidget.

Do you get a call stack when it crashes? Does it crash inside the setSize function?

Nafffen

Crash occurs after ItemContainerWidget::setSize, no recursion in this method but probably maybe somewhere else, but I can't find it. I think it's recursion issue as there is no call stack, no error output just crash. In debug mode of vsc, the app stays without any response, it's another hint that's recursion issue.

I tried to look into Widget::setSize(size) but I don't understand very well what is happenning, does this function recall setSize of my custom widget ItemContainerWidget ?

the scrollPanel is juste linked to the size of gui, m_scrollPanel->setSize("100%", "20%");
If this is a circular dependency issue, why is the crash not happenning every time ?

texus

QuoteIf this is a circular dependency issue, why is the crash not happenning every time ?
One explanation would be caching and rounding errors. Imagine if widget A becomes 100px and it tells object B to become 200px (because it used bindSize(A)*2 as its size). Now imagine A having a size defined as bindSize(B)/2. When B becomes 200px, it will tell A that is should be 100px in size. Instead of creating an endless loop, A will already be 100px and will ignore the event.
If A was set to a size of bind(B)/4 then the size would keep changing and you get an infinite loop. But even if it uses /2, if due to a float rounding it becomes 99.99999 instead of 100, it could also trigger another loop. So with certain numbers everything would be fine, but with other numbers you would get a slight rounding error and suddenly things would go wrong.

That is the theoretical explanation as to why it could be random. I however don't actually believe that what I described is happening.

Quotethink it's recursion issue as there is no call stack, no error output just crash. In debug mode of vsc, the app stays without any response, it's another hint that's recursion issue.
Recursions usually do end up with a call stack, as you get a crash at a maximum recurse depth (which happens almost immediately when you have an endless recursion).
If you see no crash but the app becomes unresponsive, then maybe it entered an endless for or while loop? When it hangs, is the program still running? If so, can you pause the program with the debugger at that time?

Nafffen

#4
Thank you for your precision,

QuoteRecursions usually do end up with a call stack, as you get a crash at a maximum recurse depth
Yes it crashes, it is definitely a recursion issue. In debugger mode, it seems to be stuck in Widget.cpp:567 in setSize function. m_boundSizeLayouts seems to be endless
for (auto& layout : m_boundSizeLayouts)
    layout->recalculateValue();
however, it is not in the call of Widget::setSize by my custom widget, it's after.

Basically, my layout can be simplified as:
gui
->  ScrollablePanel with
    m_scrollPanel->setPosition("0%", "80%");
    m_scrollPanel->setSize("100%", "20%");

    --> ItemContainerWidget (my custom widget) with
        itemContainerW->setSize(800, tgui::bindHeight(m_scrollPanel)-m_scrollPanel->getScrollbarWidth());


m_scrollPanel is not modified in ItemContainerWidget's setSize, the code is the same as my first post.

Well I obviously don't get something in here... When we resize the window, wich setSize is called first, the child widgets, or parent ones ?

texus

m_boundSizeLayouts being endless might imply data corruption. If somehow a callback gets triggered to a widget that no longer exists, then m_boundSizeLayouts would contain garbage and it could crash there. But I guess there are probably ways to get stuck there without corruption as well.

The parent size is changed first. The order of events is as follows:
- You resize the window
- The gui view is updates, which updates the size of the RootContainer (which is the part of the Gui to which all widgets are added)
- m_scrollPanel's size is part of the m_boundSizeLayouts of the RootContainer (because you called m_scrollPanel->setSize("100%", "20%")), so RootContainer::setSize calls recalculateValue which calls ScrollablePanel::setSize
- ScrollablePanel::setSize will call Widget::setSize to begin applying its size
- The layout of itemContainerW is bound to ScrollablePanel, so this will call ItemContainerWidget::setSize (via the recalculateValue call again).
- Your ItemContainerWidget::setSize function should call Widget::setSize like you do in the code you showed
- The m_boundSizeLayouts of your widget should be empty, if no other widget has bound the size of ItemContainerWidget, so no further calls are made.
- After Widget::size finished, your ItemContainerWidget::setSize function you can rely on the m_size variable to contain the current widget size.
- Each mentioned function will now finish in opposite order, i.e. the remaining code of ItemContainerWidget::setSize will be executed before setting the size of ScrollablePanel in ScrollablePanel::setSize finished.

So one thing you could still test is to check that m_boundSizeLayouts is really empty in ItemContainerWidget::setSize.

If you replace "itemContainerW->setSize(800, tgui::bindHeight(m_scrollPanel)-m_scrollPanel->getScrollbarWidth());" with just "itemContainerW->setSize(800,600)", does the program work fine?

I don't see anything wrong based on your descriptions, there is probably some small mistake somewhere. Could you perhaps share your code so that I can try to reproduce it here?

Nafffen

Ok I understand thank you
QuoteYour ItemContainerWidget::setSize function should call Widget::setSize like you do in the code you showed
Yes, the fact that I change the size before passing it to Widget::setSize could be the problem ?
Program works fine if I don't change size before passing it to Widget::setSize but that is not what I want. How it is managed with Label and its auto size feature ? I looked into it and if we use Label::setSize, autoSize is disabled, the size of Widget seems to be modified directly with setText function if I understand well, that's not really what I want.
The modification is as follows:
tgui::Layout2d newSize = size;
    if(m_alignmentSide == AlignmentSide::HORIZONTAL){
        newSize.y = nbItemPerNonAlignmentSide*m_sizeItemRect;
    }else if(m_alignmentSide == AlignmentSide::VERTICAL){
        newSize.x = nbItemPerNonAlignmentSide*m_sizeItemRect;
    }
    cout << "newSize: " << newSize.toString() << endl;
    Widget::setSize(newSize);
So it changes one of the edge with a direct value computed, the other one is still a bounded value.

QuoteIf you replace "itemContainerW->setSize(800, tgui::bindHeight(m_scrollPanel)-m_scrollPanel->getScrollbarWidth());" with just "itemContainerW->setSize(800,600)", does the program work fine?
Yes it worked, as I remember, I tried this and I passed the size unmodified to Widget::setSize, I will try this evening with modification before passing.

QuoteSo one thing you could still test is to check that m_boundSizeLayouts is really empty in ItemContainerWidget::setSize.
I will this evening
QuoteCould you perhaps share your code so that I can try to reproduce it here?
I will this evening if I don't find a solution

texus

Quotethe fact that I change the size before passing it to Widget::setSize could be the problem
No. I saw you were doing that earlier, but there is no reason why it wouldn't work.

QuoteHow it is managed with Label and its auto size feature ?
As long as you don't call setSize on the label, changing the text will trigger an update to the size at src/Widgets/Label.cpp#L699
So no layouts are used for the auto-sizing, it just recalculates the size each time the text is updated.

Nafffen

there it is, the reproductible crash in one file
#include <TGUI/TGUI.hpp>
#include <TGUI/Backend/SFML-Graphics.hpp>

#include <iostream>
#include <cmath>

using namespace std;

struct ItemStack{
    tgui::Texture texture = tgui::Texture("iron.png");
    int nb = 1;
};


class ItemContainerWidget : public tgui::Widget
{
public:
    enum AlignmentSide{
        HORIZONTAL,
        VERTICAL
    };

    // Add a Ptr typedef so that "MyCustomWidget::Ptr" has the correct type
    typedef std::shared_ptr<ItemContainerWidget> Ptr;
    typedef std::shared_ptr<const ItemContainerWidget> ConstPtr;

    // Add functions that are explained in "Basic functionality" section

    // The constructor should create the subwidgets and add them to the internal container
    ItemContainerWidget(AlignmentSide alignmentSide, unsigned nbItemPerAlignmentSide, const std::vector<ItemStack> &listIS, const char* typeName = "ItemContainerWidget", bool initRenderer = true):
    tgui::Widget(typeName, true),
    m_alignmentSide(alignmentSide),
    m_nbItemPerAlignmentSide(nbItemPerAlignmentSide)
    {
        for(const auto &is : listIS){
            addItem(is);
        }
        updateSize();
    }

    void addItem(const ItemStack &is){
        itemStruct item;

        item.is = is;

        item.sprite.setTexture(is.texture);
        item.text.setString(std::to_string(is.nb));
        //TODO override setOpacity to handle change of color
        item.text.setFont(m_fontCached);
        item.text.setColor(tgui::Color::applyOpacity(tgui::Color::White, m_opacityCached));
        item.text.setOutlineColor(tgui::Color::applyOpacity(tgui::Color::Black, m_opacityCached));

        item.selected = false;

        m_listItem.push_back(item);
    }

    // Every widget needs a static create function which creates the widget and returns a smart pointer to it.
    // This function can have any parameters you want.
    static ItemContainerWidget::Ptr create(AlignmentSide alignmentSide, unsigned nbItemPerAlignmentSide, const std::vector<ItemStack> &listIS)
    {
        return std::make_shared<ItemContainerWidget>(alignmentSide, nbItemPerAlignmentSide, listIS);
    }

    // Every widget has a static method to copy it
    static ItemContainerWidget::Ptr copy(ItemContainerWidget::ConstPtr widget)
    {
        if (widget)
            return std::static_pointer_cast<ItemContainerWidget>(widget->clone());
        else
            return nullptr;
    }

    void setSize(const tgui::Layout2d& size){
        // cout << "before" << endl;
        // cout << "size.getValue(): " << size.getValue() << endl;

        if(m_alignmentSide == AlignmentSide::HORIZONTAL){
            m_sizeItemRect = size.getValue().x/m_nbItemPerAlignmentSide;
        }else if(m_alignmentSide == AlignmentSide::VERTICAL){
            m_sizeItemRect = size.getValue().y/m_nbItemPerAlignmentSide;
        }
        // cout << "m_sizeItemRect: " << m_sizeItemRect << endl;

        unsigned nbItemPerNonAlignmentSide = std::ceil(m_listItem.size()/(float)m_nbItemPerAlignmentSide);
        // cout << "nbItemPerNonAlignmentSide: " << nbItemPerNonAlignmentSide << endl;

        //update size accordingly to m_nbItemPerAlignmentSide
        tgui::Layout2d newSize = size;
        if(m_alignmentSide == AlignmentSide::HORIZONTAL){
            newSize.y = nbItemPerNonAlignmentSide*m_sizeItemRect;
        }else if(m_alignmentSide == AlignmentSide::VERTICAL){
            newSize.x = nbItemPerNonAlignmentSide*m_sizeItemRect;
        }
        // cout << "newSize: " << newSize.toString() << endl;
        Widget::setSize(newSize);
       
        updateSize();
    }

    void updateSize(){
        // cout << "m_size: " << m_size.toString() << endl;
        // cout << "m_size.getValue(): " << m_size.getValue() << endl;
       
        m_borders = tgui::Borders{m_sizeItemRect/20};
        auto sizeInsideBorder = tgui::Vector2f{m_sizeItemRect - m_borders.getLeft() - m_borders.getRight(), m_sizeItemRect - m_borders.getTop() - m_borders.getBottom()};

        for(unsigned i=0;i<m_listItem.size();i++){
            m_listItem[i].sprite.setSize(sizeInsideBorder);
            m_listItem[i].text.setCharacterSize(m_sizeItemRect/4);
            m_listItem[i].text.setOutlineThickness(1+m_listItem[i].text.getCharacterSize()/25);
           
            if(m_alignmentSide == AlignmentSide::HORIZONTAL){
                m_listItem[i].sprite.setPosition(tgui::Vector2f{(i%m_nbItemPerAlignmentSide)*m_sizeItemRect, (i/m_nbItemPerAlignmentSide)*m_sizeItemRect}+m_borders.getOffset());
                m_listItem[i].text.setPosition(tgui::Vector2f{(i%m_nbItemPerAlignmentSide)*m_sizeItemRect, (i/m_nbItemPerAlignmentSide)*m_sizeItemRect}+m_borders.getOffset()+sizeInsideBorder-m_listItem[i].text.getSize());
            }else if(m_alignmentSide == AlignmentSide::VERTICAL){
                m_listItem[i].sprite.setPosition(tgui::Vector2f{(i/m_nbItemPerAlignmentSide)*m_sizeItemRect, (i%m_nbItemPerAlignmentSide)*m_sizeItemRect}+m_borders.getOffset());
                m_listItem[i].text.setPosition(tgui::Vector2f{(i/m_nbItemPerAlignmentSide)*m_sizeItemRect, (i%m_nbItemPerAlignmentSide)*m_sizeItemRect}+m_borders.getOffset()+sizeInsideBorder-m_listItem[i].text.getSize());
            }
        }

        // cout << "after" << endl;
    }

    // The isMouseOnWidget function should return true if the mouse is on top of the widget
    bool isMouseOnWidget(tgui::Vector2f pos) const
    {
        // The following assumes that the widget is rectangular and fills the entire size.
        // Note that "pos" is relative the the parent of the widget.
        return tgui::FloatRect{getPosition().x, getPosition().y, getSize().x, getSize().y}.contains(pos);
    }


    // The draw function will be called when it is time to render the widget to the screen
    void draw(tgui::BackendRenderTarget& target, tgui::RenderStates states) const
    {
        // To render something you can call functions on the "target" object.
        // Note that the position in "states" is already relative to this widget,
        // so you no longer have to add getPosition() anywhere.

        // This example code will draw a red rectangle with blue borders of 2px around it.
        // The m_opacityCached variable is defined in the Widget base class and contains
        // the value of the Opacity property of the widget renderer.

        tgui::RenderStates saveStates = states;

        target.drawFilledRect(states,
                            getSize(),
                            tgui::Color::applyOpacity(m_bgColor, m_opacityCached));


        unsigned nbDraw = 0;
        //draw fist all border
        for(const auto &it:m_listItem){
            target.drawBorders(states, m_borders, {m_sizeItemRect,m_sizeItemRect}, tgui::Color::applyOpacity(m_borderColorItem, m_opacityCached));
           
            if(m_alignmentSide == AlignmentSide::HORIZONTAL)
                states.transform.translate({m_sizeItemRect,0});
            else if(m_alignmentSide == AlignmentSide::VERTICAL)
                states.transform.translate({0,m_sizeItemRect});

            nbDraw++;

            if(nbDraw%m_nbItemPerAlignmentSide == 0)
            {   
                if(m_alignmentSide == AlignmentSide::HORIZONTAL)
                    states.transform.translate({-(m_sizeItemRect*m_nbItemPerAlignmentSide),m_sizeItemRect});
                else if(m_alignmentSide == AlignmentSide::VERTICAL)
                    states.transform.translate({m_sizeItemRect,-(m_sizeItemRect*m_nbItemPerAlignmentSide)});
            }
        }

        //draw then all rect
        states = saveStates;
        nbDraw = 0;
        states.transform.translate(m_borders.getOffset());
        for(const auto &it:m_listItem){
            target.drawFilledRect(states,
                {m_sizeItemRect - m_borders.getLeft() - m_borders.getRight(), m_sizeItemRect - m_borders.getTop() - m_borders.getBottom()},
                tgui::Color::applyOpacity(m_bgColorItem, m_opacityCached));
           
            if(m_alignmentSide == AlignmentSide::HORIZONTAL)
                states.transform.translate({m_sizeItemRect,0});
            else if(m_alignmentSide == AlignmentSide::VERTICAL)
                states.transform.translate({0,m_sizeItemRect});

            nbDraw++;

            if(nbDraw%m_nbItemPerAlignmentSide == 0)
            {   
                if(m_alignmentSide == AlignmentSide::HORIZONTAL)
                    states.transform.translate({-(m_sizeItemRect*m_nbItemPerAlignmentSide),m_sizeItemRect});
                else if(m_alignmentSide == AlignmentSide::VERTICAL)
                    states.transform.translate({m_sizeItemRect,-(m_sizeItemRect*m_nbItemPerAlignmentSide)});
            }
        }

        //draw all sprite
        states = saveStates;
        for(const auto &it:m_listItem){
            target.drawSprite(states,it.sprite);
        }

        //draw all text
        for(const auto &it:m_listItem){
            target.drawText(states,it.text);
        }
    }

    // Add the following line to allow the compiler to find the setSize(w, h) function,
    // which will simply call the setSize(size) function that was overridden above.
    using tgui::Widget::setSize;


protected:

    struct itemStruct{
        ItemStack is;
        tgui::Sprite sprite;
        tgui::Text text;
        bool selected;
    };

    // Every widget needs a method to copy it
    Widget::Ptr clone() const override
    {
        std::vector<ItemStack> list = {};
        auto ret = std::make_shared<ItemContainerWidget>(m_alignmentSide, m_nbItemPerAlignmentSide, list);
        ret->m_listItem = this->m_listItem;
        return ret;
    }

    unsigned m_nbItemPerAlignmentSide;

    AlignmentSide m_alignmentSide;
    tgui::Borders m_borders;
    float m_sizeItemRect;

    tgui::Color m_bgColor;
    tgui::Color m_bgColorItem;
    tgui::Color m_borderColorItem;
    tgui::Color m_borderColorItemSelected;

    std::vector<itemStruct> m_listItem;
};

bool runExample(tgui::BackendGui& gui)
{
    try
    {
        auto m_scrollPanel = tgui::ScrollablePanel::create();
        m_scrollPanel->setPosition("0%", "80%");
        m_scrollPanel->setSize("100%", "20%");
        m_scrollPanel->setVerticalScrollbarPolicy(tgui::Scrollbar::Policy::Never);
        m_scrollPanel->setHorizontalScrollbarPolicy(tgui::Scrollbar::Policy::Always);
        gui.add(m_scrollPanel);

        std::vector<ItemStack> listItemStack;
        listItemStack.push_back(ItemStack());
        listItemStack.push_back(ItemStack());
        listItemStack.push_back(ItemStack());
        listItemStack.push_back(ItemStack());
        listItemStack.push_back(ItemStack());
        listItemStack.push_back(ItemStack());

        auto m_itemContainer = ItemContainerWidget::create(ItemContainerWidget::AlignmentSide::VERTICAL, 2, listItemStack);
        m_itemContainer->setSize(800, tgui::bindHeight(m_scrollPanel)-m_scrollPanel->getScrollbarWidth());
        m_scrollPanel->add(m_itemContainer);
    }
    catch (const tgui::Exception& e)
    {
        std::cerr << "TGUI Exception: " << e.what() << std::endl;
        return false;
    }

    return true;
}

int main()
{
    sf::RenderWindow window{ {800, 600}, "TGUI example - SFML_GRAPHICS backend" };

    tgui::Gui gui{window};
    if (runExample(gui))
        gui.mainLoop(sf::Color(0,0,0));
}


texus

I can reproduce it here consistently, it happens immediately when I resize the window.
On first sight this looks like a bug in TGUI itself, it looks like the layout somehow gets corrupted, it doesn't look like a recursion issue.

It only crashes when copying "size" to the "newSize" variable (even if you don't change newSize.x) and only when the size contains a math operation (i.e. binding just the height of scrollable panel works, but when subtracting a constant from it such as the scrollbar width causes it to fail). So my guess is that it triggers some edge case in the layout system.

I will investigate this further and try to figure out where the crash is coming from tomorrow. Thanks for providing the simple code example that allowed me to quickly reproduce the issue.

While testing I did find another unrelated crash:
If you make your window too small, the size becomes negative (because the scrollable window height is less than the scrollbar height). In this case you call text.setCharacterSize with a negative number, which becomes a very large number because the character size is unsigned. This causes TGUI to crash while loading the font glyph to figure out the size of the text.

Nafffen

QuoteI will investigate this further and try to figure out where the crash is coming from tomorrow. Thanks for providing the simple code example that allowed me to quickly reproduce the issue.
I am happy to be useful, thank you to be so much active in TGUI !

QuoteIf you make your window too small, the size becomes negative (because the scrollable window height is less than the scrollbar height). In this case you call text.setCharacterSize with a negative number, which becomes a very large number because the character size is unsigned. This causes TGUI to crash while loading the font glyph to figure out the size of the text.
Nice point, I think my window will have a minimum size but it's important to be aware of that.

texus

I figured out what is going on, but I will look into the details and check if I can do something against this tomorrow.

ScrollablePanel::setSize is looping over its bound layouts to update them. This is where your ItemContainerWidget::setSize function is called. Here you call "newSize = size" which copies the layout. During the copy, the layout registers itself to ScrollablePanel. The problem is that ScrollablePanel is still looping over the list of bound layouts, which is now being changed. This causes it to get corrupted and crash.

So for now its forbidden to copy the layout (i.e. the "size" variable) inside the setSize function.

texus

The issue has been fixed (github commit).
Your code should work now with the latest 1.0-dev version.