Windows fixes with passing unit tests

[johnwason]

This PR contains fixes and passing unit tests on Windows. Most of them are pretty minor changes to unit tests. The only major issues discovered is with SceneGraph. It was being copied in a few places, and this lead to invalid pointers stored in the link and joint maps. The problem was not showing on Linux because gcc was return-value optimizing away the copy operation, but MSVC was not. SceneGraph has the copy constructor deleted, but for some reason it was still allowed to be copied. I tried adding boost::noncopyable, but it was being copied in a few places. This issue should be fixed, but for now it doesn't appear to be causing any problems.

[codecov]

Codecov Report

Merging #751 (0747bff) into master (d8579ef) will decrease coverage by 0.47%.
The diff coverage is 17.04%.

@@            Coverage Diff             @@
##           master     #751      +/-   ##
==========================================
- Coverage   90.77%   90.29%   -0.48%     
==========================================
  Files         222      222              
  Lines       13697    13781      +84     
==========================================
+ Hits        12433    12444      +11     
- Misses       1264     1337      +73     

Impacted Files	Coverage Δ
...ract_common/include/tesseract_common/joint_state.h	100.00% <ø> (ø)
..._scene_graph/include/tesseract_scene_graph/graph.h	96.72% <ø> (ø)
tesseract_scene_graph/src/graph.cpp	89.66% <6.06%> (-5.14%)	⬇️
tesseract_common/src/joint_state.cpp	40.84% <19.23%> (-59.16%)	⬇️
...ct_common/include/tesseract_common/serialization.h	92.68% <100.00%> (ø)
tesseract_state_solver/src/ofkt_state_solver.cpp	77.74% <100.00%> (ø)

[Levi-Armstrong]

Should this be removed?

[johnwason]

The structure should be declared boost::noncopyable at some point, but not in this PR. An issue should be created to fix this.

[Levi-Armstrong]

Yea, it is odd that the copy destructor is deleted but on windows it is still coping? I see where you were switched to using out parameter but those should have been moving the object and not copying it correct?

[Levi-Armstrong]

Is it that we are using windows compiler 2019 which may not have a full implementation of c++17 standard so it is not correctly moving the object. Is it possible to upgrade to 2020? Also did you by chance try adding an explicit std::move before switching to using an out parameter?

[johnwason]

I tried adding boost::noncopyable to SceneGraph on Linux and am seeing the same compiler errors:

tesseract_scene_graph_serialization_unit.cpp:212:10: error: use of deleted function ‘tesseract_scene_graph::SceneGraph::SceneGraph(const tesseract_scene_graph::SceneGraph&)’
  212 |   return g;

I think you still need to have the object be copyable to use copy elision, since it may still copy in certain situations.

[johnwason]

I tried defining a dummy move constructor after adding boost::noncopyable, and it compiles. It looks like the default move constructor isn't being generated correctly.

[johnwason]

It compiles using GCC, I still have to test MSVC with an explicit move constructor.

[johnwason]

No, I am seeing the same errors with an explicit move constructor that I was seeing earlier. I think SceneGraph needs to be allocated on the heap, or used as a reference.

[Levi-Armstrong]

Then should we just add the boost::noncopyable or does that cause issues other places?

[johnwason]

I tried adding rebuilding link_map_ and joint_map_ to the move constructor, added boost::noncopyable, and it passed the scene graph unit tests.

SceneGraph(SceneGraph&& other) : Graph(std::forward<Graph>(other)), link_map_(std::move(other.link_map_)),
    joint_map_(std::move(other.joint_map_)), acm_(std::move(other.acm_))
  {
    {
      link_map_.clear();
      joint_map_.clear();
      Graph::vertex_iterator i, iend;
      for (boost::tie(i, iend) = boost::vertices(*this); i != iend; ++i)
      {
        Link::Ptr link = boost::get(boost::vertex_link, *this)[*i];
        link_map_[link->getName()] = std::make_pair(link, *i);
      }
    }
    {
      Graph::edge_iterator i, iend;
      for (boost::tie(i, iend) = boost::edges(*this); i != iend; ++i)
      {
        Joint::Ptr joint = boost::get(boost::edge_joint, *this)[*i];
        joint_map_[joint->getName()] = std::make_pair(joint, *i);
      }
    }
  }

[johnwason]

I fixed the copy constructor, and now the return value is working as expected on Windows without needing to modify anything else. I haven't tested on Linux so we have to wait for the CI to finish.

[Levi-Armstrong]

@johnwason Thanks for the fixes!