ros-visualization / rqt_bag

http://wiki.ros.org/rqt_bag
31 stars 55 forks source link

Fully functioning ROS2 port #56

Closed mjeronimo closed 4 years ago

mjeronimo commented 4 years ago

I've continued the port started a while back on the 'ros2_port' branch. All of the features should be available with this PR, but some work remains.

The following minor improvements are included in this PR:

The following issues remain to be addressed:

FYI - This branch started from the 'ros2_port' branch and then rebased to the current master before making the changes.

dirk-thomas commented 4 years ago

The size of the patch (+918 −479 lines) as well as the large number of orthogonal things fixed / changed / ported makes it impossible to review in a reasonable way.

Imo it would be helpful to land changes in separate pull requests:

mjeronimo commented 4 years ago

@dirk-thomas Sure. I recently asked Jacob about the strategy on a change like this as I know large PRs can be a pain. The proposed approach sounds good to me. It'll be easier to do now that I have it working and can go back and review all of the changes and stage them as you suggest.

@jacobperron If OK with you, I'll close this PR and get going on submitting (lots of) separate PRs for the individual features/fixes as suggested, starting with those changes that are also applicable to ROS1.

jacobperron commented 4 years ago

Sure, having separate PRs for the improvements (not needed for the port) makes sense to me.

For changes strictly to complete the port, I don't mind if it's one large PR or several incremental ones. At least with the former, we can actually run the application and try it out.

mjeronimo commented 4 years ago

Ok, I'll close and continue. Once I get to the ROS2-specific changes, I'll try to balance providing functionality and the associated code review size.