Skip to content

Conversation

@danrbailey
Copy link
Contributor

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)

@danrbailey danrbailey requested a review from kmuseth as a code owner March 12, 2024 05:59
@Idclip
Copy link
Contributor

Idclip commented Mar 18, 2024

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

@danrbailey
Copy link
Contributor Author

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.

@danrbailey danrbailey merged commit 3b473dd into AcademySoftwareFoundation:master Sep 18, 2024
@danrbailey danrbailey deleted the tree_adapter_improvements branch September 18, 2024 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants