kindr icon indicating copy to clipboard operation
kindr copied to clipboard

NewKindr interface

Open HannesSommer opened this issue 10 years ago • 39 comments

The current new idea for the structure as a kind of compromise would be using policy classes defining the specific implementations foe e.g. a rotation.

That would look a bit simplified (especially stripped the templating on scalar) like the following:

template<typename ParamPolicy>
class Rot {
public:
  Vec3 rotate(const Vec3 & v){
    return ParamPolicy::rotate(s_, v);
  }
  ... // further functions, specifying the outer generic interface of rotations.
private:
  typename ParamPolicy::Storage s_;
}

A suitable angle axis Policy could then look like:

class AngleAxis {
public:
  typedef std::pair<double, Vec3> Storage;
  Vec3 rotate(const Storage & s, const Vec3 & v){
    return ...;
  }
  ...
}

The nice thing is : the policy classes stay quite simple and easy to understand. The most urgent problem here is that there is no way to add parametrization specific methods to the Rot class. And @simonlynen ;) probably does not want to write rot.storage().SOMETHING but rather rot.SOMETHING.

A slight complification would solve that nicely:

template<typename ParamPolicy>
class Rot : public ParamPolicy::Storage {
public:
  Vec3 rotate(const Vec3 & v){
    return ParamPolicy::rotate(*this, v);
  }
  ... // further functions, specifying the outer generic interface of rotations.
}

struct AngleAxisStorage {
    double angle;
    Vec3 vec;

    double getAngle();
    ...
}
class AngleAxis {
public:
  typedef AngleAxisStorage Storage;
  Vec3 rotate(const Storage & s, const Vec3 & v){
    return ...;
  }
  ...
}

HannesSommer avatar Nov 11 '14 14:11 HannesSommer

The biggest drawback of this extension is that you would not longer like to have e.g. a RotationVector specify Vec3 directly as Storage! Because then Rot would be a child of an Eigen Vector yielding all sorts of ambiguities I would like to avoid.

HannesSommer avatar Nov 11 '14 14:11 HannesSommer

How about the simple approach of

class AxisAngle : public Rot<AxisAnglePolicy> {
  public:
   /// other methods
}

Everyone would understand that.

furgalep avatar Nov 11 '14 14:11 furgalep

Or...funnier:

template<typename ParamPolicy>
class Rot : public ParamPolicy {
  typedef ParamPolicy::Storage Storage;
public:
  Vec3 rotate(const Vec3 & v){
    return ParamPolicy::rotate(storage_, v);
  }
  ... // further functions, specifying the outer generic interface of rotations.
};

struct AngleAxis {
public:
  typedef std::pair<double, Eigen::Vector3d> Storage;
  /// Static functions to be called by the interface...
  static Vec3 rotate(const Storage & s, const Vec3 & v){
    return ...;
  }

 /// Functions to extend the interface
    double getAngle() {
      return storage_.first;
   }
  ...
  protected:
    Storage storage_;
};

Good points:

  1. The static functions still preserve the functional interface.
  2. All code for a parameterization is in one place.
  3. The storage is still a separate type from the interface class.
  4. Still somewhat understandable.

furgalep avatar Nov 11 '14 14:11 furgalep

This is the current interface without the passive/active and without the Base classes that were neccessary to have an interface that is independent of Eigen.

template<typename Derived_>
class RotationBase {
 public:
   Vec3 rotate(Vec3 vec) const {
     return internal::RotationTraits<RotationBase<Derived_>>::rotate(this->derived(), vec);
   }
   Derived_& derived() {
     return static_cast<Derived_&>(*this);
   }
};

class AngleAxis : public RotationBase<AngleAxis> {
private:
  Storage storage;
public:
  Scalar getAngle() {
    return storage.first;
  }
};

// Method I: implement rotate specifically for AngleAxis
class RotationTraits<AngleAxis> {
 public:
  inline static Vec3 rotate(const AngleAxis& rotation, Vec3 vec){
     // impelement here
    return vecRot;
  }
};

