Fattorino / ImNodeFlow

Node based editor/blueprints for ImGui
MIT License
177 stars 22 forks source link

Dynamic Pins Crash #34

Open avlec opened 3 months ago

avlec commented 3 months ago

Was trying out the following examples and they seem to crash when connecting pins to other objects with a Pin UID not found error.

ImNodeFlow.inl:189: const T& ImFlow::BaseNode::getInVal(const U&) [with T = double; U = std::__cxx11::basic_string<char>]: Assertion `it != m_ins.end() && "Pin UID not found!"' failed.

Using the following Node definition

struct Test : ImFlow::BaseNode
{
  void draw()
  {
    ImGui::Text("test");
    showIN<double>("INPUT", 0.0, ImFlow::ConnectionFilter::SameType());
    showOUT<double>("OUTPUT", [this](){ return getInVal<double>("INPUT"); });
  }
};

I noticed this while also trying out the notion of wrapping lambdas up as nodes, which I would also imagine should work. And it does render but has the same crash.

INF.addLambda([](ImFlow::BaseNode* self){
  ImGui::Text("lambda");
  self->showIN<double>("INPUT", 0.0, ImFlow::ConnectionFilter::SameType());
  self->showOUT<double>("OUTPUT", [self](){ return self->getInVal<double>("INPUT"); });
}, {0,0});

// with this added to ImNodeFlow class
template <typename L, typename B = BaseNode>
struct NodeWrapper : B, L
{
    NodeWrapper(L&& l): BaseNode(), L(std::forward<L>(l)) {}
    void draw() { L::operator()(this); }
};

template<typename L>
std::shared_ptr<NodeWrapper<L>> addLambda(L&& lambda, const ImVec2& pos)
{
    return addNode<NodeWrapper<L>>(pos, std::forward<L>(lambda));
}
Fattorino commented 3 months ago

That's interesting... My initial guess would be some error caused by the latest updates to the recursion blacklist code. Sadly I won't be able to investigate it myself until September. As per the lambda, Im not sure the bug is exactly the same but let's hope so

avlec commented 3 months ago

Yeah it seems like they'd be broken for the same reasons. Inheriting from lambdas is something that's actually used for a trick with std::visit that's outlined on the cppreference website as the overloaded template. So it should work. I more placed that bit of code in here because I stumbled across this bug because I was doing that. And I was doing that to prove that it'd be easy to add a convenience wrapper to add simple nodes to the editor without having to write a class.

Fattorino commented 2 months ago

I just remembered, due to the dynamic nature of this type of pins, the value from an input is returned directly when calling showIN()

This was a design decision I made at the time, because I think using the same getInVal() function mixes two different things. Also searching by UID for something that won't always exist seems not the best implementation when it comes to using the feature.

But I'd like to hear some feedback and different opinions

avlec commented 2 months ago

I think that this should work because I tried it and others certainly will. With the obvious caveat that if the pin didn't render on the current draw loop before referencing it would be invalid.

Fattorino commented 2 months ago

I think I'll still keep them separate, creating a method called getDynamicInVal()