Fixed turn_towards #40

Piripant · 2017-12-09T16:59:19Z

With a small change in the turn_towards function it should now work correctly.
The followmouse example can show it functioning.

Closes #8

sunjay · 2017-12-09T17:43:44Z

Hi! I tried out your change locally and it definitely fixes the bug. Thank you for submitting this fix and figuring out what was going on!

I am ready to merge this, but if you would like to do a little more, I have two suggestions for additional changes that would make this even better:

Add comments before the different equations used in turn_towards to give the reader some idea of what each line does
- This will help future people looking at the code understand what is going on
Add some tests for the different cases that this method can encounter
- This will help make sure future contributors don't break this method
- Some docs about running tests (use cargo test --features test)
- The tests would go at the bottom of src/turtle.rs with the other tests that are already there
- You can make sure your tests are effective by changing/removing things like the fix you just did and seeing if the tests break
- Tips: use set_heading() to set the turtle's heading, assert the heading before and after (using round or assert_approx_eq)

Though both of these are probably things I should have done when I wrote the method, I think you must have a pretty good understanding of the method in order to make this fix. That means that you're in a great position to make these additions.

If you don't want to do one or both of these things, or don't have time, please let me know. There is no pressure. In that case, I'll merge your changes immediately and add those two items to my own to-do list. 😄

If you're unsure, it may be worth giving it a shot to see if you can do it. I'm happy to help you with any questions you might have. If you want me to, I'll even setup a sample test for you so you can just add additional cases that you think are important. Let me know what you're comfortable with. :)

Piripant · 2017-12-09T18:55:03Z

Thanks for your assistance!
I've written a test which should cover all the usages, and put some comments to explain what is going on and why.
Tell me if there is anything else I can do!

sunjay

Thanks for adding all of that! The comments you added are very informative!

Your test looks good. Could you also add a test to make sure the turtle can turn from any of the 8 cardinal directions to any other cardinal directions? You can do that either as part of your current test or split into a new one. Your current test only checks if the angle is increasing but I would also like to make sure that the method results in the correct heading. In that new test, it would be great if you could assert that the heading actually equals the intended value. Let me know if you want me to give you more of an idea of how to write that test. :)

sunjay · 2017-12-09T19:17:42Z

src/turtle.rs

