search

ros / ros_tutorials

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

sin(x+pi/2)-> cos(x); cos(x+pi/2)->-sin(x) in turtle.cpp #51

Closed zhoulaifu closed 5 years ago

zhoulaifu commented 5 years ago

Fixing #47.

dirk-thomas commented 5 years ago

I have reproduce the steps described in https://github.com/ros/ros_tutorials/issues/47#issuecomment-460688443 with this patch and the results remains the same.

As mentioned before I don't see a reason to merge this patch since it doesn't actually fix anything. It make one special case nicer but on the other hand "breaks" another case in the same way.

zhoulaifu commented 5 years ago

Sorry for the confusion. The PR attempted to fix issue #47, the one listed in the very beginning, not the one you pointed out in the comment (namely, the one you were able to reproduce with turtle_teleop_key).

The latter issue from your comment, in my humble opinion, is expected and should not invalidate the PR, because in lines 160-168 of ros_tutorials/turtlesim/src/turtle.cpp, the code explicitly does a "clamping" that basically specifies that the simulated turtle should go to the corner position if both coordinates (x and y) go out of the canvas.

dirk-thomas commented 5 years ago

So how does this patch fix the problem described in the ticket?

The simulated turtle moves to a wrong direction with very large linear velocity

Imo it only improves the stating situation. After any kind of rotation the same problems exists as before. In the case of a +/- PI/2 rotation it actually makes the behavior worse than without the patch.

zhoulaifu commented 5 years ago

In the original code, the expression sin(x+pi/2) opens the door for floating-point inaccuracy for all x, because the expression uses pi that is inherently imprecise. Changing sin(x+pi/2) to cos(x) as in my PR is mathematically sound and can remove the floating-point issue for all x by not using pi directly. To sum, the PR fixes an unnecessary floating-point inaccuracy issue that manifests through what I described in #47 and that can, in theory at least, manifests through any "x". Certainly not a single case IMHO.

You said, "After any kind of rotation the same problems exists as before." I wonder if you were drawing this conclusion from the scenario provided in your comment (with turtle_teleop_key). As explained above, that scenario is expected, as your code that does the "clamping". I would see the two issues as separated.

dirk-thomas commented 5 years ago

I understand you argument but I still don't think this patch is necessary. Anyway since it doesn't do any harm I am happy to merge it - at least the formula is shorter hence more readable.

Thanks for the patch.