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

Changes to support matrix grid type instantiation #723

Merged
merged 8 commits into from Nov 19, 2020

Conversation

Idclip
Copy link
Contributor

@Idclip Idclip commented May 28, 2020

I went back and forth with different approaches to get this to work. It's not ideal, but the only aim here is to allow grid types to be instantiated with VDB matrix types. Notes:

  • To avoid breaking code with other custom grid types, prune must always call operator+ on non POD/VDB types. This means that any cwiseAdd must fall back to operator+. This further means that SFINAE cannot be used in Math.h (cwise must be agnostic to any types) and instead has to be specialised by the matrix classes.
  • Moved some methods to the base Mat class. Both moves are necessary. Note that Mat3<Type>(Mat<3<type>&) Mat4<Type>(Mat<4, Type>) constructors are actually broken (cannot be compiled). Moving the Mat:: operator[] to the base class inadvertently fixes this.
  • I've had to move string operator+ into the math namespace to get ADL to work.

Signed-off-by: Nick Avramoussis 4256455+Idclip@users.noreply.github.com

Signed-off-by: Nick Avramoussis <4256455+Idclip@users.noreply.github.com>
@Idclip Idclip requested a review from pcucka as a code owner May 28, 2020 14:34
openvdb/Grid.h Outdated
OPENVDB_NO_TYPE_CONVERSION_WARNING_BEGIN
const auto value = zeroVal<ValueType>() + tolerance;
OPENVDB_NO_TYPE_CONVERSION_WARNING_END
const auto value = math::cwiseAdd(zeroVal<ValueType>(), tolerance);
this->tree().prune(static_cast<ValueType>(value));
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One other option I've been considering is to simply add explicit functionality to Grid::prune which branches on matrix types - something like:

auto addTolerance(auto& a, float tol) { return a + tol; }

template<size_t SIZE, typename T>
auto addTolerance(Mat<SIZE, T>& a, float tol) { // do cwiseadd }

prune(float a)
{
    auto value = addTolerance(zeroVal<ValueType>(), a);
    ....
}

I think I'd actually prefer this, if we assume the following:

  • cwiseAdd is only necessary as we are still debating if/what operator+ should perform on matrix types with scalars
  • Grid::prune itself is to be deprecated.

@@ -62,6 +62,8 @@ template<> inline std::string zeroVal<std::string>() { return ""; }
/// Return the @c bool value that corresponds to zero.
template<> inline bool zeroVal<bool>() { return false; }

namespace math {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important: This moves the below inline std::string operator+ ops into the math namespace so ADL picks them up from cwiseAdd

/// Array style reference to ith row
T* operator[](int i) { return &(mm[i*SIZE]); }
const T* operator[](int i) const { return &(mm[i*SIZE]); }
//@}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also fixes Mat3<T>(Mat<3, T>& in) and the same for Mat4

Copy link
Contributor

@jmlait jmlait left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like the < defined for matrices, but I recall we had it defined for fixed vectors so were stuck.

@Idclip
Copy link
Contributor Author

Idclip commented Nov 9, 2020

I don't like the < defined for matrices, but I recall we had it defined for fixed vectors so were stuck.

Vector classes derive from Tuple which implements these ops:

/// @return true if t0 < t1, comparing components in order of significance.
template<int SIZE, typename T0, typename T1>
bool
operator<(const Tuple<SIZE, T0>& t0, const Tuple<SIZE, T1>& t1)
{
    for (int i = 0; i < SIZE-1; ++i) {
        if (!isExactlyEqual(t0[i], t1[i])) return t0[i] < t1[i];
    }
    return t0[SIZE-1] < t1[SIZE-1];
}


/// @return true if t0 > t1, comparing components in order of significance.
template<int SIZE, typename T0, typename T1>
bool
operator>(const Tuple<SIZE, T0>& t0, const Tuple<SIZE, T1>& t1)
{
    for (int i = 0; i < SIZE-1; ++i) {
        if (!isExactlyEqual(t0[i], t1[i])) return t0[i] > t1[i];
    }
    return t0[SIZE-1] > t1[SIZE-1];
}

Weighing up the options here, this change does mean that Mat could derive from Tuple which will be awesome and a change that I think is long overdue. However I agree that this definition isn't great - I can't remember exactly what we concluded would be better default behaviour?

@Idclip
Copy link
Contributor Author

Idclip commented Nov 9, 2020

Okay so I dug through our old conversation on this. Forgetting deriving from Tuple for the moment, another option to get this working would be to leave < and > undefined and instead add cwiseLessthan/GreaterThan methods and only update existing tools which make sense to work on Mat grid with this logic. This takes us a step back from the ideal solution, but for the purpose of this PR which is to get mat grids supported I think this would be a better solution.

@Idclip
Copy link
Contributor Author

Idclip commented Nov 9, 2020

@jmlait I've made this change, I think it's a lot close to what you originally proposed. Take a look and let me know what you think

Signed-off-by: Nick Avramoussis <4256455+Idclip@users.noreply.github.com>
…ids are supported

Signed-off-by: Nick Avramoussis <4256455+Idclip@users.noreply.github.com>
Signed-off-by: Nick Avramoussis <4256455+Idclip@users.noreply.github.com>
@Idclip Idclip merged commit bf1492b into AcademySoftwareFoundation:master Nov 19, 2020
@Idclip Idclip deleted the mat_types branch June 5, 2021 12:11
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.

None yet

2 participants