// Method II: automatically try to convert it to a rotation matrix and rotate the vector
template<typename Rotation_>
class RotationTraits<RotationBase<Rotation_>> {
 public:
  inline static Vec3 rotate(const RotationBase<Rotation_>& rotation, Vec3 vec){
      return eigen_impl::RotationMatrix(rotation.derived()).matrix()*m;
  }
};

Good points:

  1. super fast to change current interface
  2. inteface is given by RobotBase
  3. the user will have in the end an object of type
AngleAxis<Scalar>

and not

Rot<AngleAxis<Scalar>>
  1. using traits would allow to provide default implementations, which can be useful, e.g. if a developer does not know what boxPlus() is and how to implement it for Euler angles, but knows how to convert them to a rotation matrix.

gehrinch avatar Nov 12 '14 08:11 gehrinch

-1 for the traits. This puts the implementation in different places -1 for the default implementation. It makes it not clear what code gets evoked and can cause lookup issues.

What about:

template<typename Derived>
class Rot {
public:
  typedef Derived::Storage Storage;
  Vec3 rotate(const Vec3 & v){
    return Derived::rotate(storage_, v);
  }
  ... // further functions, specifying the outer generic interface of rotations.
  protected:
    Storage storage_;
};

class AngleAxis : public Rot<AxisAngle> {
public:
  typedef std::pair<double, Eigen::Vector3d> Storage;
  /// Static functions to be called by the interface...
  static Vec3 rotate(const Storage & s, const Vec3 & v){
    return ...;
  }

 /// Functions to extend the interface
    double getAngle() {
      return storage_.first;
   }
  ...
  protected:
    Storage storage_;
};

Proper use of the CRTP

Good points:

  1. All code goes in one place for each parameterization
  2. Static functions preserve the functional interface.
  3. Type is AngleAxis<Scalar> not Rot<AngleAxis<Scalar>>

furgalep avatar Nov 12 '14 09:11 furgalep

This is broken, Rot cannot be child of Derived. Probably a typo. But without that : Why would that be more proper CRTP, than Christians code? The hierarchy and base's template arguments is then exactly the same. How would you implement conversions of A to B without traits?

HannesSommer avatar Nov 12 '14 09:11 HannesSommer

Fixed the typo. I meant "proper use of the CRTP when compared to my last proposal". I'm OK with conversion traits if everything else is sane. This even follows the policy:

  1. Everything for a single parametrization is in one place
  2. Conversions between parametrizations are in a different place.

Right?

furgalep avatar Nov 12 '14 09:11 furgalep

Yes, makes sense. Defaults we can have btw. also by delegation to a default impl, instead of actual impl. Might be easier to read.

HannesSommer avatar Nov 12 '14 09:11 HannesSommer

One thing is unspecified in the "Proper use of CRTP" version: where the storage is in. The Rot (base) or the child. Because right now they have it both. If we go for the second options. This is pretty much exactly the simplified kindr interface (as @gehrinch provided) except Rot is there called RotationBase, and that for the implementation we use traits more often (not only for conversion).

To arrive at this new form we could do that in the following steps :

  1. do the simplification Christian did: mostly removing template parameters and the one layer that was there for being prepared for other vector types.
  2. rename RotationBase to Rot (optional, as it does not simplify anything). Maybe we should discuss this name in a separate threat.
  3. Convert traits into static member functions.

The last step could probably be done in a lazy approach: At the latest, whenever somebody needs to maintain these functions and likes to convert. That would reduce the amount of conversion work a lot and would allow faster arriving at the "usable" - state.

Before we really can decide for this form we probably should have a look at the implications for the Transformation / SE3 class, as this will have a bit more complexity. I will try to provide an example code for that the next days.

We also should decide upon names. For that I opened a new issue #51.

HannesSommer avatar Nov 12 '14 17:11 HannesSommer

There is one nasty particularity I forgot to warn about the "Proper CRTP" version: One cannot just access Derived::Storage in Rot, because as Derived is a child of Rot<Derived> it is incomplete at instantiation time of Rot<Derived>. The usual solution is to have traits mapping Derived to its storage. Of course that we can circumvent by having the storage as a member of Derived, as maybe intended anyway. But at least the Scalar type we will need in Rot. But without a trait we cannot get that from Derived!

