Inconsequent behavior of 'vector()' method for Quaternion/Position
Opened this issue · 4 comments
The problem is that .vector() method returning Eigen vector works in a different way for Kindr Quaternion and Position objects. For Position, it returns a reference to the original data while for Quaternion a copy is returned.
The current situation is dangerous, because many people (including me) assume that same-name methods generally do the same. So, in my case, once I was happy with pointer my_position.vector().data(), I gladly wrote my_quaternion.vector().data() to realize that the object remains unchanged. If you're using some optimization library on top of Kindr, the last thing you suspect is that you're getting a copy and not a reference.
I understand it's not trivial to solve this misleading situation because Eigen is storing quaternions with underlying [xyzw] memory layout and Kindr 'promotes' Hamilton convention [wxyz] and you probably want to return a vector in this order.
I think the documentation should mention, that generally the safest way to get direct access to memory (e.g. for optimization purposes) is to use .toImplementation() method returning a reference to underlying Eigen object (and then using .data() to get a pointer).
I know this issue has been already discussed over mail (with @simonlynen), but @furgalep suggested that it may be worth to talk about it here and also keep track of it.
This is indeed problematic.
A possible solution would be to get rid of the underlying Eigen implementation.
For now, this should be added to the documentation as suggested.
@dymczykm Can you please add it to the documentation?
We could improve the readability by using the keyword 'get' for value returns.
For instance:
- Scalar EulerAnglesZyx::getPitch()
- Scalar& EulerAnglesZyx::pitch()
Any suggestions?
I like the first one because it is pretty standard. The second one is much more a problem. In the programming guidelines we finally decided to use getPitchRef() - to be very explicit. See ethz-asl/programming_guidelines#5 (comment)
For a const ref I would still use just getPitch()!