Looks good, could you add

• multiply state variable for joint states

Adding soon.

• add an assertion that it fails if you provide a vector of wrong size

While writing the test for this I realized something somehting else that we might need to address in a follow up PR, which is a bit problematic when inputs are eigen vectors (C++).

For example:

  auto state = CartesianState();
  auto new_incompatible_values = Eigen::VectorXd(4);
  new_incompatible_values << 1.0, 2.0, 3.0, 4.0;
  EXPECT_THROW(state.set_state_variable(new_incompatible_values, state_variable_type), exceptions::IncompatibleSizeException);

For the new methods that are exposed this is correct (as in, it throws our custom exception).

However:

  auto state = CartesianState();
  auto new_incompatible_values = Eigen::VectorXd(4);
  new_incompatible_values << 1.0, 2.0, 3.0, 4.0;
  EXPECT_THROW(state.set_position(new_incompatible_values), exceptions::IncompatibleSizeException);

is problematic, because the signature for set_position is:

void CartesianState::set_position(const Eigen::Vector3d& position) {
  this->set_state_variable(position, CartesianStateVariable::POSITION);
}

which means that C++ will implicitly cast the VectorXd down to Vector3d which will cause an exception much earlier than our internal check in set_state_variable. This might not be explicitly our responsibility, but I feel like we should mark this function (and perhaps others) as explicit and prevent this kind of uses (@eeberhard adding you mostly for this point and whether you think a follow up PR would make sense)

Thanks for writing this up. Your suggestion was to add [[explicit]] is that correct? That would have as effect that the compilation would already fail?

Thanks for writing this up. Your suggestion was to add [[explicit]] is that correct? That would have as effect that the compilation would already fail?

Well actually my bad. We can not use explicit on a function (just constructors, destructors and conversion operators), which means typesafety here would require sfinae, C++20, or deleting overloads. I don't like the first and last at the current point, and C++20 is also a big change.

I think for now we can open a small backlog item, but we might just be ok with delegating responsibility downwards for this.

I think for now we can open a small backlog item, but we might just be ok with delegating responsibility downwards for this.

Yeah these changes seem to be a bit too big for now, a backlog item with no immediate action taken seems fair for now

Thanks for writing this up. Your suggestion was to add [[explicit]] is that correct? That would have as effect that the compilation would already fail?

Well actually my bad. We can not use explicit on a function (just constructors, destructors and conversion operators), which means typesafety here would require sfinae, C++20, or deleting overloads. I don't like the first and last at the current point, and C++20 is also a big change.

I think for now we can open a small backlog item, but we might just be ok with delegating responsibility downwards for this.

For reference we could do something like:

SFINAE

template <typename T>
typename std::enable_if<std::is_same<T, Eigen::Vector3d>::value>::type
void CartesianState::set_position(const T& position) {
  this->set_state_variable(position, CartesianStateVariable::POSITION);
}

or C++20:

template <typename T>
concept IsEigenVec3 = std::same_as<T, Eigen::Vector3d>;

void CartesianState::set_position(const IsEigenVec3& position) {
  this->set_state_variable(position, CartesianStateVariable::POSITION);
}

seems a bit too much boilerplate.

Perhaps the simplest is to just:

void CartesianState::set_position(const Eigen::VectorXd& position) {
  this->set_state_variable(position, CartesianStateVariable::POSITION);
}

and let the set_state_variable throw the exception

Perhaps the simplest is to just:

Sure but that is even less correct to me IMO because the function signature tells the user that it requires a Vector3d which is the only logical choice for position

Perhaps the simplest is to just:

Sure but that is even less correct to me IMO because the function signature tells the user that it requires a Vector3d which is the only logical choice for position

I agree, yes.