In my small prototype I'm currently writing, I will just have a separate template Parameter for Rot, but that is not a good way in my opinion and I would vote for traits here.

HannesSommer avatar Nov 14 '14 14:11 HannesSommer

Why can't we put the storage in Rot?

furgalep avatar Nov 14 '14 18:11 furgalep

Oh...I understand.

furgalep avatar Nov 14 '14 18:11 furgalep

How about (ignore the names for now)

template<typename Derived>
class Rot {
public:
  typedef typename RotationTraits<Derived>::Storage Storage;
  Vec3 rotate(const Vec3 & v){
    return Derived::rotate(storage_, v);
  }
  ... // further functions, specifying the outer generic interface of rotations.
  protected:
    Storage storage_;
};

// The traits class only defines the storage type.
class AngleAxis;
template<>
struct RotationTraits<AngleAxis> {
  typedef std::pair<double, Eigen::Vector3d> Storage;
};

class AngleAxis : public Rot<AngleAxis> {
public:
   typedef typename Rot<AngleAxis>::Storage Storage;
  /// Static functions to be called by the interface...
  static Vec3 rotate(const Storage & s, const Vec3 & v){
    return ...;
  }

 /// Functions to extend the interface
    double getAngle() {
      return storage_.first;
   }

};

I don't know how this will all work with the scalar type.

furgalep avatar Nov 15 '14 08:11 furgalep

Or...better:

template<typename Derived>
class Rot {
public:
  typedef typename RotationTraits<Derived> Traits;
  typedef typename RotationTraits<Derived>::Storage Storage;
  Vec3 rotate(const Vec3 & v){
    return Traits::rotate(storage_, v);
  }
  ... // further functions, specifying the outer generic interface of rotations.
  protected:
    Storage storage_;
};

// The traits class only defines the storage type *and* the static functions
class AngleAxis;
template<>
struct RotationTraits<AngleAxis> {
  typedef std::pair<double, Eigen::Vector3d> Storage;
  /// Static functions to be called by the interface...
  static Vec3 rotate(const Storage & s, const Vec3 & v){
    return ...;
  }
};

// The class only defines extensions to the interface
class AngleAxis : public Rot<AngleAxis> {
public:
   typedef typename Rot<AngleAxis>::Storage Storage;
 /// Functions to extend the interface
    double getAngle() {
      return storage_.first;
   }

};

Good points:

  • Clear separation between the static interface (the single traits class) and the OO interface (the CRTP derived class).
  • The two pieces can go into one file so that all code is in the same place (and it is clear what code goes where).

furgalep avatar Nov 15 '14 08:11 furgalep

Here is a full toy example that shows that it is possible:

template<typename Parameterization>
struct Traits {};

template<class Derived>
class Base {
 public:
  typedef Traits<Derived> Traits;
  typedef typename Traits::Storage Storage;
  Base(const Storage& s) : storage_(s) {}
  virtual ~Base(){}

  double apply(double x) const {
    return Traits::apply(storage_, x);
  }

 protected:
  Storage storage_;
};

template<typename Scalar>
class D1;

template<typename Scalar>
struct Traits< D1<Scalar> > {
  typedef Scalar Storage;
  static double apply(Scalar a, double b){ return a*b; }
};

template<typename Scalar>
class D1 : public Base< D1<Scalar> > {
 public:
  typedef Base< D1<Scalar> > Base;
  typedef Traits< D1<Scalar> > Traits;
  typedef typename Traits::Storage Storage;
  D1(Scalar s) : Base(s) {}
  virtual ~D1(){}

  Storage getStorage(){ return Base::storage_; }
};

int main(int argc, char ** argv) {
  std::cout << "Hello CRTP!\n";

  D1<double> d1(1.0);

  std::cout << d1.apply(2.0) << std::endl;
  std::cout << d1.getStorage() << std::endl;
}

furgalep avatar Nov 15 '14 09:11 furgalep

