AcademySoftwareFoundation / openvdb

OpenVDB - Sparse volume data structure and tools
http://www.openvdb.org/
Apache License 2.0
2.71k stars 660 forks source link

Update TreeAdapter to work with const inputs #1775

Closed danrbailey closed 2 months ago

danrbailey commented 8 months ago

The TreeAdapter has a few problems, most notably related to the use of the tree()/constTree() methods. It appears that the TreeAdapter is intended to work with const inputs (because some of the typedefs remove const-ness) so it seems counter-intuitive that these methods do not always work for const inputs. In addition, the resulting error is confusing because the compiler complains about duplicate methods being defined.

I created a bunch of unit tests to validate against what I considered acceptable input and modified the TreeAdapter to fix these issues.

The one area where I extended it was to explicitly add support for TreeAdapter<const Grid<TreeT>>. This is very useful when const-ness is baked into an input template argument and seemed consistent with supporting TreeAdapter<const TreeT>.

All of the rest of the unit tests pass and none of these changes are expected to affect compatibility.

(@kmuseth)

Idclip commented 8 months ago

What happens with an adapter templated on a const ValueAccessor? Or rather, is that behaviour still the same before and after this change

danrbailey commented 8 months ago

What happens with an adapter templated on a const ValueAccessor? Or rather, is that behaviour still the same before and after this change

The behavior before is that it fails to compile, the behavior after is that it compiles successfully. I attached a unit test with these fixes, running the unit test without the fixes results in compilation errors on these lines - 544, 546, 550, 573, 575, 577, 589, 594, 595, 604, 605, 608, 610, 611, 612, 614.

Here's one example related to your question:

FloatGrid floatGrid;
TreeAdapter<tree::ValueAccessor<const FloatTree>>::tree(floatGrid);

I would expect this to work, but it results in this compile error:

openvdb/unittest/TestGrid.cc:604:38: error: no matching function for call to 'openvdb::v11_0::TreeAdapter<openvdb::v11_0::tree::ValueAccessorImpl<const openvdb::v11_0::tree::Tree<openvdb::v11_0::tree::RootNode<openvdb::v11_0::tree::InternalNode<openvdb::v11_0::tree::InternalNode<openvdb::v11_0::tree::LeafNode<float, 3>, 4>, 5> > >, true, void, openvdb::v11_0::index_sequence<0, 1, 2> > >::tree(openvdb::v11_0::FloatGrid&)'
  604 |         AdapterConstT::tree(floatGrid);
      |                                      ^
In file included from openvdb/openvdb.h:13,
                 from unittest/TestGrid.cc:5:
openvdb/Grid.h:1135:22: note: candidate: 'static openvdb::v11_0::TreeAdapter<openvdb::v11_0::tree::ValueAccessorImpl<_TreeType, true, void, typename std::decay<decltype (make_index_sequence_impl<(std::max((unsigned int)(1), _TreeType::DEPTH) - 1)>())>::type> >::TreeType& openvdb::v11_0::TreeAdapter<openvdb::v11_0::tree::ValueAccessorImpl<_TreeType, true, void, typename std::decay<decltype (make_index_sequence_impl<(std::max((unsigned int)(1), _TreeType::DEPTH) - 1)>())>::type> >::tree(openvdb::v11_0::TreeAdapter<openvdb::v11_0::tree::ValueAccessorImpl<_TreeType, true, void, typename std::decay<decltype (make_index_sequence_impl<(std::max((unsigned int)(1), _TreeType::DEPTH) - 1)>())>::type> >::TreeType&) [with _TreeType = const openvdb::v11_0::tree::Tree<openvdb::v11_0::tree::RootNode<openvdb::v11_0::tree::InternalNode<openvdb::v11_0::tree::InternalNode<openvdb::v11_0::tree::LeafNode<float, 3>, 4>, 5> > >; openvdb::v11_0::TreeAdapter<openvdb::v11_0::tree::ValueAccessorImpl<_TreeType, true, void, typename std::decay<decltype (make_index_sequence_impl<(std::max((unsigned int)(1), _TreeType::DEPTH) - 1)>())>::type> >::TreeType = const openvdb::v11_0::tree::Tree<openvdb::v11_0::tree::RootNode<openvdb::v11_0::tree::InternalNode<openvdb::v11_0::tree::InternalNode<openvdb::v11_0::tree::LeafNode<float, 3>, 4>, 5> > >]'
 1135 |     static TreeType& tree(TreeType& t) { return t; }
openvdb/Grid.h:1135:37: note:   no known conversion for argument 1 from 'openvdb::v11_0::FloatGrid' {aka 'openvdb::v11_0::Grid<openvdb::v11_0::tree::Tree<openvdb::v11_0::tree::RootNode<openvdb::v11_0::tree::InternalNode<openvdb::v11_0::tree::InternalNode<openvdb::v11_0::tree::LeafNode<float, 3>, 4>, 5> > > >'} to 'openvdb::v11_0::TreeAdapter<openvdb::v11_0::tree::ValueAccessorImpl<const openvdb::v11_0::tree::Tree<openvdb::v11_0::tree::RootNode<openvdb::v11_0::tree::InternalNode<openvdb::v11_0::tree::InternalNode<openvdb::v11_0::tree::LeafNode<float, 3>, 4>, 5> > >, true, void, openvdb::v11_0::index_sequence<0, 1, 2> > >::TreeType&' {aka 'const openvdb::v11_0::tree::Tree<openvdb::v11_0::tree::RootNode<openvdb::v11_0::tree::InternalNode<openvdb::v11_0::tree::InternalNode<openvdb::v11_0::tree::LeafNode<float, 3>, 4>, 5> > >&'}
 1135 |     static TreeType& tree(TreeType& t) { return t; }

To extract the problematic portion of the partial specialization:

template<typename _TreeType>
struct TreeAdapter<tree::ValueAccessor<_TreeType> >
{
    using TreeType             = _TreeType;
    ....
    using GridType             = Grid<TreeType>;
    ....

    ....
    static TreeType& tree(GridType& g) { return g.tree(); }
    ....
}

You can see that calling this with TreeAdapter<tree::ValueAccessor<const FloatTree>> results in tree(Grid<const FloatTree>) but no tree(Grid<FloatTree>&).

This is not expected to change behavior for any method which does instantiate correctly, just fill in all the missing instantiations.