Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug: stack overflow of destructor caused by too many scene nodes #18

Closed
NoSW opened this issue Aug 27, 2024 · 3 comments
Closed

Bug: stack overflow of destructor caused by too many scene nodes #18

NoSW opened this issue Aug 27, 2024 · 3 comments

Comments

@NoSW
Copy link

NoSW commented Aug 27, 2024

When I load a scene with N (~200'000) nodes with the same parent node, it will call N deep calls to destruct SceneGraphNode. A stack overflow occurred, but N should not be an unacceptable number.

Expand all nodes worked in my case, example code:

virtual ~SceneGraph() //= default;
{
    std::stack<std::shared_ptr<SceneGraphNode>> stack;
    SceneGraphWalker walker(m_Root.get());
    while(walker)
    {
       SceneGraphNode* cur = walker.Get();
       if (walker.Next(true) > 0)
       {
           if (cur->m_FirstChild)
           {
               stack.push(cur->m_FirstChild);
               cur->m_FirstChild.reset();
          }
       }
       else
       {
           if (cur->m_NextSibling)
           {
               stack.push(cur->m_NextSibling);
               cur->m_NextSibling.reset();   
           }
       }
    }

    while (!stack.empty())
    {
        auto n = stack.top();
        stack.pop();
        n.reset();
    }
}
@apanteleev
Copy link
Collaborator

Thank you for reporting this issue. I also noticed it recently, but the code snippet that you provide doesn't seem to be the best solution. It assumes that by the time ~SceneGraph is called, every node has a single owner. That should be true in most cases, but not necessarily, and if some other entity in the program is holding a reference to one of the nodes, that node's subgraph will be deconstructed. I think the right solution would be to switch from using a linked list of nodes to hold siblings to a more practical construct such as vector.

apanteleev added a commit that referenced this issue Sep 4, 2024
…ctor to avoid stack overflows when processing (or deleting) scene graphs with very large instance counts.

ReverseChildren() is no longer necessary because Attach(...) now inserts the new node at the end of the children vector, not at the beginning of the list.

Addresses #18
@apanteleev
Copy link
Collaborator

Fixed in e427373

@NoSW
Copy link
Author

NoSW commented Sep 4, 2024

You're right. The code I provide can only work in most cases. In e427373, however, does the node still own its children? Isn't it?🙁Assuming a scene tree with a depth of N, when the root node is destructed, it may still result in a call stack that is too deep. I think the right solution is pooling, for example:

class SceneGraphNode final : public std::enable_shared_from_this<SceneGraphNode>
{
    private:
        std::weak_ptr<SceneGraphNode> m_FirstChild;
        std::weak_ptr<SceneGraphNode> m_NextSibling;
};

class SceneGraph : public std::enable_shared_from_this<SceneGraph>
{
    private:
        std::list<std::shared_ptr<SceneGraphNode>> m_NodePool;
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants