sunjay / turtle

Create Animated Drawings in Rust
http://turtle.rs
Mozilla Public License 2.0
559 stars 54 forks source link

Adding a clock example #224

Closed JuanMarchetto closed 3 years ago

JuanMarchetto commented 3 years ago

Hi, i'am learning Rust and i find your project an excelent resource to learn this great lenguaje. So i decided to start contributing with this clock example that update every second drawing your local time

Captura de pantalla de 2021-02-16 16-12-07

it need chrono = "0.4" as a dependencie in the Cargo.toml file

i hope to add more contributions in my learning journey

sunjay commented 3 years ago

Wow! This is very cool! Thanks for taking the time to make it. 😁 πŸŽ‰

I will have a look at it over the next few days and get back to you with a review.

In the meantime, please add chrono to the dev-dependencies section in the Cargo.toml file and delete the comment at the top of the file mentioning that the user should do it themselves. Development dependencies will be compiled when examples are run but not with the rest of the crate so it's safe to add dependencies there for things like this. 😊

JuanMarchetto commented 3 years ago

Wow! This is very cool! Thanks for taking the time to make it.

I will have a look at it over the next few days and get back to you with a review.

In the meantime, please add chrono to the dev-dependencies section in the Cargo.toml file and delete the comment at the top of the file mentioning that the user should do it themselves. Development dependencies will be compiled when examples are run but not with the rest of the crate so it's safe to add dependencies there for things like this.

thanks for your reply, i made that fixes!

sunjay commented 3 years ago

Thanks for adding that dependency @JuanMarchetto! I'm still in the process of reviewing your code, but I did get a chance to pull your branch and run it locally today. This is what it currently looks like on my machine:

Peek 2021-02-17 19-13

As you can see, it's quite different from your screenshot (I'll explain why in a second). You can reproduce this locally by running cargo run --example clock in your local copy of the turtle repo.

The reason your example doesn't work is actually my fault. Without realizing, I made a breaking change to the home method that changes its behaviour to draw a line instead of instantaneously setting the turtle's position. I've updated the documentation to reflect its current behaviour. I'm really glad you decided to contribute this example because we wouldn't have found this without you! 😊

Please make sure you rebase your branch so you get the latest changes. The crate will not build otherwise.

The fixed version of your example looks like this:

Screenshot from 2021-02-17 19-34-45

The clock markings are rounded now because of another breaking change we've had to make on the master branch that hasn't been released yet. I think it still looks great!

You mentioned that you're still learning Rust and fixing this bug is a great learning exercise, so I won't spoil the fun for you. If you'd like to see how I fixed it locally, expand the section below:

(SPOILER ALERT) Bug fix ```diff diff --git a/examples/clock.rs b/examples/clock.rs index 9057337..d5a0451 100644 --- a/examples/clock.rs +++ b/examples/clock.rs @@ -16,8 +16,8 @@ fn main() { //A clock runs forever loop{ //clean everything - turtle.clear(); turtle.home(); + turtle.clear(); //Get local time let now = Local::now(); @@ -32,9 +32,13 @@ fn main() { turtle.set_pen_size(1.0); } turtle.pen_down(); + turtle.forward(10.0); + + turtle.pen_up(); turtle.home(); turtle.right(full_circle / hours as f64 * i as f64); + turtle.pen_down(); } //Draw the hour hand ``` There is actually another way to restructure your code to have the minimum number of `pen_up` and `pen_down` calls, but this diff is the simplest solution and requires the least number of code changes to get things working again.

Let me know if you have any questions! πŸŽ‰ 😁

JuanMarchetto commented 3 years ago

Thanks for your comments @sunjay, i adjusted the example to the new implementation of the home() method, and I made some little improvements.

And now i thinking that i can do some more improvements (maybe i can do another example):

codecov[bot] commented 3 years ago

Codecov Report

Merging #224 (c1e558e) into master (8f31170) will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #224   +/-   ##
=======================================
  Coverage   61.21%   61.21%           
=======================================
  Files          40       40           
  Lines        2697     2697           
=======================================
  Hits         1651     1651           
  Misses       1046     1046           

Continue to review full report at Codecov.

Legend - Click here to learn more Ξ” = absolute <relative> (impact), ΓΈ = not affected, ? = missing data Powered by Codecov. Last update 8f31170...c1e558e. Read the comment docs.

sunjay commented 3 years ago

Thanks for making those adjustments! I'll have another look at them soon. Your ideas are interesting and you are definitely welcome to explore different ways of implementing this. I am fine with your current approach or with the others you've listed. Let me know what you want to do. Take as much time as you need. :)

JuanMarchetto commented 3 years ago

Thanks for making those adjustments! I'll have another look at them soon. Your ideas are interesting and you are definitely welcome to explore different ways of implementing this. I am fine with your current approach or with the others you've listed. Let me know what you want to do. Take as much time as you need. :)

i think is better if leave this example as it is, and start to thinking in other posible examples