One other really cool thing about the above structure: you can define a custom interface class based on the traits.

// Define a custom interface class (D2) based on the traits for D1. This is possible because D1 should hold not state.
template<typename Scalar>
class D2 : public Base< D1<Scalar> > {
 public:
  typedef Base< D1<Scalar> > Base;
  typedef Traits< D1<Scalar> > Traits;
  typedef typename Traits::Storage Storage;
  D2(Scalar s) : Base(s) {}
  virtual ~D2(){}

  Storage getBlahStorage(){ return Base::storage_; }
};

It is advanced user functionality but it could be nice for dropping kindr into existing code without having to change any interfaces.

furgalep avatar Nov 15 '14 09:11 furgalep

Here is a slightly tweaked proposal:

  1. Not using CRTP anymore.
  2. You have a parameterization class that defines the storage and the static functions.
  3. You have an interface class that defines the OO interface.
  4. Use namespaces to separate interface and parameterization.
#include <iostream>

template<class Parameterization>
class SO3
{
 public:
  typedef typename Parameterization::Storage Storage;
  SO3(const Storage& s) : storage_(s) {}
  virtual ~SO3(){}

  double apply(double x) const {
    return Parameterization::apply(storage_, x);
  }

 protected:
  Storage storage_;
};

namespace parameterization {

template<typename Scalar>
struct AngleAxis {
  typedef Scalar Storage;
  static double apply(Scalar a, double b){ return a*b; }
};

} // namespace parameterization

namespace interface {

template<typename Scalar>
class AngleAxis : public SO3< parameterization::AngleAxis<Scalar> > {
 public:
  typedef parameterization::AngleAxis<Scalar> Parameterization;
  typedef SO3< Parameterization > SO3;
  typedef typename Parameterization::Storage Storage;
  AngleAxis(Scalar s) : SO3(s) {}
  virtual ~AngleAxis(){}

  Storage getStorage(){ return SO3::storage_; }
};

template<typename Scalar>
class AltAngleAxis : public SO3< parameterization::AngleAxis<Scalar> > {
 public:
  typedef parameterization::AngleAxis<Scalar> Parameterization;
  typedef SO3< Parameterization > SO3;
  typedef typename Parameterization::Storage Storage;
  AltAngleAxis(Scalar s) : SO3(s) {}
  virtual ~AltAngleAxis(){}

  double operator*(Scalar s){ return SO3::apply(s); } 

  Storage getXXStorage(){ return SO3::storage_; }
};

} // namespace interface


int main(int argc, char ** argv) {
  std::cout << "Hello no more CRTP!\n";

  interface::AngleAxis<double> d1(1.0);
  interface::AltAngleAxis<double> d2(2.0);

  std::cout << d1.apply(2.0) << std::endl;
  std::cout << d1.getStorage() << std::endl;

  std::cout << d2 * 2.0 << std::endl;
  std::cout << d2.getXXStorage() << std::endl;

}

furgalep avatar Nov 15 '14 10:11 furgalep

(Note, I updated the above a few times after posting)

furgalep avatar Nov 15 '14 10:11 furgalep

I really like this:

namespace interface {
template<typename Scalar>
class AngleAxis : public SO3< parameterization::AngleAxis<Scalar> > 
}

It makes the somewhat complicated structure seem simple because of the clear semantics.

furgalep avatar Nov 15 '14 10:11 furgalep

About your solution for the inaccessible Storage typedef: Your first solution is exactly the "usual trait" solution I was referring and voting for, but I thought you do oppose any extra trait? Did you change your mind? I need to know that because currently I'm trying to avoid them, against my preference, because you complained so often :). Or do you only oppose a specify kind of traits?

About the last version: Why did you move Storage getStorage(){ return SO3::storage_; } out of SO3? Just to allow the interface to change that? I think SO3 should have a standard interface to access the storage if it is the class having the member and not making it protected. You know that you can "delete" such functions later? Or shadow it, to change the signature for the same name.

There is an important issue with not using CRTP: SO3 would not know about the real full type. This does not allow it to clone or define signatures, which require that, like:

