Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update TreeAdapter to work with const inputs #1775

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)

… extensive unit tests

Signed-off-by: Dan Bailey <danbailey@ilm.com>
Signed-off-by: Dan Bailey <danbailey@ilm.com>
@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
41 checks passed
@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