[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
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

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

Merged
merged 1 commit into from
Feb 11, 2019
Merged

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

merged 1 commit into from
Feb 11, 2019

Conversation

zhoulaifu
Copy link
Contributor
@zhoulaifu zhoulaifu commented Feb 10, 2019

Fixing #47.

@dirk-thomas dirk-thomas changed the title sin(x+pi/2)-> cos(x); cos(x+pi/2)->-sin(x) in turtle.cpp. Fixing #47. sin(x+pi/2)-> cos(x); cos(x+pi/2)->-sin(x) in turtle.cpp Feb 11, 2019
@dirk-thomas
Copy link
Member

I have reproduce the steps described in #47 (comment) 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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author
zhoulaifu commented Feb 11, 2019

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
Copy link
Member

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.

@dirk-thomas dirk-thomas merged commit 2fb7b9f into ros:melodic-devel Feb 11, 2019
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.

2 participants