+    #[test]
+    fn turn_towards() {
+        let mut turtle = Turtle::new();
+        turtle.set_speed("instant");


This line shouldn't be necessary since we disable all animations during tests. Try running the tests with cargo test --features test and let me know if for some reason keeping this line actually makes a difference.

sunjay · 2017-12-09T20:10:03Z

src/turtle.rs

+    fn turn_towards() {
+        let mut turtle = Turtle::new();
+        turtle.set_speed("instant");
+        let mut last_delta_angle = 0.0;


Please replace this line with the following code to get rid of the warning that occurs when this compiles.

// This variable is used in the assert!(), but Rust still warns us because its value isn't // used to do anything other than that. We ignore this error since this is okay for tests. #[allow(unused_assignments)] let mut last_delta_angle = 0.0;

Piripant · 2017-12-10T14:07:29Z

I have updated the test according to your requests.
Since the animations are disabled in the tests, checking for the turning direction would be useless, as the target angle is reached immediately.
I have decided to replace my test with what you suggested, checking if the heading angle is correct while jumping from cardinal direction to cardinal direction.
P.S: setting the turtle speed to instant doesn't seem to change anything. You can see it in the followmouse example, where the turtle does not jump directly to the cursor position, nor turns immediately.

sunjay

Thank you for updating the PR and working with me to get this feature correct! Your contribution is going to make a huge difference. 😄

It's really good that we're writing these tests because I found a bug that wasn't revealed earlier when I went through and tried this out. Take a look at my review comments to find out more. I wrote you a little test application which should help you see the problem and debug for yourself.

Feel free to let me know if you have any questions!

P.S: setting the turtle speed to instant doesn't seem to change anything. You can see it in the followmouse example, where the turtle does not jump directly to the cursor position, nor turns immediately.

To clarify, examples (like followmouse) run animations because those are meant to actually be seen by users. Tests do not run examples because they have no UI and are meant to complete as fast as possible. In tests, we're really just trying to make sure we get the right values out of our functions/methods.

sunjay · 2017-12-10T17:13:48Z

src/turtle.rs

+        for n in 0..16 as u32 {
+            let original_angle = radians::TWO_PI * n as f64 / 16.0;
+            turtle.turn_towards([original_angle.cos(), original_angle.sin()]);
+            assert_eq!(turtle.heading().ceil(), original_angle.to_degrees().ceil());


Please use round() instead of ceil() because we want to make sure that rounding differences are caught by these tests.

sunjay · 2017-12-10T17:47:02Z

src/turtle.rs

@@ -1156,4 +1158,22 @@ mod tests {
        assert_eq!(turtle.position()[1].round(), 100.0);
        assert_eq!(turtle.heading(), 51.0);
    }
+
+    #[test]
+    fn turn_towards() {        


Thanks for updating this! I think you need to move lines 1168-1170 into the inner loop. Otherwise you are rotating to original_angle only once, then essentially rotating in a circle without actually going back to the original angle. The goal is to test that you can rotate from any angle to any angle. That means we need to go back to the original angle every time in order to make an exhaustive test.

If you make that change, you'll find that the test actually doesn't pass anymore. In fact, I made a little demo application which reveals that a bug still remains in the implementation of turn_towards.

Try running this by overwriting the contents of an example like examples/circle.rs and then running it with cargo run --example circle (don't commit that change, this is just to help you debug) Click to advance to each rotation. This should be a really helpful tool for you to debug what is going on. Feel free to modify it as you see fit in order to help you figure out the problem.

extern crate turtle; use turtle::Turtle; use std::f64::consts::PI; fn main() { let mut turtle = Turtle::new(); // Turn from each cardinal direction to each cardinal direction for n in 0..16 as u32 { let original_angle = 2.*PI * n as f64 / 16.0; for i in 0..16 as u32 { println!("{:?}", (n, i)); turtle.wait_for_click(); turtle.turn_towards([original_angle.cos(), original_angle.sin()]); //assert_eq!(turtle.heading().ceil(), original_angle.to_degrees().ceil()); turtle.wait(1.0); let target_angle = 2.*PI * i as f64 / 16.0; turtle.turn_towards([target_angle.cos(), target_angle.sin()]); //assert_eq!(turtle.heading().ceil(), target_angle.to_degrees().ceil()); turtle.forward(100.0); turtle.backward(100.0); } turtle.wait_for_click(); turtle.clear(); } }

Notice that this doesn't produce a full circle of 16 lines. There are "gaps" which are the result of a bug in the calculation that still needs to be fixed. Good thing we're writing these tests! If you hadn't volunteered to do that, we wouldn't have found this problem. Thank you so much!

Piripant · 2017-12-10T18:21:16Z

I have ran your changed example as you suggested and everything works fine.
I have also noticed that running your example with the old turn_towards produces your same wrong pattern... Are you sure you are using the new version?
I even adjusted the test and it passes without problems.
Using round() instead of ceil() makes it fail by one pesky 202.49999999999997 vs 202.5 rounding difference.

sunjay

Are you sure you are using the new version?

Ah! You're right! That was totally my bad. I am clearly bouncing between way too many branches right now. Thanks for clearing that up!

Using round() instead of ceil() makes it fail by one pesky 202.49999999999997 vs 202.5 rounding difference.

No problem! Everything looks good.

sunjay · 2017-12-10T20:24:43Z

@Piripant Thank you so much for working on this! You did an excellent job fixing the bug, documenting everything, and writing tests. I really appreciated that you went back and resolved all my comments so quickly and without complaint. Thanks to you, this feature will be available in the first 1.0.0 stable release!

Piripant · 2017-12-10T20:30:11Z

My pleasure! Thanks for assisting me and driving me in the right direction! I am happy to contribute!
🎉🎉🎉

sunjay self-requested a review December 9, 2017 17:18

sunjay self-assigned this Dec 9, 2017

sunjay added the bug label Dec 9, 2017

sunjay added this to the 1.0.0 - MVP milestone Dec 9, 2017

sunjay approved these changes Dec 9, 2017

View reviewed changes

Added comments and test

f945115

sunjay reviewed Dec 9, 2017

View reviewed changes

Updated turn_towards test

f398dfc

sunjay reviewed Dec 10, 2017

View reviewed changes

Fixed test

b5698f1

Cleaning code

bdf7243

sunjay approved these changes Dec 10, 2017

View reviewed changes

sunjay merged commit 1476e1a into sunjay:master Dec 10, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed turn_towards #40

Fixed turn_towards #40

Piripant commented Dec 9, 2017

sunjay commented Dec 9, 2017 •

edited

Loading

Piripant commented Dec 9, 2017 •

edited

Loading

sunjay left a comment

sunjay Dec 9, 2017

sunjay Dec 9, 2017

Piripant commented Dec 10, 2017

sunjay left a comment

sunjay Dec 10, 2017

sunjay Dec 10, 2017

Piripant commented Dec 10, 2017 •

edited

Loading

sunjay left a comment

sunjay commented Dec 10, 2017

Piripant commented Dec 10, 2017

Fixed turn_towards #40

Fixed turn_towards #40

Conversation

Piripant commented Dec 9, 2017

sunjay commented Dec 9, 2017 • edited Loading

Piripant commented Dec 9, 2017 • edited Loading

sunjay left a comment

Choose a reason for hiding this comment

sunjay Dec 9, 2017

Choose a reason for hiding this comment

sunjay Dec 9, 2017

Choose a reason for hiding this comment

Piripant commented Dec 10, 2017

sunjay left a comment

Choose a reason for hiding this comment

sunjay Dec 10, 2017

Choose a reason for hiding this comment

sunjay Dec 10, 2017

Choose a reason for hiding this comment

Piripant commented Dec 10, 2017 • edited Loading

sunjay left a comment

Choose a reason for hiding this comment

sunjay commented Dec 10, 2017

Piripant commented Dec 10, 2017

sunjay commented Dec 9, 2017 •

edited

Loading

Piripant commented Dec 9, 2017 •

edited

Loading

Piripant commented Dec 10, 2017 •

edited

Loading