??? exp (Vec3);

What would be that? Maybe we can leave this stuff always open to the param impl, like with:

auto exp(Vec3 v) -> decltype(Parameterization::exp(v));

But are you sure you want that? This would especially the "alternative interface" impossible, as the parameterization::AngleAxis<Scalar> would decide what is the outcome of an exp().

Why don't you like CRTP? Its super useful in this kind of context...

HannesSommer avatar Nov 15 '14 12:11 HannesSommer

I'm not explicitly anti-traits or anti-CRTP. I just want the solution to be

  1. Not strange
  2. Easy to understand with a few lines of explanation
  3. Not a lot of modern C++

If you have an idea that satisfies this, please let me know.

I also have nothing against the CRTP. But if you look at the subtle difference between my latest CRTP and non-CRTP examples, the non CRTP has the benefit that you can have two completely different interface classes based on the same parameterization. Nice stuff.

Regarding the exp() stuff, if we stick to the policy that the interface classes have no state this works, but the result is not the interface type:

#include <iostream>
#include <cmath>

template<class Parameterization>
class SO3
{
 public:
  typedef typename Parameterization::Storage Storage;
  typedef SO3<Parameterization> This;
  SO3(const Storage& s) : storage_(s) {}
  virtual ~SO3(){}

  double apply(double x) const {
    return Parameterization::apply(storage_, x);
  }

  static This exp(double y) { return This(Parameterization::exp(y)); }
  static double log( const This& R ){ return Parameterization::log(R.storage_); }
 protected:
  Storage storage_;
};

namespace parameterization {

template<typename Scalar>
struct AngleAxis {
  typedef Scalar Storage;
  static double apply(Scalar a, double b){ return a*b; }
  static Storage exp(Scalar a){ return ::exp(a); }
  static double log(Storage s){ return ::log(s); }
};

} // namespace parameterization

namespace interface {

template<typename Scalar>
class AngleAxis : public SO3< parameterization::AngleAxis<Scalar> > {
 public:
  typedef parameterization::AngleAxis<Scalar> Parameterization;
  typedef SO3< Parameterization > SO3;
  typedef typename Parameterization::Storage Storage;
  AngleAxis(Scalar s) : SO3(s) {}
  AngleAxis(const SO3& base) : SO3(base) {}
  virtual ~AngleAxis(){}

  Storage getStorage(){ return SO3::storage_; }
};

template<typename Scalar>
class AltAngleAxis : public SO3< parameterization::AngleAxis<Scalar> > {
 public:
  typedef parameterization::AngleAxis<Scalar> Parameterization;
  typedef SO3< Parameterization > SO3;
  typedef typename Parameterization::Storage Storage;
  AltAngleAxis(Scalar s) : SO3(s) {}
  AltAngleAxis(const SO3& base) : SO3(base) {}
  virtual ~AltAngleAxis(){}

  double operator*(Scalar s){ return SO3::apply(s); } 

  Storage getXXStorage(){ return SO3::storage_; }
};

} // namespace interface


int main(int argc, char ** argv) {
  std::cout << "Hello CRTP!\n";

  interface::AngleAxis<double> d1(1.0);
  interface::AltAngleAxis<double> d2(2.0);

  interface::AngleAxis<double> d3(interface::AngleAxis<double>::exp(3.0));
  interface::AltAngleAxis<double> d4(interface::AltAngleAxis<double>::exp(4.0));
  std::cout << d1.apply(2.0) << std::endl;
  std::cout << d1.getStorage() << std::endl;

  std::cout << d2 * 2.0 << std::endl;
  std::cout << d2.getXXStorage() << std::endl;

  std::cout << interface::AngleAxis<double>::log( d3 ) << std::endl;
  std::cout << interface::AltAngleAxis<double>::log( d4 ) << std::endl;


}

These examples all compile on my machine.

furgalep avatar Nov 15 '14 13:11 furgalep

Again, I updated the above after committing it.

furgalep avatar Nov 15 '14 13:11 furgalep

