Crash when creating an ImageWriter

[johnhaddon]

• Create a Constant Image, and keep it selected
• Create an ImageWriter
• Watch Gaffer disappear before your eyes

1 libGaffer.dylib 0x0000000105dc271b Gaffer::CompoundPlug::setInput(IECore::IntrusivePtrGaffer::Plug) + 673
2 libGafferUI.dylib 0x0000000106d5ebde GafferUI::StandardGraphLayout::connectNodeInternal(GafferUI::GraphGadget_, Gaffer::Node_, Gaffer::Set_, bool) const + 300
3 libGafferUI.dylib 0x0000000106d5f1a5 GafferUI::StandardGraphLayout::connectNode(GafferUI::GraphGadget_, Gaffer::Node_, Gaffer::Set_) const + 15
4 libGafferUIBindings.dylib 0x0000000106e7c65e boost::python::detail::caller_arity<4u>::impl<bool (GafferUI::GraphLayout::)(GafferUI::GraphGadget, Gaffer::Node*, Gaffer::Set*) const, boost::python::default_call_policies, boost::mpl::vector5<bool, GafferUI::GraphLayout&, GafferUI::GraphGadget*, Gaffer::Node*, Gaffer::Set*> >::operator()(object, object) + 258
5 libboost_python.dylib 0x000000010074bd73 boost::python::objects::function::call(object, object) const + 733 (function.cpp:234)
6 libboost_python.dylib 0x000000010074a950 boost::detail::function::void_function_ref_invoker0<boost::python::objects::(anonymous namespace)::bind_return, void>::invoke(boost::detail::function::function_buffer&) + 32 (function.cpp:585)
7 libboost_python.dylib 0x0000000100753a2c boost::python::detail::exception_handler::operator()(boost::function0 const&) const + 174 (function_template.hpp:1013)
8 libIECorePython.dylib 0x00000001023ab094 bool boost::_bi::list3boost::arg<1, boost::arg<2>, boost::_bi::value<void ()(IECore::Exception const&)> >::operator()<bool, boost::python::detail::translate_exception<IECore::Exception, void ()(IECore::Exception const&)>, boost::_bi::list2<boost::python::detail::exception_handler const&, boost::function0 const&> >(boost::_bi::type, boost::python::detail::translate_exception<IECore::Exception, void ()(IECore::Exception const&)>&, boost::_bi::list2<boost::python::detail::exception_handler const&, boost::function0 const&>&, long) + 24
9 libIECorePython.dylib 0x00000001023aaf8b boost::detail::function::function_obj_invoker2<boost::_bi::bind_t<bool, boost::python::detail::translate_exception<IECore::Exception, void ()(IECore::Exception const&)>, boost::_bi::list3boost::arg<1, boost::arg<2>, boost::_bi::value<void (*)(IECore::Exception const&)> > >, bool, boost::python::detail::exception_handler const&, boost::function0 const&>::invoke(boost::detail::function::function_buffer&, boost::python::detail::exception_handler const&, boost::function0 const&) + 43
10 libboost_python.dylib 0x0000000100753a93 boost::python::handle_exception_impl(boost::function0) + 99 (function_template.hpp:1013)
11 libboost_python.dylib 0x000000010074ae2f function_call + 79 (function_template.hpp:752)

[johnhaddon]

I've done a bit of investigation into this one, and it transpires that the crash is occurring when auto-connecting Constant.out to ImageWriter.requirements. I think this is because the requirements ArrayPlug is adding children while the CompoundPlug base class is connecting them.

This connection should never be allowed to be attempted in the first place though, because we really only want requirements plugs to accept connections from the requirement output of another Executable node. To prevent that we'd need to implement ExecutableNode::acceptsInput() to reject the connection, but that seems wrong - we don't want to have to reimplement the same thing in every type that inherits Executable. Maybe Executable can implement it itself, and it'll work when it gets multiply-inherited into ExecutableNode? Or maybe we really don't need the whole multiple-inheritance of the Executable class and could just have ExecutableNode?

[johnhaddon]

Just to clarify my previous comments a bit. There is a very easy fix for this - implement ImageWriter::acceptsInput() to take into account Executable::acceptsRequirementsInput() in the same way that ExecutableOpHolder does. If anyone wants the fix quickly I'll be more than happy to do it, but otherwise I'm going to leave this here as a thorn in my side while I think about rejigging Executable/ExecutableNode so there's less burden on subclass implementers to get things right.

[goddardl]

This one is my bad. I knew that this was an issue but it slipped through the cracks when I wrote the writer.

I like the idea of having the base class hadle it though.

[johnhaddon]

Cool. I'm definitely leaning towards having the base class simply be ExecutableNode, and removing the Executable abstract class. That would sit nicely alongside ComputeNode and DependencyNode as three sensible base classes for different circumstances that all operate along similar lines. If we do that we can remove the burden from the subclass implementers and we'll have a more straightforward class hierarchy as well.

[goddardl]

That sounds good to me. Nice and clean.

[johnhaddon]

Addressed in #782 - closing.