search

ros / ros_tutorials

Code used in tutorials found on ROS wiki
http://wiki.ros.org/ros_tutorials
805 stars 540 forks source link

Theta range from -p to +pi instead of -2pi to +2pi. #31

Closed JavaJeremy closed 7 years ago

JavaJeremy commented 8 years ago

Range of theta now from 0 to 2PI not from -2PI to 2PI. No negative values when rotating clockwise now.

Thanks for the help @v4hn!

dirk-thomas commented 8 years ago

While limiting the range of theta makes sense why should it be from 0 to 2pi? Wouldn't a range from -pi to +pi be more intuitive?

v4hn commented 8 years ago

@dirk-thomas isn't either method totally fine? In my experience both are used in everyday mathematics. The reason for this request is that the range is between [-_2_pi;+_2_pi] right now.

dirk-thomas commented 8 years ago

This is a demo application trying to show the most basic functionality in ROS. I am not sure if the interval of theta matters that much.

But if it should be changed to avoid allowing different values with the same semantic then I would suggest to normalize to [-pi, pi) since that is afaik the most common interval (others seem to have a similar opinion: e.g. http://answers.ros.org/question/61553/what-is-normalize/).

v4hn commented 8 years ago

On Mon, Jun 27, 2016 at 11:16:29AM -0700, Dirk Thomas wrote:

This is a demo application trying to show the most basic functionality in ROS. I am not sure if the interval of theta matters that much.

It doesn't. But during an introduction @JavaJeremy got confused by this behavior and I proposed he should write a merge request. :-)

Personally, I think of [0;2pi) as more intuitive and even the link you cite acknowledges both intervals.

I just talked to him and he promised to adjust the interval. This code should do the trick (untested):

orient = orient + angvel * dt orient_ - 2_MPI * std::floor((orient + M_PI)/(2_M_PI)) @JavaJeremy: would you mind testing it and update the request?

JavaJeremy commented 7 years ago

@dirk-thomas I finally got around to adjust it, please merge.

JavaJeremy commented 7 years ago

Alright @dirk-thomas, I added a comment above the line that does the magic.

dirk-thomas commented 7 years ago

Thank you!

dirk-thomas commented 7 years ago

I just switched the target branch to lunar-devel since that is now the default branch.