The last example did not compile on my machine (gcc version 4.8.2 (Ubuntu 4.8.2-19ubuntu1))

/home/gech/git/kindr_test/src/kindr_test_main.cpp:42:35: error: declaration of ‘typedef class SO3parameterization::AngleAxis<Scalar > interface::AngleAxis<Scalar>::SO3’ [-fpermissive] typedef SO3< Parameterization > SO3; ^ /home/gech/git/kindr_test/src/kindr_test_main.cpp:6:7: error: changes meaning of ‘SO3’ from ‘class SO3parameterization::AngleAxis<Scalar >’ [-fpermissive] class SO3 ^ /home/gech/git/kindr_test/src/kindr_test_main.cpp:55:35: error: declaration of ‘typedef class SO3parameterization::AngleAxis<Scalar > interface::AltAngleAxis<Scalar>::SO3’ [-fpermissive] typedef SO3< Parameterization > SO3; ^ /home/gech/git/kindr_test/src/kindr_test_main.cpp:6:7: error: changes meaning of ‘SO3’ from ‘class SO3parameterization::AngleAxis<Scalar >’ [-fpermissive] class SO3 ^

This version works:

#include <iostream>
#include <cmath>

template<class Parameterization>
class SO3
{
 public:
  typedef typename Parameterization::Storage Storage;
  typedef SO3<Parameterization> This;
  SO3(const Storage& s) : storage_(s) {}
  virtual ~SO3(){}

  double apply(double x) const {
    return Parameterization::apply(storage_, x);
  }

  static This exp(double y) { return This(Parameterization::exp(y)); }
  static double log( const This& R ){ return Parameterization::log(R.storage_); }
 protected:
  Storage storage_;
};

namespace parameterization {

template<typename Scalar>
struct AngleAxis {
  typedef Scalar Storage;
  static double apply(Scalar a, double b){ return a*b; }
  static Storage exp(Scalar a){ return ::exp(a); }
  static double log(Storage s){ return ::log(s); }
};

} // namespace parameterization

namespace interface {

template<typename Scalar>
class AngleAxis : public SO3< parameterization::AngleAxis<Scalar> > {
 public:
  typedef parameterization::AngleAxis<Scalar> Parameterization;
  typedef SO3< Parameterization > Base;
  typedef typename Parameterization::Storage Storage;
  AngleAxis(Scalar s) : Base(s) {}
  AngleAxis(const Base& base) : Base(base) {}
  virtual ~AngleAxis(){}

  Storage getStorage(){ return Base::storage_; }
};

template<typename Scalar>
class AltAngleAxis : public SO3< parameterization::AngleAxis<Scalar> > {
 public:
  typedef parameterization::AngleAxis<Scalar> Parameterization;
  typedef SO3< Parameterization > Base;
  typedef typename Parameterization::Storage Storage;
  AltAngleAxis(Scalar s) : Base(s) {}
  AltAngleAxis(const Base& base) : Base(base) {}
  virtual ~AltAngleAxis(){}

  double operator*(Scalar s){ return Base::apply(s); }

  Storage getXXStorage(){ return Base::storage_; }
};

} // namespace interface


int main(int argc, char ** argv) {
  std::cout << "Hello CRTP!\n";

  interface::AngleAxis<double> d1(1.0);
  interface::AltAngleAxis<double> d2(2.0);

  interface::AngleAxis<double> d3(interface::AngleAxis<double>::exp(3.0));
  interface::AltAngleAxis<double> d4(interface::AltAngleAxis<double>::exp(4.0));
  std::cout << d1.apply(2.0) << std::endl;
  std::cout << d1.getStorage() << std::endl;

  std::cout << d2 * 2.0 << std::endl;
  std::cout << d2.getXXStorage() << std::endl;

  std::cout << interface::AngleAxis<double>::log( d3 ) << std::endl;
  std::cout << interface::AltAngleAxis<double>::log( d4 ) << std::endl;
}

gehrinch avatar Nov 16 '14 12:11 gehrinch

