Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TimePanel port #599

Merged
merged 4 commits into from
Sep 11, 2020
Merged

TimePanel port #599

merged 4 commits into from
Sep 11, 2020

Conversation

neumann-nico
Copy link
Contributor

@neumann-nico neumann-nico commented Aug 20, 2020

This pull request adds a working version of the Time Panel to rviz_common.
Tested on Ubuntu 20.04 ROS foxy and Ubuntu 18.04 ROS eloquent.

Copy link
Member

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trying out this PR, when I start RViz, without any persistent RViz config, no panels are loaded and the render window doesn't appear. I'm on Ubuntu Focal.

I'm not sure where the bug is. Does this work for you?

Copy link
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm with CI and @jacobperron's issue addressed

I didn't test it locally.

@neumann-nico
Copy link
Contributor Author

neumann-nico commented Aug 28, 2020

@jacobperron @wjwwood Thanks for the review.

I tested it again and have the same bug that panels are not loaded. The problem is that the QMainWindow State needs to be updated too.

If I comment out the time panel in the default config again, open rviz2 and add the time panel over the gui it works fine. But saving this configuration file leads to a lot of changes also in other panels that may not be necessary. I'm not sure how to handle it properly. Maybe you have an idea @jacobperron.

I just fixed another bug: When saving the configuration and restarting rviz2 with activated time panel I get this error which results in a crash:

terminate called after throwing an instance of 'std::runtime_error'
  what():  can't subtract times with different time sources [1 != 2]

The crash happens in this part of the code:

double VisualizationManager::getROSTimeElapsed()
{
  // TODO(wjwwood): why does this function return now - begin, whereas the getWallClockElapsed
  //                returns a pre-calculated elapsed value?
  //                figure out how this function is being used and make these consistent
  // return (frame_manager_->getTime() - ros_time_begin_).nanoseconds() / 1e9;  // crashed here
  return static_cast<double>(ros_time_elapsed_) / 1e9;                          // changed to this line
}

I tried to return the ros_time_elapsed_ variable that is calculated in the updateTime method, instead of returning the calculation above and it fixes the crash.

void VisualizationManager::updateTime()
{
  rclcpp::Clock clock;  // TODO(wjwwood): replace with clock attached to node for ROS Time
  if (ros_time_begin_.nanoseconds() == 0) {
    ros_time_begin_ = clock_->now();
  }

  ros_time_elapsed_ = (clock_->now() - ros_time_begin_).nanoseconds();

  if (wall_clock_begin_.time_since_epoch().count() == 0) {
    wall_clock_begin_ = std::chrono::system_clock::now();
  }

  wall_clock_elapsed_ = std::chrono::system_clock::now() - wall_clock_begin_;
}

Tested on foxy branch (ROS foxy + Ubuntu 20.04) and on eloquent branch (ROS eloquent + Ubuntu 18.04).

@jacobperron
Copy link
Member

It would be nice to have the TimePanel on by default.

Sorry, I'm starting a vacation and won't be able to take a closer look until a couple weeks from now. I can take a look then, if this ticket is still open.

fixed indentation
Copy link
Member

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to be working now (I guess the issue was caused by an incorrect indentation level in the default config?).

@jacobperron
Copy link
Member

jacobperron commented Sep 10, 2020

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@neumann-nico
Copy link
Contributor Author

@jacobperron Thanks for approving!
Yes the issue was caused by an incorrect indentation.

The time panel is activated and running by default now. That's how you wanted to have it?
I tried to let the panel activated but hide it by default, but somehow I couldn't manage to change it.

@jacobperron
Copy link
Member

@neumann-nico I think having the time panel active by default is good. This is also the default in ROS 1, I think.

Thanks for the feature!

@jacobperron jacobperron merged commit 3f6108c into ros2:ros2 Sep 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants