search

ubi-agni / mujoco_ros_pkgs

Wrappers, tools and additional API's for using ROS with MuJoCo
68 stars 12 forks source link

Avoid publishing /clock twice per cycle #9

Closed rhaschke closed 1 year ago

rhaschke commented 1 year ago

Was there a specific reason to publish the time at the beginning of this function already?

DavidPL1 commented 1 year ago

It was initially meant to make sure the current time was known to ROS. The line you point out should definitely be deleted.

This call and this call are also unnecessary, because time should not change between the main sim loop and where these calls happen.

And as we don't allow starting the simulation with a non-zero time anymore and IIRC each ROS component should start at time 0 when using sim time, this call should also be removed. But this is only safe to assume if /use_sim_time is set before any node is started, so we should actually exit here, instead of setting it manually.

The reason I'm commenting all this is that I think it makes more sense to incorporate these changes into this PR, repurposing it to "Avoid redundant clock publishing" and make it a single commit.

rhaschke commented 1 year ago

All valid comments. Please just add more commits to this PR.

And as we don't allow starting the simulation with a non-zero time anymore and IIRC each ROS component should start at time 0 when using sim time, this call should also be removed.

I'm not sure about that one. If I'm not mistaken, this publishes the initial time (0) even if the simulation didn't yet start (i.e. is paused). If so, this should be kept in order to have a valid time initially - even if the MuJoCo is started in paused mode.

rhaschke commented 1 year ago

@DavidPL1, I rebased your additional changes from branch pr/9 to this original PR branch.