ros2 / ros1_bridge

ROS 2 package that provides bidirectional communication between ROS 1 and ROS 2
Apache License 2.0
425 stars 275 forks source link

Fix message mapping by removing early return so other rules can still be applied #380

Closed LoyVanBeek closed 1 year ago

LoyVanBeek commented 1 year ago

In determine_field_mapping, there was an early return inside a loop over all mapping rules. If there were any mapping rules but they don't specify field mappings, the early return made the function return without creating mappings automatically.

For a particular message type, ROS 1's uuid_msgs/UniqueID vs ROS 2's unique_identifier_msgs/UUID, the message definition is exactly the same but type name is not. The only mapping rule defined for unique_identifier_msgs/UUID is that it maps to uuid_msgs/UniqueID, but no field mappings are needed because the definitions are the same: https://github.com/ros2/unique_identifier_msgs/blob/rolling/mapping_rules.yaml

But, then we hit the early return (because the for-loop is ran without any rule applying to the message at hand and thus not continue-ing in a code branch handling a rule) and return without applying the normal automatic field mapping generation rules.

By removing the early return, the other rules are applied and the mapping rules for handling the exact same message definitions are applied.

This fixes the issue found in https://github.com/ros2/unique_identifier_msgs/pull/25 in ros1_bridge instead of unique_identifier_msgs

This PR applies to Foxy because I combine it with Noetic and can test only that combo, but the early return causing this issue is still there on the main branch: https://github.com/ros2/ros1_bridge/blob/master/ros1_bridge/__init__.py#L788

LoyVanBeek commented 1 year ago

Not entirely correct yet, closing and will open a new PR soon

LoyVanBeek commented 1 year ago

Fixed version is in https://github.com/ros2/ros1_bridge/pull/382