weird dependency cycle with custom widget

Started by Nafffen, 08 October 2023, 12:35:57

Nafffen

Hello,
I am facing a warning :
QuoteTGUI warning: Dependency cycle detected in layout!
with this layout:
    m_timeProgressBar = TimeProgressBar::create();
    m_timeProgressBar->setOrigin(0.5f, 0.f);
    m_timeProgressBar->setPosition("50%", tgui::bindBottom(m_label));
    m_timeProgressBar->setSize("90%", 40);
    m_group->add(m_timeProgressBar);
The warning is caused by the line m_timeProgressBar->setPosition("50%", tgui::bindBottom(m_label)); If I comment it, everything is fine.
With the embedded progress bar, everything is fine :
    auto test = tgui::ProgressBar::create();
    test->setOrigin(0.5f, 0.f);
    test->setPosition("50%", tgui::bindBottom(m_label));
    test->setSize("90%", 40);
    m_group->add(test);

The weird thing is that I don't override setPosition in my custom bar:
#pragma once

#include "../../../Utils/utils.hpp"

class TimeProgressBar : public tgui::ProgressBar
{
public:
    // Add a Ptr typedef so that "MyCustomWidget::Ptr" has the correct type
    typedef std::shared_ptr<TimeProgressBar> Ptr;
    typedef std::shared_ptr<const TimeProgressBar> ConstPtr;

    TimeProgressBar();
    static TimeProgressBar::Ptr create();
    static TimeProgressBar::Ptr copy(TimeProgressBar::ConstPtr widget);

    /**
     * @brief Set the Percentage, between 0 and 1 or will be truncated
     *
     * @param percentage between 0 and 1
     */
    void setPercentage(float percentage);

    /**
     * @brief Set the Current Time, between 0 and maxTime or will be clamped
     *
     * @param time between 0 and maxTime
     */
    void setCurrentTime(sf::Time time);

    void setMaxTime(sf::Time time);
    void setReverse(bool act);


private:
    Widget::Ptr clone() const override;

    void updateText();
    void updateValue();

    /**
     * @brief the time is display with raw value currentTime / timeMax
     */
    bool m_rawValue;

    /**
     * @brief time will start from 0 to maxTime, or maxTime to 0
     */
    bool m_reverse;

    sf::Time m_currentTime;
    sf::Time m_maxTime;
};

texus

How are you setting the size and position of the label? Does either the position or size of m_label depend on m_timeProgressBar in any way?
If m_label has a fixed position and size then this warning shouldn't happen.

Nafffen

padding is a float
m_label->setAutoSize(true);
m_label->setPosition("50%", tgui::bindWidth(m_group)*padding);

Then later in the same function :
m_group->setSize("25%", tgui::bindBottom(lastPtr)+tgui::bindWidth(m_group)*padding);

Indeed if m_label has a fixed pos, no warning. But I don't understand the dependency cycle here.

Everything is binded to the known width of m_group (25% of screen), then height of m_group is computed from last widget in the layout

