RainerKuemmerle / g2o

g2o: A General Framework for Graph Optimization
3.04k stars 1.1k forks source link

BaseBinaryEdge implicitly is non copyable. #609

Closed Lishen1 closed 1 year ago

Lishen1 commented 1 year ago

BaseBinaryEdge has no explicit deleted copy constructor or copy assignment operator but

typename BaseFixedSizedEdge<D, E, 
VertexXi, VertexXj>::template JacobianType<
      D, VertexXi::Dimension>& _jacobianOplusXi =
      std::get<0>(this->_jacobianOplus);

reference made it is not possible to copy. it invoke some problems with std containers

Lishen1 commented 1 year ago

maybe better to make something like that:

 T& jacobianOplusXi() {}
 const T& jacobianOplusXi() const {return ...;}

T& jacobianOplusXj() {}
 const T& jacobianOplusXj() const {return ...;}
RainerKuemmerle commented 1 year ago

It was implemented like that to maintain backwards compatibility with the former member variable, to allow implementation of analytic Jacobians. For example in here https://github.com/RainerKuemmerle/g2o/blob/master/g2o/types/slam2d/edge_se2.cpp#L63-L83

As some other internal data structures are based on pointers I wonder if the better solution is not to delete the copy c'tor and assignment operator?

Lishen1 commented 1 year ago

if it non copyable anyway, better to mark it explicitly