I find the version with three classes more difficult to understand. I'm not really sure where I should implement a method which holds only for one parameterization, for instance, getVectorNorm() of the angle axis.

Guessing from the names I would add the method to parameterization::AngleAxis, but I see the method getXXStorage() in interface::AltAngleAxis::getXXStorage(), which I believe is specific for AngleAxis.

gehrinch avatar Nov 16 '14 13:11 gehrinch

Hi Christian, Thanks for the comment. I would love to get this sorted out this weekend if possible!

I agree it should be easy to understand. Let me try to explain and see if that helps. There are three levels here:

  • Parameterization -- This layer implements all of the necessary functions for a single parameterization of SO3. It defines the following:
    • A Storage type that says how the memory of the parameterization is stored.
    • A functional interface for manipulating the parameteriazation: static functions that act the storage type and vectors. All of the required static functions will be specified somewhere. Think of things like log(), exp(), rotate(), compose(), etc.
  • SO3<Parameterization> -- This is the top level that implements a common Object Oriented interface (lowest common denominator) for all parameterizations of SO3.
  • The interface layer -- this layer adds parameterization-specific functions to an SO3<Parameterization> object. The interface layer is not strictly necessary (i.e. objects of type SO3<AngleAxis> can be created without an extended interface), but it adds parameterization-specific syntactic sugar, like getVectorNorm(), getAngle(), etc.

Is that clear?

@gehrinch @HannesSommer

furgalep avatar Nov 16 '14 13:11 furgalep

Kind of clear, but I have the same concerns as Hannes:

"There is an important issue with not using CRTP: SO3 would not know about the real full type."

I tried to implement a method convert to do the following:


  interface::EulerAnglesZyx<double> eulerAngles;
  interface::AngleAxis<double> angleAxis;
  double angle = angleAxis.convert(eulerAngles).getAngle();

Note that the convert method in this example modifies the angleAxis object (which can be arguable).

I tried to implement this as following, but failed due to the unknown type problem:

template<class Parameterization_>
class SO3
{
  template<typename OtherParameterization_>
  ??? convert(const SO3<OtherParameterization_>& other) {
    storage_ = Parameterization_::convert(storage_, other);
    return *this;
  }
}

namespace parameterization {

template<typename Scalar_>
struct AngleAxis {

  template<typename OtherParameterization_>
  static Storage convert(const Storage& storage, const SO3<OtherParameterization_>& other) {
    // do some conversion
    return storage;
  }
};

} // namespace parameterization

gehrinch avatar Nov 16 '14 15:11 gehrinch

Things to decide then quickly (not based on @gehrinch last commit):

  • how would the default way be to access storage? Can one generically access the storage on a SO3<Param> object? Read only? RW? (I would suggest : storage() to get a non const reference to the storage. Could be a good compromise.)
  • Do we support mixed compositions and how do we decide on the result type and impl? (I would have traits for that, even a default impl, that uses conversion, because there are many parametrization nobody wants to implement a composition for, like EulerAngles).
  • and can we agree on the meaning of model and frame neutral semantics of the parametrization: #52

HannesSommer avatar Nov 16 '14 15:11 HannesSommer

No about the missing type. That is exactly my problem. But I found some kind of peace with that by proposing that the functional interface always needs to be given the full information: any conversion functions needs to know in which format it should convert and in which storage it should write (I would not even use return there - just input parameters).

The OO interface to conversion I would do like this, for the example above:

angleAxis = eulerAngles;
double angle = angleAxis.getAngle();

if the change is desired, or if not

double angle = AngleAxis(eulerAngles).getAngle();

(this is both possible even without SO3<EulerParam> knowing of Euler or AngleAxis).

HannesSommer avatar Nov 16 '14 15:11 HannesSommer

After optimization the latter expression will even be nicely freed from most unnecessary computations :). That is pretty nice, I think :).

HannesSommer avatar Nov 16 '14 15:11 HannesSommer

How about

double angle = angleAxis.invert().getAngle();

would this also be impossible? We use these chainings a lot. Would be super bad if this would not work anymore.

gehrinch avatar Nov 16 '14 15:11 gehrinch