The whole function is needed :
assert(m_group);
    tgui::Widget::Ptr lastPtr = nullptr;
    //between 0 and 1, %
    float padding = 0.05f;

    auto layoutVisuStack = [this, &lastPtr, padding](std::vector<C_RecipeProcessing::ItemStackVisu> &listVisu){
        if(listVisu.size()){
            auto groupItem = tgui::Group::create();

            if(lastPtr)
                groupItem->setPosition(0,tgui::bindBottom(lastPtr)+tgui::bindWidth(groupItem)*padding);
            else
                groupItem->setPosition(0,tgui::bindWidth(groupItem)*padding);

            auto nbInt = (int)listVisu.size();
            auto nbDiv = std::max(3,nbInt);
            auto l = (tgui::bindWidth(groupItem)-(tgui::bindWidth(groupItem)*padding*(nbInt+1)))/nbDiv;
            groupItem->setSize("100%", l);

            for(auto i=0;i<listVisu.size();i++){
                auto itemWidget = PictureTextWidget::create(PictureText::fromItemStack(listVisu[i].itemStack));
                itemWidget->setOrigin(0.5f,0.f);
                itemWidget->setSize(l,l);
                itemWidget->setPosition(tgui::bindWidth(groupItem)/(nbInt+1) * (i+1),0);
                groupItem->add(itemWidget);
                listVisu[i].itemStackWidget = itemWidget;
            }

            m_group->add(groupItem);
            lastPtr = groupItem;
        }
    };


    //inputs
    layoutVisuStack(m_inputs);

    //middle
    m_label = tgui::Label::create();
    m_label->setRenderer(TextureLoader::getTheme().getRenderer("LabelInfoWindow"));
    m_label->setAutoSize(true);
    m_label->setOrigin(0.5f, 0.f);
    if(lastPtr)
        m_label->setPosition("50%", tgui::bindBottom(lastPtr)+tgui::bindWidth(m_group)*padding);
    else
        m_label->setPosition("50%", tgui::bindWidth(m_group)*padding);
    m_group->add(m_label);
    lastPtr = m_label;

    if(m_recipe){
        m_panelRecipe = getGUI(m_recipe);
        m_panelRecipe->setWidth(tgui::bindWidth(m_group)*0.8);
    }

    //WARNING
    m_timeProgressBar = TimeProgressBar::create();
    m_timeProgressBar->setOrigin(0.5f, 0.f);
    m_timeProgressBar->setPosition("50%", tgui::bindBottom(m_label));
    m_timeProgressBar->setSize("90%", 40);
    m_group->add(m_timeProgressBar);
    lastPtr = m_timeProgressBar;

    //NO WARNING
    auto test = tgui::ProgressBar::create();
    test->setOrigin(0.5f, 0.f);
    test->setPosition("50%", tgui::bindBottom(m_label));
    test->setSize("90%", 40);
    m_group->add(test);
    lastPtr = test;

    //outputs
    layoutVisuStack(m_outputs);

    if(lastPtr)
        m_group->setSize("25%", tgui::bindBottom(lastPtr)+tgui::bindWidth(m_group)*padding);
    else
        m_group->setSize("25%", 0);

    updateGUI();

    std::cout << m_timeProgressBar->getPositionLayout().toString() << std::endl;
    std::cout << m_timeProgressBar->getSizeLayout().toString() << std::endl;

texus

#3
It's possible that the cycle detection is a bit too strict. The group depends on the height of the last widget, so it depends on the size of the last widget. And the last widget depends on the width of the group, so it depends on the size of the group. A cyclic dependency is seen on the size, even though there is no cycle on either width or height alone.

The code used to crash when you created a cycle, the detection was added to stop the endless loop that caused the stack overflow to allow the program to still continue instead of crashing. But maybe the detection is triggering too early in this complex case and it wouldn't have crashed if the detection code is removed.

I'm not sure how to best fix this though, the initial detection code is already with more overhead than I'd like, so I'm not looking forward to adding even more complexity to it. And just removing the detection and causing hard to debug crashes again isn't a great option either. So I'll see if I can still find something to improve the situation (maybe replace the bool flag with an int counter and only show the error once you are several layers deep in the recursion).

The thing that I don't understand though is why you would only have an issue with m_timeProgressBar. I already have a cyclic dependency on the "m_group->setSize("25%", tgui::bindBottom(lastPtr)+tgui::bindWidth(m_group)*padding);" if I just create the "test" progressbar. I didn't even add the TimeProgressBar yet.

EDIT: The detection is definitely broken, I already get the dependency cycle just from doing "m_group->setSize(250, tgui::bindWidth(m_group) + tgui::bindWidth(m_group));". So it might just not like the same layout from being evaluated multiple times within the same chain.

texus

TGUI can't update the width or height of a widget separately, it always goes through the setSize function that updates both. If the height depends on the width or vice versa, then the setSize function will be called multiple times recursively. I have updated the Layout class to only trigger once the recursion reaches 5 to 10 levels deep, so it should no longer trigger on this case.