Make Quaternion valid by default

[jdlangs]

It was often a gotcha in ROS1 that creating a default Quaternion object didn't give you the identity quaternion (w,x,y,z == 1,0,0,0) but rather an invalid quaternion with everything initialized to zero. I always thought this was the perfect use of default values in the .msg file. Any particular reason why this hasn't been done already?

[jacobperron]

It seems like a reasonable change to me. I'm not sure if there's any ramifications we should worry about given that a lot of code is using this message. I'm not familiar enough with ros1_bridge to know if there is anything else that needs to be done.

I'd like to get feedback from others.

[dirk-thomas]

I'm not familiar enough with ros1_bridge to know if there is anything else that needs to be done.

The ros1_bridge is agnostic to this change. It simply uses the values coming in on one side and copies them to the others. Therefore it doesn't matter to the bridge if the message had default values or not.

[mjcarroll]

My concern would that someone is potentially relying on this behavior already. An example I can think of would be:

if (quaternion.valid) {
  // do something
} else {
  // do something else
}

I don't have any cases of this happening readily, but it's potentially out there.

[jdlangs]

That pseudocode seems too abstract to address as a potential concern. So far I'm not able to think of any valid use case that this change would break. The only common usage I can think of for the else clause would be do to a renormalization, but that would give a divide by zero error anyway for the all-zeros default.

I can think of two possibilities for breakage that I would consider invalid use cases:

• The all-zeros Quaternion is used as a null value, and program logic uses it's presence to make a decision.
• Quaternion is used just as an abstract tuple of 4 values with w relied on to be zero-initialized.

This both seem like abuses of the intended purpose of generated interfaces and I assume there's no commitment to not breaking mis-use like that.

[mjcarroll]

Apologies, it was a bit hand-wavy. I meant to say initialized vs uninitialized rather than valid/invalid. To your first bullet point, an example I could think of is in the IMU message, where if the quaternion value is not all-zeros, it can be assumed that the sensor is providing a filtered orientation, where alternatively the program could choose to use the angular rates instead and compute an orientation.

I agree that this is probably a shortcoming of the downstream users of the Quaternion message, rather than the Quaternion message itself. I believe at least in robot_localization, the user has to directly specify the fields from the IMU message itself, so it wouldn't be an issue there.

[jacobperron]

My concern would that someone is potentially relying on this behavior already.

That's what releases/versioning is for? It will probably break something for someone, but it does seem like a step forward IMO.

[mjcarroll]

That's what releases/versioning is for? It will probably break something for someone, but it does seem like a step forward IMO.

Fair enough, I'm not opposed. It probably warrants a note on the Eloquent release page.

[jdlangs]

I appreciated the discussion to make sure we'd thought through possible consequences. Hopefully this will turn out to be a very easy win.

[jacobperron]

• Linux
• Linux-aarch64
• macOS
• Windows

Look like this change introduces test failures to tf2_geometry_msgs and tf2_kdl. I believe the other failures are unrelated (occurring with the master branch).

[jdlangs]

Those tests are manually "initializing" quaternions by setting a member other than w to 1 while assuming the others are zero, which probably does happen in a handful of cases.

[jacobperron]

Those tests are manually "initializing" quaternions by setting a member other than w to 1 while assuming the others are zero, which probably does happen in a handful of cases.

@jdlangs Would you mind contributing some PRs to update those tests so that they are compatible with this change?

[jdlangs]

PR made: ros2/geometry2#177

[jacobperron]

PR made: ros2/geometry2#177

Thanks! I'll run CI again to see where we stand:

• Linux
• Linux-aarch64
• macOS
• Windows

Edit: re-triggered with up-to-date repos file.

All test failures are unrelated; also occurring in the most recent nightly CI jobs.

[jacobperron]

@jdlangs Thanks for the contribution! I'll document this change in the Eloquent release notes.

[jdlangs]

Thanks for your time checking this and getting it included!